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