Re: [PATCH 1/8] clk: spacemit: Add support for K1 SoC

[Yao Zi <me@ziyao.cc>]

On Sun, May 10, 2026 at 08:06:23AM -0400, Guodong Xu wrote:
> From: Junhui Liu <junhui.liu@pigmoral.tech>
> 
> The K1 SoC exposes four clock providers in the kernel mainline DT: one
> PLL controller ("spacemit,k1-pll") and three syscon clock nodes
> ("spacemit,k1-syscon-{mpmu,apbc,apmu}"). Register a separate
> U_BOOT_DRIVER for each.
> 
> Inter-controller ordering is enforced where the registers actually
> depend on each other.

What do you mean by "registers actually depend on each other"? Do you
refer to the fact that some APBC clocks are derived from APMU/PLL
controllers thus must be registered after them? If so, would this
series[1] help which allows arbitrary clock registration order and
handles dependencies in CCF instead?

> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
> Signed-off-by: Raymond Mao <raymond.mao@riscstar.com>
> Signed-off-by: Guodong Xu <guodong@riscstar.com>
> ---
>  drivers/clk/Kconfig               |    5 +-
>  drivers/clk/Makefile              |    1 +
>  drivers/clk/spacemit/Kconfig      |   23 +
>  drivers/clk/spacemit/Makefile     |    7 +
>  drivers/clk/spacemit/clk-k1.c     | 1722 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/spacemit/clk_common.h |   79 ++
>  drivers/clk/spacemit/clk_ddn.c    |   93 ++
>  drivers/clk/spacemit/clk_ddn.h    |   53 ++
>  drivers/clk/spacemit/clk_mix.c    |  403 +++++++++
>  drivers/clk/spacemit/clk_mix.h    |  224 +++++
>  drivers/clk/spacemit/clk_pll.c    |  157 ++++
>  drivers/clk/spacemit/clk_pll.h    |   81 ++
>  include/soc/spacemit/k1-syscon.h  |  149 ++++
>  13 files changed, 2995 insertions(+), 2 deletions(-)

...

> diff --git a/drivers/clk/spacemit/Kconfig b/drivers/clk/spacemit/Kconfig
> new file mode 100644
> index 00000000000..03aecefddc4
> --- /dev/null
> +++ b/drivers/clk/spacemit/Kconfig
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (c) 2025, Junhui Liu <junhui.liu@pigmoral.tech>
> +
> +config CLK_SPACEMIT
> +	bool "Clock support for SpacemiT SoCs"
> +	depends on CLK

What about

	depends on CLK || COMPILE_TEST

to allow building-only tests?

> +	select REGMAP
> +	help
> +	  This enables support clock driver for Spacemit SoC
> +	  family.
> +
> +if CLK_SPACEMIT
> +
> +config CLK_SPACEMIT_K1
> +	bool "SpacemiT K1 clock support"
> +	select CLK_CCF
> +	select LIB_RATIONAL

CLK_SPACEMIT instead of CLK_SPACEMIT_K1 should selects LIB_RATIONAL...

> +	help
> +	  This enables support clock driver for Spacemit K1 SoC.
> +	  It's based on Common Clock Framework.
> +
> +endif
> diff --git a/drivers/clk/spacemit/Makefile b/drivers/clk/spacemit/Makefile
> new file mode 100644
> index 00000000000..824e94d1f74
> --- /dev/null
> +++ b/drivers/clk/spacemit/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2025 Junhui Liu <junhui.liu@pigmoral.tech>
> +
> +obj-$(CONFIG_CLK_SPACEMIT) += clk_ddn.o clk_mix.o clk_pll.o

... since the driver actually makes use of rational routines, clk_ddn.o,
is guarded by CONFIG_CLK_SPACEMIT, not CONFIG_CLK_SPACEMIT_K1.

> +obj-$(CONFIG_CLK_SPACEMIT_K1)	+= clk-k1.o
> diff --git a/drivers/clk/spacemit/clk-k1.c b/drivers/clk/spacemit/clk-k1.c
> new file mode 100644
> index 00000000000..4c0972d952e
> --- /dev/null
> +++ b/drivers/clk/spacemit/clk-k1.c

...

> +struct clk_retry_item {
> +	struct ccu_common *common;
> +	struct list_head link;
> +};
> +
> +static LIST_HEAD(retry_list);
> +
> +static int k1_clk_retry_register(void)
> +{
> +	struct clk_retry_item *item, *tmp;
> +	int retries = 5;
> +	int ret;
> +
> +	while (!list_empty(&retry_list) && retries) {
> +		list_for_each_entry_safe(item, tmp, &retry_list, link) {
> +			struct ccu_common *common = item->common;
> +
> +			ret = common->init(common);
> +			if (ret)
> +				return ret;
> +
> +			list_del(&item->link);
> +			kfree(item);
> +		}
> +		retries--;
> +	}
> +
> +	return 0;
> +}

Is retrying for handling dependencies between clocks? Could the series
I mentioned above help? This version looks very hacking...

Another solution might be carefully specify the order in which clocks
are registered to ensure parents are always registered before children.

...

> diff --git a/drivers/clk/spacemit/clk_ddn.c b/drivers/clk/spacemit/clk_ddn.c
> new file mode 100644
> index 00000000000..7b93f30d5c3
> --- /dev/null
> +++ b/drivers/clk/spacemit/clk_ddn.c

...

> +static unsigned long ccu_ddn_calc_best_rate(struct ccu_ddn *ddn,
> +					    unsigned long rate, unsigned long prate,
> +					    unsigned long *num, unsigned long *den)
> +{
> +	rational_best_approximation(rate, prate / ddn->pre_div,
> +				    ddn->den_mask >> ddn->den_shift,
> +				    ddn->num_mask >> ddn->num_shift,
> +				    den, num);

Rational routines are used here.

> +	return ccu_ddn_calc_rate(prate, *num, *den, ddn->pre_div);
> +}

Best regards,
Yao Zi

[1]: https://lore.kernel.org/u-boot/20260120-clk-reparent-v3-0-0d43d4b362ac@outlook.com/