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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_dBN6b8LfeMq1gz@ketchup>
Date: Thu, 10 Apr 2025 03:55:35 +0000
From: Haylen Chu <heylenay@....org>
To: Yixun Lan <dlan@...too.org>
Cc: Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Haylen Chu <heylenay@...look.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
	linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	spacemit@...ts.linux.dev, Inochi Amaoto <inochiama@...look.com>,
	Chen Wang <unicornxdotw@...mail.com>,
	Jisheng Zhang <jszhang@...nel.org>,
	Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>
Subject: Re: [PATCH v6 3/6] clk: spacemit: Add clock support for SpacemiT K1
 SoC

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