[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230518-driving-secluding-793b3192776e@spud>
Date: Thu, 18 May 2023 20:53:05 +0100
From: Conor Dooley <conor@...nel.org>
To: Jisheng Zhang <jszhang@...nel.org>
Cc: 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>, Guo Ren <guoren@...nel.org>,
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
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?
> +
> + 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@...f019050 {
^^^^^^^^^^^^^^^^^
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
>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists