[PATCH 0/2] clk: spacemit: fix i2s clock

[PATCH 0/2] 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-rate 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>
---
Troy Mitchell (2):
      dt-bindings: clock: spacemit: introduce i2s pre-clock
      clk: spacemit: introduce i2s pre-clock and fix i2s clock

 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/dt-bindings/clock/spacemit,k1-syscon.h |  3 ++
 include/soc/spacemit/k1-syscon.h               |  7 ++--
 8 files changed, 164 insertions(+), 38 deletions(-)
---
base-commit: e991acf1bce7a428794514cbbe216973c9c0a3c8
change-id: 20250804-k1-clk-i2s-generation-eee7049ee17a

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

[PATCH 1/2] dt-bindings: clock: spacemit: introduce i2s pre-clock

[Troy Mitchell]

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

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

Re: [PATCH 1/2] dt-bindings: clock: spacemit: introduce i2s pre-clock

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On Thu, Aug 07, 2025 at 09:30:10AM +0800, Troy Mitchell wrote:
> Previously, the K1 clock driver did not include the parent clocks of
> the I2S sysclk.
> 
> 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")

I'm not sure that a fixes tag is suitable here, seems to be some new
feature/development. The driver change seems to talk about how modelling
as a fixed-clock is incorrect, but this change mentions none of that.
Without whatever context that provides, it's hard to see how this can be
justified as a fix rather than a new feature being added.

> 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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: clock: spacemit: introduce i2s pre-clock

[Troy Mitchell]

On Thu, Aug 07, 2025 at 05:19:51PM +0100, Conor Dooley wrote:
> On Thu, Aug 07, 2025 at 09:30:10AM +0800, Troy Mitchell wrote:
> > Previously, the K1 clock driver did not include the parent clocks of
> > the I2S sysclk.
> > 
> > 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")
> 
> I'm not sure that a fixes tag is suitable here, seems to be some new
> feature/development. The driver change seems to talk about how modelling
> as a fixed-clock is incorrect, but this change mentions none of that.
> Without whatever context that provides, it's hard to see how this can be
> justified as a fix rather than a new feature being added.

Introduce pre-clock to fix I2S clock; otherwise, the I2S clock may not work as expected.

I'll explain it in next version.
> 
> > 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 2/2] clk: spacemit: introduce i2s pre-clock and fix i2s clock

[Troy Mitchell]

Defining i2s_bclk and i2s_sysclk as fixed-rate 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_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.

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 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     | 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);
 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_DEFINE(i2s_sysclk_src, i2s_sysclk_src_parents, MPMU_ISCCR, 30, 1, 0);
+
+CCU_DDN_GATE_DEFINE(i2s_sysclk, i2s_sysclk_src, MPMU_ISCCR, 0, 15, 15, 12, BIT(31), 1, 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 */ },
+};
+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/drivers/clk/spacemit/ccu_common.h b/drivers/clk/spacemit/ccu_common.h
index da72f3836e0b5c3482ffbb32d9d0f7f9185bfb54..ebe225dcfaee9ae79915a0d0f6dcbe8929038b3a 100644
--- a/drivers/clk/spacemit/ccu_common.h
+++ b/drivers/clk/spacemit/ccu_common.h
@@ -31,11 +31,24 @@ struct ccu_common {
 	struct clk_hw hw;
 };
 
+
+/**
+ * struct ccu_gate_config - Gate configuration
+ *
+ * @mask:	Mask to enable the gate. Some clocks may have more than one bit
+ *		set in this field.
+ */
+struct ccu_gate_config {
+	u32 mask;
+};
+
 static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
 {
 	return container_of(hw, struct ccu_common, hw);
 }
 
+#define CCU_GATE_INIT(_mask)		{ .mask = _mask }
+
 #define ccu_read(c, reg)						\
 	({								\
 		u32 tmp;						\
diff --git a/drivers/clk/spacemit/ccu_ddn.c b/drivers/clk/spacemit/ccu_ddn.c
index be311b045698e95a688a35858a8ac1bcfbffd2c7..c2b19dcb1b35c5e2092e5930e53236443f1355a9 100644
--- a/drivers/clk/spacemit/ccu_ddn.c
+++ b/drivers/clk/spacemit/ccu_ddn.c
@@ -22,21 +22,46 @@
 
 #include "ccu_ddn.h"
 
-static unsigned long ccu_ddn_calc_rate(unsigned long prate,
-				       unsigned long num, unsigned long den)
+static void ccu_gate_disable(struct clk_hw *hw)
 {
-	return prate * den / 2 / num;
+	struct ccu_ddn *ddn  = hw_to_ccu_ddn(hw);
+
+	ccu_update(&ddn->common, ctrl, ddn->gate.mask, 0);
+}
+
+static int ccu_gate_enable(struct clk_hw *hw)
+{
+	struct ccu_ddn *ddn = hw_to_ccu_ddn(hw);
+	struct ccu_gate_config *gate = &ddn->gate;
+
+	ccu_update(&ddn->common, ctrl, gate->mask, gate->mask);
+
+	return 0;
+}
+
+static int ccu_gate_is_enabled(struct clk_hw *hw)
+{
+	struct ccu_ddn *ddn = hw_to_ccu_ddn(hw);
+	struct ccu_gate_config *gate = &ddn->gate;
+
+	return (ccu_read(&ddn->common, ctrl) & gate->mask) == gate->mask;
+}
+
+static unsigned long ccu_ddn_calc_rate(unsigned long prate, unsigned long num,
+				       unsigned long den, unsigned int pre_div)
+{
+	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 +83,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,
@@ -81,3 +106,13 @@ const struct clk_ops spacemit_ccu_ddn_ops = {
 	.round_rate	= ccu_ddn_round_rate,
 	.set_rate	= ccu_ddn_set_rate,
 };
+
+const struct clk_ops spacemit_ccu_ddn_gate_ops = {
+	.disable	= ccu_gate_disable,
+	.enable		= ccu_gate_enable,
+	.is_enabled	= ccu_gate_is_enabled,
+
+	.recalc_rate	= ccu_ddn_recalc_rate,
+	.round_rate	= ccu_ddn_round_rate,
+	.set_rate	= ccu_ddn_set_rate,
+};
diff --git a/drivers/clk/spacemit/ccu_ddn.h b/drivers/clk/spacemit/ccu_ddn.h
index a52fabe77d62eba16426867a9c13481e72f025c0..e44fb93065ed99041003f235a4f6f42233020298 100644
--- a/drivers/clk/spacemit/ccu_ddn.h
+++ b/drivers/clk/spacemit/ccu_ddn.h
@@ -13,18 +13,22 @@
 #include "ccu_common.h"
 
 struct ccu_ddn {
+	struct ccu_gate_config gate;
 	struct ccu_common common;
 	unsigned int num_mask;
 	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_GATE_INIT(_name, _parent, _flags) \
+	CLK_HW_INIT_HW(#_name, &_parent.common.hw, &spacemit_ccu_ddn_gate_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 +37,23 @@ 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,						\
+}
+
+#define CCU_DDN_GATE_DEFINE(_name, _parent, _reg_ctrl, _num_shift, _num_width,		\
+			    _den_shift, _den_width, _mask_gate, _pre_div, _flags)	\
+static struct ccu_ddn _name = {								\
+	.gate	= CCU_GATE_INIT(_mask_gate),						\
+	.common = {									\
+		.reg_ctrl	= _reg_ctrl,						\
+		.hw.init	= CCU_DDN_GATE_INIT(_name, _parent, _flags),		\
+	},										\
+	.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,							\
+	.pre_div	= _pre_div,							\
 }
 
 static inline struct ccu_ddn *hw_to_ccu_ddn(struct clk_hw *hw)
@@ -44,5 +64,6 @@ static inline struct ccu_ddn *hw_to_ccu_ddn(struct clk_hw *hw)
 }
 
 extern const struct clk_ops spacemit_ccu_ddn_ops;
+extern const struct clk_ops spacemit_ccu_ddn_gate_ops;
 
 #endif
diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
index 9b852aa61f78aed5256bfe6fc3b01932d6db6256..183f56ccb92a63ab5f41fe093836bebc73e4162f 100644
--- a/drivers/clk/spacemit/ccu_mix.c
+++ b/drivers/clk/spacemit/ccu_mix.c
@@ -56,7 +56,7 @@ 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);
+	return divider_recalc_rate(hw, parent_rate, val, div->table, 0, div->width);
 }
 
 /*
@@ -92,6 +92,28 @@ static int ccu_factor_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
+static void ccu_mix_try_update_best(unsigned long tmp_rate, unsigned long rate,
+				    unsigned long *best_rate, u32 tmp_val,
+				    u32 *div_val, struct clk_hw *parent,
+				    struct clk_hw **best_parent,
+				    unsigned long *best_parent_rate)
+{
+	if (abs(tmp_rate - rate) < abs(*best_rate - rate)) {
+		*best_rate = tmp_rate;
+
+		if (div_val)
+			*div_val = tmp_val;
+
+		if (!best_parent)
+			return;
+
+		*best_parent = parent;
+		if (best_parent_rate)
+			*best_parent_rate = clk_hw_get_rate(parent);
+	}
+}
+
+
 static unsigned long
 ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate,
 		       struct clk_hw **best_parent,
@@ -113,20 +135,21 @@ ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate,
 
 		parent_rate = clk_hw_get_rate(parent);
 
-		for (int j = 1; j <= div_max; j++) {
-			unsigned long tmp = DIV_ROUND_CLOSEST_ULL(parent_rate, j);
+		if (div->table) {
+			int tmp_val = divider_get_val(rate, parent_rate, div->table, div->width, 0);
+			unsigned long tmp_rate = divider_recalc_rate(hw, parent_rate, tmp_val,
+								     div->table, 0, div->width);
 
-			if (abs(tmp - rate) < abs(best_rate - rate)) {
-				best_rate = tmp;
+			ccu_mix_try_update_best(tmp_rate, rate, &best_rate, tmp_val,
+						div_val, parent, best_parent, best_parent_rate);
+			continue;
+		}
 
-				if (div_val)
-					*div_val = j - 1;
+		for (int j = 1; j <= div_max; j++) {
+			unsigned long tmp = DIV_ROUND_CLOSEST_ULL(parent_rate, j);
 
-				if (best_parent) {
-					*best_parent      = parent;
-					*best_parent_rate = parent_rate;
-				}
-			}
+			ccu_mix_try_update_best(tmp, rate, &best_rate, j - 1,
+						div_val, parent, best_parent, best_parent_rate);
 		}
 	}
 
diff --git a/drivers/clk/spacemit/ccu_mix.h b/drivers/clk/spacemit/ccu_mix.h
index 54d40cd39b2752260f57d2a96eb8d3eed8116ecd..e93142f74458c0d91394c86d73f64e1521a17913 100644
--- a/drivers/clk/spacemit/ccu_mix.h
+++ b/drivers/clk/spacemit/ccu_mix.h
@@ -11,16 +11,6 @@
 
 #include "ccu_common.h"
 
-/**
- * struct ccu_gate_config - Gate configuration
- *
- * @mask:	Mask to enable the gate. Some clocks may have more than one bit
- *		set in this field.
- */
-struct ccu_gate_config {
-	u32 mask;
-};
-
 struct ccu_factor_config {
 	u32 div;
 	u32 mul;
@@ -34,6 +24,7 @@ struct ccu_mux_config {
 struct ccu_div_config {
 	u8 shift;
 	u8 width;
+	const struct clk_div_table *table;
 };
 
 struct ccu_mix {
@@ -44,10 +35,11 @@ struct ccu_mix {
 	struct ccu_common common;
 };
 
-#define CCU_GATE_INIT(_mask)		{ .mask = _mask }
 #define CCU_FACTOR_INIT(_div, _mul)	{ .div = _div, .mul = _mul }
 #define CCU_MUX_INIT(_shift, _width)	{ .shift = _shift, .width = _width }
 #define CCU_DIV_INIT(_shift, _width)	{ .shift = _shift, .width = _width }
+#define CCU_DIV_TABLE_INIT(_shift, _width, _table)	\
+					{ .shift = _shift, .width = _width, .table = _table}
 
 #define CCU_PARENT_HW(_parent)		{ .hw = &_parent.common.hw }
 #define CCU_PARENT_NAME(_name)		{ .fw_name = #_name }
@@ -141,6 +133,18 @@ static struct ccu_mix _name = {							\
 	}									\
 }
 
+#define CCU_DIV_TABLE_GATE_DEFINE(_name, _parent, _reg_ctrl, _shift, _width,	\
+				  _table, _mask_gate, _flags)			\
+static struct ccu_mix _name = {							\
+	.gate	= CCU_GATE_INIT(_mask_gate),					\
+	.div	= CCU_DIV_TABLE_INIT(_shift, _width, _table),			\
+	.common = {								\
+		.reg_ctrl	= _reg_ctrl,					\
+		CCU_MIX_INITHW(_name, _parent, spacemit_ccu_div_gate_ops,	\
+			       _flags),						\
+	}									\
+}
+
 #define CCU_MUX_DIV_GATE_DEFINE(_name, _parents, _reg_ctrl, _mshift, _mwidth,	\
 				 _muxshift, _muxwidth, _mask_gate, _flags)	\
 static struct ccu_mix _name = {							\
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)
 #define MPMU_SUCCR			0x0014
 #define MPMU_ISCCR			0x0044
 #define MPMU_WDTPCR			0x0200

-- 
2.50.1

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

[Haylen Chu]

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

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

[Troy Mitchell]

Hi, Haylen!

On Thu, Aug 07, 2025 at 03:02:00AM +0000, Haylen Chu wrote:
> 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".
>
I'll change it in next version.

> 
> > 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.
Uh, our new chip(K3) may uses this macro that I introduced..
so I don't wanna take a gate as parent everywhere..
how about we leave it? ;)

> 
> > 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?
ditto.

I know you don't wanna introduce new macro..
But K3 requires this, so whether it is introduced now or future,
the final result is the same.

Please leave it..
> 
> > 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.
Ok, I will split them into separate patches.

...
> 
> ...
> 
> > 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.
It's worth that create a new commit to reformatting it?

                - Troy
> 
> >  #define MPMU_SUCCR			0x0014
> >  #define MPMU_ISCCR			0x0044
> >  #define MPMU_WDTPCR			0x0200
> > 
> > -- 
> > 2.50.1
> > 
> 
> Best regards,
> Haylen Chu
> 

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

[Haylen Chu]

On Fri, Aug 08, 2025 at 10:10:48AM +0800, Troy Mitchell wrote:
> Hi, Haylen!
> 
> On Thu, Aug 07, 2025 at 03:02:00AM +0000, Haylen Chu wrote:
> > 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".
> >
> I'll change it in next version.
> 
> > 
> > > 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.
> Uh, our new chip(K3) may uses this macro that I introduced..
> so I don't wanna take a gate as parent everywhere..
> how about we leave it? ;)

I wasn't proposing a workaround. What will go wrong if a gate is taken
as parent of DDN everywhere? Not to mention this DDN variant actually
duplicates the code in ccu_mix.c.

> > 
> > > 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?
> ditto.
> 
> I know you don't wanna introduce new macro..

It's not about new macros. It's about new clock types. And the solution
I proposed for the divider with a factor isn't meant to be a workaround.

For the divider's case, I think combining a fixed-factor and a normal
divider looks more clean than introducing a new LUT. It solves the
problem for K1, right?

> But K3 requires this, so whether it is introduced now or future,
> the final result is the same.

Could you please confirm whether K3's dividers requiring this patch are
really non-linear or just imply a fixed-factor?

> Please leave it..
> > 
> > > 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.
> Ok, I will split them into separate patches.

Thanks, that'll be easier to review.

> ...
> > 
> > ...
> > 
> > > 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.
> It's worth that create a new commit to reformatting it?

IIRC, the indentation is intended to show the relationship between
register bits and offsets, which seems easier to read for me. Do you
have a good reason to change it?

>                 - Troy
> > 
> > >  #define MPMU_SUCCR			0x0014
> > >  #define MPMU_ISCCR			0x0044
> > >  #define MPMU_WDTPCR			0x0200
> > > 
> > > -- 
> > > 2.50.1
> > > 
> > 
> > Best regards,
> > Haylen Chu
> > 

Best regards,
Haylen Chu

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

[Troy Mitchell]

On Fri, Aug 08, 2025 at 03:59:12AM +0000, Haylen Chu wrote:
> On Fri, Aug 08, 2025 at 10:10:48AM +0800, Troy Mitchell wrote:
> > Hi, Haylen!
> > 
> > On Thu, Aug 07, 2025 at 03:02:00AM +0000, Haylen Chu wrote:
> > > 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".
> > >
> > I'll change it in next version.
> > 
> > > 
> > > > 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.
> > Uh, our new chip(K3) may uses this macro that I introduced..
> > so I don't wanna take a gate as parent everywhere..
> > how about we leave it? ;)
> 
> I wasn't proposing a workaround. What will go wrong if a gate is taken
> as parent of DDN everywhere? 
I think this a bit troublesome...

> Not to mention this DDN variant actually
> duplicates the code in ccu_mix.c.
>
So I’ve ultimately decided not to introduce DDN_GATE.
I’ll change the macro for i2s_sysclk_src from
CCU_MUX_DEFINE to CCU_MUX_GATE_DEFINE.

What do you think? From the clock tree perspective, this should be fine.
> 
> > > 
> > > > 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?
> > ditto.
> > 
> > I know you don't wanna introduce new macro..
> 
> It's not about new macros. It's about new clock types. And the solution
> I proposed for the divider with a factor isn't meant to be a workaround.
> 
> For the divider's case, I think combining a fixed-factor and a normal
> divider looks more clean than introducing a new LUT. It solves the
> problem for K1, right?
yes, It solves.

> 
> > But K3 requires this, so whether it is introduced now or future,
> > the final result is the same.
> 
> Could you please confirm whether K3's dividers requiring this patch are
> really non-linear or just imply a fixed-factor?
I will confirm this point.

If I send v2 without removing the CCU_DIV_TABLE_GATE_DEFINE macro,
that would mean K3 really needs it.

> 
> > Please leave it..
> > > 
> > > > 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.
> > Ok, I will split them into separate patches.
> 
> Thanks, that'll be easier to review.
> 
> > ...
> > > 
> > > ...
> > > 
> > > > 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.
> > It's worth that create a new commit to reformatting it?
> 
> IIRC, the indentation is intended to show the relationship between
> register bits and offsets, which seems easier to read for me. 
Sry I ignore this..

But isn’t the POSR prefix already sufficient to indicate the relationship?

Have a nice day!

                - Troy

> Do you
> have a good reason to change it?
> 
> >                 - Troy
> > > 
> > > >  #define MPMU_SUCCR			0x0014
> > > >  #define MPMU_ISCCR			0x0044
> > > >  #define MPMU_WDTPCR			0x0200
> > > > 
> > > > -- 
> > > > 2.50.1
> > > > 
> > > 
> > > Best regards,
> > > Haylen Chu
> > > 
> 
> Best regards,
> Haylen Chu
> 

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

[Haylen Chu]

On Fri, Aug 08, 2025 at 02:34:00PM +0800, Troy Mitchell wrote:
> On Fri, Aug 08, 2025 at 03:59:12AM +0000, Haylen Chu wrote:
> > On Fri, Aug 08, 2025 at 10:10:48AM +0800, Troy Mitchell wrote:
> > > Hi, Haylen!
> > > 
> > > On Thu, Aug 07, 2025 at 03:02:00AM +0000, Haylen Chu wrote:
> > > > 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".
> > > >
> > > I'll change it in next version.
> > > 
> > > > 
> > > > > 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.
> > > Uh, our new chip(K3) may uses this macro that I introduced..
> > > so I don't wanna take a gate as parent everywhere..
> > > how about we leave it? ;)
> > 
> > I wasn't proposing a workaround. What will go wrong if a gate is taken
> > as parent of DDN everywhere? 
> I think this a bit troublesome...

I don't agree. It just costs one extra line and one extra macro.

> > Not to mention this DDN variant actually
> > duplicates the code in ccu_mix.c.
> >
> So I’ve ultimately decided not to introduce DDN_GATE.
> I’ll change the macro for i2s_sysclk_src from
> CCU_MUX_DEFINE to CCU_MUX_GATE_DEFINE.
> 
> What do you think? From the clock tree perspective, this should be fine.

Thanks for the change, it's fine for me, too.

> > 
> > > > 
> > > > > 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?
> > > ditto.
> > > 
> > > I know you don't wanna introduce new macro..
> > 
> > It's not about new macros. It's about new clock types. And the solution
> > I proposed for the divider with a factor isn't meant to be a workaround.
> > 
> > For the divider's case, I think combining a fixed-factor and a normal
> > divider looks more clean than introducing a new LUT. It solves the
> > problem for K1, right?
> yes, It solves.
> 
> > 
> > > But K3 requires this, so whether it is introduced now or future,
> > > the final result is the same.
> > 
> > Could you please confirm whether K3's dividers requiring this patch are
> > really non-linear or just imply a fixed-factor?
> I will confirm this point.
> 
> If I send v2 without removing the CCU_DIV_TABLE_GATE_DEFINE macro,
> that would mean K3 really needs it.

Thanks, this will help.

> > 
> > > Please leave it..
> > > > 
> > > > > 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.
> > > Ok, I will split them into separate patches.
> > 
> > Thanks, that'll be easier to review.
> > 
> > > ...
> > > > 
> > > > ...
> > > > 
> > > > > 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.
> > > It's worth that create a new commit to reformatting it?
> > 
> > IIRC, the indentation is intended to show the relationship between
> > register bits and offsets, which seems easier to read for me. 
> Sry I ignore this..
> 
> But isn’t the POSR prefix already sufficient to indicate the relationship?

I think the original form is easier to read, isn't it? Even if you don't
think so, this change obviously exceeds scope of this patch and please
separate a series for discussion.

> Have a nice day!
> 
>                 - Troy

Best regards,
Haylen Chu

> > Do you
> > have a good reason to change it?
> > 
> > >                 - Troy
> > > > 
> > > > >  #define MPMU_SUCCR			0x0014
> > > > >  #define MPMU_ISCCR			0x0044
> > > > >  #define MPMU_WDTPCR			0x0200
> > > > > 
> > > > > -- 
> > > > > 2.50.1
> > > > > 
> > > > 
> > > > Best regards,
> > > > Haylen Chu
> > > > 
> > 
> > Best regards,
> > Haylen Chu
> >