| Message ID | 20250424062154.2999219-17-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Apr 24, 2025 at 08:21:46AM +0200, Dario Binacchi wrote: > Though adding the PLLs to clocks and clock-names properties will break > the ABI, it is required to accurately describe the hardware. Indeed, > the Clock Control Module (CCM) receives clocks from the PLLs and > oscillators and generates clocks for on-chip peripherals. > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > (no changes since v11) > > Changes in v11: > - Fix conflict while rebasing on master > > Changes in v7: > - Add 'Reviewed-by' tag of Krzysztof Kozlowski > > Changes in v6: > - New > > .../bindings/clock/imx8m-clock.yaml | 27 ++++++++++++++----- > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > index 4fec55832702..e83f08abd44c 100644 > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > @@ -29,12 +29,12 @@ properties: > maxItems: 2 > > clocks: > - minItems: 6 > - maxItems: 7 > + minItems: 7 Increasing the minimum entries looks like an ABI break to me. The .dts files not being in linux-next confirms that (from 0 warnings in mainline): arch/arm64/boot/dts/freescale:859:50 122 clock-controller@30380000 (fsl,imx8mm-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short 120 clock-controller@30380000 (fsl,imx8mp-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clocks' is a required property 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clock-names' is a required property 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clocks' is a required property 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clock-names' is a required property 36 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short 36 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short 32 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short 28 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short 26 clock-controller@30380000 (fsl,imx8mn-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clocks' is a required property 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clock-names' is a required property 14 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[44], [45], [46], [47], [48], [49]] is too short 14 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[23], [24], [25], [26], [27], [28]] is too short 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clocks' is a required property 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clock-names' is a required property 12 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[26], [27], [28], [29], [30], [31]] is too short 10 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[38], [39], [40], [41], [42], [43]] is too short 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[20], [21], [22], [23], [24], [25]] is too short 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[28], [29], [30], [31], [32], [33]] is too short 8 bcrmf@1 (brcm,bcm4329-fmac): $nodename:0: 'bcrmf@1' does not match '^wifi(@.*)?$' 6 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[41], [42], [43], [44], [45], [46]] is too short 6 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[43], [44], [45], [46], [47], [48]] is too short 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[40], [41], [42], [43], [44], [45]] is too short 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[36], [37], [38], [39], [40], [41]] is too short 4 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short Please fix the binding or drop what's been applied so far. Rob To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On Fri, May 9, 2025 at 6:25 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Apr 24, 2025 at 08:21:46AM +0200, Dario Binacchi wrote: > > Though adding the PLLs to clocks and clock-names properties will break > > the ABI, it is required to accurately describe the hardware. Indeed, > > the Clock Control Module (CCM) receives clocks from the PLLs and > > oscillators and generates clocks for on-chip peripherals. > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > --- > > > > (no changes since v11) > > > > Changes in v11: > > - Fix conflict while rebasing on master > > > > Changes in v7: > > - Add 'Reviewed-by' tag of Krzysztof Kozlowski > > > > Changes in v6: > > - New > > > > .../bindings/clock/imx8m-clock.yaml | 27 ++++++++++++++----- > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > index 4fec55832702..e83f08abd44c 100644 > > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > @@ -29,12 +29,12 @@ properties: > > maxItems: 2 > > > > clocks: > > - minItems: 6 > > - maxItems: 7 > > + minItems: 7 > > Increasing the minimum entries looks like an ABI break to me. The .dts > files not being in linux-next confirms that (from 0 warnings in > mainline): > > arch/arm64/boot/dts/freescale:859:50 > 122 clock-controller@30380000 (fsl,imx8mm-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short > 120 clock-controller@30380000 (fsl,imx8mp-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short > 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clocks' is a required property > 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clock-names' is a required property > 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clocks' is a required property > 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clock-names' is a required property > 36 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short > 36 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short > 32 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short > 28 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short > 26 clock-controller@30380000 (fsl,imx8mn-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short > 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clocks' is a required property > 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clock-names' is a required property > 14 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[44], [45], [46], [47], [48], [49]] is too short > 14 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[23], [24], [25], [26], [27], [28]] is too short > 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clocks' is a required property > 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clock-names' is a required property > 12 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[26], [27], [28], [29], [30], [31]] is too short > 10 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[38], [39], [40], [41], [42], [43]] is too short > 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short > 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[20], [21], [22], [23], [24], [25]] is too short > 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short > 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[28], [29], [30], [31], [32], [33]] is too short > 8 bcrmf@1 (brcm,bcm4329-fmac): $nodename:0: 'bcrmf@1' does not match '^wifi(@.*)?$' > 6 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[41], [42], [43], [44], [45], [46]] is too short > 6 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short > 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[43], [44], [45], [46], [47], [48]] is too short > 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[40], [41], [42], [43], [44], [45]] is too short > 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[36], [37], [38], [39], [40], [41]] is too short > 4 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short > > Please fix the binding or drop what's been applied so far. Abel and Shawn are already aware of the issue: https://lore.kernel.org/all/CABGWkvqfyH=dcuw6EDZaFVVebj8SZhJF0P944+mmzL5YK3-Pug@mail.gmail.com/ Thanks and regards, Dario > > Rob >
On 25-05-09 20:22:18, Dario Binacchi wrote: > On Fri, May 9, 2025 at 6:25 PM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Apr 24, 2025 at 08:21:46AM +0200, Dario Binacchi wrote: > > > Though adding the PLLs to clocks and clock-names properties will break > > > the ABI, it is required to accurately describe the hardware. Indeed, > > > the Clock Control Module (CCM) receives clocks from the PLLs and > > > oscillators and generates clocks for on-chip peripherals. > > > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > > > --- > > > > > > (no changes since v11) > > > > > > Changes in v11: > > > - Fix conflict while rebasing on master > > > > > > Changes in v7: > > > - Add 'Reviewed-by' tag of Krzysztof Kozlowski > > > > > > Changes in v6: > > > - New > > > > > > .../bindings/clock/imx8m-clock.yaml | 27 ++++++++++++++----- > > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > > index 4fec55832702..e83f08abd44c 100644 > > > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > > @@ -29,12 +29,12 @@ properties: > > > maxItems: 2 > > > > > > clocks: > > > - minItems: 6 > > > - maxItems: 7 > > > + minItems: 7 > > > > Increasing the minimum entries looks like an ABI break to me. The .dts > > files not being in linux-next confirms that (from 0 warnings in > > mainline): > > > > arch/arm64/boot/dts/freescale:859:50 > > 122 clock-controller@30380000 (fsl,imx8mm-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short > > 120 clock-controller@30380000 (fsl,imx8mp-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short > > 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clocks' is a required property > > 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clock-names' is a required property > > 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clocks' is a required property > > 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clock-names' is a required property > > 36 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short > > 36 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short > > 32 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short > > 28 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short > > 26 clock-controller@30380000 (fsl,imx8mn-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short > > 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clocks' is a required property > > 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clock-names' is a required property > > 14 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[44], [45], [46], [47], [48], [49]] is too short > > 14 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[23], [24], [25], [26], [27], [28]] is too short > > 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clocks' is a required property > > 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clock-names' is a required property > > 12 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[26], [27], [28], [29], [30], [31]] is too short > > 10 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[38], [39], [40], [41], [42], [43]] is too short > > 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short > > 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[20], [21], [22], [23], [24], [25]] is too short > > 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short > > 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[28], [29], [30], [31], [32], [33]] is too short > > 8 bcrmf@1 (brcm,bcm4329-fmac): $nodename:0: 'bcrmf@1' does not match '^wifi(@.*)?$' > > 6 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[41], [42], [43], [44], [45], [46]] is too short > > 6 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short > > 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[43], [44], [45], [46], [47], [48]] is too short > > 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[40], [41], [42], [43], [44], [45]] is too short > > 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[36], [37], [38], [39], [40], [41]] is too short > > 4 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short > > > > Please fix the binding or drop what's been applied so far. > > Abel and Shawn are already aware of the issue: > > https://lore.kernel.org/all/CABGWkvqfyH=dcuw6EDZaFVVebj8SZhJF0P944+mmzL5YK3-Pug@mail.gmail.com/ So Shawn suggested I pick up the dts patches from this series as well. I'm waiting for another -next to get merged and if there are still issues, I'll ask Stephen to ignore the pull request I already sent. > > Thanks and regards, > Dario > > > > > Rob > > > > > -- > > Dario Binacchi > > Senior Embedded Linux Developer > > dario.binacchi@amarulasolutions.com > > __________________________________ > > > Amarula Solutions SRL > > Via Le Canevare 30, 31100 Treviso, Veneto, IT > > T. +39 042 243 5310 > info@amarulasolutions.com > > www.amarulasolutions.com To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On 09/05/2025 20:22, Dario Binacchi wrote: > On Fri, May 9, 2025 at 6:25 PM Rob Herring <robh@kernel.org> wrote: >> >> On Thu, Apr 24, 2025 at 08:21:46AM +0200, Dario Binacchi wrote: >>> Though adding the PLLs to clocks and clock-names properties will break >>> the ABI, it is required to accurately describe the hardware. Indeed, >>> the Clock Control Module (CCM) receives clocks from the PLLs and >>> oscillators and generates clocks for on-chip peripherals. >>> >>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> >>> --- >>> >>> (no changes since v11) >>> >>> Changes in v11: >>> - Fix conflict while rebasing on master >>> >>> Changes in v7: >>> - Add 'Reviewed-by' tag of Krzysztof Kozlowski >>> >>> Changes in v6: >>> - New >>> >>> .../bindings/clock/imx8m-clock.yaml | 27 ++++++++++++++----- >>> 1 file changed, 21 insertions(+), 6 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml >>> index 4fec55832702..e83f08abd44c 100644 >>> --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml >>> +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml >>> @@ -29,12 +29,12 @@ properties: >>> maxItems: 2 >>> >>> clocks: >>> - minItems: 6 >>> - maxItems: 7 >>> + minItems: 7 >> >> Increasing the minimum entries looks like an ABI break to me. The .dts >> files not being in linux-next confirms that (from 0 warnings in >> mainline): >> >> arch/arm64/boot/dts/freescale:859:50 >> 122 clock-controller@30380000 (fsl,imx8mm-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >> 120 clock-controller@30380000 (fsl,imx8mp-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >> 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clocks' is a required property >> 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clock-names' is a required property >> 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clocks' is a required property >> 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clock-names' is a required property >> 36 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short >> 36 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short >> 32 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short >> 28 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short >> 26 clock-controller@30380000 (fsl,imx8mn-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >> 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clocks' is a required property >> 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clock-names' is a required property >> 14 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[44], [45], [46], [47], [48], [49]] is too short >> 14 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[23], [24], [25], [26], [27], [28]] is too short >> 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clocks' is a required property >> 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clock-names' is a required property >> 12 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[26], [27], [28], [29], [30], [31]] is too short >> 10 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[38], [39], [40], [41], [42], [43]] is too short >> 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short >> 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[20], [21], [22], [23], [24], [25]] is too short >> 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short >> 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[28], [29], [30], [31], [32], [33]] is too short >> 8 bcrmf@1 (brcm,bcm4329-fmac): $nodename:0: 'bcrmf@1' does not match '^wifi(@.*)?$' >> 6 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[41], [42], [43], [44], [45], [46]] is too short >> 6 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short >> 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[43], [44], [45], [46], [47], [48]] is too short >> 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[40], [41], [42], [43], [44], [45]] is too short >> 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[36], [37], [38], [39], [40], [41]] is too short >> 4 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short >> >> Please fix the binding or drop what's been applied so far. > > Abel and Shawn are already aware of the issue: ... and I objected to that ABI break: https://lore.kernel.org/linux-devicetree/gbymcmoya7dfmedq4nkopqpswh63d2ujxl2elc2x7x325b75bu@anp36sdya43v/ to which you replied "correct hardware description". Now I admit, I should not give the review tag, because that's really poor reason to: 1. break ABI 2. break users 3. break boot as it turns out I have impression that at later stage I actually NAKed entire patchset on the basis of being obsolete... but never mind. I am quite picky reviewer but once I cut people slack, some poor explanation about ABI beak is added to commit msg but no one else cares and it bites back. We keep repeating that it is the platform's maintainer decision but apparently still people don't know it. Quite a lesson for me. Anyway, I am dissapointed, regardless of my review, that this patchset could not actually keep backward compatibility. My recommendation to Abel is to drop entire patchset. If there is going to be any re-submits (v13), please kindly drop my Reviewed-by from this and next patch breaking the ABI. Best regards, Krzysztof To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On 13/05/2025 15:24, Abel Vesa wrote: >>>> clocks: >>>> - minItems: 6 >>>> - maxItems: 7 >>>> + minItems: 7 >>> >>> Increasing the minimum entries looks like an ABI break to me. The .dts >>> files not being in linux-next confirms that (from 0 warnings in >>> mainline): >>> >>> arch/arm64/boot/dts/freescale:859:50 >>> 122 clock-controller@30380000 (fsl,imx8mm-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >>> 120 clock-controller@30380000 (fsl,imx8mp-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >>> 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clocks' is a required property >>> 61 clock-controller@30360000 (fsl,imx8mm-anatop): 'clock-names' is a required property >>> 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clocks' is a required property >>> 60 clock-controller@30360000 (fsl,imx8mp-anatop): 'clock-names' is a required property >>> 36 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short >>> 36 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short >>> 32 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short >>> 28 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short >>> 26 clock-controller@30380000 (fsl,imx8mn-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >>> 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clocks' is a required property >>> 17 clock-controller@30360000 (fsl,imx8mq-anatop): 'clock-names' is a required property >>> 14 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[44], [45], [46], [47], [48], [49]] is too short >>> 14 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[23], [24], [25], [26], [27], [28]] is too short >>> 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clocks' is a required property >>> 13 clock-controller@30360000 (fsl,imx8mn-anatop): 'clock-names' is a required property >>> 12 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[26], [27], [28], [29], [30], [31]] is too short >>> 10 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[38], [39], [40], [41], [42], [43]] is too short >>> 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short >>> 8 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[20], [21], [22], [23], [24], [25]] is too short >>> 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short >>> 8 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[28], [29], [30], [31], [32], [33]] is too short >>> 8 bcrmf@1 (brcm,bcm4329-fmac): $nodename:0: 'bcrmf@1' does not match '^wifi(@.*)?$' >>> 6 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[41], [42], [43], [44], [45], [46]] is too short >>> 6 clock-controller@30380000 (fsl,imx8mn-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short >>> 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[43], [44], [45], [46], [47], [48]] is too short >>> 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[40], [41], [42], [43], [44], [45]] is too short >>> 4 clock-controller@30380000 (fsl,imx8mp-ccm): clocks: [[36], [37], [38], [39], [40], [41]] is too short >>> 4 clock-controller@30380000 (fsl,imx8mm-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short >>> >>> Please fix the binding or drop what's been applied so far. >> >> Abel and Shawn are already aware of the issue: >> >> https://lore.kernel.org/all/CABGWkvqfyH=dcuw6EDZaFVVebj8SZhJF0P944+mmzL5YK3-Pug@mail.gmail.com/ > > So Shawn suggested I pick up the dts patches from this series as well. > Sorry, I am against of it. This patchset breaks the ABI. If platform maintainer is happy with ABI break, it is happy with all the complains from users, all boot failures, all the issues. The solution is not to put DTS into driver's subsystem. The solution would have been not to break ABI, but if that's ok for platform maintainer then live with the results: broken boots. DTS must go via arm-soc DTS branch. > I'm waiting for another -next to get merged and if there are still > issues, I'll ask Stephen to ignore the pull request I already sent. > Best regards, Krzysztof To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On 24/04/2025 08:21, Dario Binacchi wrote: > Though adding the PLLs to clocks and clock-names properties will break > the ABI, it is required to accurately describe the hardware. Indeed, > the Clock Control Module (CCM) receives clocks from the PLLs and > oscillators and generates clocks for on-chip peripherals. > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> For future version if it happens: Un-reviewed. Best regards, Krzysztof To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml index 4fec55832702..e83f08abd44c 100644 --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml @@ -29,12 +29,12 @@ properties: maxItems: 2 clocks: - minItems: 6 - maxItems: 7 + minItems: 7 + maxItems: 10 clock-names: - minItems: 6 - maxItems: 7 + minItems: 7 + maxItems: 10 '#clock-cells': const: 1 @@ -93,6 +93,10 @@ allOf: - description: ext2 clock input - description: ext3 clock input - description: ext4 clock input + - description: audio1 PLL input + - description: audio2 PLL input + - description: dram PLL input + - description: video PLL input clock-names: items: @@ -102,20 +106,31 @@ allOf: - const: clk_ext2 - const: clk_ext3 - const: clk_ext4 + - const: audio_pll1 + - const: audio_pll2 + - const: dram_pll + - const: video_pll additionalProperties: false examples: # Clock Control Module node: - | + #include <dt-bindings/clock/imx8mm-clock.h> + clock-controller@30380000 { compatible = "fsl,imx8mm-ccm"; reg = <0x30380000 0x10000>; #clock-cells = <1>; clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>, - <&clk_ext3>, <&clk_ext4>; + <&clk_ext3>, <&clk_ext4>, + <&anatop IMX8MM_ANATOP_AUDIO_PLL1>, + <&anatop IMX8MM_ANATOP_AUDIO_PLL2>, + <&anatop IMX8MM_ANATOP_DRAM_PLL>, + <&anatop IMX8MM_ANATOP_VIDEO_PLL>; clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2", - "clk_ext3", "clk_ext4"; + "clk_ext3", "clk_ext4", "audio_pll1", "audio_pll2", + "dram_pll", "video_pll"; fsl,operating-mode = "nominal"; };