Re: [PATCH 2/2] clk: spacemit: introduce i2s pre-clock and fix i2s clock

[Haylen Chu <heylenay@4d2.org>]

On Thu, Aug 07, 2025 at 09:30:11AM +0800, Troy Mitchell wrote:
> Defining i2s_bclk and i2s_sysclk as fixed-rate clocks is insufficient
> for real I2S use cases.

This is a little misleading: they're modeled as gates with fixed-factor
for now whose rate is calculated from their parents instead of defined
statically. You could avoid possible confusion by replacing "fixed-rate"
with "fixed-factor".

> Moreover, the current I2S clock configuration does not work as expected
> due to missing parent clocks.
> 
> This patch adds the missing parent clocks, defines i2s_sysclk as
> a DDN clock, and i2s_bclk as a DIV clock.
> 
> The i2s_sysclk behaves as an M/N fractional divider in hardware,
> with an additional gate control.
> 
> To properly model this, CCU_DDN_GATE_DEFINE is introduced.

Could it be represented as a DDN clock taking a gate as parent? Just
like what is described in the published clock diagram. Generally I'd
like to avoid introducing more clock types, there're already a lot.

> The original DDN operations applied an implicit divide-by-2, which should
> not be a default behavior.
> 
> This patch removes that assumption, letting each clock define its
> actual behavior explicitly.
> 
> The i2s_bclk is a non-linear, discrete divider clock.
> The previous macro only supported linear dividers,
> so CCU_DIV_TABLE_GATE_DEFINE is introduced to support
> the hardware accurately.

The divider IS linear, from the perspective of functionality, it just
implies a 1/2 factor. Could it be represented as an usual divider and a
1/2 fixed factor?

> The I2S-related clock registers can be found here [1].

So this patch actually does four separate things,

- Introduce a gate-capable variant of DDN clocks
- Make the pre-divider of DDN clocks flexible
- Support looking up mappings between register values and divisors
  through a table when calculating rates of dividers
- Fix the definition of i2s_bclk and i2s_sysclk

IMHO it's better to split them into separate patches for clearness.

> Link:
> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb
> [1]
> 
> Fixes: 1b72c59db0add ("clk: spacemit: Add clock support for SpacemiT K1 SoC")
> Co-developer: Jinmei Wei <weijinmei@linux.spacemit.com>
> Signed-off-by: Jinmei Wei <weijinmei@linux.spacemit.com>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/clk/spacemit/ccu-k1.c     | 34 ++++++++++++++++++++++++----
>  drivers/clk/spacemit/ccu_common.h | 13 +++++++++++
>  drivers/clk/spacemit/ccu_ddn.c    | 47 ++++++++++++++++++++++++++++++++++-----
>  drivers/clk/spacemit/ccu_ddn.h    | 25 +++++++++++++++++++--
>  drivers/clk/spacemit/ccu_mix.c    | 47 +++++++++++++++++++++++++++++----------
>  drivers/clk/spacemit/ccu_mix.h    | 26 +++++++++++++---------
>  include/soc/spacemit/k1-syscon.h  |  7 +++---
>  7 files changed, 161 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index 65e6de030717afa60eefab7bda88f9a13b857650..a6773d4c2ff32d270e1f09b0d93cfff727ea98fa 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
> @@ -136,13 +136,36 @@ CCU_GATE_DEFINE(pll1_d3_819p2, CCU_PARENT_HW(pll1_d3), MPMU_ACGR, BIT(14), 0);

...

> +static const struct clk_div_table i2s_bclk_div_table[] = {
> +	{ .val = 0, .div = 2 },
> +	{ .val = 1, .div = 4 },
> +	{ .val = 2, .div = 6 },
> +	{ .val = 3, .div = 8 },
> +	{ /* sentinel */ },
> +};

This is just a normal 0-based divider if you divide the divisor by 2.

> +CCU_DIV_TABLE_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s_sysclk), MPMU_ISCCR,
> +			  27, 2, i2s_bclk_div_table, BIT(29), 0);
>  
>  static const struct clk_parent_data apb_parents[] = {
>  	CCU_PARENT_HW(pll1_d96_25p6),
> @@ -756,6 +779,9 @@ static struct clk_hw *k1_ccu_mpmu_hws[] = {
>  	[CLK_I2S_BCLK]		= &i2s_bclk.common.hw,
>  	[CLK_APB]		= &apb_clk.common.hw,
>  	[CLK_WDT_BUS]		= &wdt_bus_clk.common.hw,
> +	[CLK_I2S_153P6]		= &i2s_153p6.common.hw,
> +	[CLK_I2S_153P6_BASE]	= &i2s_153p6_base.common.hw,
> +	[CLK_I2S_SYSCLK_SRC]	= &i2s_sysclk_src.common.hw,
>  };
>  
>  static const struct spacemit_ccu_data k1_ccu_mpmu_data = {

...

> diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
> index c59bd7a38e5b4219121341b9c0d9ffda13a9c3e2..253db8a602fe43a1109e2ba248af11109c7baa22 100644
> --- a/include/soc/spacemit/k1-syscon.h
> +++ b/include/soc/spacemit/k1-syscon.h
> @@ -29,10 +29,11 @@ to_spacemit_ccu_adev(struct auxiliary_device *adev)
>  #define APBS_PLL3_SWCR3			0x12c
>  
>  /* MPMU register offset */
> +#define MPMU_FCCR			0x0008
>  #define MPMU_POSR			0x0010
> -#define  POSR_PLL1_LOCK			BIT(27)
> -#define  POSR_PLL2_LOCK			BIT(28)
> -#define  POSR_PLL3_LOCK			BIT(29)
> +#define POSR_PLL1_LOCK			BIT(27)
> +#define POSR_PLL2_LOCK			BIT(28)
> +#define POSR_PLL3_LOCK			BIT(29)

This reformatting doesn't seem related to the patch.

>  #define MPMU_SUCCR			0x0014
>  #define MPMU_ISCCR			0x0044
>  #define MPMU_WDTPCR			0x0200
> 
> -- 
> 2.50.1
> 

Best regards,
Haylen Chu