| Message ID | 20240827061954.351773-1-ganboing@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: analogbits: Fix incorrect calculation of vco rate delta | expand |
Quoting Bo Gan (2024-08-26 23:19:54) > In function `wrpll_configure_for_rate`, we try to determine the best PLL > configuration for a target rate. However, in the loop where we try values > of R, we should compare the derived `vco` with `target_vco_rate`. However, > we were in fact comparing it with `target_rate`, which is actually after > Q shift. This is incorrect, and sometimes can result in suboptimal clock > rates. This patch fixes it. > > Signed-off-by: Bo Gan <ganboing@gmail.com> > --- Please add a Fixes tag. Also, your patch has tons of diff context. Why?
On 8/28/24 11:52, Stephen Boyd wrote: > Quoting Bo Gan (2024-08-26 23:19:54) >> In function `wrpll_configure_for_rate`, we try to determine the best PLL >> configuration for a target rate. However, in the loop where we try values >> of R, we should compare the derived `vco` with `target_vco_rate`. However, >> we were in fact comparing it with `target_rate`, which is actually after >> Q shift. This is incorrect, and sometimes can result in suboptimal clock >> rates. This patch fixes it. >> >> Signed-off-by: Bo Gan <ganboing@gmail.com> >> --- > > Please add a Fixes tag. > > Also, your patch has tons of diff context. Why? Hi Stephen, Thanks for the reply. I'll add the Fixes tag in v2. I explicitly enlarged the diff to show more surrounding contexts for better readability. Any other issue I should fix? Bo
Quoting Bo Gan (2024-08-28 14:23:37) > On 8/28/24 11:52, Stephen Boyd wrote: > > Quoting Bo Gan (2024-08-26 23:19:54) > >> In function `wrpll_configure_for_rate`, we try to determine the best PLL > >> configuration for a target rate. However, in the loop where we try values > >> of R, we should compare the derived `vco` with `target_vco_rate`. However, > >> we were in fact comparing it with `target_rate`, which is actually after > >> Q shift. This is incorrect, and sometimes can result in suboptimal clock > >> rates. This patch fixes it. > >> > >> Signed-off-by: Bo Gan <ganboing@gmail.com> > >> --- > > > > Please add a Fixes tag. > > > > Also, your patch has tons of diff context. Why? > > Hi Stephen, > > Thanks for the reply. I'll add the Fixes tag in v2. I explicitly enlarged the > diff to show more surrounding contexts for better readability. Ok. > Any other issue > I should fix? > Nope.
diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c index 65d422a588e1..9d178afc73bd 100644 --- a/drivers/clk/analogbits/wrpll-cln28hpc.c +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c @@ -255,81 +255,81 @@ int wrpll_configure_for_rate(struct wrpll_cfg *c, u32 target_rate, } c->flags &= ~WRPLL_FLAGS_BYPASS_MASK; /* Calculate the Q shift and target VCO rate */ divq = __wrpll_calc_divq(target_rate, &target_vco_rate); if (!divq) return -1; c->divq = divq; /* Precalculate the pre-Q divider target ratio */ ratio = div64_u64((target_vco_rate << ROUND_SHIFT), parent_rate); fbdiv = __wrpll_calc_fbdiv(c); best_r = 0; best_f = 0; best_delta = MAX_VCO_FREQ; /* * Consider all values for R which land within * [MIN_POST_DIVR_FREQ, MAX_POST_DIVR_FREQ]; prefer smaller R */ for (r = c->init_r; r <= c->max_r; ++r) { f_pre_div = ratio * r; f = (f_pre_div + (1 << ROUND_SHIFT)) >> ROUND_SHIFT; f >>= (fbdiv - 1); post_divr_freq = div_u64(parent_rate, r); vco_pre = fbdiv * post_divr_freq; vco = vco_pre * f; /* Ensure rounding didn't take us out of range */ if (vco > target_vco_rate) { --f; vco = vco_pre * f; } else if (vco < MIN_VCO_FREQ) { ++f; vco = vco_pre * f; } - delta = abs(target_rate - vco); + delta = abs(target_vco_rate - vco); if (delta < best_delta) { best_delta = delta; best_r = r; best_f = f; } } c->divr = best_r - 1; c->divf = best_f - 1; post_divr_freq = div_u64(parent_rate, best_r); /* Pick the best PLL jitter filter */ range = __wrpll_calc_filter_range(post_divr_freq); if (range < 0) return range; c->range = range; return 0; } EXPORT_SYMBOL_GPL(wrpll_configure_for_rate); /** * wrpll_calc_output_rate() - calculate the PLL's target output rate * @c: ptr to a struct wrpll_cfg record to read from * @parent_rate: PLL refclk rate * * Given a pointer to the PLL's current input configuration @c and the * PLL's input reference clock rate @parent_rate (before the R * pre-divider), calculate the PLL's output clock rate (after the Q * post-divider). * * Context: Any context. Caller must protect the memory pointed to by @c * from simultaneous modification. * * Return: the PLL's output clock rate, in Hz. The return value from * this function is intended to be convenient to pass directly * to the Linux clock framework; thus there is no explicit * error return value. */
In function `wrpll_configure_for_rate`, we try to determine the best PLL configuration for a target rate. However, in the loop where we try values of R, we should compare the derived `vco` with `target_vco_rate`. However, we were in fact comparing it with `target_rate`, which is actually after Q shift. This is incorrect, and sometimes can result in suboptimal clock rates. This patch fixes it. Signed-off-by: Bo Gan <ganboing@gmail.com> --- drivers/clk/analogbits/wrpll-cln28hpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)