Re: [RFC PATCH 2/2] dt-bindings: add device tree binding for silex multipk

[Gupta, Nipun]

On 12-03-2025 16:21, Krzysztof Kozlowski wrote:
> On 12/03/2025 10:54, Nipun Gupta wrote:
> > Add binding documentation for Silex multipk device node with compatible
> > string as 'silex,mutlipk'.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Sure will update.

> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > ---
> >   .../bindings/misc/silex,multipk.yaml          | 50 +++++++++++++++++++
>
> Bindings are before users.

Will send this as first patch in next spin

> >   MAINTAINERS                                   |  1 +
> >   2 files changed, 51 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/misc/silex,multipk.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/misc/silex,multipk.yaml b/Documentation/devicetree/bindings/misc/silex,multipk.yaml
> > new file mode 100644
> > index 000000000000..6951886734ca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/silex,multipk.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/misc/silex,multipk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Silex MultiPK driver
>
> Drop "driver" and describe hardware.

okay

> > +
> > +maintainers:
> > +  - Nipun Gupta <nipun.gupta@xxxxxxx>
> > +  - Praveen Jain <praveen.jain@xxxxxxx>
> > +
> > +description: |
> > +  Silex Multipk device handles the Asymmetric crypto operations. The
> > +  driver provides interface to user-space to directly interact with the
> > +  Silex MultiPK device.
>
> Why this isn't in crypto?

It is mentioned in patch RFC 1/2 - because Crypto AF_ALG does not 
support offloading asymmetric operations from user-space (which was 
attempted to be added earlier in Linux at: 
https://lore.kernel.org/all/146672253157.23101.15291203749122389409.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/)

> > +
> > +properties:
> > +  compatible:
> > +    const: silex,mutlipk
>
> Unknown vendor prefix
>
> Device name part is weirdly generic. How is this device exactly called?
> Where is it used? Where is datasheet?

The device is on AMD versal series and is named "Multi PKI" device. I
will update to use compatible as xlnx,multipk (AMD versal series link: 
https://www.amd.com/en/products/adaptive-socs-and-fpgas/versal/premium-series.html). 
Seems also renaming files to xlnx_multipk.c etc would be better. Will 
update.

The device is used for PKI offload for OpenSSL based applications.
Unfortunately data sheet is not available in public domain.

> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reg:
> > +    items:
> > +      - description: PKI Queues memory region
> > +      - description: PKI TRNG memory region
> > +      - description: PKI reset memory region
>
> reset? Like reset controller? Why is this here instead of using existing
> reset framework?

Not exactly, there is a clock reset which is separate from the device. I 
explored and there is a soft reset as well which will do the 
functionality and we do not need this memory region. Will remove it.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - iommus
>
> You did not test your patches.

We have tested the driver code, but device tree yaml needs some changes 
you mentioned (and so the dts)

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    multipk@20400000000 {
> > +      compatible = "silex,multipk";
> > +      interrupts = <0x00000000 0x0000009b IRQ_TYPE_LEVEL_HIGH>;
>
> This is nowhere DTS coding style. See how other bindings do it.

Will check and update the binding.

> > +      reg = <0x00000204 0x00000000 0x00000000 0x00010000>,
> > +            <0x00000204 0x00020000 0x00000000 0x00000050>,
> > +            <0x00000000 0xEC200340 0x00000000 0x00000004>;
>
> lowercase hex, drop the padings of r0.

Sure.

> > +      iommus = <&smmu 0x25B>;
>
> Lowercase hex

Okay.

> > +    };
> Why is this patch a RFC? What is incomplete here?
>
> RFC means patch is not ready so you will not get full review. Full
> review will come once you send proper patch (and remember about
> changelog and versioning - this is v1).

Will send a non RFC version for full review.

Thanks,
Nipun

> Best regards,
> Krzysztof