Re: [PATCH 3/6] clk: rockchip: pll: fix RK3588 frac PLL result for k=-32768

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

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> Current code needlessly sets the k value to 0 when it is calculated as
> -32768, which is a valid value for the RK3588 frac PLL. This results in
> the PLL output frequency being higher than requested when the requested
> frequency is exactly halfway between two integer-multiplier PLL output
> frequencies.
> 
> Negative k values can never go below -32768 either, because that case is
> handled just above this code, so the check for k > 32767 is redundant.
> 

and because we add 1 to m, which is eventually multiplied by 65536 and 
thus if k is >32767 before adding 1 to m, it can only be <=32768 after 
adding 1 to m as we also invert the sign of k.

> What remains of the if statement is a hand-rolled two's complement
> negation of the result, so write it out as such for clarity, and return
> the true S16 type of k as specified in the TRM.
> 
> 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 | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
> index 69d2d182dcb5..c6fbeb71c77a 100644
> --- a/drivers/clk/rockchip/clk_pll.c
> +++ b/drivers/clk/rockchip/clk_pll.c
> @@ -167,11 +167,11 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
>   	return rate_table;
>   }
>   
> -static u32
> +static s16
>   rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
>   {
>   	u64 fref, ffrac;
> -	u32 k = 0;
> +	int k;
>   
>   	fref = fin_hz / p;
>   	ffrac = fvco - (m * fref);
> @@ -181,11 +181,7 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
>   		/*
>   		 * Round up to avoid overshooting requested rate for negative k
>   		 */
> -		k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
> -		if (k > 32767)
> -			k = 0;
> -		else
> -			k = ~k + 1;
> +		k = -(int)DIV64_U64_ROUND_UP(ffrac * 65536, fref);

I don't like migrating k to s16 in multiple commits, especially since 
there's also a mix of int/s16 in there. It's quite confusing.

It's kinda bad to store the return value of this function and then do 
some additional based on its value and set the rate_table based on it. 
Considering the next patch, I think it also itches you :)
I'm thinking to move the whole rate_table->X assignment within 
rockchip_rk3588_pll_k_get(), also pass rate_table pointer as argument 
and simply return 0 if it worked, 1 (or -EINVAL or whatever) otherwise 
and have the for-loop return rate_table if rockchip_rk3588_pll_k_get() 
returns 0.

Keep the current type when moving the assignments into the function, 
then migrate rate_table->k to be an s16 (including the struct 
definition) in another commit, then fix the k=-32768 case. I think it's 
clearer that way and avoid implicit casts and a mix of signed and 
unsigned types.

Cheers,
Quentin