Re: [PATCH v2 2/5] clk: rockchip: pll: fix rounding of negative k in RK3588 frac PLL

Hi Alexey,

On 7/30/26 2:25 PM, Alexey Charkov wrote:
> Hi Jonas,
> 
> On Thu, Jul 30, 2026 at 4:16 PM Jonas Karlman <[email protected]> wrote:
>>
>> Hi Alexey,
>>
>> On 7/23/2026 5:04 PM, Alexey Charkov wrote:
>>> Current code uses magical constants when rounding up the magnitude of
>>> negative fractional PLL component k. This leads to overshooting the
>>> requested rate when the calculated fractional part has less than 0.3 in
>>> its decimal part due to failure to round up the fractional part.
>>>
>>> Use a proper rounding up function to avoid overshooting the requested
>>> rate and make the calculation more readable.
>>>
>>> Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll calculation")
>>> Reviewed-by: Quentin Schulz <[email protected]>
>>> Signed-off-by: Alexey Charkov <[email protected]>
>>> ---
>>>   drivers/clk/rockchip/clk_pll.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
>>> index d0df3b8fb49d..69d2d182dcb5 100644
>>> --- a/drivers/clk/rockchip/clk_pll.c
>>> +++ b/drivers/clk/rockchip/clk_pll.c
>>> @@ -11,6 +11,7 @@
>>>   #include <asm/arch-rockchip/hardware.h>
>>>   #include <div64.h>
>>>   #include <linux/delay.h>
>>> +#include <linux/math64.h>
>>>
>>>   static struct rockchip_pll_rate_table rockchip_auto_table;
>>>
>>> @@ -177,7 +178,10 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
>>>        k = ffrac * 65536 / fref;
>>>        if (k > 32767) {
>>>                ffrac = ((m + 1) * fref) - fvco;
>>> -             k = ((ffrac * 65536 * 10 / fref) + 7) / 10;
>>> +             /*
>>> +              * Round up to avoid overshooting requested rate for negative k
>>> +              */
>>> +             k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
>>
>> Use of DIV64_U64_ROUND_UP() seem to cause build errors for Rockchip 32-bit
>> tagets, see CI job at [1].
>>
>> [1] https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.u-boot-project.org%2Fu-boot%2Fcontributors%2Fkwiboo%2Fu-boot%2F-%2Fjobs%2F54629%23L4016&data=05%7C02%7Cquentin.schulz%40cherry.de%7C74dcb2010c7c473b440708deee35b6c5%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C639210111643377177%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=inNBaFIDx51MBU4%2Fefh2jPAQ49esWECfkahMG%2FWC1HI%3D&reserved=0
> 
> Weirdly, the macro is only defined for 64bit [2], even though the
> function it wraps around also exists for other word sizes.
> This calls for a separate fix to take the macro definition to the
> bottom of the header, like Linux does [3].
> 
> [2] https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Fu-boot%2Fv2026.07%2Fsource%2Finclude%2Flinux%2Fmath64.h%23L51-L53&data=05%7C02%7Cquentin.schulz%40cherry.de%7C74dcb2010c7c473b440708deee35b6c5%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C639210111643413367%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=J4lUntNUU4AFPi%2Bd%2BnoP0QKyBHTsEqYj9wQeMluOpxA%3D&reserved=0
> [3] https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv7.2-rc3%2Fsource%2Finclude%2Flinux%2Fmath64.h%23L344-L355&data=05%7C02%7Cquentin.schulz%40cherry.de%7C74dcb2010c7c473b440708deee35b6c5%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C639210111643448158%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=HJuazQjmgof5qWRnTLjY%2Fy2XSkImAm11%2BFb3HPCL8i8%3D&reserved=0
> 
> Thanks for spotting this!
> 

If possible, please simply synchronize with Linux. I'm guessing we need 
include/linux/math.h and include/linux/math64.h? Ideally, use a release 
tag (not -rc, and *definitely* not a random sha :) ).

Cheers,
Quentin