[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message incorrectly claims that CLK_MPMU_I2S1_SYSCLK keeps its existing ID of 47, when its actual ID is 34.
--

--- Patch [1]: [PATCH 1/3] dt-bindings: soc: spacemit: k3: add i2s_sysclk, i2s_bclk_factor and i2s1_sysclk_src IDs ---
commit 1a78396155ff5bfe1746259b3e076109f3ecbd38
Author: Troy Mitchell <troy.mitchell@linux.spacemit.com>

dt-bindings: soc: spacemit: k3: add i2s_sysclk, i2s_bclk_factor and i2s1_sysclk_src IDs

This commit adds three new clock IDs for the SpacemiT K3 SoC to expose
clocks related to an I2S topology fix.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-k3-clk-fix-i2s-v1-0-e95001a692ee@linux.spacemit.com?part=1

[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The commit message mathematically contradicts the driver's actual hardware implementation.

Pre-existing issues:
- [High] Kernel panic due to unhandled zero divisor (`num`) in clock rate calculations.
--

--- Patch [3]: [PATCH 3/3] clk: spacemit: k3: fix missing /2 factor in i2s sysclk dividers ---
commit f6d65354cd50f2c66ffa439212d0df739e8b5882
Author: Troy Mitchell <troy.mitchell@linux.spacemit.com>

clk: spacemit: k3: fix missing /2 factor in i2s sysclk dividers

This commit updates the i2s sysclk dividers in the spacemit k3 clock driver to
set pre_div to 2. This accounts for a fixed 1/2 hardware divider after the
configurable divider.

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

drivers/clk/spacemit/ccu_ddn.c:ccu_ddn_calc_rate() {
    ...
    return prate * den / pre_div / num;
    ...
}

The file's documentation also notes:

 *              Fin     denominator
 *      Fout = ----- * -------------
 *               2       numerator

Could the commit message be updated to reflect the actual hardware behavior?

> 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 abcc5caa67bd7..675cf03d98f05 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);

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

drivers/clk/spacemit/ccu_ddn.c:ccu_ddn_recalc_rate() {
    ...
    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:

drivers/clk/spacemit/ccu_ddn.c:ccu_ddn_calc_rate() {
    ...
    return prate * den / pre_div / num;
    ...
}

Additionally, in ccu_ddn_calc_best_rate(), if the parent rate is less than
ddn->pre_div (for example, if prate is 0), the calculation
prate / ddn->pre_div evaluates to 0. This causes rational_best_approximation()
to set num to 0, which is then passed to ccu_ddn_calc_rate(), again leading to
a division by zero.

Should we add validation to prevent division by zero in these paths?

> +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),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-k3-clk-fix-i2s-v1-0-e95001a692ee@linux.spacemit.com?part=3