Re: [PATCH v3] dt-bindings: clock: versal: Convert the xlnx,zynqmp-clk.txt to yaml

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

On 28/07/2023 18:19, Conor Dooley wrote:
> On Fri, Jul 28, 2023 at 06:41:50AM +0000, Datta, Shubhrajyoti wrote:
>> [AMD Official Use Only - General]
>>
>>> -----Original Message-----
>>> From: Conor Dooley <conor@kernel.org>
>>> Sent: Wednesday, July 26, 2023 12:57 AM
>>> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
>>> Cc: devicetree@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; linux-
>>> clk@vger.kernel.org; Simek, Michal <michal.simek@amd.com>;
>>> conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>>> robh+dt@kernel.org; sboyd@kernel.org; mturquette@baylibre.com
>>> Subject: Re: [PATCH v3] dt-bindings: clock: versal: Convert the xlnx,zynqmp-
>>> clk.txt to yaml
>>>
>>> On Tue, Jul 25, 2023 at 05:28:07AM +0000, Datta, Shubhrajyoti wrote:
>>>> [AMD Official Use Only - General]
>>>>
>>
>> <snip>
>>>>>>    clocks:
>>>>>>      description: List of clock specifiers which are external input
>>>>>>        clocks to the given clock controller.
>>>>>> -    items:
>>>>>> -      - description: reference clock
>>>>>> -      - description: alternate reference clock
>>>>>> -      - description: alternate reference clock for programmable logic
>>>>>> +    minItems: 3
>>>>>> +    maxItems: 7
>>>>>
>>>>> This doesn't seem right to me. The original binding requires 5 clock
>>>>> inputs, but this will relax it such that only three are needed, no?
>>>>> You'll need to set constraints on a per compatible basis.
>>>>>
>>>> Does below look good.
>>>
>>> I don't think that you tested it with < 5 clocks (hint, if you remove one of the
>>> clocks from your example below, dt_binding_check should fail).
>>> All the constraints need to move into the `if` bits AFAIU.
>>
>>
>> https://lore.kernel.org/all/20230720113110.25047-1-shubhrajyoti.datta@amd.com/
>> Here I had it in the if .
>> Then what I understood from below is that
>>
>> https://lore.kernel.org/all/745fccb0-e49d-7da7-9556-eb28aee4a32b@linaro.org/
>> it should be dropped from the if and added to the above.
>>
>> Maybe I am missing something.
> 
> (Background I got this mail once off-list and tried to make the
> binding's validation work)
> 
> With the current conditions, validation is completely broken. You can
> put in just 1 clock and 1 clock-name and dt-binding-check will pass. The
> only way I could satisfy it, 

I don't understand why you think 1 clock would work. The binding has
clear min/max constraints in top level and strict constraints per each
variant (through listing items). In my opinion this is correct, except
what I wrote - mismatched number of clocks for zynqmp (8 against 7).


> while keeping 7 as the maximum number of
> clocks, was moving the constraints into the if/else. My guess was that 7
> being fewer than the number of clocks in the items: list is part of the
> problem.

Best regards,
Krzysztof