| Message ID | 20240930093054.215809-8-pavitrakumarm@vayavyalabs.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add SPAcc Crypto Driver | expand |
On 30/09/2024 11:30, 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. > > Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> > --- > .../bindings/crypto/snps,dwc-spacc.yaml | 71 +++++++++++++++++++ > 1 file changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml Bindings come before users, so please re-order your patches. > > 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..6b94d0aa7280 > --- /dev/null > +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml > @@ -0,0 +1,71 @@ > +# 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: Nope, you cannot have contains. From where did you get it? Please use existing, recent bindings as starting point or just use exampl-eschema. Eh, you already got this comment and just ignored it. You ignored all other comments as well. This is quite disappointing to ask us to do the same review over and over. Best regards, Krzysztof
Hi Krzysztof, Thanks for the quick review and I do really appreciate everybody's time here. If something got missed, it's just because of the exhaustive hardware and the SystemC Model testing. We make minimal/incremental changes and run things in debug mode which takes a lot of time, since this is a large code base. Never ignored anything till date. Every single comment has been and will be addressed. We will work on code quality as per your inputs. Warm regards, PK On Mon, Sep 30, 2024 at 6:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 30/09/2024 11:30, 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. > > > > Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > > Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > > Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > > Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> > > --- > > .../bindings/crypto/snps,dwc-spacc.yaml | 71 +++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml > > Bindings come before users, so please re-order your patches. PK: Will re-order > > > > > > 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..6b94d0aa7280 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml > > @@ -0,0 +1,71 @@ > > +# 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: > > Nope, you cannot have contains. From where did you get it? Please use > existing, recent bindings as starting point or just use exampl-eschema. PK: Will fix that. > > > Eh, you already got this comment and just ignored it. PK: It got missed, never ignored. Too valuable to ignore comments from demigods. > > > You ignored all other comments as well. This is quite disappointing to > ask us to do the same review over and over. PK: That never was the intent nor the impression I wanted to make. Appreciate everybody's time here. > > > Best regards, > Krzysztof >
On 01/10/2024 04:57, Pavitrakumar Managutte wrote: > Hi Krzysztof, > Thanks for the quick review and I do really appreciate everybody's time here. > If something got missed, it's just because of the exhaustive > hardware and the SystemC Model testing. > We make minimal/incremental changes and run things in debug mode > which takes a lot of time, > since this is a large code base. Never ignored anything till date. > Every single comment has been and will be addressed. We will work > on code quality as per your inputs. No, it was not addressed. Do you want proofs? Look: 1. Drop contains. The list of compatible strings and order must be defined. Not addressed at all. 2. crypto@40000000 Ignored completely 3. Generally drivers aren't limited to some number of instances (except... Also ignored completely and more.. > > Warm regards, > PK > > > On Mon, Sep 30, 2024 at 6:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 30/09/2024 11:30, 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. >>> >>> Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> >>> Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> >>> Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> >>> Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> >>> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> >>> --- >>> .../bindings/crypto/snps,dwc-spacc.yaml | 71 +++++++++++++++++++ >>> 1 file changed, 71 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml >> >> Bindings come before users, so please re-order your patches. > > PK: Will re-order >> >> >>> >>> 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..6b94d0aa7280 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml >>> @@ -0,0 +1,71 @@ >>> +# 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: >> >> Nope, you cannot have contains. From where did you get it? Please use >> existing, recent bindings as starting point or just use exampl-eschema. > > PK: Will fix that. >> >> >> Eh, you already got this comment and just ignored it. > > PK: It got missed, never ignored. Too valuable to ignore comments from demigods. >> >> >> You ignored all other comments as well. This is quite disappointing to >> ask us to do the same review over and over. > > PK: That never was the intent nor the impression I wanted to make. > Appreciate everybody's time here. Then why do you ignore review? Best regards, Krzysztof
On Mon, Sep 30, 2024 at 03:00:54PM +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. > > Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> Please run scripts/checkpatch.pl and fix reported warnings. Then please run and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
Hi Krzysztof, I fully agree, some things got missed out; they were supposed to be addressed in the v9 patch. Not contesting that. My apologies. We are doing a code cleanup based on your inputs and previous comments. I will address everything. Warm regards, PK On Tue, Oct 1, 2024 at 12:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, Sep 30, 2024 at 03:00:54PM +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. > > > > Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > > Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > > Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > > Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> > > Please run scripts/checkpatch.pl and fix reported warnings. Then please > run and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. > > Best regards, > Krzysztof >
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..6b94d0aa7280 --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml @@ -0,0 +1,71 @@ +# 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 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + snps,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 + + snps,vspacc-id: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Virtual spacc index for validation and driver functioning. + minimum: 0 + maximum: 7 + + snps,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>; + snps,vspacc-priority = <4>; + snps,spacc-wdtimer = <0x20000>; + snps,vspacc-id = <0>; + };