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

[Jerome Brunet <jbrunet@baylibre.com>]

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

-- 
Jerome