On Fri, May 19, 2023 at 3:53 AM Conor Dooley <conor@xxxxxxxxxx> 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@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. > > 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@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 { 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
