Message-ID: <CALxtO0mpQtqPB0h_Wff2dLGo=Mxk02JJQkK4rn+=TuScNdSfxQ@mail.gmail.com>
Date: Tue, 3 Jun 2025 17:15:31 +0530
From: Pavitrakumar Managutte <pavitrakumarm@...avyalabs.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, herbert@...dor.apana.org.au, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, Ruud.Derwig@...opsys.com, 
	manjunath.hadli@...avyalabs.com, adityak@...avyalabs.com, 
	Bhoomika Kadabi <bhoomikak@...avyalabs.com>
Subject: Re: [PATCH v3 1/6] dt-bindings: crypto: Document support for SPAcc

Hi Krzysztof,
  Thanks for the inputs, my comments are embedded below.

Warm regards,
PK

On Mon, Jun 2, 2025 at 11:28 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 02/06/2025 07:32, Pavitrakumar Managutte 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@...avyalabs.com>
> > Signed-off-by: Bhoomika Kadabi <bhoomikak@...avyalabs.com>
> > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@...avyalabs.com>
> > Acked-by: Ruud Derwig <Ruud.Derwig@...opsys.com>
>
> Where was this Ack given? It's not on the lists, it's not public, so it
> cannot be after your SoB.

PK: Yes, its not on the mailing list. I will remove that.

>
> > ---
> >  .../bindings/crypto/snps,dwc-spacc.yaml       | 77 +++++++++++++++++++
> >  1 file changed, 77 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..2780b3db2182
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
> > @@ -0,0 +1,77 @@
> > +# 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@...opsys.com>
> > +
> > +description: |
> > +  This binding describes the Synopsys DWC Security Protocol Accelerator (SPAcc),
>
> Don't say that binding describes a binding.  Describe here hardware.

PK: Sure, I will fix that.

>
> > +  which is a hardware IP designed to accelerate cryptographic operations, such
> > +  as encryption, decryption, and hashing.
> > +
> > +  The SPAcc supports virtualization where a single physical SPAcc can be
> > +  accessed as multiple virtual SPAcc instances, each with its own register set.
> > +  These virtual instances can be assigned different priorities.
> > +
> > +  In this configuration, the SPAcc IP is instantiated within the Synopsys
> > +  NSIMOSCI virtual SoC platform, a SystemC simulation environment used for
> > +  software development and testing. The device is accessed as a memory-mapped
> > +  peripheral and generates interrupts to the ARC interrupt controller.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: snps,nsimosci-hs-spacc
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  snps,vspacc-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Virtual SPAcc instance identifier.
> > +      The SPAcc hardware supports multiple virtual instances (determined by
> > +      ELP_SPACC_CONFIG_VSPACC_CNT parameter), and this ID is used to identify
> > +      which virtual instance this node represents.
>
> No, IDs are not accepted.

PK: This represents the specific virtual SPAcc that is being used in
the current configuration. It is used to index into the register banks
and the context memories of the virtual SPAcc that is being used. The
SPAcc IP can be configured as dedicated virtual SPAccs in
heterogeneous environments.

This was also discssed with Rob Herring and updated from
"vpsacc-index" to "vspacc-id" based on Rob's inputs
https://lore.kernel.org/linux-crypto/CALxtO0mkmyaDYta0tfx9Q1qi_GY0OwUoFDDVmcL15UH_fEZ25w@mail.gmail.com/

>
> > +    minimum: 0
> > +    maximum: 7
> > +
> > +  snps,spacc-internal-counter:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Hardware counter that generates an interrupt based on a count value.
> > +      This counter starts ticking when there is a completed job sitting on
> > +      the status fifo to be serviced. This makes sure that no jobs are
> > +      starved of processing.
>
> Not a DT property.

PK: This is a hardware counter which starts ticking when a processed
job is sitting on the STAT FIFO. This makes sure a JOB does not stay
in STATUS FIFO unprocessed.

This was called watchdog timer - wdtimer, which we renamed to
"spacc-internal-counter" based on your inputs.
https://lore.kernel.org/linux-crypto/CALxtO0k4RkopERap_ykrMTZ4Qtdzm8hEPJGLCQ2pknQGjfQ4Eg@mail.gmail.com/

If you think this "does not qualify" as a DT property, I will make
this into a Kconfig input for the driver.

>
> > +    minimum: 0x19000
> > +    maximum: 0xFFFFF
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
>
> Drop blank line.

PK: I will fix that

>
> > +    crypto@...00000 {
> > +        compatible = "snps,nsimosci-hs-spacc";
> > +        reg = <0x40000000 0x3FFFF>;
>
> Lowercase hex only.

PK: I will fix that

>
>
>
> Best regards,
> Krzysztof

Powered by blists - more mailing lists