[PATCH v2 0/4] clk: spacemit: fix i2s clock

[PATCH v2 0/4] clk: spacemit: fix i2s clock

[Troy Mitchell]

Previously, the driver defined two clocks for the I2S controller:
i2s_bclk and its parent i2s_sysclk.

Both i2s_bclk and i2s_sysclk were treated as fixed-factor clocks,
which clearly does not reflect the practical requirements for I2S operation.

Additionally, the original driver overlooked some upstream clock sources.

To fix the I2S clock, this series also introduces several new clock definition macros.

The I2S clock hierarchy can be found here [1].

Link:
https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb
[1]

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changes in v2:
- change the macro for i2s_sysclk_src to CCU_MUX_GATE_DEFINE
- introduce factor for div clock
- modify commit messages
- remove CCU_DDN_GATE_DEFINE
- remove CCU_DIV_TABLE_GATE_DEFINE
- remove reformatting in k1-syscon.h
- split patch2/2 into separate patches
- Link to v1: https://lore.kernel.org/r/20250807-k1-clk-i2s-generation-v1-0-7dc25eb4e4d3@linux.spacemit.com

---
Troy Mitchell (4):
      dt-bindings: clock: spacemit: introduce i2s pre-clock to fix i2s clock
      clk: spacemit: introduce pre-div for ddn clock
      clk: spacemit: introduce factor for div clock
      clk: spacemit: fix i2s clock

 drivers/clk/spacemit/ccu-k1.c                  | 28 +++++++++++++++++++++-----
 drivers/clk/spacemit/ccu_ddn.c                 | 12 +++++------
 drivers/clk/spacemit/ccu_ddn.h                 |  6 ++++--
 drivers/clk/spacemit/ccu_mix.c                 |  7 ++++++-
 drivers/clk/spacemit/ccu_mix.h                 |  4 +++-
 include/dt-bindings/clock/spacemit,k1-syscon.h |  3 +++
 include/soc/spacemit/k1-syscon.h               |  1 +
 7 files changed, 46 insertions(+), 15 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250804-k1-clk-i2s-generation-eee7049ee17a

Best regards,
-- 
Troy Mitchell <troy.mitchell@linux.spacemit.com>

[PATCH v2 1/4] dt-bindings: clock: spacemit: introduce i2s pre-clock to fix i2s clock

[Troy Mitchell]

Previously, the K1 clock driver did not include the parent clocks of
the I2S sysclk.

Otherwise, the I2S clock may not work as expected.

This patch adds their definitions to allow proper registration
in the driver and usage in the device tree.

Fixes: 1b72c59db0add ("clk: spacemit: Add clock support for SpacemiT K1 SoC")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 include/dt-bindings/clock/spacemit,k1-syscon.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h
index 2714c3fe66cd5b49e12c8b20689f5b01da36b774..524268246fedd3f1238320f35244baf32354fbd2 100644
--- a/include/dt-bindings/clock/spacemit,k1-syscon.h
+++ b/include/dt-bindings/clock/spacemit,k1-syscon.h
@@ -77,6 +77,9 @@
 #define CLK_I2S_BCLK		30
 #define CLK_APB			31
 #define CLK_WDT_BUS		32
+#define CLK_I2S_153P6		33
+#define CLK_I2S_153P6_BASE	34
+#define CLK_I2S_SYSCLK_SRC	35
 
 /* MPMU resets */
 #define RESET_WDT		0

-- 
2.50.1

[PATCH v2 2/4] clk: spacemit: introduce pre-div for ddn clock

[Troy Mitchell]

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.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/clk/spacemit/ccu_ddn.c | 12 ++++++------
 drivers/clk/spacemit/ccu_ddn.h |  6 ++++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/spacemit/ccu_ddn.c b/drivers/clk/spacemit/ccu_ddn.c
index be311b045698e95a688a35858a8ac1bcfbffd2c7..06d86748182bd1959cdab5c18d0a882ee25dcade 100644
--- a/drivers/clk/spacemit/ccu_ddn.c
+++ b/drivers/clk/spacemit/ccu_ddn.c
@@ -22,21 +22,21 @@
 
 #include "ccu_ddn.h"
 
-static unsigned long ccu_ddn_calc_rate(unsigned long prate,
-				       unsigned long num, unsigned long den)
+static unsigned long ccu_ddn_calc_rate(unsigned long prate, unsigned long num,
+				       unsigned long den, unsigned int pre_div)
 {
-	return prate * den / 2 / num;
+	return prate * den / pre_div / num;
 }
 
 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 / 2,
+	rational_best_approximation(rate, prate / ddn->pre_div,
 				    ddn->den_mask >> ddn->den_shift,
 				    ddn->num_mask >> ddn->num_shift,
 				    den, num);
-	return ccu_ddn_calc_rate(prate, *num, *den);
+	return ccu_ddn_calc_rate(prate, *num, *den, ddn->pre_div);
 }
 
 static long ccu_ddn_round_rate(struct clk_hw *hw, unsigned long rate,
@@ -58,7 +58,7 @@ static unsigned long ccu_ddn_recalc_rate(struct clk_hw *hw, unsigned long prate)
 	num = (val & ddn->num_mask) >> ddn->num_shift;
 	den = (val & ddn->den_mask) >> ddn->den_shift;
 
-	return ccu_ddn_calc_rate(prate, num, den);
+	return ccu_ddn_calc_rate(prate, num, den, ddn->pre_div);
 }
 
 static int ccu_ddn_set_rate(struct clk_hw *hw, unsigned long rate,
diff --git a/drivers/clk/spacemit/ccu_ddn.h b/drivers/clk/spacemit/ccu_ddn.h
index a52fabe77d62eba16426867a9c13481e72f025c0..4838414a8e8dc04af49d3b8d39280efedbd75616 100644
--- a/drivers/clk/spacemit/ccu_ddn.h
+++ b/drivers/clk/spacemit/ccu_ddn.h
@@ -18,13 +18,14 @@ struct ccu_ddn {
 	unsigned int num_shift;
 	unsigned int den_mask;
 	unsigned int den_shift;
+	unsigned int pre_div;
 };
 
 #define CCU_DDN_INIT(_name, _parent, _flags) \
 	CLK_HW_INIT_HW(#_name, &_parent.common.hw, &spacemit_ccu_ddn_ops, _flags)
 
 #define CCU_DDN_DEFINE(_name, _parent, _reg_ctrl, _num_shift, _num_width,	\
-		       _den_shift, _den_width, _flags)				\
+		       _den_shift, _den_width, _pre_div, _flags)		\
 static struct ccu_ddn _name = {							\
 	.common = {								\
 		.reg_ctrl	= _reg_ctrl,					\
@@ -33,7 +34,8 @@ static struct ccu_ddn _name = {							\
 	.num_mask	= GENMASK(_num_shift + _num_width - 1, _num_shift),	\
 	.num_shift	= _num_shift,						\
 	.den_mask	= GENMASK(_den_shift + _den_width - 1, _den_shift),	\
-	.den_shift	= _den_shift,					\
+	.den_shift	= _den_shift,						\
+	.pre_div	= _pre_div,						\
 }
 
 static inline struct ccu_ddn *hw_to_ccu_ddn(struct clk_hw *hw)

-- 
2.50.1

[PATCH v2 3/4] clk: spacemit: introduce factor for div clock

[Troy Mitchell]

From the definition of register, The i2s_bclk is a non-linear,
discrete divider clock.

The following table shows the correspondence between index
and frequency division coefficients:

| index |  div  |
|-------|-------|
|   0   |   2   |
|   1   |   4   |
|   2   |   6   |
|   3   |   8   |

But from a software perspective and this table, dividing the
actual div value by 2 is sufficient to obtain a continuous
divider clock.

Rather than introducing a new clock type to handle this case,
a factor parameter has been added to CCU_DIV_GATE_DEFINE.

Suggested-by: Haylen Chu <heylenay@4d2.org>
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/clk/spacemit/ccu_mix.c | 7 ++++++-
 drivers/clk/spacemit/ccu_mix.h | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
index 9b852aa61f78aed5256bfe6fc3b01932d6db6256..dbd2cf234bf81d8e110b19868ff9af7373e2ab55 100644
--- a/drivers/clk/spacemit/ccu_mix.c
+++ b/drivers/clk/spacemit/ccu_mix.c
@@ -56,7 +56,10 @@ static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
 	val = ccu_read(&mix->common, ctrl) >> div->shift;
 	val &= (1 << div->width) - 1;
 
-	return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width);
+	if (!div->factor)
+		return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width);
+
+	return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width) / div->factor;
 }
 
 /*
@@ -115,6 +118,8 @@ ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate,
 
 		for (int j = 1; j <= div_max; j++) {
 			unsigned long tmp = DIV_ROUND_CLOSEST_ULL(parent_rate, j);
+			if (mix->div.factor)
+				tmp /= mix->div.factor;
 
 			if (abs(tmp - rate) < abs(best_rate - rate)) {
 				best_rate = tmp;
diff --git a/drivers/clk/spacemit/ccu_mix.h b/drivers/clk/spacemit/ccu_mix.h
index 54d40cd39b2752260f57d2a96eb8d3eed8116ecd..7dd00d24ec4b1dab70663b9cb7b9ebb02abeaecb 100644
--- a/drivers/clk/spacemit/ccu_mix.h
+++ b/drivers/clk/spacemit/ccu_mix.h
@@ -34,6 +34,7 @@ struct ccu_mux_config {
 struct ccu_div_config {
 	u8 shift;
 	u8 width;
+	unsigned int factor;
 };
 
 struct ccu_mix {
@@ -130,10 +131,11 @@ static struct ccu_mix _name = {							\
 }
 
 #define CCU_DIV_GATE_DEFINE(_name, _parent, _reg_ctrl, _shift, _width,		\
-			    _mask_gate,	_flags)					\
+			    _mask_gate,	_factor, _flags)			\
 static struct ccu_mix _name = {							\
 	.gate	= CCU_GATE_INIT(_mask_gate),					\
 	.div	= CCU_DIV_INIT(_shift, _width),					\
+	.div.factor = _factor,						\
 	.common = {								\
 		.reg_ctrl	= _reg_ctrl,					\
 		CCU_MIX_INITHW(_name, _parent, spacemit_ccu_div_gate_ops,	\

-- 
2.50.1

[PATCH v2 4/4] clk: spacemit: fix i2s clock

[Troy Mitchell]

Defining i2s_bclk and i2s_sysclk as fixed-factor clocks is insufficient
for real I2S use cases.

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-related clock registers can be found here [1].

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    | 28 +++++++++++++++++++++++-----
 include/soc/spacemit/k1-syscon.h |  1 +
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
index 65e6de030717afa60eefab7bda88f9a13b857650..3a885d64fb09144bb0d40024fea9415d66eba01b 100644
--- a/drivers/clk/spacemit/ccu-k1.c
+++ b/drivers/clk/spacemit/ccu-k1.c
@@ -136,13 +136,28 @@ CCU_GATE_DEFINE(pll1_d3_819p2, CCU_PARENT_HW(pll1_d3), MPMU_ACGR, BIT(14), 0);
 CCU_GATE_DEFINE(pll1_d2_1228p8, CCU_PARENT_HW(pll1_d2), MPMU_ACGR, BIT(16), 0);
 
 CCU_GATE_DEFINE(slow_uart, CCU_PARENT_NAME(osc), MPMU_ACGR, BIT(1), CLK_IGNORE_UNUSED);
-CCU_DDN_DEFINE(slow_uart1_14p74, pll1_d16_153p6, MPMU_SUCCR, 16, 13, 0, 13, 0);
-CCU_DDN_DEFINE(slow_uart2_48, pll1_d4_614p4, MPMU_SUCCR_1, 16, 13, 0, 13, 0);
+CCU_DDN_DEFINE(slow_uart1_14p74, pll1_d16_153p6, MPMU_SUCCR, 16, 13, 0, 13, 2, 0);
+CCU_DDN_DEFINE(slow_uart2_48, pll1_d4_614p4, MPMU_SUCCR_1, 16, 13, 0, 13, 2, 0);
 
 CCU_GATE_DEFINE(wdt_clk, CCU_PARENT_HW(pll1_d96_25p6), MPMU_WDTPCR, BIT(1), 0);
 
-CCU_FACTOR_GATE_DEFINE(i2s_sysclk, CCU_PARENT_HW(pll1_d16_153p6), MPMU_ISCCR, BIT(31), 50, 1);
-CCU_FACTOR_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s_sysclk), MPMU_ISCCR, BIT(29), 1, 1);
+CCU_FACTOR_DEFINE(i2s_153p6, CCU_PARENT_HW(pll1_d8_307p2), 2, 1);
+
+static const struct clk_parent_data i2s_153p6_base_parents[] = {
+	CCU_PARENT_HW(i2s_153p6),
+	CCU_PARENT_HW(pll1_d8_307p2),
+};
+CCU_MUX_DEFINE(i2s_153p6_base, i2s_153p6_base_parents, MPMU_FCCR, 29, 1, 0);
+
+static const struct clk_parent_data i2s_sysclk_src_parents[] = {
+	CCU_PARENT_HW(pll1_d96_25p6),
+	CCU_PARENT_HW(i2s_153p6_base)
+};
+CCU_MUX_GATE_DEFINE(i2s_sysclk_src, i2s_sysclk_src_parents, MPMU_ISCCR, 30, 1, BIT(31), 0);
+
+CCU_DDN_DEFINE(i2s_sysclk, i2s_sysclk_src, MPMU_ISCCR, 0, 15, 15, 12, 1, 0);
+
+CCU_DIV_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s_sysclk), MPMU_ISCCR, 27, 2, BIT(29), 2, 0);
 
 static const struct clk_parent_data apb_parents[] = {
 	CCU_PARENT_HW(pll1_d96_25p6),
@@ -639,7 +654,7 @@ static const struct clk_parent_data emmc_parents[] = {
 CCU_MUX_DIV_GATE_FC_DEFINE(emmc_clk, emmc_parents, APMU_PMUA_EM_CLK_RES_CTRL, 8, 3, BIT(11),
 			   6, 2, BIT(4), 0);
 CCU_DIV_GATE_DEFINE(emmc_x_clk, CCU_PARENT_HW(pll1_d2_1228p8), APMU_PMUA_EM_CLK_RES_CTRL, 12,
-		    3, BIT(15), 0);
+		    3, BIT(15), 1, 0);
 
 static const struct clk_parent_data audio_parents[] = {
 	CCU_PARENT_HW(pll1_aud_245p7),
@@ -756,6 +771,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..354751562c55523ef8a22be931ddd8aca9651084 100644
--- a/include/soc/spacemit/k1-syscon.h
+++ b/include/soc/spacemit/k1-syscon.h
@@ -30,6 +30,7 @@ to_spacemit_ccu_adev(struct auxiliary_device *adev)
 
 /* MPMU register offset */
 #define MPMU_POSR			0x0010
+#define MPMU_FCCR			0x0008
 #define  POSR_PLL1_LOCK			BIT(27)
 #define  POSR_PLL2_LOCK			BIT(28)
 #define  POSR_PLL3_LOCK			BIT(29)

-- 
2.50.1

Re: [PATCH v2 4/4] clk: spacemit: fix i2s clock

[kernel test robot]

Hi Troy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8f5ae30d69d7543eee0d70083daf4de8fe15d585]

url:    https://github.com/intel-lab-lkp/linux/commits/Troy-Mitchell/dt-bindings-clock-spacemit-introduce-i2s-pre-clock-to-fix-i2s-clock/20250811-221114
base:   8f5ae30d69d7543eee0d70083daf4de8fe15d585
patch link:    https://lore.kernel.org/r/20250811-k1-clk-i2s-generation-v2-4-e4d3ec268b7a%40linux.spacemit.com
patch subject: [PATCH v2 4/4] clk: spacemit: fix i2s clock
config: riscv-randconfig-r112-20250813 (https://download.01.org/0day-ci/archive/20250814/202508140828.p7rvt41N-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce: (https://download.01.org/0day-ci/archive/20250814/202508140828.p7rvt41N-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508140828.p7rvt41N-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/clk/spacemit/ccu-k1.c:160:1: sparse: sparse: Initializer entry defined twice
   drivers/clk/spacemit/ccu-k1.c:160:1: sparse:   also defined here
   drivers/clk/spacemit/ccu-k1.c:656:1: sparse: sparse: Initializer entry defined twice
   drivers/clk/spacemit/ccu-k1.c:656:1: sparse:   also defined here

vim +160 drivers/clk/spacemit/ccu-k1.c

   159	
 > 160	CCU_DIV_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s_sysclk), MPMU_ISCCR, 27, 2, BIT(29), 2, 0);
   161	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/4] dt-bindings: clock: spacemit: introduce i2s pre-clock to fix i2s clock

[Krzysztof Kozlowski]

On Mon, Aug 11, 2025 at 10:04:27PM +0800, Troy Mitchell wrote:
> Previously, the K1 clock driver did not include the parent clocks of
> the I2S sysclk.
> 
> Otherwise, the I2S clock may not work as expected.
> 
> This patch adds their definitions to allow proper registration
> in the driver and usage in the device tree.
> 
> Fixes: 1b72c59db0add ("clk: spacemit: Add clock support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  include/dt-bindings/clock/spacemit,k1-syscon.h | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 2/4] clk: spacemit: introduce pre-div for ddn clock

[Haylen Chu]

On Mon, Aug 11, 2025 at 10:04:28PM +0800, Troy Mitchell wrote:
> 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.
> 
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/clk/spacemit/ccu_ddn.c | 12 ++++++------
>  drivers/clk/spacemit/ccu_ddn.h |  6 ++++--
>  2 files changed, 10 insertions(+), 8 deletions(-)

The code change looks good to me, but

> diff --git a/drivers/clk/spacemit/ccu_ddn.h b/drivers/clk/spacemit/ccu_ddn.h
> index a52fabe77d62eba16426867a9c13481e72f025c0..4838414a8e8dc04af49d3b8d39280efedbd75616 100644
> --- a/drivers/clk/spacemit/ccu_ddn.h
> +++ b/drivers/clk/spacemit/ccu_ddn.h
> @@ -18,13 +18,14 @@ struct ccu_ddn {
>  	unsigned int num_shift;
>  	unsigned int den_mask;
>  	unsigned int den_shift;
> +	unsigned int pre_div;
>  };
>  
>  #define CCU_DDN_INIT(_name, _parent, _flags) \
>  	CLK_HW_INIT_HW(#_name, &_parent.common.hw, &spacemit_ccu_ddn_ops, _flags)
>  
>  #define CCU_DDN_DEFINE(_name, _parent, _reg_ctrl, _num_shift, _num_width,	\
> -		       _den_shift, _den_width, _flags)				\
> +		       _den_shift, _den_width, _pre_div, _flags)		\

You changed the definition of CCU_DDN_DEFINE without adjusting consumers
of this macro. If I'm correct, this creates a build failure.

>  static struct ccu_ddn _name = {							\
>  	.common = {								\
>  		.reg_ctrl	= _reg_ctrl,					\
> @@ -33,7 +34,8 @@ static struct ccu_ddn _name = {							\
>  	.num_mask	= GENMASK(_num_shift + _num_width - 1, _num_shift),	\
>  	.num_shift	= _num_shift,						\
>  	.den_mask	= GENMASK(_den_shift + _den_width - 1, _den_shift),	\
> -	.den_shift	= _den_shift,					\
> +	.den_shift	= _den_shift,						\
> +	.pre_div	= _pre_div,						\
>  }
>  
>  static inline struct ccu_ddn *hw_to_ccu_ddn(struct clk_hw *hw)
> 
> -- 
> 2.50.1
> 

Best regards,
Haylen Chu

Re: [PATCH v2 3/4] clk: spacemit: introduce factor for div clock

[Haylen Chu]

On Mon, Aug 11, 2025 at 10:04:29PM +0800, Troy Mitchell wrote:
> From the definition of register, The i2s_bclk is a non-linear,
> discrete divider clock.
> 
> The following table shows the correspondence between index
> and frequency division coefficients:
> 
> | index |  div  |
> |-------|-------|
> |   0   |   2   |
> |   1   |   4   |
> |   2   |   6   |
> |   3   |   8   |
> 
> But from a software perspective and this table, dividing the
> actual div value by 2 is sufficient to obtain a continuous
> divider clock.
> 
> Rather than introducing a new clock type to handle this case,
> a factor parameter has been added to CCU_DIV_GATE_DEFINE.

Actually, I expected you to represent the factor simply with
CCU_FACTOR_DEFINE, introducing no new clock-calculation code...

> Suggested-by: Haylen Chu <heylenay@4d2.org>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/clk/spacemit/ccu_mix.c | 7 ++++++-
>  drivers/clk/spacemit/ccu_mix.h | 4 +++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
> index 9b852aa61f78aed5256bfe6fc3b01932d6db6256..dbd2cf234bf81d8e110b19868ff9af7373e2ab55 100644
> --- a/drivers/clk/spacemit/ccu_mix.c
> +++ b/drivers/clk/spacemit/ccu_mix.c
> @@ -56,7 +56,10 @@ static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>  	val = ccu_read(&mix->common, ctrl) >> div->shift;
>  	val &= (1 << div->width) - 1;
>  
> -	return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width);
> +	if (!div->factor)
> +		return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width);

Please adapt all div-related macros to make them assign one to the
factor, which helps you get rid of this if and the one in
ccu_mix_calc_best_rate().

> +	return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width) / div->factor;
>  }
>  
>  /*
> @@ -115,6 +118,8 @@ ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate,
>  
>  		for (int j = 1; j <= div_max; j++) {
>  			unsigned long tmp = DIV_ROUND_CLOSEST_ULL(parent_rate, j);
> +			if (mix->div.factor)
			---- this if ------

> +				tmp /= mix->div.factor;
>  
>  			if (abs(tmp - rate) < abs(best_rate - rate)) {
>  				best_rate = tmp;
> diff --git a/drivers/clk/spacemit/ccu_mix.h b/drivers/clk/spacemit/ccu_mix.h
> index 54d40cd39b2752260f57d2a96eb8d3eed8116ecd..7dd00d24ec4b1dab70663b9cb7b9ebb02abeaecb 100644
> --- a/drivers/clk/spacemit/ccu_mix.h
> +++ b/drivers/clk/spacemit/ccu_mix.h
> @@ -34,6 +34,7 @@ struct ccu_mux_config {
>  struct ccu_div_config {
>  	u8 shift;
>  	u8 width;
> +	unsigned int factor;
>  };
>  
>  struct ccu_mix {
> @@ -130,10 +131,11 @@ static struct ccu_mix _name = {							\
>  }
>  
>  #define CCU_DIV_GATE_DEFINE(_name, _parent, _reg_ctrl, _shift, _width,		\
> -			    _mask_gate,	_flags)					\
> +			    _mask_gate,	_factor, _flags)			\

This isn't that consistent: why could only divider-gate come with a
factor? This is another reason why I think representing the factor
separately with the CCU_FACTOR_DEFINE() macro is better.

>  static struct ccu_mix _name = {							\
>  	.gate	= CCU_GATE_INIT(_mask_gate),					\
>  	.div	= CCU_DIV_INIT(_shift, _width),					\
> +	.div.factor = _factor,						\
>  	.common = {								\
>  		.reg_ctrl	= _reg_ctrl,					\
>  		CCU_MIX_INITHW(_name, _parent, spacemit_ccu_div_gate_ops,	\
> 
> -- 
> 2.50.1
> 

Best regards,
Haylen Chu

Re: [PATCH v2 2/4] clk: spacemit: introduce pre-div for ddn clock

[Troy Mitchell]

On Mon, Aug 18, 2025 at 02:04:39AM +0000, Haylen Chu wrote:
> On Mon, Aug 11, 2025 at 10:04:28PM +0800, Troy Mitchell wrote:
> > 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.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/clk/spacemit/ccu_ddn.c | 12 ++++++------
> >  drivers/clk/spacemit/ccu_ddn.h |  6 ++++--
> >  2 files changed, 10 insertions(+), 8 deletions(-)
Hi Haylen,
Thanks for ur review.

> 
> The code change looks good to me, but
> 
> > diff --git a/drivers/clk/spacemit/ccu_ddn.h b/drivers/clk/spacemit/ccu_ddn.h
> > index a52fabe77d62eba16426867a9c13481e72f025c0..4838414a8e8dc04af49d3b8d39280efedbd75616 100644
> > --- a/drivers/clk/spacemit/ccu_ddn.h
> > +++ b/drivers/clk/spacemit/ccu_ddn.h
> > @@ -18,13 +18,14 @@ struct ccu_ddn {
> >  	unsigned int num_shift;
> >  	unsigned int den_mask;
> >  	unsigned int den_shift;
> > +	unsigned int pre_div;
> >  };
> >  
> >  #define CCU_DDN_INIT(_name, _parent, _flags) \
> >  	CLK_HW_INIT_HW(#_name, &_parent.common.hw, &spacemit_ccu_ddn_ops, _flags)
> >  
> >  #define CCU_DDN_DEFINE(_name, _parent, _reg_ctrl, _num_shift, _num_width,	\
> > -		       _den_shift, _den_width, _flags)				\
> > +		       _den_shift, _den_width, _pre_div, _flags)		\
> 
> You changed the definition of CCU_DDN_DEFINE without adjusting consumers
> of this macro. If I'm correct, this creates a build failure.
So I need to adjust consumers in same patch?

> 
> >  static struct ccu_ddn _name = {							\
> >  	.common = {								\
> >  		.reg_ctrl	= _reg_ctrl,					\
> > @@ -33,7 +34,8 @@ static struct ccu_ddn _name = {							\
> >  	.num_mask	= GENMASK(_num_shift + _num_width - 1, _num_shift),	\
> >  	.num_shift	= _num_shift,						\
> >  	.den_mask	= GENMASK(_den_shift + _den_width - 1, _den_shift),	\
> > -	.den_shift	= _den_shift,					\
> > +	.den_shift	= _den_shift,						\
> > +	.pre_div	= _pre_div,						\
> >  }
> >  
> >  static inline struct ccu_ddn *hw_to_ccu_ddn(struct clk_hw *hw)
> > 
> > -- 
> > 2.50.1
> > 
> 
> Best regards,
> Haylen Chu
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] clk: spacemit: introduce factor for div clock

[Troy Mitchell]

On Mon, Aug 18, 2025 at 02:20:03AM +0000, Haylen Chu wrote:
> On Mon, Aug 11, 2025 at 10:04:29PM +0800, Troy Mitchell wrote:
> > From the definition of register, The i2s_bclk is a non-linear,
> > discrete divider clock.
> > 
> > The following table shows the correspondence between index
> > and frequency division coefficients:
> > 
> > | index |  div  |
> > |-------|-------|
> > |   0   |   2   |
> > |   1   |   4   |
> > |   2   |   6   |
> > |   3   |   8   |
> > 
> > But from a software perspective and this table, dividing the
> > actual div value by 2 is sufficient to obtain a continuous
> > divider clock.
> > 
> > Rather than introducing a new clock type to handle this case,
> > a factor parameter has been added to CCU_DIV_GATE_DEFINE.
> 
> Actually, I expected you to represent the factor simply with
> CCU_FACTOR_DEFINE, introducing no new clock-calculation code...
I'll change it in the next version.

> 
> > Suggested-by: Haylen Chu <heylenay@4d2.org>
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/clk/spacemit/ccu_mix.c | 7 ++++++-
> >  drivers/clk/spacemit/ccu_mix.h | 4 +++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
> > index 9b852aa61f78aed5256bfe6fc3b01932d6db6256..dbd2cf234bf81d8e110b19868ff9af7373e2ab55 100644
> > --- a/drivers/clk/spacemit/ccu_mix.c
> > +++ b/drivers/clk/spacemit/ccu_mix.c
> > @@ -56,7 +56,10 @@ static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
> >  	val = ccu_read(&mix->common, ctrl) >> div->shift;
> >  	val &= (1 << div->width) - 1;
> >  
> > -	return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width);
> > +	if (!div->factor)
> > +		return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width);
> 
> Please adapt all div-related macros to make them assign one to the
> factor, which helps you get rid of this if and the one in
> ccu_mix_calc_best_rate().
> 
> > +	return divider_recalc_rate(hw, parent_rate, val, NULL, 0, div->width) / div->factor;
> >  }
> >  
> >  /*
> > @@ -115,6 +118,8 @@ ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate,
> >  
> >  		for (int j = 1; j <= div_max; j++) {
> >  			unsigned long tmp = DIV_ROUND_CLOSEST_ULL(parent_rate, j);
> > +			if (mix->div.factor)
> 			---- this if ------
> 
> > +				tmp /= mix->div.factor;
> >  
> >  			if (abs(tmp - rate) < abs(best_rate - rate)) {
> >  				best_rate = tmp;
> > diff --git a/drivers/clk/spacemit/ccu_mix.h b/drivers/clk/spacemit/ccu_mix.h
> > index 54d40cd39b2752260f57d2a96eb8d3eed8116ecd..7dd00d24ec4b1dab70663b9cb7b9ebb02abeaecb 100644
> > --- a/drivers/clk/spacemit/ccu_mix.h
> > +++ b/drivers/clk/spacemit/ccu_mix.h
> > @@ -34,6 +34,7 @@ struct ccu_mux_config {
> >  struct ccu_div_config {
> >  	u8 shift;
> >  	u8 width;
> > +	unsigned int factor;
> >  };
> >  
> >  struct ccu_mix {
> > @@ -130,10 +131,11 @@ static struct ccu_mix _name = {							\
> >  }
> >  
> >  #define CCU_DIV_GATE_DEFINE(_name, _parent, _reg_ctrl, _shift, _width,		\
> > -			    _mask_gate,	_flags)					\
> > +			    _mask_gate,	_factor, _flags)			\
> 
> This isn't that consistent: why could only divider-gate come with a
> factor? This is another reason why I think representing the factor
> separately with the CCU_FACTOR_DEFINE() macro is better.
> 
> >  static struct ccu_mix _name = {							\
> >  	.gate	= CCU_GATE_INIT(_mask_gate),					\
> >  	.div	= CCU_DIV_INIT(_shift, _width),					\
> > +	.div.factor = _factor,						\
> >  	.common = {								\
> >  		.reg_ctrl	= _reg_ctrl,					\
> >  		CCU_MIX_INITHW(_name, _parent, spacemit_ccu_div_gate_ops,	\
> > 
> > -- 
> > 2.50.1
> > 
> 
> Best regards,
> Haylen Chu
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv