Re: [PATCH v2 4/9] dt-binding: riscv: add T-HEAD CPU reset

[Guo Ren]

On Mon, May 22, 2023 at 3:09 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> Hey,
>
> On Mon, May 22, 2023 at 10:16:19AM +0800, Guo Ren wrote:
> > On Fri, May 19, 2023 at 3:53 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > 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@xxxxxxxxxx>
> > > > ---
> > > >  .../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@xxxxxxxxxx>
> > > > +
> > > > +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.
>
> > > > +
> > > > +  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.
>
> Okay, thanks. Sounds like this SoC will have multiple reset controllers
> then, since there is likely one for the peripherals too?
> thead,th1520-cpu-reset seems like a good idea to me?
>
> > > > +  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.
>
> In this case "firmware" means opensbi, since that's where Jisheng
> mentioned in their cover that they intended using this.
Yes, firmware -> opensbi. The hardware guys just modify dtb without
recompiling the opensbi.

>
> > > Putting register values into the DT is always "suspect"!
> > It's not register values, it's register offset/ CSR number.
>
> So "control-val" is not a value? "The value to be set into the control
> reg" makes it sound oddly like one!!
>
> My point I guess is that this entry could be written like
>
> reset-controller@ffff019050 {
>   compatible = "thead,th1520-cpu-reset";
>   reg = <0xff 0xff019050 0xfoo 0xbar>, <0xff 0xff015004 0xfoo 0xbar>;
> };
>
> or even:
>
> reset-controller@ffff019050 {
>   compatible = "thead,th1520-cpu-reset";
>   reg = <0xff 0xff019050 0xfoo 0xbar>, <0xff 0xff015004 0xfoo 0xbar>;
>   reg-names = "entry", "control";
> };
Better naming, I agree with you at this point.
But, we have to make changes to the current opensbi implementation to
match your advice.

>
> And csr-copy, entry-cn and control-val can be derived from the
> compatible string given you've said they are set on a per-soc basis.
>
> > > > +required:
> > > > +  - compatible
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    cpurst: cpurst@ffff019050 {
> > >                ^^^^^^^^^^^^^^^^^
> > > 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 {
>
> I don't think it should! Firstly, "cpurst" is not a generic node name,
Yes, reset-controller is better.

> but I also don't agree that "control-reg" and "entry-reg" should not
> just be 2 reg entries.
Yes, Yes. You've said.

>
> Cheers,
> Conor.



-- 
Best Regards
 Guo Ren