Message-ID: <270daefd-22a6-9359-edcb-c514644cfe4f@amd.com>
Date: Thu, 13 Mar 2025 14:55:07 +0530
From: "Gupta, Nipun" <nipun.gupta@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 derek.kiernan@....com, dragan.cvetic@....com, arnd@...db.de,
 gregkh@...uxfoundation.org
Cc: praveen.jain@....com, harpreet.anand@....com, nikhil.agarwal@....com,
 srivatsa@...il.mit.edu, code@...icks.com, ptsm@...ux.microsoft.com
Subject: Re: [RFC PATCH 2/2] dt-bindings: add device tree binding for silex
 multipk



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@....com>
>> ---
>>   .../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@....com>
>> +  - Praveen Jain <praveen.jain@....com>
>> +
>> +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@tstruk-mobl1.ra.intel.com/)

> 
>> +
>> +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@...00000000 {
>> +      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

Powered by blists - more mailing lists