The previous algorithm was pretty broken.
- The inner loop had a '(m > m_max)' condition, and the value of 'm'
would increase in each iteration;
- Each iteration would actually multiply 'm' by two, so it is not needed
to re-compute the whole equation at each iteration;
- It would loop until (m & 1) == 0, which means it would loop at most
once.
- The outer loop would divide the 'n' value by two at the end of each
iteration. This meant that for a 12 MHz parent clock and a 1.2 GHz
requested clock, it would first try n=12, then n=6, then n=3, then
n=1, none of which would work; the only valid value is n=2 in this
case.
Simplify this algorithm with a single for loop, which decrements 'n'
after each iteration, addressing all of the above problems.
Fixes: bdbfc029374f ("clk: ingenic: Add support for the JZ4760")
Cc: <[email protected]>
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/clk/ingenic/jz4760-cgu.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/ingenic/jz4760-cgu.c b/drivers/clk/ingenic/jz4760-cgu.c
index ecd395ac8a28..e407f00bd594 100644
--- a/drivers/clk/ingenic/jz4760-cgu.c
+++ b/drivers/clk/ingenic/jz4760-cgu.c
@@ -58,7 +58,7 @@ jz4760_cgu_calc_m_n_od(const struct ingenic_cgu_pll_info *pll_info,
unsigned long rate, unsigned long parent_rate,
unsigned int *pm, unsigned int *pn, unsigned int *pod)
{
- unsigned int m, n, od, m_max = (1 << pll_info->m_bits) - 2;
+ unsigned int m, n, od, m_max = (1 << pll_info->m_bits) - 1;
/* The frequency after the N divider must be between 1 and 50 MHz. */
n = parent_rate / (1 * MHZ);
@@ -66,19 +66,17 @@ jz4760_cgu_calc_m_n_od(const struct ingenic_cgu_pll_info *pll_info,
/* The N divider must be >= 2. */
n = clamp_val(n, 2, 1 << pll_info->n_bits);
- for (;; n >>= 1) {
- od = (unsigned int)-1;
+ rate /= MHZ;
+ parent_rate /= MHZ;
- do {
- m = (rate / MHZ) * (1 << ++od) * n / (parent_rate / MHZ);
- } while ((m > m_max || m & 1) && (od < 4));
-
- if (od < 4 && m >= 4 && m <= m_max)
- break;
+ for (m = m_max; m >= m_max && n >= 2; n--) {
+ m = rate * n / parent_rate;
+ od = m & 1;
+ m <<= od;
}
*pm = m;
- *pn = n;
+ *pn = n + 1;
*pod = 1 << od;
}
--
2.35.1
Quoting Paul Cercueil (2022-12-14 04:37:04)
> The previous algorithm was pretty broken.
>
> - The inner loop had a '(m > m_max)' condition, and the value of 'm'
> would increase in each iteration;
>
> - Each iteration would actually multiply 'm' by two, so it is not needed
> to re-compute the whole equation at each iteration;
>
> - It would loop until (m & 1) == 0, which means it would loop at most
> once.
>
> - The outer loop would divide the 'n' value by two at the end of each
> iteration. This meant that for a 12 MHz parent clock and a 1.2 GHz
> requested clock, it would first try n=12, then n=6, then n=3, then
> n=1, none of which would work; the only valid value is n=2 in this
> case.
>
> Simplify this algorithm with a single for loop, which decrements 'n'
> after each iteration, addressing all of the above problems.
>
> Fixes: bdbfc029374f ("clk: ingenic: Add support for the JZ4760")
> Cc: <[email protected]>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
Applied to clk-fixes