diff mbox series

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

Message ID 20230518184541.2627-5-jszhang@kernel.org (mailing list archive)
State Superseded
Delegated to: Conor Dooley
Headers show
Series Add Sipeed Lichee Pi 4A RISC-V board support | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes, riscv/for-next or riscv/master

Commit Message

Jisheng Zhang May 18, 2023, 6:45 p.m. UTC 
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@kernel.org>
---
 .../bindings/riscv/thead,cpu-reset.yaml       | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml

Comments

Rob Herring May 18, 2023, 7:39 p.m. UTC | #1 
On Fri, 19 May 2023 02:45:36 +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@kernel.org>
> ---
>  .../bindings/riscv/thead,cpu-reset.yaml       | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/riscv/thead,cpu-reset.example.dts:18.35-25.11: Warning (unit_address_vs_reg): /example-0/cpurst@ffff019050: node has a unit name, but no reg or ranges property
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/thead,cpu-reset.example.dtb: cpurst@ffff019050: control-reg:0: [255, 4278276100] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230518184541.2627-5-jszhang@kernel.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Conor Dooley May 18, 2023, 7:53 p.m. UTC | #2 
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@kernel.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@kernel.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?

> +
> +  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.

> +  entry-reg:
> +    $ref: /schemas/types.yaml#/definitions/uint64
> +    description: |
> +      The entry reg address.
> +
> +  entry-cnt:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      The entry reg count.
> +
> +  control-reg:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    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?
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?

Putting register values into the DT is always "suspect"!

> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    cpurst: cpurst@ffff019050 {
               ^^^^^^^^^^^^^^^^^
This is also "suspect" and implies that "entry reg" should just be a
normal "reg" property.


> +      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
>
Guo Ren May 22, 2023, 2:16 a.m. UTC | #3 
On Fri, May 19, 2023 at 3:53 AM Conor Dooley <conor@kernel.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@kernel.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@kernel.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@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
> >
Conor Dooley May 22, 2023, 7:09 a.m. UTC | #4 
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@kernel.org> 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@kernel.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@kernel.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.

> > > +
> > > +  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.

> > 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";
};

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,
but I also don't agree that "control-reg" and "entry-reg" should not
just be 2 reg entries.

Cheers,
Conor.
Guo Ren May 22, 2023, 7:42 a.m. UTC | #5 
On Mon, May 22, 2023 at 3:09 PM Conor Dooley <conor.dooley@microchip.com> 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@kernel.org> 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@kernel.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@kernel.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.
>
> > > > +
> > > > +  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.
Krzysztof Kozlowski May 30, 2023, 12:55 p.m. UTC | #6 
On Fri, 19 May 2023 02:45:36 +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@kernel.org>
> ---
>  .../bindings/riscv/thead,cpu-reset.yaml       | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/riscv/thead,cpu-reset.example.dts:18.35-25.11: Warning (unit_address_vs_reg): /example-0/cpurst@ffff019050: node has a unit name, but no reg or ranges property
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/thead,cpu-reset.example.dtb: cpurst@ffff019050: control-reg:0: [255, 4278276100] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/thead,cpu-reset.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1783487

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
diff mbox series

Patch

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@kernel.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.
+
+properties:
+  $nodename:
+    pattern: "^cpurst"
+
+  compatible:
+    oneOf:
+      - description: CPU reset on T-HEAD TH1520 SoC
+        items:
+          - const: thead,reset-th1520
+
+  entry-reg:
+    $ref: /schemas/types.yaml#/definitions/uint64
+    description: |
+      The entry reg address.
+
+  entry-cnt:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      The entry reg count.
+
+  control-reg:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    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.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    cpurst: cpurst@ffff019050 {
+      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>;
+    };