Re: [PATCH] clk: rockchip: Fractional PLL coefficient on RK3588/RK3576 is two's complement

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Alexey,

On 7/21/26 9:17 PM, Alexey Charkov wrote:
> When the PLL rates table was first committed for RK3588 (and later reused
> for RK3576), the fractional PLL coefficient was defined as an unsigned
> value, while the TRM clearly states that it is a two's complement 16-bit
> value.
> 
> Rockchip's downstream kernel later revised the fractional PLL code [1] to
> account for the two's complement nature of the coefficient, but that
> change wasn't upstreamed.
> 
> Change the PLL table definition to use two's complement for the
> fractional coefficient and update its users accordingly.
> 
> Note that a negative fractional coefficient is meant to be subtracted from
> the next larger integer multiplier, so the _m values in the table are
> also adjusted accordingly for the two negative-k entries.
> 
> While at it, fix the denominator of the fractional PLL calculation to use
> 65536 instead of 65535, as per the TRM (RK3576 TRM Part 1 V1.2, Section
> 2.13.1.4 Setting Guide on P, M, S, and K):
> 
> Fout = ((m + k/65536) * Fin) / (p * 2^s)
> 
> Link: https://github.com/flipperdevices/rockchip-linux/commit/7a72bc05dcc3a51e85ae531749e6270bf9b9212d [1]
> Fixes: f1c506d152ff ("clk: rockchip: add clock controller for the RK3588")
> Fixes: cc40f5baa91b ("clk: rockchip: Add clock controller for the RK3576")
> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
> Not adding Cc stable, because while this fixes a real bug it's not a
> regression, as the issue was introduced in the same commit that added the
> RK3576/RK3588 support.
> 

I don't think this is a valid reason :)

In any case, this patch is doing too many things at once. I see the 
following things that would warrant individual patches:

1) fix the wrong denominator, stable candidate IMO,
2) fix the table (using unsigned int still), to match what Rockchip did 
in their downstream fork (maybe check they did maths properly first :) ) 
stable candidate IMO, except if they are related to 1) in which case 
squash with 1),
3) switch to signed integers wherever applicable, not stable candidate 
IMO (but eventually may be backported to facilitate backports of future 
fixes),

> Note that there is a separate unrelated issue with the rate table, namely
> the 2256000000 Hz entry currently leads to a VCO frequency of 4512 MHz,
> which is just above the TRM-stated maximum of 4500 MHz. Also multiple
> entries in the table end up with Fvco < 3 GHz, which according to the
> TRM leads to a PLL period jitter of +-2% vs. the +-1% for Fvco > 3 GHz.
> To be revisited separately.
> ---
>   drivers/clk/rockchip/clk-pll.c    | 8 ++++----
>   drivers/clk/rockchip/clk-rk3576.c | 4 ++--
>   drivers/clk/rockchip/clk-rk3588.c | 4 ++--
>   drivers/clk/rockchip/clk.h        | 8 ++++----
>   4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index 6b853800cb6b..f445b01aabd0 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -13,6 +13,7 @@
>   #include <linux/delay.h>
>   #include <linux/clk-provider.h>
>   #include <linux/iopoll.h>
> +#include <linux/math64.h>
>   #include <linux/regmap.h>
>   #include <linux/clk.h>
>   #include "clk.h"
> @@ -913,11 +914,10 @@ static unsigned long rockchip_rk3588_pll_recalc_rate(struct clk_hw *hw, unsigned
>   
>   	if (cur.k) {
>   		/* fractional mode */
> -		u64 frac_rate64 = prate * cur.k;
> +		s64 frac_rate64 = (s64)prate * cur.k;
>   
> -		postdiv = cur.p * 65535;
> -		do_div(frac_rate64, postdiv);
> -		rate64 += frac_rate64;
> +		postdiv = cur.p * 65536;
> +		rate64 += div_s64(frac_rate64, postdiv);
>   	}
>   	rate64 = rate64 >> cur.s;
>   
> diff --git a/drivers/clk/rockchip/clk-rk3576.c b/drivers/clk/rockchip/clk-rk3576.c
> index 2557358e0b9d..63f229e73a45 100644
> --- a/drivers/clk/rockchip/clk-rk3576.c
> +++ b/drivers/clk/rockchip/clk-rk3576.c
> @@ -79,13 +79,13 @@ static struct rockchip_pll_rate_table rk3576_pll_rates[] = {
>   	RK3588_PLL_RATE(1008000000, 2, 336, 2, 0),
>   	RK3588_PLL_RATE(1000000000, 3, 500, 2, 0),
>   	RK3588_PLL_RATE(983040000, 4, 655, 2, 23592),
> -	RK3588_PLL_RATE(955520000, 3, 477, 2, 49806),
> +	RK3588_PLL_RATE(955520000, 3, 478, 2, -15730),
>   	RK3588_PLL_RATE(903168000, 6, 903, 2, 11009),
>   	RK3588_PLL_RATE(900000000, 2, 300, 2, 0),
>   	RK3588_PLL_RATE(816000000, 2, 272, 2, 0),
>   	RK3588_PLL_RATE(786432000, 2, 262, 2, 9437),
>   	RK3588_PLL_RATE(786000000, 1, 131, 2, 0),
> -	RK3588_PLL_RATE(785560000, 3, 392, 2, 51117),
> +	RK3588_PLL_RATE(785560000, 3, 393, 2, -14419),
>   	RK3588_PLL_RATE(722534400, 8, 963, 2, 24850),
>   	RK3588_PLL_RATE(600000000, 2, 200, 2, 0),
>   	RK3588_PLL_RATE(594000000, 2, 198, 2, 0),

For some reason Rockchip didn't fix this for RK3576 in their vendor 
kernel, so it's still using the value from RK3588 from before the commit 
you pointed at.

> diff --git a/drivers/clk/rockchip/clk-rk3588.c b/drivers/clk/rockchip/clk-rk3588.c
> index 75d42fea2a11..24baa0ef9bf3 100644
> --- a/drivers/clk/rockchip/clk-rk3588.c
> +++ b/drivers/clk/rockchip/clk-rk3588.c
> @@ -79,14 +79,14 @@ static struct rockchip_pll_rate_table rk3588_pll_rates[] = {
>   	RK3588_PLL_RATE(1008000000, 2, 336, 2, 0),
>   	RK3588_PLL_RATE(1000000000, 3, 500, 2, 0),
>   	RK3588_PLL_RATE(983040000, 4, 655, 2, 23592),
> -	RK3588_PLL_RATE(955520000, 3, 477, 2, 49806),
> +	RK3588_PLL_RATE(955520000, 3, 478, 2, -15730),
>   	RK3588_PLL_RATE(903168000, 6, 903, 2, 11009),
>   	RK3588_PLL_RATE(900000000, 2, 300, 2, 0),
>   	RK3588_PLL_RATE(850000000, 3, 425, 2, 0),
>   	RK3588_PLL_RATE(816000000, 2, 272, 2, 0),
>   	RK3588_PLL_RATE(786432000, 2, 262, 2, 9437),
>   	RK3588_PLL_RATE(786000000, 1, 131, 2, 0),
> -	RK3588_PLL_RATE(785560000, 3, 392, 2, 51117),
> +	RK3588_PLL_RATE(785560000, 3, 393, 2, -14419),

Are you sure this is proper? Rockchip changed 51117 to 51119 (so -14419 
to -14417) and 49806 to 49807 (so -15730 to -15729) in the commit you 
linked.

>   	RK3588_PLL_RATE(722534400, 8, 963, 2, 24850),
>   	RK3588_PLL_RATE(600000000, 2, 200, 2, 0),
>   	RK3588_PLL_RATE(594000000, 2, 198, 2, 0),

In the commit you provided, they also change this line (though they 
don't change k, so unsure why (if) that is related). Wondering if this 
isn't related to the denominator fix they also have done in the same commit?

Cheers,
Quentin