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
