| Message ID | 20240906-fix_clk-v2-1-7a3941eb2cdf@amlogic.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: meson: Fix an issue with inaccurate hifi_pll frequency | expand |
On Fri 06 Sep 2024 at 18:34, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > From: Chuan Liu <chuan.liu@amlogic.com> > > Some PLLS with fractional multipliers have fractional denominators with > fixed values, instead of the previous "(1 << pll-> frc.width)". > > Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> > --- > drivers/clk/meson/clk-pll.c | 8 +++++--- > drivers/clk/meson/clk-pll.h | 1 + > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c > index bc570a2ff3a3..a141ab450009 100644 > --- a/drivers/clk/meson/clk-pll.c > +++ b/drivers/clk/meson/clk-pll.c > @@ -57,12 +57,13 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate, > struct meson_clk_pll_data *pll) > { > u64 rate = (u64)parent_rate * m; > + unsigned int frac_max = unlikely(pll->frac_max) ? pll->frac_max > : ^ Please don't play with this unless you've got justification a for it. By justification, I mean actual numbers showing the difference it makes and not just for the platforms listed in this series, but all the platforms supported by this driver. > + (1 << pll->frac.width); > > if (frac && MESON_PARM_APPLICABLE(&pll->frac)) { > u64 frac_rate = (u64)parent_rate * frac; > > - rate += DIV_ROUND_UP_ULL(frac_rate, > - (1 << pll->frac.width)); > + rate += DIV_ROUND_UP_ULL(frac_rate, frac_max); > } > > return DIV_ROUND_UP_ULL(rate, n); > @@ -100,7 +101,8 @@ static unsigned int __pll_params_with_frac(unsigned long rate, > unsigned int n, > struct meson_clk_pll_data *pll) > { > - unsigned int frac_max = (1 << pll->frac.width); > + unsigned int frac_max = unlikely(pll->frac_max) ? pll->frac_max : > + (1 << pll->frac.width); > u64 val = (u64)rate * n; > > /* Bail out if we are already over the requested rate */ > diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h > index 7b6b87274073..949157fb7bf5 100644 > --- a/drivers/clk/meson/clk-pll.h > +++ b/drivers/clk/meson/clk-pll.h > @@ -43,6 +43,7 @@ struct meson_clk_pll_data { > unsigned int init_count; > const struct pll_params_table *table; > const struct pll_mult_range *range; > + unsigned int frac_max; > u8 flags; > };
Hi, Jerome, Thank you for your quick reply! I didn't get back to you because of the weekend. On 2024/9/6 19:22, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Fri 06 Sep 2024 at 18:34, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > >> From: Chuan Liu <chuan.liu@amlogic.com> >> >> Some PLLS with fractional multipliers have fractional denominators with >> fixed values, instead of the previous "(1 << pll-> frc.width)". >> >> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> >> --- >> drivers/clk/meson/clk-pll.c | 8 +++++--- >> drivers/clk/meson/clk-pll.h | 1 + >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c >> index bc570a2ff3a3..a141ab450009 100644 >> --- a/drivers/clk/meson/clk-pll.c >> +++ b/drivers/clk/meson/clk-pll.c >> @@ -57,12 +57,13 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate, >> struct meson_clk_pll_data *pll) >> { >> u64 rate = (u64)parent_rate * m; >> + unsigned int frac_max = unlikely(pll->frac_max) ? pll->frac_max >> : > ^ Please don't play with this unless > you've got justification a for it. Sorry, I don't quite understand this one. Is it because you suggest keeping "(1 << pll->frac_max)" here, followed by "if" to determine whether to assign "pll->frac_max"? "unlikely" is used here. My idea is that it will be possible to determine the value of "frac_max" at compile time, which will result in one less "if" judgment and slightly improve drive performance. > > By justification, I mean actual numbers showing the difference it makes > and not just for the platforms listed in this series, but all the > platforms supported by this driver. You're right, In this way, even if the latter value changes, our driver will be compatible. >> + (1 << pll->frac.width); >> >> if (frac && MESON_PARM_APPLICABLE(&pll->frac)) { >> u64 frac_rate = (u64)parent_rate * frac; >> >> - rate += DIV_ROUND_UP_ULL(frac_rate, >> - (1 << pll->frac.width)); >> + rate += DIV_ROUND_UP_ULL(frac_rate, frac_max); >> } >> >> return DIV_ROUND_UP_ULL(rate, n); >> @@ -100,7 +101,8 @@ static unsigned int __pll_params_with_frac(unsigned long rate, >> unsigned int n, >> struct meson_clk_pll_data *pll) >> { >> - unsigned int frac_max = (1 << pll->frac.width); >> + unsigned int frac_max = unlikely(pll->frac_max) ? pll->frac_max : >> + (1 << pll->frac.width); >> u64 val = (u64)rate * n; >> >> /* Bail out if we are already over the requested rate */ >> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h >> index 7b6b87274073..949157fb7bf5 100644 >> --- a/drivers/clk/meson/clk-pll.h >> +++ b/drivers/clk/meson/clk-pll.h >> @@ -43,6 +43,7 @@ struct meson_clk_pll_data { >> unsigned int init_count; >> const struct pll_params_table *table; >> const struct pll_mult_range *range; >> + unsigned int frac_max; >> u8 flags; >> }; > -- > Jerome
On Mon 09 Sep 2024 at 09:55, Chuan Liu <chuan.liu@amlogic.com> wrote: > Sorry, I don't quite understand this one. Is it because you suggest keeping > > "(1 << pll->frac_max)" here, followed by "if" to determine whether to assign > > "pll->frac_max"? > > > "unlikely" is used here. My idea is that it will be possible to determine > the value > > of "frac_max" at compile time, which will result in one less "if" judgment > and > > slightly improve drive performance. I'll rephrase. Please drop the 'unlikely()' call. You may add that : * in a separate change * if you really really wish to * if you provide profiling numbers for the different supported platforms and PLLs, not just the one targeted by this patchset.
Hi, Jerome: Thank you for your meticulous explanation. On 2024/9/9 15:40, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Mon 09 Sep 2024 at 09:55, Chuan Liu <chuan.liu@amlogic.com> wrote: > >> Sorry, I don't quite understand this one. Is it because you suggest keeping >> >> "(1 << pll->frac_max)" here, followed by "if" to determine whether to assign >> >> "pll->frac_max"? >> >> >> "unlikely" is used here. My idea is that it will be possible to determine >> the value >> >> of "frac_max" at compile time, which will result in one less "if" judgment >> and >> >> slightly improve drive performance. > I'll rephrase. > > Please drop the 'unlikely()' call. > > You may add that : > * in a separate change > * if you really really wish to > * if you provide profiling numbers for the different supported > platforms and PLLs, not just the one targeted by this patchset. Okay, Understood. So you suggest like this? static unsigned long __pll_params_to_rate(unsigned long parent_rate, struct meson_clk_pll_data *pll) { u64 rate = (u64)parent_rate * m; + unsigned int frac_max = (1 << pll->frac.width); if (frac && MESON_PARM_APPLICABLE(&pll->frac)) { u64 frac_rate = (u64)parent_rate * frac; - rate += DIV_ROUND_UP_ULL(frac_rate, - (1 << pll->frac.width)); + if (pll->frac_max) + frac_max = pll->frac_max; + + rate += DIV_ROUND_UP_ULL(frac_rate, frac_max); In my opinion, this change seems more logical, but the amount of change is larger?
On Mon 09 Sep 2024 at 16:46, Chuan Liu <chuan.liu@amlogic.com> wrote: > Hi, Jerome: > > Thank you for your meticulous explanation. > > > On 2024/9/9 15:40, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Mon 09 Sep 2024 at 09:55, Chuan Liu <chuan.liu@amlogic.com> wrote: >> >>> Sorry, I don't quite understand this one. Is it because you suggest keeping >>> >>> "(1 << pll->frac_max)" here, followed by "if" to determine whether to assign >>> >>> "pll->frac_max"? >>> >>> >>> "unlikely" is used here. My idea is that it will be possible to determine >>> the value >>> >>> of "frac_max" at compile time, which will result in one less "if" judgment >>> and >>> >>> slightly improve drive performance. >> I'll rephrase. >> >> Please drop the 'unlikely()' call. >> >> You may add that : >> * in a separate change >> * if you really really wish to >> * if you provide profiling numbers for the different supported >> platforms and PLLs, not just the one targeted by this patchset. > > > Okay, Understood. So you suggest like this? No. drop the call to unlikely(). Keep the rest. That's it. > > static unsigned long __pll_params_to_rate(unsigned long parent_rate, > struct meson_clk_pll_data *pll) > { > u64 rate = (u64)parent_rate * m; > + unsigned int frac_max = (1 << pll->frac.width); > > if (frac && MESON_PARM_APPLICABLE(&pll->frac)) { > u64 frac_rate = (u64)parent_rate * frac; > > - rate += DIV_ROUND_UP_ULL(frac_rate, > - (1 << pll->frac.width)); > + if (pll->frac_max) > + frac_max = pll->frac_max; > + > + rate += DIV_ROUND_UP_ULL(frac_rate, frac_max); > > > In my opinion, this change seems more logical, but the amount of > > change is larger?
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c index bc570a2ff3a3..a141ab450009 100644 --- a/drivers/clk/meson/clk-pll.c +++ b/drivers/clk/meson/clk-pll.c @@ -57,12 +57,13 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate, struct meson_clk_pll_data *pll) { u64 rate = (u64)parent_rate * m; + unsigned int frac_max = unlikely(pll->frac_max) ? pll->frac_max : + (1 << pll->frac.width); if (frac && MESON_PARM_APPLICABLE(&pll->frac)) { u64 frac_rate = (u64)parent_rate * frac; - rate += DIV_ROUND_UP_ULL(frac_rate, - (1 << pll->frac.width)); + rate += DIV_ROUND_UP_ULL(frac_rate, frac_max); } return DIV_ROUND_UP_ULL(rate, n); @@ -100,7 +101,8 @@ static unsigned int __pll_params_with_frac(unsigned long rate, unsigned int n, struct meson_clk_pll_data *pll) { - unsigned int frac_max = (1 << pll->frac.width); + unsigned int frac_max = unlikely(pll->frac_max) ? pll->frac_max : + (1 << pll->frac.width); u64 val = (u64)rate * n; /* Bail out if we are already over the requested rate */ diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h index 7b6b87274073..949157fb7bf5 100644 --- a/drivers/clk/meson/clk-pll.h +++ b/drivers/clk/meson/clk-pll.h @@ -43,6 +43,7 @@ struct meson_clk_pll_data { unsigned int init_count; const struct pll_params_table *table; const struct pll_mult_range *range; + unsigned int frac_max; u8 flags; };