Re: [PATCH] clk: thead: th1520-ap: Correctly refer the parent for c910 and osc_12m

[Yao Zi <ziyao@disroot.org>]

On Sat, Jul 05, 2025 at 09:31:29PM -0700, Drew Fustini wrote:
> On Sun, Jul 06, 2025 at 02:07:51AM +0000, Yao Zi wrote:
> > On Sat, Jul 05, 2025 at 05:08:09PM -0700, Drew Fustini wrote:
> > > On Sat, Jul 05, 2025 at 05:20:28AM +0000, Yao Zi wrote:
> > > > clk_orphan_dump shows two suspicious orphan clocks on TH1520 when
> > > > booting the kernel with mainline U-Boot,
> > > > 
> > > > 	$ cat /sys/kernel/debug/clk/clk_orphan_dump | jq 'keys'
> > > > 	[
> > > > 	  "c910",
> > > > 	  "osc_12m"
> > > > 	]
> > > > 
> > > > where the correct parents should be c910-i0 for c910, and osc_24m for
> > > > osc_12m.
> > > 
> > > Thanks for sending this patch. However, I only see "osc_12m" listed in
> > > clk_orphan_dump. I tried the current next, torvalds master and v6.15 but
> > > I didn't ever see "c910" appear [1]. What branch are you using?
> > 
> > I think it has something to do with the bootloader: as you could see in
> > your clk_orphan_dump, the c910 clock is reparented to cpu-pll1, the
> > second possible parent which could be correctly resolved by the CCF,
> > thus c910 doesn't appear in the clk_orphan_dump.
> > 
> > But with the mainline U-Boot which doesn't reparent or reclock c910 on
> > startup, c910 should remain the reset state and take c910-i0 as parent,
> > and appear in the clk_orphan_dump.
> 
> Ah, thanks for the explanation. I'm on an old build:
> 
> U-Boot SPL 2020.01-g55b713fa (Jan 12 2024 - 02:17:34 +0000)
> FM[1] lpddr4x dualrank freq=3733 64bit dbi_off=n sdram init
> U-Boot 2020.01-g55b713fa (Jan 12 2024 - 02:17:34 +0000)
> 
> I would like to run mainline but I have the 8GB RAM LPi4a. Does mainline
> only work for the 16GB version right now?

I only tested the DRAM driver on the 16GiB version, but have seen some
working reports on the 8GiB one. Btw, the mainline U-Boot is still in an
early stage (only MMC/eMMC is working and netboot is still WIP).

> > Another way to confirm the bug is to examine
> > /sys/kernel/debug/clk/c910/clk_possible_parents: without the patch, it
> > should be something like
> > 
> > 	osc_24m cpu-pll1
> > 
> > c910's parents are defined as
> > 
> > 	static const struct clk_parent_data c910_parents[] = {
> > 		{ .hw = &c910_i0_clk.common.hw },
> > 		{ .hw = &cpu_pll1_clk.common.hw }
> > 	};
> > 
> > and the debugfs output looks obviously wrong.
> 
> Thanks, yeah, without the patch I also see:
> 
> ==> c910-i0/clk_possible_parents <==
> cpu-pll0 osc_24m
> 
> > 
> > There's another bug in CCF[1] which causes unresolvable parents are
> > shown as the clock-output-names of the clock controller's first parent
> > in debugfs, explaining the output.
> 
> Thanks for that fix. I now see '(missing)' for c910 too when I apply
> that patch:
> 
> root@lpi4amain:/sys/kernel/debug/clk# head c910/clk_possible_parents
> (missing) cpu-pll1
> 
> > 
> > > I think it would be best for this patch to be split into separate
> > > patches for osc_12m and c910.
> > 
> > Okay, I originally thought these are relatively small fixes targeting
> > a single driver, hence put them together. I'll split it into two patches
> > in v2.
> 
> I think the osc_12m is good as-is but I'm not sure what Stephen will
> think about using the string "c910-i0" in c910_parents[]. I think
> splitting it up will make discussion go faster.

Okay, I'm willing to do so.

> Thanks,
> Drew

Best regards,
Yao Zi