[PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/19/25 2:59 PM, Adam Ford wrote:
> On Fri, Apr 18, 2025 at 9:53 AM Marek Vasut <marex at denx.de> wrote:
>>
>> On 4/17/25 7:11 PM, Fabio Estevam wrote:
>>> From: Fabio Estevam <festevam at denx.de>
>>>
>>> Currently, fixed-rate clocks in U-Boot are named based on their devicetree
>>> node names. For example, given the following node:
>>>
>>> osc_24m: clock-osc-24m {
>>>        compatible = "fixed-clock";
>>>        #clock-cells = <0>;
>>>        clock-frequency = <24000000>;
>>>        clock-output-names = "osc_24m";
>>> };
>>>
>>> U-Boot registers the clock as "clock-osc-24m", derived from the node name,
>>> ignoring the clock-output-names property.
>>>
>>> This differs from Linux, which uses the clock-output-names property when
>>> assigning clock names. As a result, clock consumers expecting names
>>> like "osc_24m" (as defined in the property) may fail to resolve clocks
>>> correctly in U-Boot.
>>>
>>> Update the fixed-rate clock driver to check for the clock-output-names
>>> property and use it as the clock name if present. If not, the fallback
>>> remains the node name. This makes U-Boot behavior consistent with Linux.
>>
>> This part above ^ is fine.
>>
>> This part below v is wrong .
>>
>>> One concrete impact is on i.MX8MP, where USB clock lookup failed following
>>> commit b4734c9c333b ("clk: imx: Convert clock-osc-* back to osc_*").
>>>
>>> With this change applied, fixed-clocks are correctly registered with their
>>> expected names:
>>>
>>> u-boot=> clk dump
>>>    Rate               Usecnt      Name
>>> ------------------------------------------
>>>    32768                0        |-- osc_32k
>>>    24000000             5        |-- osc_24m
>>>
>>> This change restores i.MX8MP USB functionality by ensuring the proper clock
>>> names are used.
>>
>> This is wrong, because this is NOT how the clock look up works at all.
>>
>> The lookup works this way. Take for example:
>>
>> drivers/clk/imx/clk-imx8mp.c
>>
>> 240         clk_dm(IMX8MP_ARM_PLL_REF_SEL, imx_clk_mux(dev,
>> "arm_pll_ref_sel", base + 0x84, 0, 2, pll_ref_sels,
>> ARRAY_SIZE(pll_ref_sels)));
>>
>> pll_ref_sels:
>>
>>    21 static const char * const pll_ref_sels[] = { "osc_24m", "dummy",
>> "dummy", "dummy", };
>>
>> the "osc_24m" is looked up in the clock-controller at 30380000 clock-names
>> property:
>>
>> dts/upstream/src/arm64/freescale/imx8mp.dtsi
>>
>>    745 clk: clock-controller at 30380000 {
>> ...
>>    751         clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
>>    752                  <&clk_ext3>, <&clk_ext4>;
>>    753         clock-names = "osc_32k", "osc_24m", ...
>>                                          ^^^^^^^
>>
>> The offset of osc_24m in that clock-names property is 1 , so the phandle
>> in clocks property array at offset 1 is used to look up those clock:
>>
>>    745 clk: clock-controller at 30380000 {
>> ...
>>    751         clocks = <&osc_32k>, <&osc_24m>,
>>                                      ^^^^^^^^
>>
>> Which leads to this clock node in DT:
>>
>>    188         osc_24m: clock-osc-24m {
>>                ^^^^^^^^
>>    189                 compatible = "fixed-clock";
>>    190                 #clock-cells = <0>;
>>
>> The "clock-output-names" is NEVER used for any clock look up.
> 
> For what it's worth, I remove the "clock-out-names" from the device
> tree in Linux, and the Linux driver properly coped with it, but I am
> curious to know what the point of this entry is.

DT schema dtschema/schemas/clock/clock.yaml says:

  82   clock-output-names:
  83     description: |
  84       Recommended to be a list of strings of clock output signal
  85       names indexed by the first cell in the clock specifier.
  86       However, the meaning of clock-output-names is domain
  87       specific to the clock provider, and is only provided to
  88       encourage using the same meaning for the majority of clock
  89       providers.  This format may not work for clock providers
  90       using a complex clock specifier format.  In those cases it
  91       is recommended to omit this property and create a binding
  92       specific names property.
  93
  94       Clock consumer nodes must never directly reference
  95       the provider\'s clock-output-names property.

>>
>> ...
>>
>> I am fine with the first half of the description, but the second half is
>> wrong and this actually does not fix anything. Simply remove
>> clock-output-names from the osc_24m and the code will be broken again.
>>
>> The clk_resolve_parent_clk() is meant to perform the aforementioned
>> resolution(), that is what "[PATCH v2 00/24] clk: Add
>> clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
>> It seems there is something still missing and clk_resolve_parent_clk()
>> does not do the resolution properly for some clock, but that is what
>> needs to be understood and fixed, not worked around this way.
> 
> I added some debug code to clk_resolve_parent_clk it does return
> osc_24m as the name of the clock, but when searching for index, it
> appears to fail.

What exactly fails and where does it fail ?

> Do you have any suggestion on how searching for the
> index can query up the chain to the parent CCM driver on how to return
> the reference to proper clock?  The clock-names property is at the CCM
> parent and not inside the children, and the number of levels vary
> depending on the child clocks, you can't assume the parent's parent
> has the 'clock-name's property.

It is always the clock-controller at 30380000 node to which the 
drivers/clk/imx/clk-imx8mp.c is bound , so drivers/clk/imx/clk-imx8mp.c 
does look up in the clock-controller at 30380000 node clock-names and 
clocks . And from there on, it is always on phandle to &osc_24m , right ?