| Message ID | 20221214123704.7305-1-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: ingenic: jz4760: Update M/N/OD calculation algorithm | expand |
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: <stable@vger.kernel.org> > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- Applied to clk-fixes
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; }
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: <stable@vger.kernel.org> Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/clk/ingenic/jz4760-cgu.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)