Re: [PATCH v6 3/6] clk: spacemit: Add clock support for SpacemiT K1 SoC

[Haylen Chu]

On Thu, Apr 10, 2025 at 12:55:22AM +0000, Yixun Lan wrote:
> On 17:24 Tue 01 Apr     , Haylen Chu wrote:
> > The clock tree of K1 SoC contains three main types of clock hardware
> > (PLL/DDN/MIX) and has control registers split into several multifunction
> > devices: APBS (PLLs), MPMU, APBC and APMU.
> > 
> > All register operations are done through regmap to ensure atomiciy
> > between concurrent operations of clock driver and reset,
> > power-domain driver that will be introduced in the future.
> > 
> > Signed-off-by: Haylen Chu <heylenay@xxxxxxx>
> > ---
> >  drivers/clk/Kconfig               |    1 +
> >  drivers/clk/Makefile              |    1 +
> >  drivers/clk/spacemit/Kconfig      |   18 +
> >  drivers/clk/spacemit/Makefile     |    5 +
> >  drivers/clk/spacemit/apbc_clks    |  100 +++
> >  drivers/clk/spacemit/ccu-k1.c     | 1316 +++++++++++++++++++++++++++++
> >  drivers/clk/spacemit/ccu_common.h |   48 ++
> >  drivers/clk/spacemit/ccu_ddn.c    |   83 ++
> >  drivers/clk/spacemit/ccu_ddn.h    |   47 ++
> >  drivers/clk/spacemit/ccu_mix.c    |  268 ++++++
> >  drivers/clk/spacemit/ccu_mix.h    |  218 +++++
> >  drivers/clk/spacemit/ccu_pll.c    |  157 ++++
> >  drivers/clk/spacemit/ccu_pll.h    |   86 ++
> >  13 files changed, 2348 insertions(+)
> >  create mode 100644 drivers/clk/spacemit/Kconfig
> >  create mode 100644 drivers/clk/spacemit/Makefile
> >  create mode 100644 drivers/clk/spacemit/apbc_clks
> >  create mode 100644 drivers/clk/spacemit/ccu-k1.c
> >  create mode 100644 drivers/clk/spacemit/ccu_common.h
> >  create mode 100644 drivers/clk/spacemit/ccu_ddn.c
> >  create mode 100644 drivers/clk/spacemit/ccu_ddn.h
> >  create mode 100644 drivers/clk/spacemit/ccu_mix.c
> >  create mode 100644 drivers/clk/spacemit/ccu_mix.h
> >  create mode 100644 drivers/clk/spacemit/ccu_pll.c
> >  create mode 100644 drivers/clk/spacemit/ccu_pll.h
> > 

...

> > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > new file mode 100644
> > index 000000000000..cd95c4f9c127
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/ccu-k1.c
> > @@ -0,0 +1,1316 @@

...

> > +/* APBC clocks start */
> > +static const struct clk_parent_data uart_clk_parents[] = {
> > +	CCU_PARENT_HW(pll1_m3d128_57p6),
> > +	CCU_PARENT_HW(slow_uart1_14p74),
> > +	CCU_PARENT_HW(slow_uart2_48),
> > +};
> > +CCU_MUX_GATE_DEFINE(uart0_clk, uart_clk_parents, APBC_UART1_CLK_RST, 4, 3,
> > +		    BIT(1), CLK_IS_CRITICAL);
> I'd request adding an explict documents for why need CLK_IS_CRITICAL flag
> (there are more place, I won't add comments)
> 
> Can you check this one? I think it's probably not necessary here,
> I can understand your concern of afraid of serial console breakage once clk
> driver merged, since we already enabled uart driver and using a dummy clk.. 
> 
> I think we probably could handle this carefully, sending an incrimental
> patch of uart to enable clk along with clk merged..

Yes, I've seen Alex's series on adding bus clocks to UART nodes and
could then depend on the series and add the correct UART clocks in
devicetree, then CLK_IS_CRITICAL to uart0_clk and uart0_bus_clk could go
away.

For other places applying CLK_IS_CRITICAL: it should be unnecessary for
cpu_c1_hi_clk, which is only a possible parent of CPU cluster 1's clock.
cci550_clk, cpu_c0_{core,ace,tcm}_clk and cpu_c1_{core,ace}_clk are
clocks for CPU cores. I think there's no good way to describe the
dependency in devicetree for now as we're lacking of a proper CPUfreq
driver, so I'd like to keep them as is (and may add a comment).

If there's a better way to handle these CPU clocks, I'd like to remove
the CLK_IS_CRITICAL flag as well.

> [...]
> -- 
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55

Thanks,
Haylen Chu