[5/6] dt-bindings: crypto: Add bindings for Starfive crypto driver

  Add documentation to describe Starfive crypto
driver bindings.

Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
---
 .../bindings/crypto/starfive-crypto.yaml      | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
  

On 30/11/2022 06:52, Jia Jie Ho wrote:
> Add documentation to describe Starfive crypto
> driver bindings.

Please wrap commit message according to Linux coding style / submission
process:
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586


Subject: drop second, redundant "bindings".


> 
> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> ---
>  .../bindings/crypto/starfive-crypto.yaml      | 109 ++++++++++++++++++
>  1 file changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
> 
> diff --git a/Documentation/devicetree/bindings/crypto/starfive-crypto.yaml b/Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
> new file mode 100644
> index 000000000000..6b852f774c32
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/starfive-crypto.yaml

Filename based on compatible, so starfive,jh7110-crypto.yaml

> @@ -0,0 +1,109 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/starfive-crypto.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive Crypto Controller Device Tree Bindings

Drop "Device Tree Bindings"

> +
> +maintainers:
> +  - Jia Jie Ho <jiajie.ho@starfivetech.com>
> +  - William Qiu <william.qiu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-crypto
> +
> +  reg:
> +    maxItems: 1
> +
> +  reg-names:
> +    items:
> +      - const: secreg

Why do you need reg-names for one entry?

> +
> +  clocks:
> +    items:
> +      - description: Hardware reference clock
> +      - description: AHB reference clock
> +
> +  clock-names:
> +    items:
> +      - const: sec_hclk
> +      - const: sec_ahb

sec seems redundant, so just "ahb". The first clock then "hclk" or "ref"?

> +
> +  interrupts:
> +    items:
> +      - description: Interrupt pin for algo completion
> +      - description: Interrupt pin for DMA transfer completion
> +
> +  interrupt-names:
> +    items:
> +      - const: secirq
> +      - const: dmairq

Drop "irq" from both.

> +
> +  resets:
> +    items:
> +      - description: STG domain reset line
> +
> +  reset-names:
> +    items:
> +      - const: sec_hre

Drop "sec". Why do you need the names for one entry?

> +
> +  enable-side-channel-mitigation:
> +    description: Enable side-channel-mitigation feature for AES module.
> +        Enabling this feature will affect the speed performance of
> +        crypto engine.
> +    type: boolean

Why exactly this is a hardware (DT) property, not runtime?

> +
> +  enable-dma:
> +    description: Enable data transfer using dedicated DMA controller.
> +    type: boolean

Usually the presence of dmas indicates whether to use or not to use DMA.
Do you expect a case where DMA channels are provided by you don't want
DMA? Explain such case and describe why it is a hardware/system
integration property.

> +
> +  dmas:
> +    items:
> +      - description: TX DMA channel
> +      - description: RX DMA channel
> +
> +  dma-names:
> +    items:
> +      - const: sec_m

tx

> +      - const: sec_p

rx

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/starfive-jh7110.h>
> +    #include <dt-bindings/reset/starfive-jh7110.h>
> +
> +    soc {
> +            #address-cells = <2>;
> +            #size-cells = <2>;

Use 4 spaces for example indentation.

> +

Best regards,
Krzysztof
  

On Wed, 30 Nov 2022 13:52:13 +0800, Jia Jie Ho wrote:
> Add documentation to describe Starfive crypto
> driver bindings.
> 
> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> ---
>  .../bindings/crypto/starfive-crypto.yaml      | 109 ++++++++++++++++++
>  1 file changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/crypto/starfive-crypto.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/crypto/starfive-crypto.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No such file or directory
   21 |         #include <dt-bindings/clock/starfive-jh7110.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/crypto/starfive-crypto.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221130055214.2416888-6-jiajie.ho@starfivetech.com

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.
  

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, November 30, 2022 9:21 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH 5/6] dt-bindings: crypto: Add bindings for Starfive crypto
> driver
> 
> On 30/11/2022 06:52, Jia Jie Ho wrote:
> > Add documentation to describe Starfive crypto driver bindings.
> 
> Please wrap commit message according to Linux coding style / submission
> process:
> https://elixir.bootlin.com/linux/v5.18-
> rc4/source/Documentation/process/submitting-patches.rst#L586
> 
> 
> Subject: drop second, redundant "bindings".
> 
> 

I'll fix the commit title and message in the next version.

> >
> > Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > ---
> >  .../bindings/crypto/starfive-crypto.yaml      | 109 ++++++++++++++++++
> >  1 file changed, 109 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
> > b/Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
> > new file mode 100644
> > index 000000000000..6b852f774c32
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
> 
> Filename based on compatible, so starfive,jh7110-crypto.yaml
> 

Will update filename.

> > @@ -0,0 +1,109 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/crypto/starfive-crypto.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: StarFive Crypto Controller Device Tree Bindings
> 
> Drop "Device Tree Bindings"
> 

Will update title.

> > +
> > +maintainers:
> > +  - Jia Jie Ho <jiajie.ho@starfivetech.com>
> > +  - William Qiu <william.qiu@starfivetech.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: starfive,jh7110-crypto
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reg-names:
> > +    items:
> > +      - const: secreg
> 
> Why do you need reg-names for one entry?
> 

Will remove need of reg-names and update probe function accordingly.

> > +
> > +  clocks:
> > +    items:
> > +      - description: Hardware reference clock
> > +      - description: AHB reference clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: sec_hclk
> > +      - const: sec_ahb
> 
> sec seems redundant, so just "ahb". The first clock then "hclk" or "ref"?
> 

Will fix in next version.

> > +
> > +  interrupts:
> > +    items:
> > +      - description: Interrupt pin for algo completion
> > +      - description: Interrupt pin for DMA transfer completion
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: secirq
> > +      - const: dmairq
> 
> Drop "irq" from both.
> 

Will fix.

> > +
> > +  resets:
> > +    items:
> > +      - description: STG domain reset line
> > +
> > +  reset-names:
> > +    items:
> > +      - const: sec_hre
> 
> Drop "sec". Why do you need the names for one entry?
> 

Will remove names.

> > +
> > +  enable-side-channel-mitigation:
> > +    description: Enable side-channel-mitigation feature for AES module.
> > +        Enabling this feature will affect the speed performance of
> > +        crypto engine.
> > +    type: boolean
> 
> Why exactly this is a hardware (DT) property, not runtime?
> 

This is a hardware setting provided in StarFive crypto engine only.
The crypto API doesn't control this setting during runtime and leaving this always on will impact speed performance.
So, I added this property to allow user to control this in dtb.

> > +
> > +  enable-dma:
> > +    description: Enable data transfer using dedicated DMA controller.
> > +    type: boolean
> 
> Usually the presence of dmas indicates whether to use or not to use DMA.
> Do you expect a case where DMA channels are provided by you don't want
> DMA? Explain such case and describe why it is a hardware/system integration
> property.
> 

I'll remove this property as DMA is always enabled.

> > +
> > +  dmas:
> > +    items:
> > +      - description: TX DMA channel
> > +      - description: RX DMA channel
> > +
> > +  dma-names:
> > +    items:
> > +      - const: sec_m
> 
> tx
> 

Will fix.

> > +      - const: sec_p
> 
> rx
> 

Will fix.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - reset-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/starfive-jh7110.h>
> > +    #include <dt-bindings/reset/starfive-jh7110.h>
> > +
> > +    soc {
> > +            #address-cells = <2>;
> > +            #size-cells = <2>;
> 
> Use 4 spaces for example indentation.

I'll fix the indentation.

Thank you for taking time to review and provide helpful comments for this patch.

Best regards,
Jia Jie
  

On 01/12/2022 10:01, JiaJie Ho wrote:

>>> +
>>> +  enable-side-channel-mitigation:
>>> +    description: Enable side-channel-mitigation feature for AES module.
>>> +        Enabling this feature will affect the speed performance of
>>> +        crypto engine.
>>> +    type: boolean
>>
>> Why exactly this is a hardware (DT) property, not runtime?
>>
> 
> This is a hardware setting provided in StarFive crypto engine only.
> The crypto API doesn't control this setting during runtime and leaving this always on will impact speed performance.
> So, I added this property to allow user to control this in dtb.

Devicetree should not describe policies, so without justification it
does not look like hardware property. Drop.


Best regards,
Krzysztof
  

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, November 30, 2022 9:48 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: linux-crypto@vger.kernel.org; linux-riscv@lists.infradead.org; Rob
> Herring <robh+dt@kernel.org>; Herbert Xu
> <herbert@gondor.apana.org.au>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; David S . Miller <davem@davemloft.net>
> Subject: Re: [PATCH 5/6] dt-bindings: crypto: Add bindings for Starfive crypto
> driver
> 
> 
> On Wed, 30 Nov 2022 13:52:13 +0800, Jia Jie Ho wrote:
> > Add documentation to describe Starfive crypto driver bindings.
> >
> > Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > ---
> >  .../bindings/crypto/starfive-crypto.yaml      | 109 ++++++++++++++++++
> >  1 file changed, 109 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/crypto/starfive-crypto.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/crypto/starfive-
> crypto.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No
> such file or directory
>    21 |         #include <dt-bindings/clock/starfive-jh7110.h>
>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> make[1]: *** [scripts/Makefile.lib:406:
> Documentation/devicetree/bindings/crypto/starfive-crypto.example.dtb]
> Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1492: dt_binding_check] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-
> bindings/patch/20221130055214.2416888-6-jiajie.ho@starfivetech.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 

Hi Rob Herring,

The #include in example have dependencies on the following patches:
https://patchwork.kernel.org/project/linux-riscv/cover/20221118010627.70576-1-hal.feng@starfivetech.com/
https://patchwork.kernel.org/project/linux-riscv/cover/20221118011714.70877-1-hal.feng@starfivetech.com/
I've noted them in the cover letter.
How do I add them in this patch?

Thanks,
Jia Jie
  

On 06/12/2022 04:48, JiaJie Ho wrote:
> 
> 
>> -----Original Message-----
>> From: Rob Herring <robh@kernel.org>
>> Sent: Wednesday, November 30, 2022 9:48 PM
>> To: JiaJie Ho <jiajie.ho@starfivetech.com>
>> Cc: linux-crypto@vger.kernel.org; linux-riscv@lists.infradead.org; Rob
>> Herring <robh+dt@kernel.org>; Herbert Xu
>> <herbert@gondor.apana.org.au>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; David S . Miller <davem@davemloft.net>
>> Subject: Re: [PATCH 5/6] dt-bindings: crypto: Add bindings for Starfive crypto
>> driver
>>
>>
>> On Wed, 30 Nov 2022 13:52:13 +0800, Jia Jie Ho wrote:
>>> Add documentation to describe Starfive crypto driver bindings.
>>>
>>> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
>>> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
>>> ---
>>>  .../bindings/crypto/starfive-crypto.yaml      | 109 ++++++++++++++++++
>>>  1 file changed, 109 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/crypto/starfive-crypto.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/crypto/starfive-
>> crypto.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No
>> such file or directory
>>    21 |         #include <dt-bindings/clock/starfive-jh7110.h>
>>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> compilation terminated.
>> make[1]: *** [scripts/Makefile.lib:406:
>> Documentation/devicetree/bindings/crypto/starfive-crypto.example.dtb]
>> Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> make: *** [Makefile:1492: dt_binding_check] Error 2
>>
>> doc reference errors (make refcheckdocs):
>>
>> See https://patchwork.ozlabs.org/project/devicetree-
>> bindings/patch/20221130055214.2416888-6-jiajie.ho@starfivetech.com
>>
>> The base for the series is generally the latest rc1. A different dependency
>> should be noted in *this* patch.
>>
> 
> Hi Rob Herring,
> 
> The #include in example have dependencies on the following patches:
> https://patchwork.kernel.org/project/linux-riscv/cover/20221118010627.70576-1-hal.feng@starfivetech.com/
> https://patchwork.kernel.org/project/linux-riscv/cover/20221118011714.70877-1-hal.feng@starfivetech.com/
> I've noted them in the cover letter.
> How do I add them in this patch?

You cannot. Testing bot does not take dependencies, so it did not have
above clock IDs. This also should point your attention that if crypto
maintainers pick up this patch, they also won't have the clock IDs so
their tree will have such failure as well.

You can wait with your patch till dependency hits mainline or you can
just replace clock IDs with numbers and drop the header (and later you
can correct the example if needed... or leave it as is).

Best regards,
Krzysztof
  

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, December 6, 2022 4:26 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Rob Herring <robh@kernel.org>
> Cc: linux-crypto@vger.kernel.org; linux-riscv@lists.infradead.org; Rob
> Herring <robh+dt@kernel.org>; Herbert Xu
> <herbert@gondor.apana.org.au>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; David S . Miller <davem@davemloft.net>
> Subject: Re: [PATCH 5/6] dt-bindings: crypto: Add bindings for Starfive crypto
> driver
> 
> On 06/12/2022 04:48, JiaJie Ho wrote:
> >
> >
> > The #include in example have dependencies on the following patches:
> > https://patchwork.kernel.org/project/linux-riscv/cover/20221118010627.
> > 70576-1-hal.feng@starfivetech.com/
> > https://patchwork.kernel.org/project/linux-riscv/cover/20221118011714.
> > 70877-1-hal.feng@starfivetech.com/
> > I've noted them in the cover letter.
> > How do I add them in this patch?
> 
> You cannot. Testing bot does not take dependencies, so it did not have
> above clock IDs. This also should point your attention that if crypto
> maintainers pick up this patch, they also won't have the clock IDs so their tree
> will have such failure as well.
> 
> You can wait with your patch till dependency hits mainline or you can just
> replace clock IDs with numbers and drop the header (and later you can
> correct the example if needed... or leave it as is).
> 
I'll replace the IDs and drop the header then.
Thanks.

Regards,
Jia Jie
  

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, December 1, 2022 5:27 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH 5/6] dt-bindings: crypto: Add bindings for Starfive crypto
> driver
> 
> On 01/12/2022 10:01, JiaJie Ho wrote:
> 
> >>> +
> >>> +  enable-side-channel-mitigation:
> >>> +    description: Enable side-channel-mitigation feature for AES module.
> >>> +        Enabling this feature will affect the speed performance of
> >>> +        crypto engine.
> >>> +    type: boolean
> >>
> >> Why exactly this is a hardware (DT) property, not runtime?
> >>
> >
> > This is a hardware setting provided in StarFive crypto engine only.
> > The crypto API doesn't control this setting during runtime and leaving this
> always on will impact speed performance.
> > So, I added this property to allow user to control this in dtb.
> 
> Devicetree should not describe policies, so without justification it does not
> look like hardware property. Drop.
> 
I'll remove this in v2 and set the value using module param instead.
Thanks.

Best regards,
Jia Jie