Re: [PATCH v2 1/3] clk: meson: Support PLL with fixed fractional denominators
From: Jerome Brunet
Date: Mon Sep 09 2024 - 04:51:06 EST
On Mon 09 Sep 2024 at 16:46, Chuan Liu <chuan.liu@xxxxxxxxxxx> 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@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?
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