| Message ID | 20230724111843.18706-1-shubhrajyoti.datta@amd.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] dt-bindings: clock: versal: Convert the xlnx,zynqmp-clk.txt to yaml | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 102 lines checked |
| robh/patch-applied | fail | build log |
On Mon, Jul 24, 2023 at 04:48:43PM +0530, Shubhrajyoti Datta wrote: > Convert the xlnx,zynqmp-clk.txt to yaml. > versal-clk.yaml already exists that's why ZynqMP is converted and > merged. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > --- > > Changes in v3: > Update the min and maxitems > > Changes in v2: > add enum in compatible > fix the description > add constraints for clocks > name the clock-controller1 to clock-controller > > .../bindings/clock/xlnx,versal-clk.yaml | 78 ++++++++++++++++--- > .../bindings/clock/xlnx,zynqmp-clk.txt | 63 --------------- > 2 files changed, 69 insertions(+), 72 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt > > diff --git a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > index e9cf747bf89b..deebbfd084e8 100644 > --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > @@ -19,7 +19,9 @@ select: false > properties: > compatible: > oneOf: > - - const: xlnx,versal-clk > + - enum: > + - xlnx,versal-clk > + - xlnx,zynqmp-clk > - items: > - enum: > - xlnx,versal-net-clk > @@ -31,16 +33,12 @@ properties: > 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. Thanks, Conor. > clock-names: > - items: > - - const: ref > - - const: alt_ref > - - const: pl_alt_ref > + minItems: 3 > + maxItems: 7 > > required: > - compatible > @@ -50,6 +48,59 @@ required: > > additionalProperties: false > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - xlnx,versal-clk > + > + then: > + properties: > + clocks: > + items: > + - description: reference clock > + - description: alternate reference clock > + - description: alternate reference clock for programmable logic > + > + clock-names: > + items: > + - const: ref > + - const: alt_ref > + - const: pl_alt_ref > + > + - if: > + properties: > + compatible: > + contains: > + enum: > + - xlnx,zynqmp-clk > + > + then: > + properties: > + clocks: > + items: > + - description: PS reference clock > + - description: reference clock for video system > + - description: alternative PS reference clock > + - description: auxiliary reference clock > + - description: transceiver reference clock > + - description: (E)MIO clock source (Optional clock) > + - description: GEM emio clock (Optional clock) > + - description: Watchdog external clock (Optional clock) > + > + clock-names: > + items: > + - const: pss_ref_clk > + - const: video_clk > + - const: pss_alt_ref_clk > + - const: aux_ref_clk > + - const: gt_crx_ref_clk > + - pattern: "^mio_clk[00-77]+.*$" > + - pattern: "gem[0-3]+_emio_clk.*$" > + - pattern: "swdt[0-1]+_ext_clk.*$" > + > examples: > - | > firmware { > @@ -64,4 +115,13 @@ examples: > }; > }; > }; > + > + clock-controller { > + #clock-cells = <1>; > + compatible = "xlnx,zynqmp-clk"; > + clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, > + <&aux_ref_clk>, <>_crx_ref_clk>; > + clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk", > + "aux_ref_clk", "gt_crx_ref_clk"; > + }; > ... > diff --git a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt b/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt > deleted file mode 100644 > index 391ee1a60bed..000000000000 > --- a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt > +++ /dev/null > @@ -1,63 +0,0 @@ > --------------------------------------------------------------------------- > -Device Tree Clock bindings for the Zynq Ultrascale+ MPSoC controlled using > -Zynq MPSoC firmware interface > --------------------------------------------------------------------------- > -The clock controller is a h/w block of Zynq Ultrascale+ MPSoC clock > -tree. It reads required input clock frequencies from the devicetree and acts > -as clock provider for all clock consumers of PS clocks. > - > -See clock_bindings.txt for more information on the generic clock bindings. > - > -Required properties: > - - #clock-cells: Must be 1 > - - compatible: Must contain: "xlnx,zynqmp-clk" > - - clocks: List of clock specifiers which are external input > - clocks to the given clock controller. Please refer > - the next section to find the input clocks for a > - given controller. > - - clock-names: List of clock names which are exteral input clocks > - to the given clock controller. Please refer to the > - clock bindings for more details. > - > -Input clocks for zynqmp Ultrascale+ clock controller: > - > -The Zynq UltraScale+ MPSoC has one primary and four alternative reference clock > -inputs. These required clock inputs are: > - - pss_ref_clk (PS reference clock) > - - video_clk (reference clock for video system ) > - - pss_alt_ref_clk (alternative PS reference clock) > - - aux_ref_clk > - - gt_crx_ref_clk (transceiver reference clock) > - > -The following strings are optional parameters to the 'clock-names' property in > -order to provide an optional (E)MIO clock source: > - - swdt0_ext_clk > - - swdt1_ext_clk > - - gem0_emio_clk > - - gem1_emio_clk > - - gem2_emio_clk > - - gem3_emio_clk > - - mio_clk_XX # with XX = 00..77 > - - mio_clk_50_or_51 #for the mux clock to gem tsu from 50 or 51 > - > - > -Output clocks are registered based on clock information received > -from firmware. Output clocks indexes are mentioned in > -include/dt-bindings/clock/xlnx-zynqmp-clk.h. > - > -------- > -Example > -------- > - > -firmware { > - zynqmp_firmware: zynqmp-firmware { > - compatible = "xlnx,zynqmp-firmware"; > - method = "smc"; > - zynqmp_clk: clock-controller { > - #clock-cells = <1>; > - compatible = "xlnx,zynqmp-clk"; > - clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, <&aux_ref_clk>, <>_crx_ref_clk>; > - clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk"; > - }; > - }; > -}; > -- > 2.17.1 >
[AMD Official Use Only - General] > -----Original Message----- > From: Conor Dooley <conor@kernel.org> > Sent: Tuesday, July 25, 2023 12:18 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 Mon, Jul 24, 2023 at 04:48:43PM +0530, Shubhrajyoti Datta wrote: > > Convert the xlnx,zynqmp-clk.txt to yaml. > > versal-clk.yaml already exists that's why ZynqMP is converted and > > merged. > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > > > --- > > > > Changes in v3: > > Update the min and maxitems > > > > Changes in v2: > > add enum in compatible > > fix the description > > add constraints for clocks > > name the clock-controller1 to clock-controller > > > > .../bindings/clock/xlnx,versal-clk.yaml | 78 ++++++++++++++++--- > > .../bindings/clock/xlnx,zynqmp-clk.txt | 63 --------------- > > 2 files changed, 69 insertions(+), 72 deletions(-) delete mode > > 100644 Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt > > > > diff --git > > a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > index e9cf747bf89b..deebbfd084e8 100644 > > --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > @@ -19,7 +19,9 @@ select: false > > properties: > > compatible: > > oneOf: > > - - const: xlnx,versal-clk > > + - enum: > > + - xlnx,versal-clk > > + - xlnx,zynqmp-clk > > - items: > > - enum: > > - xlnx,versal-net-clk > > @@ -31,16 +33,12 @@ properties: > > 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. diff --git a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml index e9cf747bf89b..89b8d592a6d4 100644 --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml @@ -19,7 +19,9 @@ select: false properties: compatible: oneOf: - - const: xlnx,versal-clk + - enum: + - xlnx,versal-clk + - xlnx,zynqmp-clk - items: - enum: - xlnx,versal-net-clk @@ -31,16 +33,12 @@ properties: 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 clock-names: - items: - - const: ref - - const: alt_ref - - const: pl_alt_ref + minItems: 3 + maxItems: 7 required: - compatible @@ -50,6 +48,61 @@ required: additionalProperties: false +allOf: + - if: + properties: + compatible: + contains: + enum: + - xlnx,versal-clk + + then: + properties: + clocks: + items: + - description: reference clock + - description: alternate reference clock + - description: alternate reference clock for programmable logic + + clock-names: + items: + - const: ref + - const: alt_ref + - const: pl_alt_ref + + - if: + properties: + compatible: + contains: + enum: + - xlnx,zynqmp-clk + + then: + properties: + clocks: + minItems: 5 + items: + - description: PS reference clock + - description: reference clock for video system + - description: alternative PS reference clock + - description: auxiliary reference clock + - description: transceiver reference clock + - description: (E)MIO clock source (Optional clock) + - description: GEM emio clock (Optional clock) + - description: Watchdog external clock (Optional clock) + + clock-names: + minItems: 5 + items: + - const: pss_ref_clk + - const: video_clk + - const: pss_alt_ref_clk + - const: aux_ref_clk + - const: gt_crx_ref_clk + - pattern: "^mio_clk[00-77]+.*$" + - pattern: "gem[0-3]+_emio_clk.*$" + - pattern: "swdt[0-1]+_ext_clk.*$" + examples: - | firmware { @@ -64,4 +117,13 @@ examples: }; }; }; + + clock-controller { + #clock-cells = <1>; + compatible = "xlnx,zynqmp-clk"; + clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, + <&aux_ref_clk>, <>_crx_ref_clk>; + clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk", + "aux_ref_clk", "gt_crx_ref_clk"; + };
On Tue, Jul 25, 2023 at 05:28:07AM +0000, Datta, Shubhrajyoti wrote: > [AMD Official Use Only - General] > > > -----Original Message----- > > From: Conor Dooley <conor@kernel.org> > > Sent: Tuesday, July 25, 2023 12:18 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 Mon, Jul 24, 2023 at 04:48:43PM +0530, Shubhrajyoti Datta wrote: > > > Convert the xlnx,zynqmp-clk.txt to yaml. > > > versal-clk.yaml already exists that's why ZynqMP is converted and > > > merged. > > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > > > > > --- > > > > > > Changes in v3: > > > Update the min and maxitems > > > > > > Changes in v2: > > > add enum in compatible > > > fix the description > > > add constraints for clocks > > > name the clock-controller1 to clock-controller > > > > > > .../bindings/clock/xlnx,versal-clk.yaml | 78 ++++++++++++++++--- > > > .../bindings/clock/xlnx,zynqmp-clk.txt | 63 --------------- > > > 2 files changed, 69 insertions(+), 72 deletions(-) delete mode > > > 100644 Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt > > > > > > diff --git > > > a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > > b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > > index e9cf747bf89b..deebbfd084e8 100644 > > > --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > > +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml > > > @@ -19,7 +19,9 @@ select: false > > > properties: > > > compatible: > > > oneOf: > > > - - const: xlnx,versal-clk > > > + - enum: > > > + - xlnx,versal-clk > > > + - xlnx,zynqmp-clk > > > - items: > > > - enum: > > > - xlnx,versal-net-clk > > > @@ -31,16 +33,12 @@ properties: > > > 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. Thanks, Conor.
[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. > > Thanks, > Conor.
On 24/07/2023 13:18, Shubhrajyoti Datta wrote: > Convert the xlnx,zynqmp-clk.txt to yaml. > versal-clk.yaml already exists that's why ZynqMP is converted and > merged. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > + > + - if: > + properties: > + compatible: > + contains: > + enum: > + - xlnx,zynqmp-clk > + > + then: > + properties: > + clocks: > + items: > + - description: PS reference clock > + - description: reference clock for video system > + - description: alternative PS reference clock > + - description: auxiliary reference clock > + - description: transceiver reference clock > + - description: (E)MIO clock source (Optional clock) > + - description: GEM emio clock (Optional clock) > + - description: Watchdog external clock (Optional clock) This is 8 items, not 7 as your top-level property says. > + > + clock-names: > + items: > + - const: pss_ref_clk > + - const: video_clk > + - const: pss_alt_ref_clk > + - const: aux_ref_clk > + - const: gt_crx_ref_clk > + - pattern: "^mio_clk[00-77]+.*$" > + - pattern: "gem[0-3]+_emio_clk.*$" > + - pattern: "swdt[0-1]+_ext_clk.*$" > + Best regards, Krzysztof
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, 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. Thanks, Conor.
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
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 >
On 28/07/2023 18:47, Conor Dooley wrote: > 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. > Yep, select: false. This is some junk code :( Best regards, Krzysztof
[AMD Official Use Only - General] > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, July 28, 2023 12:26 PM > To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; > devicetree@vger.kernel.org > Cc: 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 > > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > On 24/07/2023 13:18, Shubhrajyoti Datta wrote: > > Convert the xlnx,zynqmp-clk.txt to yaml. > > versal-clk.yaml already exists that's why ZynqMP is converted and > > merged. > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > > > + > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - xlnx,zynqmp-clk > > + > > + then: > > + properties: > > + clocks: > > + items: > > + - description: PS reference clock > > + - description: reference clock for video system > > + - description: alternative PS reference clock > > + - description: auxiliary reference clock > > + - description: transceiver reference clock > > + - description: (E)MIO clock source (Optional clock) > > + - description: GEM emio clock (Optional clock) > > + - description: Watchdog external clock (Optional clock) > > This is 8 items, not 7 as your top-level property says. > Fixed it below From: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> Date: Tue, 23 May 2023 12:01:53 +0530 Subject: [PATCH v4] dt-bindings: clock: versal: Convert the xlnx,zynqmp-clk.txt to yaml Convert the xlnx,zynqmp-clk.txt to yaml. versal-clk.yaml already exists that's why ZynqMP is converted and merged. Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> --- Changes in v4: Do not relax the constraints Update the constraints to max as 8 Changes in v3: Update the common constraints Changes in v2: add enum in compatible fix the description add constraints for clocks name the clock-controller1 to clock-controller .../bindings/clock/xlnx,versal-clk.yaml | 80 ++++++++++++++++--- .../bindings/clock/xlnx,zynqmp-clk.txt | 63 --------------- 2 files changed, 71 insertions(+), 72 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt diff --git a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml index e9cf747bf89b..1ed17dfca241 100644 --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml @@ -19,7 +19,9 @@ select: false properties: compatible: oneOf: - - const: xlnx,versal-clk + - enum: + - xlnx,versal-clk + - xlnx,zynqmp-clk - items: - enum: - xlnx,versal-net-clk @@ -31,16 +33,12 @@ properties: 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: 8 clock-names: - items: - - const: ref - - const: alt_ref - - const: pl_alt_ref + minItems: 3 + maxItems: 8 required: - compatible @@ -50,6 +48,61 @@ required: additionalProperties: false +allOf: + - if: + properties: + compatible: + contains: + enum: + - xlnx,versal-clk + + then: + properties: + clocks: + items: + - description: reference clock + - description: alternate reference clock + - description: alternate reference clock for programmable logic + + clock-names: + items: + - const: ref + - const: alt_ref + - const: pl_alt_ref + + - if: + properties: + compatible: + contains: + enum: + - xlnx,zynqmp-clk + + then: + properties: + clocks: + minItems: 5 + items: + - description: PS reference clock + - description: reference clock for video system + - description: alternative PS reference clock + - description: auxiliary reference clock + - description: transceiver reference clock + - description: (E)MIO clock source (Optional clock) + - description: GEM emio clock (Optional clock) + - description: Watchdog external clock (Optional clock) + + clock-names: + minItems: 5 + items: + - const: pss_ref_clk + - const: video_clk + - const: pss_alt_ref_clk + - const: aux_ref_clk + - const: gt_crx_ref_clk + - pattern: "^mio_clk[00-77]+.*$" + - pattern: "gem[0-3]+_emio_clk.*$" + - pattern: "swdt[0-1]+_ext_clk.*$" + examples: - | firmware { @@ -64,4 +117,13 @@ examples: }; }; }; + + clock-controller { + #clock-cells = <1>; + compatible = "xlnx,zynqmp-clk"; + clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, + <&aux_ref_clk>, <>_crx_ref_clk>; + clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk", + "aux_ref_clk", "gt_crx_ref_clk"; + }; ... diff --git a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt b/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt deleted file mode 100644 index 391ee1a60bed..000000000000 --- a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt +++ /dev/null @@ -1,63 +0,0 @@ --------------------------------------------------------------------------- -Device Tree Clock bindings for the Zynq Ultrascale+ MPSoC controlled using -Zynq MPSoC firmware interface --------------------------------------------------------------------------- -The clock controller is a h/w block of Zynq Ultrascale+ MPSoC clock -tree. It reads required input clock frequencies from the devicetree and acts -as clock provider for all clock consumers of PS clocks. - -See clock_bindings.txt for more information on the generic clock bindings. - -Required properties: - - #clock-cells: Must be 1 - - compatible: Must contain: "xlnx,zynqmp-clk" - - clocks: List of clock specifiers which are external input - clocks to the given clock controller. Please refer - the next section to find the input clocks for a - given controller. - - clock-names: List of clock names which are exteral input clocks - to the given clock controller. Please refer to the - clock bindings for more details. - -Input clocks for zynqmp Ultrascale+ clock controller: - -The Zynq UltraScale+ MPSoC has one primary and four alternative reference clock -inputs. These required clock inputs are: - - pss_ref_clk (PS reference clock) - - video_clk (reference clock for video system ) - - pss_alt_ref_clk (alternative PS reference clock) - - aux_ref_clk - - gt_crx_ref_clk (transceiver reference clock) - -The following strings are optional parameters to the 'clock-names' property in -order to provide an optional (E)MIO clock source: - - swdt0_ext_clk - - swdt1_ext_clk - - gem0_emio_clk - - gem1_emio_clk - - gem2_emio_clk - - gem3_emio_clk - - mio_clk_XX # with XX = 00..77 - - mio_clk_50_or_51 #for the mux clock to gem tsu from 50 or 51 - - -Output clocks are registered based on clock information received -from firmware. Output clocks indexes are mentioned in -include/dt-bindings/clock/xlnx-zynqmp-clk.h. - -------- -Example -------- - -firmware { - zynqmp_firmware: zynqmp-firmware { - compatible = "xlnx,zynqmp-firmware"; - method = "smc"; - zynqmp_clk: clock-controller { - #clock-cells = <1>; - compatible = "xlnx,zynqmp-clk"; - clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, <&aux_ref_clk>, <>_crx_ref_clk>; - clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk"; - }; - }; -}; -- 2.17.1
On Tue, Aug 01, 2023 at 07:17:20AM +0000, Datta, Shubhrajyoti wrote: > [AMD Official Use Only - General] > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Sent: Friday, July 28, 2023 12:26 PM > > To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; > > devicetree@vger.kernel.org > > Cc: 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 > > > > Caution: This message originated from an External Source. Use proper > > caution when opening attachments, clicking links, or responding. > > > > > > On 24/07/2023 13:18, Shubhrajyoti Datta wrote: > > > Convert the xlnx,zynqmp-clk.txt to yaml. > > > versal-clk.yaml already exists that's why ZynqMP is converted and > > > merged. > > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > > > > > > + > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > + - xlnx,zynqmp-clk > > > + > > > + then: > > > + properties: > > > + clocks: > > > + items: > > > + - description: PS reference clock > > > + - description: reference clock for video system > > > + - description: alternative PS reference clock > > > + - description: auxiliary reference clock > > > + - description: transceiver reference clock > > > + - description: (E)MIO clock source (Optional clock) > > > + - description: GEM emio clock (Optional clock) > > > + - description: Watchdog external clock (Optional clock) > > > > This is 8 items, not 7 as your top-level property says. > > > Fixed it below Can you send that as a real v4 please?
diff --git a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml index e9cf747bf89b..deebbfd084e8 100644 --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml @@ -19,7 +19,9 @@ select: false properties: compatible: oneOf: - - const: xlnx,versal-clk + - enum: + - xlnx,versal-clk + - xlnx,zynqmp-clk - items: - enum: - xlnx,versal-net-clk @@ -31,16 +33,12 @@ properties: 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 clock-names: - items: - - const: ref - - const: alt_ref - - const: pl_alt_ref + minItems: 3 + maxItems: 7 required: - compatible @@ -50,6 +48,59 @@ required: additionalProperties: false +allOf: + - if: + properties: + compatible: + contains: + enum: + - xlnx,versal-clk + + then: + properties: + clocks: + items: + - description: reference clock + - description: alternate reference clock + - description: alternate reference clock for programmable logic + + clock-names: + items: + - const: ref + - const: alt_ref + - const: pl_alt_ref + + - if: + properties: + compatible: + contains: + enum: + - xlnx,zynqmp-clk + + then: + properties: + clocks: + items: + - description: PS reference clock + - description: reference clock for video system + - description: alternative PS reference clock + - description: auxiliary reference clock + - description: transceiver reference clock + - description: (E)MIO clock source (Optional clock) + - description: GEM emio clock (Optional clock) + - description: Watchdog external clock (Optional clock) + + clock-names: + items: + - const: pss_ref_clk + - const: video_clk + - const: pss_alt_ref_clk + - const: aux_ref_clk + - const: gt_crx_ref_clk + - pattern: "^mio_clk[00-77]+.*$" + - pattern: "gem[0-3]+_emio_clk.*$" + - pattern: "swdt[0-1]+_ext_clk.*$" + examples: - | firmware { @@ -64,4 +115,13 @@ examples: }; }; }; + + clock-controller { + #clock-cells = <1>; + compatible = "xlnx,zynqmp-clk"; + clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, + <&aux_ref_clk>, <>_crx_ref_clk>; + clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk", + "aux_ref_clk", "gt_crx_ref_clk"; + }; ... diff --git a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt b/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt deleted file mode 100644 index 391ee1a60bed..000000000000 --- a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt +++ /dev/null @@ -1,63 +0,0 @@ --------------------------------------------------------------------------- -Device Tree Clock bindings for the Zynq Ultrascale+ MPSoC controlled using -Zynq MPSoC firmware interface --------------------------------------------------------------------------- -The clock controller is a h/w block of Zynq Ultrascale+ MPSoC clock -tree. It reads required input clock frequencies from the devicetree and acts -as clock provider for all clock consumers of PS clocks. - -See clock_bindings.txt for more information on the generic clock bindings. - -Required properties: - - #clock-cells: Must be 1 - - compatible: Must contain: "xlnx,zynqmp-clk" - - clocks: List of clock specifiers which are external input - clocks to the given clock controller. Please refer - the next section to find the input clocks for a - given controller. - - clock-names: List of clock names which are exteral input clocks - to the given clock controller. Please refer to the - clock bindings for more details. - -Input clocks for zynqmp Ultrascale+ clock controller: - -The Zynq UltraScale+ MPSoC has one primary and four alternative reference clock -inputs. These required clock inputs are: - - pss_ref_clk (PS reference clock) - - video_clk (reference clock for video system ) - - pss_alt_ref_clk (alternative PS reference clock) - - aux_ref_clk - - gt_crx_ref_clk (transceiver reference clock) - -The following strings are optional parameters to the 'clock-names' property in -order to provide an optional (E)MIO clock source: - - swdt0_ext_clk - - swdt1_ext_clk - - gem0_emio_clk - - gem1_emio_clk - - gem2_emio_clk - - gem3_emio_clk - - mio_clk_XX # with XX = 00..77 - - mio_clk_50_or_51 #for the mux clock to gem tsu from 50 or 51 - - -Output clocks are registered based on clock information received -from firmware. Output clocks indexes are mentioned in -include/dt-bindings/clock/xlnx-zynqmp-clk.h. - -------- -Example -------- - -firmware { - zynqmp_firmware: zynqmp-firmware { - compatible = "xlnx,zynqmp-firmware"; - method = "smc"; - zynqmp_clk: clock-controller { - #clock-cells = <1>; - compatible = "xlnx,zynqmp-clk"; - clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, <&aux_ref_clk>, <>_crx_ref_clk>; - clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk"; - }; - }; -};
Convert the xlnx,zynqmp-clk.txt to yaml. versal-clk.yaml already exists that's why ZynqMP is converted and merged. Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> --- Changes in v3: Update the min and maxitems Changes in v2: add enum in compatible fix the description add constraints for clocks name the clock-controller1 to clock-controller .../bindings/clock/xlnx,versal-clk.yaml | 78 ++++++++++++++++--- .../bindings/clock/xlnx,zynqmp-clk.txt | 63 --------------- 2 files changed, 69 insertions(+), 72 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.txt