Message-ID: <49da270d-6802-79e9-843f-d46efc9ef728@linaro.org>
Date:   Fri, 3 Mar 2023 10:58:53 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 4/9] dt-bindings: crypto: add fsl-sec4-snvs DT schema
Content-Language: en-US
To:     "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
        herbert@gondor.apana.org.au, davem@davemloft.net,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        horia.geanta@nxp.com, pankaj.gupta@nxp.com, gaurav.jain@nxp.com,
        shawnguo@kernel.org, s.hauer@pengutronix.de
Cc:     kernel@pengutronix.de, stefan@agner.ch,
        linux-crypto@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Peng Fan <peng.fan@nxp.com>
References: <20230301015702.3388458-1-peng.fan@oss.nxp.com>
 <20230301015702.3388458-5-peng.fan@oss.nxp.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230301015702.3388458-5-peng.fan@oss.nxp.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/03/2023 02:56, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Convert fsl-sec4.txt SNVS RTC and PowerKey to DT schema

This is a mess. Subject says add, commit msg says convert and body does
what?

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../bindings/crypto/fsl-sec4-snvs.yaml        | 153 ++++++++++++++++++
>  1 file changed, 153 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/crypto/fsl-sec4-snvs.yaml
> 
> diff --git a/Documentation/devicetree/bindings/crypto/fsl-sec4-snvs.yaml b/Documentation/devicetree/bindings/crypto/fsl-sec4-snvs.yaml
> new file mode 100644
> index 000000000000..633e70f9b303
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/fsl-sec4-snvs.yaml

Filename matching compatibles.

> @@ -0,0 +1,153 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/fsl-sec4-snvs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP SEC4 SNVS Binding

Drop Binding.


> +
> +description:
> +  CONTENTS
> +    -Secure Non-Volatile Storage (SNVS) Node
> +    -Secure Non-Volatile Storage (SNVS) Low Power (LP) RTC Node

OK, this is some copy-paste from some poor other code. Please fix all
your bindings like I mentioned in previous emails.

> +
> +  Node defines address range and the associated interrupt for the SNVS
> +  function.  This function monitors security state information & reports
> +  security violations. This also included rtc, system power off and ON/OFF
> +  key.
> +
> +  For more information on SEC4, ref fsl-sec4-crypto.yaml
> +
> +maintainers:
> +  - Peng Fan <peng.fan@nxp.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: fsl,sec-v4.0-mon
> +          - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges:
> +    description:
> +      A standard property. Specifies the physical address range of the SNVS

So the rest are non-standard properties?


> +      register space.  A triplet that includes the child address, parent
> +      address, & length.
> +
> +  interrupts:
> +    description:
> +      Specifies the interrupts generated by this device.  The value of the
> +      interrupts property consists of one interrupt specifier. The format
> +      of the specifier is defined by the binding document describing the
> +      node's interrupt parent.

Please point me to any useful information in this description. Anything
useful. All interrupts are generated from the devices, aren't they?

> +    minItems: 1
> +    maxItems: 2

No, you need to describe the items instead.

> +

(...)

> +    sec_mon: sec_mon@314000 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

No underscores in node names.

> +        compatible = "fsl,sec-v4.0-mon", "syscon";
> +        reg = <0x314000 0x1000>;
> +
> +        snvs-rtc-lp {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            compatible = "fsl,sec-v4.0-mon-rtc-lp";
> +            regmap = <&sec_mon>;
> +            offset = <0x34>;
> +            interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&clks IMX7D_SNVS_CLK>;
> +            clock-names = "snvs-rtc";
> +        };
> +
> +        snvs-powerkey {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Best regards,
Krzysztof