Re: [PATCH v1] dts: arm64: freescale: move imx9*-clock.h imx9*-power.h into dt-bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/09/2025 13:07, E Shattow wrote:
> 
> 
> On 9/1/25 03:54, Krzysztof Kozlowski wrote:
>> On 01/09/2025 12:30, Marek Vasut wrote:
>>> On 9/1/25 5:33 AM, Krzysztof Kozlowski wrote:
>>>> On 01/09/2025 04:22, Marek Vasut wrote:
>>>>> On 9/1/25 5:22 AM, Peng Fan wrote:
>>>>>> On Sun, Aug 31, 2025 at 01:04:45PM -0700, E Shattow wrote:
>>>>>>> Move imx9*-{clock,power}.h headers into
>>>>>>> include/dt-bindings/{clock,power}/ and fix up the DTs
>>>>>>
>>>>>> No. The files should be under arch/arm64/boot/dts/freescale/
>>>>> Why ? Linux already has include/dt-bindings/clock/ and
>>>>> include/dt-bindings/power directories for exactly those headers , why
>>>>> did iMX9 suddenly start conflating them into arch/arm64/boot/dts/freescale ?
>>>>
>>>>
>>>> Because maybe these are not bindings?
>>>
>>> Please compare arch/arm64/boot/dts/freescale/imx95-clock.h and 
>>> include/dt-bindings/clock/imx8mp-clock.h and clarify to me, why the 
>>> imx95-clock.h is not bindings and the imx8mp-clock.h is bindings.
>>
>> That's uno reverse card. I do not have to prove why these are different.
>> You need to prove why imx95 are bindings.
>>
>>>
>>> Both files list clock IDs for the clock nodes, one clock one is SCMI 
>>> clock (iMX95), the other clock node is CCM clock (iMX8MP), and they are 
>>
>> Yeah, entirely different things. Like comparing apples and oranges.
>>
>>> both (SCMI and CCM) clock nodes in DT. Both header files may have to be 
>>> included in drivers, the iMX8MP headers already are, the iMX95 headers 
>>
>> No, the SCMI cannot be used in the drivers, because these are not
>> abstract IDs mapping between driver and DTS.
>>
>>> currently are included only in U-Boot drivers.
>>>
>>> I really don't see the difference here, sorry.
>>
>> You just pointed out difference - no usage in drivers, no ABI!
>>
>> Instead of playing this "I found this code somewhere, so I can do
>> whatever the same" answer the first implied question - why these are
>> bindings? Provide arguments what do they bind.
>>
>>>
>>>> Regardless whether you agree or
>>>> not, the commit should clearly explain the reason behind.
>>> Which commit ?
>>
>> This patch.
>>
>>
>> Best regards,
>> Krzysztof
> 
> Providing some clarification, the user of this (in U-Boot) via
> devicetree-rebasing leads to some path gymnastics:
> 
> (U-Boot code base) arch/arm/mach-imx/imx9/scmi/clock.c:9:#include
> "../../../../../dts/upstream/src/arm64/freescale/imx95-clock.h"

Why nothing like this was described in the commit msg? We really do not
need lengthy discussions to discover the basic needs why patch is being
sent.

Anyway, U-boot driver requesting clocks in C code not via DTS is the
culprit here and not necessarily right approach.

> 
> Compared to the patterns of what else is going on I would guess this
> should instead be:
> 
> #include <dt-bindings/clock/imx95-clock.h>
> 
> which agrees with how it is done for all the other closely similar build
> targets and is less fragile in that build context (although you may not
> be interested in this since it's not Linux kernel code base). Ideally
> I'm trying to make U-Boot flexible to build against different locations
> of devicetree-rebasing or perhaps Linux kernel source tree. This is a
> "one thing is not like the others" moment and it was suggested I send a
> patch.

Why instead not fixing U-boot and not taking these via DT property, like
every other case? see include/clk.h for the API.

> 
> Whatever is best I'm open to improve the commit message, or take what
> you think ought to be fixed in U-Boot there to improve.

You should rather align with other U-boot maintainers and contributors
how clock consumers should be written. Proposed patches depend on that
outcome. If you just ask me, then I am telling you the same - requesting
clocks by consumers should be via DT, not via C code. Someone just added
ABI for driver consumers, not even providers...

Best regards,
Krzysztof