Re: [PATCH v2 1/2] clk: ti: mux: resolve parent clocks by DT index, not by name

["Mathieu Dubois-Briand" <mathieu.dubois-briand@bootlin.com>]

Hi Geert,

On Wed Aug 19, 2026 at 2:32 PM CEST, Geert Uytterhoeven wrote:
> Hi Mathieu,
>
> On Wed, 19 Aug 2026 at 07:39, Mathieu Dubois-Briand
> <mathieu.dubois-briand@bootlin.com> wrote:
>> On Tue Aug 18, 2026 at 6:02 PM CEST, Geert Uytterhoeven wrote:
>> > On Mon, 27 Jul 2026 at 09:44, Mathieu Dubois-Briand
>> > <mathieu.dubois-briand@bootlin.com> wrote:
>> >> Resolve parent clocks by their index into the device tree "clocks"
>> >> property rather than matching names as strings. Name-based matching is
>> >> fragile because a clock's "clock-output-names" value in its provider
>> >> node can differ from the name used to reference it in a consumer node,
>> >> and because names must be globally unique across all clock providers.
>> >>
>> >> On AM335x, this caused broken clock trees where some clocks failed to
>> >> enable because their parents could not be found.
>> >>
>> >> Replace of_clk_parent_fill() with a clk_parent_data array that sets
>> >> .index to the array position.
>> >>
>> >> Fixes: ec7aa25fa483 ("ARM: dts: Use clock-output-names for am3")
>> >> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
>> >
>> > Thanks for your patch, which is now commit 667f420c09f1417c ("clk: ti:
>> > mux: resolve parent clocks by DT index, not by name") in clk/clk-next.
>> >
>> > This breaks booting BeagleBone Black for me: I don't get any output
>> > on the serial console, even with "earlycon keep_bootcon".  When the
>> > watchdog kicks in (or on kernel panic?), the system reboots.
>>
>> Sorry for that!
>>
>> It's a bit unexpected, as the beaglebone black was one of my test
>> platforms and I didn't see any issue.
>>
>> > Reverting this commit fixes the issue.
>> > After reverting, I added debug code to print all parents, and compared
>> > them to the DTB, but didn't see any discrepancies.
>> >
>> > Your similar change to drivers/clk/ti/composite.c does not cause any
>> > issues for me.
>> >
>> > Do you have a clue?
>> > Thanks!
>>
>> Can you describe what bootloader and device tree you are using for your
>> tests? Any specific configuration?
>>
>> On my side, I have:
>> - BeagleBone Black Wireless A5
>> - am335x-boneblack-wireless.dtb
>> - U-boot 2025.10-ti-g6825d60bea17. Maybe I should try with u-boot master
>>   branch.
>
> BeagleBone Black Rev C3 (by Seeed Studio)
> am335x-boneblack.dtb
> Stock U-Boot SPL 2019.04-00002-g31a8ae0206 (May 13 2020 - 09:26:17 -0500)
>
> I am using my own config.
>
> With omap2plus_defconfig and multi_v7_defconfig, it does boot.  However,
> this commit does have an impact on /sys/kernel/debug/clk/clk_summary:
>
> --- summary-7.2.0-rc2-00106-g0d4d262c1664       2026-08-19
> 10:56:51.622275674 +0200
> +++ summary-7.2.0-rc2-00107-g667f420c09f1       2026-08-19
> 10:58:13.929823890 +0200
> ...
>
> I.e. wdt1_fck changed its parent from clk_rc32k_ck to
> clk-24mhz-clkctrl:0000:0, which is probably not intended?
>

This was definitely not intended, but I believe this is a valid
configuration.

wdt1_fck has two potential parents. In the device tree
(am33xx-clocks.dtsi), we have:

	wdt1_fck: clock-wdt1-fck@538 {
		...
		clocks = <&clk_rc32k_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>;
		...
	};

As I understand it, the kernel is free to select any of the parents
here. Is that correct?

Previously, the link with the second parent was broken, and only the
first parent was known. That was my initial issue that triggered my
changes. Now, both parents can be selected. I assume the second one is
seen as a better option for some reason, and so now it is selected.

But maybe my assumption are wrongs, so feel free correct me here.

> However, that is not the cause of the crash.  My .config has
> CONFIG_INIT_STACK_ALL_PATTERN enabled, so any uninitialized local
> variables contain garbage.
>
>> @@ -136,7 +136,7 @@ static struct clk *_register_mux(struct device_node *node, const char *name,
>>         init.name = name;
>>         init.ops = &ti_clk_mux_ops;
>>         init.flags = flags;
>> -       init.parent_names = parent_names;
>> +       init.parent_data = parent_data;
>>         init.num_parents = num_parents;
>>
>>         /* struct clk_mux assignments
>
> Init is not fully initialized, hence init.parent_names contains garbage
> (before, .parent_data contained garbage).
> As clk_core_populate_parent_map() prioritizes .parent_names over
> .parent_data, the uninitialized .parent_data didn't cause any harm before.
>

Nice catch. I didn't realise that while doing my changes.

Thanks for taking the time to track this issue.

Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com