| Message ID | 20240905150910.239832-2-pavitrakumarm@vayavyalabs.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | dt-bindings: crypto: Document support for SPAcc | expand |
On 05/09/2024 17:09, 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. > <form letter> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC (and consider --no-git-fallback argument). It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. </form letter> You already sent it... and now send again? That's like third time! Version your patches and provide changelog under ---. Anyway, this is just pointless without users. No explanation either. :/ I won't review it since you keep sending the same buggy code, without changes. Best regards, Krzysztof
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 >
HI Rob, Thanks for the review and feedback. On Thu, Sep 5, 2024 at 11:53 PM Rob Herring <robh@kernel.org> wrote: > > 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. PK: I will club this with the SPAcc driver patch-set. > > > > > 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. PK: Its co-developed. I will add co-developed-by tag for Bhoomika in all the patches. > > > 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. PK: Will drop it as the SPAcc driver is using just "snps,dwc-spacc". > > > + 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. PK: Will fix this to "snps,dwc-spacc" > > > + > > + 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. PK: Will remove it. > > > + > > + little-endian: true > > Do you really need this? You have a BE CPU this is used with? PK: Its a configurable IP. Can be used in little and big-endian configurations. We have tested it on Little-endian CPUs only. (Xilinx Zynq Ultrascale ZCU104) > > > + > > + vspacc-priority: > > Custom properties need a vendor prefix (snps,). PK: Will add vendor prefix. > > > + $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. PK: This is NOT for indexing into DT but more like an ID of a virtual SPAcc. The SPAcc IP can be configured as virtual SPAccs for multi-processor support, where they appear to be dedicated to each processor. The SPAcc hardware supports 2-8 virtual SPAccs (each vSPAcc has its Register bank and context-memory for crypto operations). The index here represents the vSPAcc to be referenced. > > > + 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 > >
On Thu, Sep 5, 2024 at 2:28 PM Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> wrote: > > HI Rob, > Thanks for the review and feedback. > > On Thu, Sep 5, 2024 at 11:53 PM Rob Herring <robh@kernel.org> wrote: > > > > 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. > PK: I will club this with the SPAcc driver patch-set. > > > > > > > > > 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. > PK: Its co-developed. I will add co-developed-by tag for Bhoomika in > all the patches. Also, please use full names. > > > 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. > PK: Will drop it as the SPAcc driver is using just "snps,dwc-spacc". > > > > > + 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. > PK: Will fix this to "snps,dwc-spacc" > > > > > + > > > + 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. > PK: Will remove it. > > > > > + > > > + little-endian: true > > > > Do you really need this? You have a BE CPU this is used with? > PK: Its a configurable IP. Can be used in little and big-endian configurations. > We have tested it on Little-endian CPUs only. (Xilinx Zynq > Ultrascale ZCU104) Not testing BE is clear from reading the driver... It's probably safe to assume the IP will be configured to match the processor. The endian properties are for the exceptions where the peripherals don't match the CPU's endianness. This property can be added when and if there's a platform needing it. Any specific platform should have a specific compatible added as well. > > > > > + > > > + vspacc-priority: > > > > Custom properties need a vendor prefix (snps,). > PK: Will add vendor prefix. > > > > > + $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. > PK: This is NOT for indexing into DT but more like an ID of a virtual SPAcc. > The SPAcc IP can be configured as virtual SPAccs for > multi-processor support, > where they appear to be dedicated to each processor. > The SPAcc hardware supports 2-8 virtual SPAccs (each vSPAcc has > its Register > bank and context-memory for crypto operations). The index here > represents the > vSPAcc to be referenced. Okay, I'd use 'id' rather than 'index' in that case. Rob
Hi Rob, Thanks for the review, I will update and push the patches accordingly. Warm regards, PK On Fri, Sep 6, 2024 at 1:47 AM Rob Herring <robh@kernel.org> wrote: > > On Thu, Sep 5, 2024 at 2:28 PM Pavitrakumar Managutte > <pavitrakumarm@vayavyalabs.com> wrote: > > > > HI Rob, > > Thanks for the review and feedback. > > > > On Thu, Sep 5, 2024 at 11:53 PM Rob Herring <robh@kernel.org> wrote: > > > > > > 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. > > PK: I will club this with the SPAcc driver patch-set. > > > > > > > > > > > > > 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. > > PK: Its co-developed. I will add co-developed-by tag for Bhoomika in > > all the patches. > > Also, please use full names. PK: Will do that > > > > > 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. > > PK: Will drop it as the SPAcc driver is using just "snps,dwc-spacc". > > > > > > > + 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. > > PK: Will fix this to "snps,dwc-spacc" > > > > > > > + > > > > + 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. > > PK: Will remove it. > > > > > > > + > > > > + little-endian: true > > > > > > Do you really need this? You have a BE CPU this is used with? > > PK: Its a configurable IP. Can be used in little and big-endian configurations. > > We have tested it on Little-endian CPUs only. (Xilinx Zynq > > Ultrascale ZCU104) > > Not testing BE is clear from reading the driver... > > It's probably safe to assume the IP will be configured to match the > processor. The endian properties are for the exceptions where the > peripherals don't match the CPU's endianness. This property can be > added when and if there's a platform needing it. Any specific platform > should have a specific compatible added as well. PK: Agreed, I will remove "little-endian" property but the code defaults to little endian implementation. In case the big-endian support comes, we will add a "big-endian" property. > > > > > > > > + > > > > + vspacc-priority: > > > > > > Custom properties need a vendor prefix (snps,). > > PK: Will add vendor prefix. > > > > > > > + $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. > > PK: This is NOT for indexing into DT but more like an ID of a virtual SPAcc. > > The SPAcc IP can be configured as virtual SPAccs for > > multi-processor support, > > where they appear to be dedicated to each processor. > > The SPAcc hardware supports 2-8 virtual SPAccs (each vSPAcc has > > its Register > > bank and context-memory for crypto operations). The index here > > represents the > > vSPAcc to be referenced. > > Okay, I'd use 'id' rather than 'index' in that case. PK: Sure, I will change it to "vspacc-id". > > Rob
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: + enum: + - snps,dwc-spacc + - snps,dwc-spacc-6.0 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + maxItems: 1 + + little-endian: true + + vspacc-priority: + $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. + 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 { + 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; + };