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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 13/03/2025 10:25, Gupta, Nipun wrote:
>>> +
>>> +maintainers:
>>> +  - Nipun Gupta <nipun.gupta@amd.com>
>>> +  - Praveen Jain <praveen.jain@amd.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/)

And if crypto framework starts supporting it, would it mean that
hardware should be relocated? No, place it in directory matching the
class of the device.

> 
>>
>>> +
>>> +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: 

Then a SoC? So no, you are supposed to use SoC specific compatibles.

But this does not really answer about undocumented prefix, although
maybe you want to say that it is too generic?

> 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 

We talk about binding here.

> 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.

So that's a separate device and you need to use here resets property.

> 
>>
>>> +
>>> +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)

Bouncing back such argument is just making me disappointed in this work.

You did not test it. Period. Send patches only after testing. See
documentation in the DT directory (or talks or online resources).

Best regards,
Krzysztof