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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 5, 2024 at 2:28 PM Pavitrakumar Managutte
<pavitrakumarm@xxxxxxxxxxxxxxx> wrote:
>
> HI Rob,
>   Thanks for the review and feedback.
>
> On Thu, Sep 5, 2024 at 11:53 PM Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Pavitrakumar M <pavitrakumarm@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > > ---
> > >  .../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@xxxxxxxxxxxx>
> > > +
> > > +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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux