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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

On Fri, Jul 28, 2023 at 06:39:23PM +0200, Krzysztof Kozlowski wrote:
> 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).

I didn't expect it would work - I tried 4 w/ the zynqmp compatible,
which passed, although it should not have. And then I took it to the
extreme (1) which also passed. There's something wrong with either the
binding or the example that I can't spot.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]