Message-ID: <CAJF2gTTGgLJ3SNcmhRGpAAw7jhOfYA5j=G9ZibwA8wUmeQN3UQ@mail.gmail.com>
Date:   Mon, 22 May 2023 10:16:19 +0800
From:   Guo Ren <guoren@...nel.org>
To:     Conor Dooley <conor@...nel.org>
Cc:     Jisheng Zhang <jszhang@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-riscv@...ts.infradead.org, Yangtao Li <frank.li@...o.com>,
        Wei Fu <wefu@...hat.com>, Icenowy Zheng <uwu@...nowy.me>
Subject: Re: [PATCH v2 4/9] dt-binding: riscv: add T-HEAD CPU reset

On Fri, May 19, 2023 at 3:53 AM Conor Dooley <conor@...nel.org> wrote:
>
> Hey Jisheng,
>
> On Fri, May 19, 2023 at 02:45:36AM +0800, Jisheng Zhang wrote:
> > The secondary CPUs in T-HEAD SMP capable platforms need some special
> > handling. The first one is to write the warm reset entry to entry
> > register. The second one is write a SoC specific control value to
> > a SoC specific control reg. The last one is to clone some CSRs for
> > secondary CPUs to ensure these CSRs' values are the same as the
> > main boot CPU. This DT node is mainly used by opensbi firmware.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> > ---
> >  .../bindings/riscv/thead,cpu-reset.yaml       | 69 +++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml b/Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml
> > new file mode 100644
> > index 000000000000..ba8c87583b6b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/riscv/thead,cpu-reset.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: T-HEAD cpu reset controller
> > +
> > +maintainers:
> > +  - Jisheng Zhang <jszhang@...nel.org>
> > +
> > +description: |
> > +  The secondary CPUs in T-HEAD SMP capable platforms need some special
> > +  handling. The first one is to write the warm reset entry to entry
> > +  register. The second one is write a SoC specific control value to
> > +  a SoC specific control reg. The last one is to clone some CSRs for
> > +  secondary CPUs to ensure these CSRs' values are the same as the
> > +  main boot CPU.
>
> Okay..
>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^cpurst"
>
> Firstly, why the nodename enforcement? We have a compatible, so we
> should be okay, no?
Yes, it needn't.

>
> > +
> > +  compatible:
> > +    oneOf:
> > +      - description: CPU reset on T-HEAD TH1520 SoC
> > +        items:
> > +          - const: thead,reset-th1520
>
> You've only got one thing here, you don't need the oneOf.
> Also, s/reset-th1520/th1520-reset/ please - although I do not know if
> "reset" is the right word here. Do we know what the IP block is called
> in the TRM/T-Head docs? Perhaps Guo Ren does if not.
It's called CPU reset controller; every core has reset_ctrl &
reset_entry signals; Soc just gathers them into some regs.
For th1520, we have 4 reset_entries registers and 1 reset_ctrl
register. Fu Wei would give out more details about it.

>
> > +  entry-reg:
> > +    $ref: /schemas/types.yaml#/definitions/uint64
> > +    description: |
> > +      The entry reg address.
> > +
> > +  entry-cn
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      The entry reg count.
> > +
> > +  control-reg:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
It should be uint64.

> > +    description: |
> > +      The control reg address.
> > +
> > +  control-val:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      The value to be set into the control reg.
> > +
> > +  csr-copy:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description: |
> > +      The CSR registers to be cloned during CPU warm reset.
>
> All of these values set on a per-soc basis, right?
Yes
> If so, I don't think they should be in here at all since you should be
> able to figure out the offsets from the base & the values to write based
> on the compatible string alone, no?
The driver works with all T-HEAD CPUs, not only for th1520. Some
vendors may have their own custom CSRs, so the csr-copy feature is
flexible enough to adjust in dts. As far as I can tell, hardware teams
typically prefer to focus on the firmware binary rather than setting
up the software compiling environment.

>
> Putting register values into the DT is always "suspect"!
It's not register values, it's register offset/ CSR number.

>
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    cpurst: cpurst@...f019050 {
>                ^^^^^^^^^^^^^^^^^
> This is also "suspect" and implies that "entry reg" should just be a
> normal "reg" property.
Yes, but we needn't reg, here. It should be:

cpurst {

Thx for debugging.

>
>
> > +      compatible = "thead,reset-th1520";
> > +      entry-reg = <0xff 0xff019050>;
> > +      entry-cnt = <4>;
> > +      control-reg = <0xff 0xff015004>;
> > +      control-val = <0x1c>;
> > +      csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > +    };
> > --
> > 2.40.0
> >



-- 
Best Regards
 Guo Ren

Powered by blists - more mailing lists