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

[sashiko-bot@kernel.org]

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