Re: [PATCH v12 16/19] dt-bindings: clock: imx8m-clock: add PLLs

[Krzysztof Kozlowski <krzk@kernel.org>]

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