Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 May 2023 20:53:05 +0100
From:   Conor Dooley <conor@kernel.org>
To:     Jisheng Zhang <jszhang@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-riscv@lists.infradead.org, Yangtao Li <frank.li@vivo.com>,
        Wei Fu <wefu@redhat.com>, Icenowy Zheng <uwu@icenowy.me>
Subject: Re: [PATCH v2 4/9] dt-binding: riscv: add T-HEAD CPU reset
Message-ID: <20230518-driving-secluding-793b3192776e@spud>
References: <20230518184541.2627-1-jszhang@kernel.org>
 <20230518184541.2627-5-jszhang@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="4uiDuJOB38i7bEfg"
Content-Disposition: inline
In-Reply-To: <20230518184541.2627-5-jszhang@kernel.org>
Precedence: bulk


--4uiDuJOB38i7bEfg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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-res=
et.yaml
>=20
> 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 =3D "thead,reset-th1520";
> +      entry-reg =3D <0xff 0xff019050>;
> +      entry-cnt =3D <4>;
> +      control-reg =3D <0xff 0xff015004>;
> +      control-val =3D <0x1c>;
> +      csr-copy =3D <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> +    };
> --=20
> 2.40.0
>=20

--4uiDuJOB38i7bEfg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZGaCIQAKCRB4tDGHoIJi
0n9SAP92K/2ImcXGHXRjUY3v5X+WdDMZxN/kIxZBW3btQoK6twD8CWE0LOA195ry
vmZPV/di2h45fbZHO5L8dKmuh9vxHwU=
=0QHL
-----END PGP SIGNATURE-----

--4uiDuJOB38i7bEfg--