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

[Inochi Amaoto]

On Thu, Apr 10, 2025 at 01:16:11AM +0000, Yixun Lan wrote:
> Hi Inochi,
> 
> On 08:57 Thu 10 Apr     , Inochi Amaoto wrote:
> > On Thu, Apr 10, 2025 at 08:54:51AM +0800, Inochi Amaoto wrote:
> > > On Thu, Apr 10, 2025 at 12:37:56AM +0000, Yixun Lan wrote:
> > > > On 14:37 Tue 08 Apr     , Alex Elder wrote:
> > > > > On 4/1/25 12:24 PM, 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 at 4d2.org>
> > > > > 
> > > > > I have a few more comments here but I think this is getting very
> > > > > close to ready.  You addressed pretty much everything I mentioned.
> > > > > 
> > > > > > ---
> > > > > >   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/Kconfig b/drivers/clk/Kconfig
> > > > > > index 713573b6c86c..19c1ed280fd7 100644
> > > > > > --- a/drivers/clk/Kconfig
> > > > > > +++ b/drivers/clk/Kconfig
> > > > > > @@ -517,6 +517,7 @@ source "drivers/clk/samsung/Kconfig"
> > > > > >   source "drivers/clk/sifive/Kconfig"
> > > > > >   source "drivers/clk/socfpga/Kconfig"
> > > > > >   source "drivers/clk/sophgo/Kconfig"
> > > > > > +source "drivers/clk/spacemit/Kconfig"
> > > > > >   source "drivers/clk/sprd/Kconfig"
> > > > > >   source "drivers/clk/starfive/Kconfig"
> > > > > >   source "drivers/clk/sunxi/Kconfig"
> > > > > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > > > > > index bf4bd45adc3a..42867cd37c33 100644
> > > > > > --- a/drivers/clk/Makefile
> > > > > > +++ b/drivers/clk/Makefile
> > > > > > @@ -145,6 +145,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
> > > > > >   obj-$(CONFIG_CLK_SIFIVE)		+= sifive/
> > > > > >   obj-y					+= socfpga/
> > > > > >   obj-y					+= sophgo/
> > > > > > +obj-y					+= spacemit/
> > > > > >   obj-$(CONFIG_PLAT_SPEAR)		+= spear/
> > > > > >   obj-y					+= sprd/
> > > > > >   obj-$(CONFIG_ARCH_STI)			+= st/
> > > > > > diff --git a/drivers/clk/spacemit/Kconfig b/drivers/clk/spacemit/Kconfig
> > > > > > new file mode 100644
> > > > > > index 000000000000..4c4df845b3cb
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/clk/spacemit/Kconfig
> > > > > > @@ -0,0 +1,18 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-only
> > > > > > +
> > > > > > +config SPACEMIT_CCU
> > > > > > +	tristate "Clock support for SpacemiT SoCs"
> > > > > 
> > > > > I don't know the answer to this, but...  Should this be a Boolean
> > > > > rather than tristate?  Can a SpacemiT K1 SoC function without the
> > > > > clock driver built in to the kernel?
> > > > > 
> > > > I agree to make it a Boolean, we've already made pinctrl driver Boolean
> > > > and pinctrl depend on clk, besides, the SoC is unlikely functional
> > > > without clock built in as it's such critical..
> > > > 
> > > 
> > > I disagree. The kernel is only for spacemit only, and the pinctrl
> > 
> > Sorry for a mistake, this first "only" should be "not".
> > 
> > > should also be a module. It is the builder's right to decide whether
> > > the driver is builtin or a module. In this view, you should always
> > > allow the driver to be built as a module if possible.
> > > 
> No, we've already made pinctrl as Boolean (still depend on ARCH_SPACEMIT)
> if people don't want this feature, he/she can disable CONFIG_ARCH_SPACEMIT
> 
> https://github.com/torvalds/linux/blob/master/drivers/pinctrl/spacemit/Kconfig#L7

I have a question for this. Does the minimum system can boot without
pinctrl (I mean purge the pinctrl node in dts and use uart only)? If
so, why you treat it a must? In some cases, probe fail without pinctrl
driver is a expected behavior. And if the pin config can be optional,
it will be better to fix the driver and the pinctrl framework, but
not to just convert the module as built-in. It is a waste of space.

Regards,
Inochi