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

[Adam Ford]

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.
>
> ...
>
> 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. 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.

adam