Re: [PATCH v2 2/2] clk: thead: th1520-ap: Correctly refer the parent of c910-i0

[Yao Zi <ziyao@disroot.org>]

On Fri, Jul 11, 2025 at 01:52:15AM -0700, Drew Fustini wrote:
> On Thu, Jul 10, 2025 at 09:21:35AM +0000, Yao Zi wrote:
> > 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.
> > 
> > Refer c910-i0 by its name instead to avoid turning c910 into an orphan
> > clock.
> > 
> > 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> > index 42feb4bb6329..41ed72b1a915 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" },
> 
> Thanks for the patch. Unfortunately, I chatted with Stephen about this
> on irc and we need to avoid using strings in clk_parent_data. I'm trying
> to see how to correctly assign the pointer in the c910_parents[] after
> c910_io_clk has been registered.

If we stop using *_register_mux() for all the muxes, the problem should
go away: the key cause is that *_register_mux() always allocates a new
clk_mux structure, which in turn contains a new clk_hw structure.

We could avoid the ccu_mux structure, instead defining clk_muxes
directly and register them with devm_clk_hw_register(), for example,

	static struct clk_mux c910_i0_clk = {
		.reg		= 0x100,
		.mask		= BIT(0),
		.shift		= 1,
		.hw.init	= CLK_HW_INIT_PARENT_DATA("c910-i0",
							  c910_i0_parents,
							  &clk_mux_ops,
							  0),
	};

	c910_i0_clk.reg += base;
	ret = devm_clk_hw_register(dev, &c910_i0_clk.hw);

(not tested, just for demostration)

Now no new clk_hw structure is created and we could refer to muxes by
its hw member when defining other clocks.

> Thanks,
> Drew

Regards,
Yao Zi

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv