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

[Quentin Schulz via U-Boot <u-boot@lists.u-boot-project.org>]

Hi Alexey,

Resending so it hits the ML because I once again triggered the spam 
filter by using the "wrong" mail address.

On 7/13/26 8:35 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.
 >

The choice of magic numbers sure is odd.

 > Use a proper rounding up function to avoid overshooting the requested
 > rate and make the calculation more readable.
 >

Just for posterity, ffrac is guaranteed to not underflow in the if 
condition because the caller of rockchip_rk3588_pll_k_get() actually 
check that fvco < ffrac (computed with m+1).

ffrac * 65536 won't overflow because ffrac can be roughly max 1024 * 
fin_hz, so fin_hz needs to be U64_MAX / 1024 / 65536 which is 256GHz.

Therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

 > Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll 
calculation")
 > Signed-off-by: Alexey Charkov <alchark@flipper.net>
 > ---
 >   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);
 >           if (k > 32767)
 >               k = 0;
 >           else
 >