Re: [PATCH v2 1/3] clk: meson: Support PLL with fixed fractional denominators

[Chuan Liu]

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@xxxxxxxxxxx> 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?😮