[PATCH 0/3] clk: spacemit: k3: fix i2s clock topology and divider rates

[PATCH 0/3] clk: spacemit: k3: fix i2s clock topology and divider rates

[Troy Mitchell]

The K3 i2s clock tree was modelled on the K1 layout but does not match
the actual K3 hardware topology, leading to incorrect clock rates being
reported and programmed.

Three fixes are provided in dependency order:

First, the MPMU_ISCCR syscon register is renamed to MPMU_ISCCR1 to
match the hardware register name, and MPMU_ISCCR0 (offset 0x0040) is
added. This prepares the header for the clock topology corrections that
follow.

Second, the clock topology is reworked to reflect the real hardware:
the common i2s_sysclk DDN at MPMU_ISCCR1 is renamed from "i2s1_sysclk",
an implicit 1/2 factor (i2s_bclk_factor) is introduced before i2s_bclk,
and the dedicated i2s1 clock path (i2s1_sysclk_src MUX + i2s1_sysclk
DDN backed by MPMU_ISCCR0) is added. CLK_MPMU_I2S1_SYSCLK keeps its
existing ID but now refers to the real per-instance i2s1 clock; no
in-tree user references this ID so the semantic change is contained.

Third, the per-instance i2s sysclk DDNs (i2s{0,2,3,4,5}_sysclk_div)
are corrected to account for the hardware-internal 1/2 factor by setting
pre_div to 2 instead of 1, so clk_get_rate() and clk_set_rate() report
and program the real hardware rate.

---
Troy Mitchell (3):
      dt-bindings: soc: spacemit: k3: add i2s_sysclk, i2s_bclk_factor and i2s1_sysclk_src IDs
      clk: spacemit: k3: fix i2s clock topology
      clk: spacemit: k3: fix missing /2 factor in i2s sysclk dividers

 drivers/clk/spacemit/ccu-k3.c                  | 31 +++++++++++++++++---------
 include/dt-bindings/clock/spacemit,k3-clocks.h |  3 +++
 include/soc/spacemit/k3-syscon.h               |  3 ++-
 3 files changed, 26 insertions(+), 11 deletions(-)
---
base-commit: c425609d6ac4012c8bbf01ec2e10e801b1923a7b
change-id: 20260615-k3-clk-fix-i2s-5e278a472061

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

[PATCH 1/3] dt-bindings: soc: spacemit: k3: add i2s_sysclk, i2s_bclk_factor and i2s1_sysclk_src IDs

[Troy Mitchell]

Add three new clock IDs to expose clocks introduced by the topology fix:
- CLK_MPMU_I2S_SYSCLK (51): the common i2s sysclk DDN at MPMU_ISCCR1
- CLK_MPMU_I2S_BCLK_FACTOR (52): the implicit /2 factor feeding i2s_bclk
- CLK_MPMU_I2S1_SYSCLK_SRC (53): the dedicated i2s1 sysclk source mux

CLK_MPMU_I2S1_SYSCLK keeps its existing ID (47) but will be repointed to
the real per-instance i2s1 clock in a subsequent patch. No in-tree user
references this ID so the semantic change is contained.

Fixes: efe897b557e2 ("dt-bindings: soc: spacemit: k3: add clock support")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 include/dt-bindings/clock/spacemit,k3-clocks.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/clock/spacemit,k3-clocks.h b/include/dt-bindings/clock/spacemit,k3-clocks.h
index dfae52547cda..df0b9fe43b76 100644
--- a/include/dt-bindings/clock/spacemit,k3-clocks.h
+++ b/include/dt-bindings/clock/spacemit,k3-clocks.h
@@ -145,6 +145,9 @@
 #define CLK_MPMU_I2S3_SYSCLK     48
 #define CLK_MPMU_I2S4_SYSCLK     49
 #define CLK_MPMU_I2S5_SYSCLK     50
+#define CLK_MPMU_I2S_SYSCLK      51
+#define CLK_MPMU_I2S_BCLK_FACTOR 52
+#define CLK_MPMU_I2S1_SYSCLK_SRC 53
 
 /* APBC clocks */
 #define CLK_APBC_UART0           0

-- 
2.55.0

Re: [PATCH 1/3] dt-bindings: soc: spacemit: k3: add i2s_sysclk, i2s_bclk_factor and i2s1_sysclk_src IDs

[Troy Mitchell]

> CLK_MPMU_I2S1_SYSCLK keeps its existing ID (47) but will be repointed to
> the real per-instance i2s1 clock in a subsequent patch. No in-tree user
> references this ID so the semantic change is contained.
>
> [Severity: Medium]
> Does the commit message reference the correct clock ID here?
>
> Looking at include/dt-bindings/clock/spacemit,k3-clocks.h, 47 is actually
> assigned to a different clock:
>
> #define CLK_MPMU_I2S1_SYSCLK     34
> ...
> #define CLK_MPMU_I2S2_SYSCLK     47
>
> Could the commit message be updated to reflect the accurate existing ID for
> CLK_MPMU_I2S1_SYSCLK?

Good catch. CLK_MPMU_I2S1_SYSCLK has ID 34, while ID 47 belongs to
CLK_MPMU_I2S2_SYSCLK. I will correct the commit message in the next
version.

                                            - Troy

[PATCH 2/3] clk: spacemit: k3: fix i2s clock topology

[Troy Mitchell]

The K3 i2s clocks were modelled as a single path behind one MPMU
register:

    pll1_d96_25p6 / i2s_153p6_base
      `-- i2s_sysclk_src (mux+gate, MPMU_ISCCR)
            `-- i2s1_sysclk (DDN, MPMU_ISCCR)
                  |-- i2s_bclk (div+gate, MPMU_ISCCR)
                  `-- i2s2_sysclk (mux, parent 0)

The hardware actually has two i2s clock control registers, ISCCR0
(0x0040) and ISCCR1 (0x0044): ISCCR1 drives the common sysclk shared
by i2s0/2/3/4/5 and the common bclk, whose divider always implies a
fixed 1/2 factor in front, while ISCCR0 drives a dedicated path for
i2s1:

    pll1_d96_25p6 / i2s_153p6_base
      |-- i2s_sysclk_src (mux+gate, MPMU_ISCCR1)
      |     `-- i2s_sysclk (DDN, MPMU_ISCCR1)
      |           |-- i2s_bclk_factor (fixed factor, /2)
      |           |     `-- i2s_bclk (div+gate, MPMU_ISCCR1)
      |           `-- i2s2_sysclk (mux, parent 0)
      `-- i2s1_sysclk_src (mux+gate, MPMU_ISCCR0)
            `-- i2s1_sysclk (DDN, MPMU_ISCCR0)

Because of this mismatch, i2s_bclk reported twice the real rate, and
the dedicated i2s1 clock path could not be described in DT at all.

Model the tree as above: split the MPMU_ISCCR register macro into
MPMU_ISCCR0 and MPMU_ISCCR1 to match the hardware register names,
rename the common DDN to i2s_sysclk, insert the fixed 1/2 factor
i2s_bclk_factor in front of i2s_bclk, and add the i2s1_sysclk_src mux
and i2s1_sysclk DDN backed by MPMU_ISCCR0. CLK_MPMU_I2S1_SYSCLK now
refers to the dedicated i2s1 clock; no in-tree user references this
ID, so nothing is affected by the change of meaning.

Fixes: e371a77255b8 ("clk: spacemit: k3: add the clock tree")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/clk/spacemit/ccu-k3.c    | 21 ++++++++++++++++-----
 include/soc/spacemit/k3-syscon.h |  3 ++-
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/spacemit/ccu-k3.c b/drivers/clk/spacemit/ccu-k3.c
index cb0c4277f72a..abcc5caa67bd 100644
--- a/drivers/clk/spacemit/ccu-k3.c
+++ b/drivers/clk/spacemit/ccu-k3.c
@@ -218,11 +218,19 @@ 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_MUX_GATE_DEFINE(i2s_sysclk_src, i2s_sysclk_src_parents, MPMU_ISCCR1, 30, 1, BIT(31), 0);
 
-CCU_DDN_DEFINE(i2s1_sysclk, i2s_sysclk_src, MPMU_ISCCR, 0, 15, 15, 12, 1, 0);
+CCU_DDN_DEFINE(i2s_sysclk, i2s_sysclk_src, MPMU_ISCCR1, 0, 15, 15, 12, 1, 0);
 
-CCU_DIV_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s1_sysclk), MPMU_ISCCR, 27, 2, BIT(29), 0);
+CCU_FACTOR_DEFINE(i2s_bclk_factor, CCU_PARENT_HW(i2s_sysclk), 2, 1);
+/*
+ * Divider of i2s_bclk always implies a 1/2 factor, which is
+ * described by i2s_bclk_factor.
+ */
+CCU_DIV_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s_bclk_factor), MPMU_ISCCR1, 27, 2, BIT(29), 0);
+
+CCU_MUX_GATE_DEFINE(i2s1_sysclk_src, i2s_sysclk_src_parents, MPMU_ISCCR0, 30, 1, BIT(31), 0);
+CCU_DDN_DEFINE(i2s1_sysclk, i2s1_sysclk_src, MPMU_ISCCR0, 0, 15, 15, 12, 1, 0);
 
 static const struct clk_parent_data i2s_sysclk_parents[] = {
 	CCU_PARENT_HW(pll1_d4_614p4),
@@ -243,7 +251,7 @@ CCU_DDN_DEFINE(i2s4_sysclk_div, i2s4_sysclk_sel, MPMU_I2S4_SYSCLK, 0, 16, 16, 16
 CCU_DDN_DEFINE(i2s5_sysclk_div, i2s5_sysclk_sel, MPMU_I2S5_SYSCLK, 0, 16, 16, 16, 1, 0);
 
 static const struct clk_parent_data i2s2_sysclk_parents[] = {
-	CCU_PARENT_HW(i2s1_sysclk),
+	CCU_PARENT_HW(i2s_sysclk),
 	CCU_PARENT_HW(i2s2_sysclk_div),
 };
 CCU_GATE_DEFINE(i2s0_sysclk, CCU_PARENT_HW(i2s0_sysclk_div), MPMU_I2S_SYSCLK_CTRL, BIT(2), 0);
@@ -1161,8 +1169,11 @@ static struct clk_hw *k3_ccu_mpmu_hws[] = {
 	[CLK_MPMU_I2S_153P6]		= &i2s_153p6.common.hw,
 	[CLK_MPMU_I2S_153P6_BASE]	= &i2s_153p6_base.common.hw,
 	[CLK_MPMU_I2S_SYSCLK_SRC]	= &i2s_sysclk_src.common.hw,
-	[CLK_MPMU_I2S1_SYSCLK]		= &i2s1_sysclk.common.hw,
+	[CLK_MPMU_I2S_SYSCLK]		= &i2s_sysclk.common.hw,
+	[CLK_MPMU_I2S_BCLK_FACTOR]	= &i2s_bclk_factor.common.hw,
 	[CLK_MPMU_I2S_BCLK]		= &i2s_bclk.common.hw,
+	[CLK_MPMU_I2S1_SYSCLK_SRC]	= &i2s1_sysclk_src.common.hw,
+	[CLK_MPMU_I2S1_SYSCLK]		= &i2s1_sysclk.common.hw,
 	[CLK_MPMU_I2S0_SYSCLK_SEL]	= &i2s0_sysclk_sel.common.hw,
 	[CLK_MPMU_I2S2_SYSCLK_SEL]	= &i2s2_sysclk_sel.common.hw,
 	[CLK_MPMU_I2S3_SYSCLK_SEL]	= &i2s3_sysclk_sel.common.hw,
diff --git a/include/soc/spacemit/k3-syscon.h b/include/soc/spacemit/k3-syscon.h
index a68255dd641f..959d8d0786fe 100644
--- a/include/soc/spacemit/k3-syscon.h
+++ b/include/soc/spacemit/k3-syscon.h
@@ -45,7 +45,8 @@
 #define POSR_PLL7_LOCK			BIT(30)
 #define POSR_PLL8_LOCK			BIT(31)
 #define MPMU_SUCCR			0x0014
-#define MPMU_ISCCR			0x0044
+#define MPMU_ISCCR0			0x0040
+#define MPMU_ISCCR1			0x0044
 #define MPMU_WDTPCR			0x0200
 #define MPMU_RIPCCR			0x0210
 #define MPMU_ACGR			0x1024

-- 
2.55.0

[PATCH 3/3] clk: spacemit: k3: fix missing /2 factor in i2s sysclk dividers

[Troy Mitchell]

The i2s{0,2,3,4,5}_sysclk_div DDNs have an additional fixed 1/2
divider in the hardware IP after the configurable divider, so the
real output rate is:

    rate = parent_rate * num / (den * 2)

Set pre_div to 2 to account for it.

Fixes: e371a77255b8 ("clk: spacemit: k3: add the clock tree")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/clk/spacemit/ccu-k3.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/spacemit/ccu-k3.c b/drivers/clk/spacemit/ccu-k3.c
index abcc5caa67bd..675cf03d98f0 100644
--- a/drivers/clk/spacemit/ccu-k3.c
+++ b/drivers/clk/spacemit/ccu-k3.c
@@ -244,11 +244,11 @@ CCU_MUX_DEFINE(i2s3_sysclk_sel, i2s_sysclk_parents, MPMU_I2S_SYSCLK_CTRL, 12, 2,
 CCU_MUX_DEFINE(i2s4_sysclk_sel, i2s_sysclk_parents, MPMU_I2S_SYSCLK_CTRL, 16, 2, 0);
 CCU_MUX_DEFINE(i2s5_sysclk_sel, i2s_sysclk_parents, MPMU_I2S_SYSCLK_CTRL, 20, 2, 0);
 
-CCU_DDN_DEFINE(i2s0_sysclk_div, i2s0_sysclk_sel, MPMU_I2S0_SYSCLK, 0, 16, 16, 16, 1, 0);
-CCU_DDN_DEFINE(i2s2_sysclk_div, i2s2_sysclk_sel, MPMU_I2S2_SYSCLK, 0, 16, 16, 16, 1, 0);
-CCU_DDN_DEFINE(i2s3_sysclk_div, i2s3_sysclk_sel, MPMU_I2S3_SYSCLK, 0, 16, 16, 16, 1, 0);
-CCU_DDN_DEFINE(i2s4_sysclk_div, i2s4_sysclk_sel, MPMU_I2S4_SYSCLK, 0, 16, 16, 16, 1, 0);
-CCU_DDN_DEFINE(i2s5_sysclk_div, i2s5_sysclk_sel, MPMU_I2S5_SYSCLK, 0, 16, 16, 16, 1, 0);
+CCU_DDN_DEFINE(i2s0_sysclk_div, i2s0_sysclk_sel, MPMU_I2S0_SYSCLK, 0, 16, 16, 16, 2, 0);
+CCU_DDN_DEFINE(i2s2_sysclk_div, i2s2_sysclk_sel, MPMU_I2S2_SYSCLK, 0, 16, 16, 16, 2, 0);
+CCU_DDN_DEFINE(i2s3_sysclk_div, i2s3_sysclk_sel, MPMU_I2S3_SYSCLK, 0, 16, 16, 16, 2, 0);
+CCU_DDN_DEFINE(i2s4_sysclk_div, i2s4_sysclk_sel, MPMU_I2S4_SYSCLK, 0, 16, 16, 16, 2, 0);
+CCU_DDN_DEFINE(i2s5_sysclk_div, i2s5_sysclk_sel, MPMU_I2S5_SYSCLK, 0, 16, 16, 16, 2, 0);
 
 static const struct clk_parent_data i2s2_sysclk_parents[] = {
 	CCU_PARENT_HW(i2s_sysclk),

-- 
2.55.0

Re: [PATCH 3/3] clk: spacemit: k3: fix missing /2 factor in i2s sysclk dividers

[Troy Mitchell]

> The i2s{0,2,3,4,5}_sysclk_div DDNs have an additional fixed 1/2
> divider in the hardware IP after the configurable divider, so the
> real output rate is:
>
>     rate = parent_rate * num / (den * 2)
>
> [Severity: Medium]
> Does this formula mathematically contradict the driver's implementation?
>
> The code in drivers/clk/spacemit/ccu_ddn.c calculates the rate by
> multiplying by den and dividing by num:
>
>     return prate * den / pre_div / num;
>
> Could the commit message be updated to reflect the actual hardware behavior?

Good catch. The formula in the commit message has num and den reversed. The
correct formula is:

    rate = parent_rate * den / (num * 2)

I will correct the commit message in the next revision. The pre_div = 2
change itself remains correct.

> [...]
>
> [Severity: High]
> This is a pre-existing issue, but can this lead to a kernel panic due to an
> unhandled zero divisor?
>
> In drivers/clk/spacemit/ccu_ddn.c, ccu_ddn_recalc_rate() reads hardware
> registers directly:
>
>     num = (val & ddn->num_mask) >> ddn->num_shift;
>     den = (val & ddn->den_mask) >> ddn->den_shift;
>
>     return ccu_ddn_calc_rate(prate, num, den, ddn->pre_div);
>
> If the register contains 0 for num, ccu_ddn_calc_rate() will divide by zero.
> Additionally, ccu_ddn_calc_best_rate() can pass num = 0 when
> prate / ddn->pre_div evaluates to 0.
>
> Should we add validation to prevent division by zero in these paths?

Yes, these paths should guard against a zero divisor. This issue predates
the K3 I2S change and is independent of the fixed-divider correction, so I
will address it in a separate ccu_ddn patch rather than expanding the scope
of this series.

                                            - Troy

Re: [PATCH 0/3] clk: spacemit: k3: fix i2s clock topology and divider rates

[Yixun Lan]

On 00:26 Fri 17 Jul     , Troy Mitchell wrote:
> The K3 i2s clock tree was modelled on the K1 layout but does not match
> the actual K3 hardware topology, leading to incorrect clock rates being
> reported and programmed.
> 
> Three fixes are provided in dependency order:
> 
> First, the MPMU_ISCCR syscon register is renamed to MPMU_ISCCR1 to
> match the hardware register name, and MPMU_ISCCR0 (offset 0x0040) is
> added. This prepares the header for the clock topology corrections that
> follow.
> 
> Second, the clock topology is reworked to reflect the real hardware:
> the common i2s_sysclk DDN at MPMU_ISCCR1 is renamed from "i2s1_sysclk",
> an implicit 1/2 factor (i2s_bclk_factor) is introduced before i2s_bclk,
> and the dedicated i2s1 clock path (i2s1_sysclk_src MUX + i2s1_sysclk
> DDN backed by MPMU_ISCCR0) is added. CLK_MPMU_I2S1_SYSCLK keeps its
> existing ID but now refers to the real per-instance i2s1 clock; no
> in-tree user references this ID so the semantic change is contained.
> 
> Third, the per-instance i2s sysclk DDNs (i2s{0,2,3,4,5}_sysclk_div)
> are corrected to account for the hardware-internal 1/2 factor by setting
> pre_div to 2 instead of 1, so clk_get_rate() and clk_set_rate() report
> and program the real hardware rate.
> 
For the series, looks good
Reviewed-by: Yixun Lan <dlan@kernel.org>

> ---
> Troy Mitchell (3):
>       dt-bindings: soc: spacemit: k3: add i2s_sysclk, i2s_bclk_factor and i2s1_sysclk_src IDs
>       clk: spacemit: k3: fix i2s clock topology
>       clk: spacemit: k3: fix missing /2 factor in i2s sysclk dividers
> 
>  drivers/clk/spacemit/ccu-k3.c                  | 31 +++++++++++++++++---------
>  include/dt-bindings/clock/spacemit,k3-clocks.h |  3 +++
>  include/soc/spacemit/k3-syscon.h               |  3 ++-
>  3 files changed, 26 insertions(+), 11 deletions(-)
> ---
> base-commit: c425609d6ac4012c8bbf01ec2e10e801b1923a7b
> change-id: 20260615-k3-clk-fix-i2s-5e278a472061
> 
> Best regards,
> --  
> Troy Mitchell <troy.mitchell@linux.spacemit.com>
> 

-- 
Yixun Lan (dlan)