| Message ID | 20221130055214.2416888-6-jiajie.ho@starfivetech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: starfive: Add driver for cryptographic engine | expand |
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
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 @@ -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 + +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 + + clocks: + items: + - description: Hardware reference clock + - description: AHB reference clock + + clock-names: + items: + - const: sec_hclk + - const: sec_ahb + + interrupts: + items: + - description: Interrupt pin for algo completion + - description: Interrupt pin for DMA transfer completion + + interrupt-names: + items: + - const: secirq + - const: dmairq + + resets: + items: + - description: STG domain reset line + + reset-names: + items: + - const: sec_hre + + 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 + + enable-dma: + description: Enable data transfer using dedicated DMA controller. + type: boolean + + dmas: + items: + - description: TX DMA channel + - description: RX DMA channel + + dma-names: + items: + - const: sec_m + - const: sec_p + +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>; + + crypto: crypto@16000000 { + compatible = "starfive,jh7110-crypto"; + reg = <0x0 0x16000000 0x0 0x4000>; + reg-names = "secreg"; + clocks = <&stgcrg JH7110_STGCLK_SEC_HCLK>, + <&stgcrg JH7110_STGCLK_SEC_MISCAHB>; + interrupts = <28>, <29>; + interrupt-names = "secirq", "dmairq"; + clock-names = "sec_hclk","sec_ahb"; + resets = <&stgcrg JH7110_STGRST_SEC_TOP_HRESETN>; + reset-names = "sec_hre"; + enable-side-channel-mitigation; + enable-dma; + dmas = <&sec_dma 1 2>, + <&sec_dma 0 2>; + dma-names = "sec_m","sec_p"; + }; + };