Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard

[Michal Simek <michal.simek@amd.com>]

On 10/3/22 12:50, Krzysztof Kozlowski wrote:
> On 03/10/2022 12:37, Michal Simek wrote:
>>
>>
>> On 10/3/22 10:10, Krzysztof Kozlowski wrote:
>>> On 03/10/2022 09:58, Michal Simek wrote:
>>>>
>>>>
>>>> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>>>>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>>>>
>>>>>>> Do we need a whole new schema doc?
>>>>>>
>>>>>> It is completely new IP with different logic compare to origin one.
>>>>>>
>>>>>>>
>>>>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>>>>> once. And it's only a new compatible string.
>>>>>>
>>>>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>>
>>>>> So we already have out of staging document:
>>>>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>
>>>> in 6.1 yes.
>>>>
>>>>>
>>>>> and author wants to add one more:
>>>>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>>>
>>>> as I said it is completely different IP which requires complete different driver
>>>> but IP designers choose similar name which is out of developer control.
>>>>
>>>>>
>>>>> Shall we expect in two years, a third document like:
>>>>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>>>>> ?
>>>>
>>>> Developer definitely doesn't know. If new SoC requires for the same purpose
>>>> different IP with completely different driver is something out of developer
>>>> control. As of today I am not aware about such a requirement and need and
>>>> personally I can just hope that if they need to do such a change they will be
>>>> able to keep current SW driver compatible with new HW IP.
>>>
>>> Then please start naming them reasonable, not two (and in future
>>> x-times) the same names for entirely different blocks. And by name I
>>> mean compatible, filename and device name.
>>>
>>>>>> also for this IP if that's fine with you.
>>>>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>>>>
>>>>> That old binding also explained nr-outputs as "Number of outputs".
>>>>> Perfect... :(
>>>>
>>>> Anyway if description should be improved let's just do it. I just want to get
>>>> guidance if we should update current dt binding for similar IP or just create
>>>> new one as this one is trying to do.
>>>
>>> IMHO, new binding is extremely confusing. We already have support for
>>> devices named "xlnx,clocking-wizard" and now you add exactly the same
>>> (clk=clocking) with almost the same properties, named
>>> "xlnx,clk-wizard-1.0". For a different IP?
>>>
>>> How anyone (even Xilinx' customer) can understand which block is for
>>> what if they have exactly the same name and (almost) the same
>>> properties, but as you said - these are entirely different IP?
>>
>> Let me start from last one. Xilinx has IP catalog in design tools called Vivado.
>> You choose device you have and then you will find clk wizard and you get an IP.
> 
> So you have a specific device? Why it is not part of name and compatible?
> 
>> It means depends on SOC you have ZynqMP or Versal and based on that one or
>> another is taken.
> 
> Exactly. The names xlnx,clocking-wizard and xlnx,clk-wizard-1.0 are
> therefore not specific enough and mixing different devices.

And just to be clear these IPs can be combined with systems where the main cpu 
can be Microblaze. I have also seen some vendors mixing RISC-V with Xilinx IPs.

Please look below.
> 
>> And because this is fpga world none is really describing programmable logic by
>> hand because it would take a look a lot of time. That's why I created long time
>> ago device-tree generator (DTG) which gets design data and based on it generate
>> device tree description. Newest version is available for example here.
>> https://github.com/Xilinx/device-tree-xlnx
>> There is also newer version called system device tree generato
>> https://github.com/Xilinx/system-device-tree-xlnx
>>
>> Because of this infrastructure user will all the time get proper compatible
>> string which is aligned with IP catalog.
> 
> I don't think so. Let's skip for now "clk" and "clocking" differences
> and assume both are "clocking". You have then compatibles:
> 
> xlnx,clocking-wizard and xlnx,clocking-wizard-1.0
> 
> and you said these are entirely different blocks.
> 
> There is no way this creates readable DTS.

And I really thank you for this discussion to do it properly and have proper 
compatible string and description for this block.

Shubhrajyoti: please correct me if I am wrong.

All Xilinx SOCs have programmable logic aligned with FPGAs. Zynq is based 28nm,
ZynqMP (Ultrascale MPSOC) is based on 16nm and Versal is based on 7nm.

I think these clocking IPs are using low level primitives available in PL logic.
Which means there is connection to fpga/pl technology instead of SOC family and 
main cpu.
It can be of course said that if this is ZynqMP SOC that IP A is used. The same 
for Versal SOC. But for soft cores this can't be said.

Would it be better to reflect PL technology in compatible string?

xlnx,clocking-wizard-vX.X (used now for ZynqMP and previous families)
xlnx,clocking-wizard-7nm-vX.X (or find better name names which reflect PL logic 
type)

Thanks,
Michal