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

[Drew Fustini <fustini@kernel.org>]

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 would be best for this patch to be split into separate
patches for osc_12m and c910.

> The correct parent of c910, c910-i0, is registered with
> devm_clk_hw_register_mux_parent_data_table(), which creates a clk_hw
> structure from scratch. But it's assigned as c910's parent by
> referring &c910_i0_clk.common.hw, confusing the CCF since this clk_hw
> structure is never registered.

I recall Stephen Boyd had the feedback when trying to upstream this
driver to avoid strings for parents and instead use clk_parent_data or
clk_hw pointers directly [2]. It was difficult to find alternitves to
parent strings in all instances.

> Meanwhile, osc_12m refers the external oscillator by setting
> clk_parent_data.fw_name to osc_24m, which is obviously wrong since no
> clock-names property is allowed for compatible thead,th1520-clk-ap.
> 
> For c910, refer c910-i0 by its name; for osc_12m, refer the external
> clock input by index. This eliminates these orphan clocks.
> 
> Fixes: ae81b69fd2b1 ("clk: thead: Add support for T-Head TH1520 AP_SUBSYS clocks")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  drivers/clk/thead/clk-th1520-ap.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index ebfb1d59401d..74da1a61e6f0 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -427,7 +427,7 @@ static struct ccu_mux c910_i0_clk = {
>  };
>  
>  static const struct clk_parent_data c910_parents[] = {
> -	{ .hw = &c910_i0_clk.common.hw },
> +	{ .index = -1, .name = "c910-i0" },

Stephen - would this use of a parent string be acceptable?

>  	{ .hw = &cpu_pll1_clk.common.hw }
>  };
>  
> @@ -582,7 +582,14 @@ static const struct clk_parent_data peri2sys_apb_pclk_pd[] = {
>  	{ .hw = &peri2sys_apb_pclk.common.hw }
>  };
>  
> -static CLK_FIXED_FACTOR_FW_NAME(osc12m_clk, "osc_12m", "osc_24m", 2, 1, 0);
> +struct clk_fixed_factor osc12m_clk = {
> +	.div		= 2,
> +	.mult		= 1,
> +	.hw.init	= CLK_HW_INIT_PARENTS_DATA("osc_12m",
> +						   osc_24m_clk,
> +						   &clk_fixed_factor_ops,
> +						   0),
> +};

I think this hunk is a good fix for osc_12m. I applied the patch and
osc_12m no longer appears in clk_orphan_dump [3]. clk_summary now shows
osc_12m under osc_24m.

>  
>  static const char * const out_parents[] = { "osc_24m", "osc_12m" };
>  
> -- 
> 2.49.0
> 

[1] https://gist.github.com/pdp7/d00f0f4fe3fcf368ce253d606dc7b01f
[2] https://lore.kernel.org/all/91c3373b5b00afc1910b704a16c1ac89.sboyd@kernel.org/
[3] https://gist.github.com/pdp7/30e51ed013d4bedf0c6abc5717e0b6a5