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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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

Best regards,
Krzysztof