Re: [PATCH v1 1/1] dt-bindings: crypto: Document support for SPAcc

[Rob Herring <robh@kernel.org>]

On Thu, Sep 05, 2024 at 08:39:10PM +0530, Pavitrakumar M wrote:
> Add DT bindings related to the SPAcc driver for Documentation.
> DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto
> Engine is a crypto IP designed by Synopsys.

This belongs with the rest of your driver series.

> 
> Signed-off-by: Bhoomika K <bhoomikak@vayavyalabs.com>
> Signed-off-by: Pavitrakumar M <pavitrakumarm@vayavyalabs.com>

There's 2 possibilities: Bhoomika is the author and you are just 
submitting it, or you both developed it. The former needs the git author 
fixed to be Bhoomika. The latter needs a Co-developed-by tag for 
Bhoomika.

> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com>
> ---
>  .../bindings/crypto/snps,dwc-spacc.yaml       | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
> new file mode 100644
> index 000000000000..a58d1b171416
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/snps,dwc-spacc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DesignWare Security Protocol Accelerator(SPAcc) Crypto Engine
> +
> +maintainers:
> +  - Ruud Derwig <Ruud.Derwig@synopsys.com>
> +
> +description:
> +  DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto Engine is
> +  a crypto IP designed by Synopsys, that can accelerate cryptographic
> +  operations.
> +
> +properties:
> +  compatible:
> +    contains:

Drop contains. The list of compatible strings and order must be defined.

> +      enum:
> +        - snps,dwc-spacc
> +        - snps,dwc-spacc-6.0

What's the difference between these 2? The driver only had 1 compatible, 
so this should too.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    maxItems: 1

No, you must define the name. But really, just drop it because you don't 
need names with only 1 name.

> +
> +  little-endian: true

Do you really need this? You have a BE CPU this is used with?

> +
> +  vspacc-priority:

Custom properties need a vendor prefix (snps,).

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Set priority mode on the Virtual SPAcc. This is Virtual SPAcc priority
> +      weight. Its used in priority arbitration of the Virtual SPAccs.
> +    minimum: 0
> +    maximum: 15
> +    default: 0
> +
> +  vspacc-index:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Virtual spacc index for validation and driver functioning.

We generally don't do indexes in DT. Need a better description of why 
this is needed.

> +    minimum: 0
> +    maximum: 7
> +
> +  spacc-wdtimer:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Watchdog timer count to replace the default value in driver.
> +    minimum: 0x19000
> +    maximum: 0xFFFFF
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spacc@40000000 {

crypto@40000000

> +        compatible = "snps,dwc-spacc";
> +        reg = <0x40000000 0x3FFFF>;
> +        interrupt-parent = <&gic>;
> +        interrupts = <0 89 4>;
> +        clocks = <&clock>;
> +        clock-names = "ref_clk";
> +        vspacc-priority = <4>;
> +        spacc-wdtimer = <0x20000>;
> +        vspacc-index = <0>;
> +        little-endian;
> +    };
> -- 
> 2.25.1
>