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

From: Drew Fustini <fustini@kernel.org>
To: Yao Zi <ziyao@disroot.org>
Cc: Guo Ren <guoren@kernel.org>, Fu Wei <wefu@redhat.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Jisheng Zhang <jszhang@kernel.org>,
	Yangtao Li <frank.li@vivo.com>,
	linux-riscv@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: thead: th1520-ap: Correctly refer the parent for c910 and osc_12m
Date: Sat, 5 Jul 2025 21:31:29 -0700	[thread overview]
Message-ID: <aGn8IVkWQJjHMfCT@x1> (raw)
In-Reply-To: <aGnaZjMoWbW_FZfj@pie>

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?

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

Thanks,
Drew