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

[Marek Vasut]

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.

...

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.