Re: [PATCH] clk: rockchip: Fractional PLL coefficient on RK3588/RK3576 is two's complement
From: Quentin Schulz
Date: Wed Jul 22 2026 - 06:38:25 EST
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@xxxxxxxxxxx>
---
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