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
