On 4/19/25 2:59 PM, Adam Ford wrote:
On Fri, Apr 18, 2025 at 9:53 AM Marek Vasut <ma...@denx.de> wrote:
On 4/17/25 7:11 PM, Fabio Estevam wrote:
From: Fabio Estevam <feste...@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@30380000 clock-names
property:
dts/upstream/src/arm64/freescale/imx8mp.dtsi
745 clk: clock-controller@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@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@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@30380000 node clock-names and
clocks . And from there on, it is always on phandle to &osc_24m , right ?
