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

Hi Quentin,

On Thu, Jul 30, 2026 at 4:56 PM Quentin Schulz <[email protected]> wrote:
>
> 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 :) ).

Just checked v7.1 in greater detail. We won't need <linux/math.h>
because the only thing we need out of there for this header is abs64()
which lives in U-Boot's <linux/kernel.h>. Nor the vdso include (I can
instead inline __iter_div_u64_rem into its only user, which is in turn
never used in current mainline U-Boot but might be good to keep for
API consistency vs. Linux).

Some precursor fixups will be necessary, so let me take a stab at it
and send as a proper series.

Best regards,
Alexey