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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/05/2025 10:24, Pavitrakumar Managutte wrote:
> Hi Krzysztof,
>   My comments are embedded below. Appreciate your inputs.
> 
> Warm regards,
> PK
> 
> On Sun, May 18, 2025 at 7:00 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 13/05/2025 08:30, Pavitrakumar Managutte wrote:
>>>>>>>
>>>>>>> I do not see any improvements. It seems you ignored all comments, not
>>>>>>> single one was responded to or addressed.
>>>>>
>>>>> PK: Addressed all the below
>>>>>
>>>>> 1. SoC Bindings: We dont have any SoC bindings since its tested on the
>>>>> Zynq platform (on FPGA). So I have retained just the Synopsys SPAcc
>>>>> device here. Also added a detailed description for the same, which
>>>>> describes how we have tested the SPAcc peripheral on Zynq. This was
>>>>> based on your inputs to describe the existing hardware.
>>>>
>>>> 1. I asked to use SoC specific compatibles and after such explanation
>>>> that you use it in some different, hardware configuration, I asked to
>>>> use that.
>>>>
>>>> Reflect whatever your hardware is called in the compatible.
>>>
>>> PK: Some context from my side which might clear up things
>>> 1. We have developed the SPAcc Crypto Linux driver for the Synopsys SPAcc IP.
>>> 2. Yes, this is technically a soft IP which we test on FPGA (Zynq
>>> Ultrascale Boards).
>>> 3. We are NOT evaluating SPAcc IP and thus its not a custom use case
>>> or a custom hardware.
>>> 4. Also SPAcc IP is NOT part of any SoC yet, but it may be in future.
>>>
>>> Synopsys Semiconductor IP Business:
>>> Synopsys develops Semiconductor IPs (aka DesignWare IPs) and provides
>>> Linux device drivers to the SoC Vendors. We, as partners of Synopsys,
>>> develop Linux device drivers for the IP, in this case SPAcc. So as of
>>> now SPAcc is just a semiconductor IP which is not part of any SoC. A
>>> 3rd party SoC vendor would take this and integrate this as part of
>>> their upcoming SoC.
>>>
>>> SPAcc Semiconductor IP details:
>>> https://www.synopsys.com/designware-ip/security-ip/security-protocol-accelerators.html
>>>
>>> Synopsys DesignWare IPs
>>> 1. DWC MMC Host controller drivers : drivers/mmc/host/dw_mmc.c
>>> 2. DWC HSOTG Driver : drivers/usb/dwc2, drivers/usb/dwc3
>>> 3. DWC Ethernet driver : drivers/net/ethernet/synopsys
>>> 4. DWC DMA driver : drivers/dma/dw/
>>>
>>> Intent of upstreaming IP drivers by Synopsys
>>> 1. As a Semiconductor IP designer, Synopsys provides Linux device
>>> drivers with their IPs to the customers.
>>> 2. These Linux drivers handle all the configurations in those respective IPs.
>>> 3. At this stage of driver development, the focus is on the Semiconductor IP
>>> 4. Yes, the IP can be configured differently for different SoCs and
>>> the driver has to take care of that.
>>> 5. The driver might need some enhancements based on the SoC
>>> configurations, which could be done later.
>>> 6. Its a good approach to upstream IP drivers, so the vendors could
>>> use/enhance the same open sourced drivers.
>>
>>
>> Yeah, I am familiar with this...
>>
>>>
>>>>
>>>> I claim this cannot be used in a SoC without customization. If I
>>>
>>> PK: Synopsys SPAcc is a highly configurable semiconductor IP. I agree
>>> that it can be customized for the SoC vendors. But I dont understand
>>> why it can't be used without SoC customizations for a default
>>
>>
>> Ask hardware team what is necessary to implement given IP in an SoC. SoC
>> architectures are not that simple, that you copy&paste some piece of
>> VHDL code and it plugs into existing wiring. You need that wiring, you
>> need that SoC specific bits in your design.
> 
> PK: I discussed this with my hardware team and their response is as below.
> 
> "Besides the bus interface (base address) and interrupt described in
> the new binding there are standard power and clock and possibly a
> reset interface. However, these have no influence on the driver, so
> are not included in the dts to keep things simple.
> The hardware IP can be configured to run synchronously to the bus or
> have a clock crossing, but as there is no notion of time/frequency in
> the driver that's not relevant to the driver.
> Same for power signals, there is no additional power management in the IP block.
> If you prefer power/clock/reset to be added, can you please point us
> to an example which you consider best practice that we can follow?"

Example not to follow but example to look and see that same block is
customized per SoC:

1. qcom,dwc3
2. rockchip,rk3328-dwc3

Quite different clock inputs, resets, interconnects and power-domains.

> 
>>
>>> configuration. All the IP customizations are handled by the driver.
>>
>> I don't talk about driver. We talk about hardware and bindings.
>>
>>> Say, in the case of SPAcc, all the IP customizations are accessible as
>>> part of the "Version" and "Version Extension-1, 2, 3" registers. So
>>> the driver uses these IP customizations and nothing gets hardcoded. In
>>> other cases, those customizations will come as vendor specific DT
>>> properties.
>>
>> Do you understand the problem discussed here? There is a long standing
>> policy, based on actual real hardware and real cases, that you cannot
>> have generic compatibles for custom IP blocks. That's it.
>>
> PK: Agreed
> 
>>>
>>> As an IP, which can be memory mapped and with interrupt support, it
>>> works perfectly with a default test configuration. And this is what
>>> the current driver has.
>>>
>>>> understood correctly this is soft IP in FPGA for evaluation, so no one
>>>> will be ever able to use it. Therefore this binding makes no sense to me
>>>
>>> PK: No, we are not evaluating, but we have developed a driver for
>>> SPAcc, which has been tested on a FPGA.
>>
>> So some sort of FPGA in some sort of setup which you claim with this
>> patch is exactly the same for every other SoC. That is the meaning of
>> your patch, to which I objected.
> PK: Agreed
> 
>>
>>>
>>>> in general: you do not add anything any customer could use. It is fine
>>>> to add something which you use internally only, but again describe the
>>>> hardware properly.
>>>
>>> PK: Its not an internal use case. We have tested the SPAcc driver on a
>>> FPGA, as detailed above. We dont have any custom hardware and the
>>> SPAcc IP is tested in a default configuration.
>>>
>>> Question : Could you help me understand how a semiconductor IP vendor
>>> like Synopsys, upstream Linux drivers for its IPs? In the current
>>
>> We are not even talking here about drives. I do not have to provide you
>> answers for drivers.
>>
>> I explained already what I expect from bindings: real hardware
>> description, so either real SoC or whatever you are having there.
> 
> PK: The SPAcc, is also tested on "nsimosci", which is an ARC based
> environment. This is our real use case. We already have the ARC dts
> files upstreamed as shown below
> 
> linux/arch/arc/boot/dts/skeleton.dtsi

This feels (and looks inside) like not a SoC, but discouraged skeleton.

> linux/arch/arc/boot/dts/skeleton_hs.dtsi
> linux/arch/arc/boot/dts/nscimosci.dts
> linux/arch/arc/boot/dts/nscimosci_hs.dts
> 
> I can add a SPAcc device node to
> linux/arch/arc/boot/dts/nscimosci_hs_spacc.dts and accordingly create
> the dts yaml bindings. With this change my SPAcc yaml binding is going
> to look like the below snippet.
> 
> -------------------------------------------------------------
> properties:
>   compatible:
>       - items:
>           - const: snps,skeleton_hs-spacc
>           - const: snps,dwc-spacc

Still, drop the last and add one only for your ARC platform (s/_/-/).
Just name it after your platform, SoC or whatever you have there.

Best regards,
Krzysztof