Re: [PATCH] clk: rockchip: Fractional PLL coefficient on RK3588/RK3576 is two's complement
From: Quentin Schulz
Date: Wed Jul 22 2026 - 09:06:23 EST
On 7/22/26 1:00 PM, Alexey Charkov wrote:
Hi Quentin,
On Wed, Jul 22, 2026 at 2:35 PM Quentin Schulz <quentin.schulz@xxxxxxxxx> wrote:
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 :)
I believe Linus frowns upon changes like "it never worked, but we've
fixed it now" being submitted as fixes. It's been broken for years,
and since nobody complained yet, going via the normal development path
(i.e. -next) seems perfectly fine to me.
My bet is because of the Fixes: it'll get backported anyway even if you don't put Cc: stable. In any case, I don't care too much :)
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,
I could split this one out, but since it's a trivial one-liner, I'd
like to hear what Heiko prefers.
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),
Not related to 1), but directly related to 3). The old table was
calculated using a flawed logic as if the k is unsigned and purely
additive (and thus k > 32767 stayed at the lower value of m), while in
reality the hardware subtracts negative k from m (thus k < 0 should
come with m++).
Yeah ok, I missed that we aren't using the signed decimal value in rockchip_rk3588_pll_recalc_rate() and thus k > 32767 really is used as a positive k, instead of the signed value (thus negative).
This is confirmed by manual recalculation of effective PLL rates under
both approaches:
- If k is treated as unsigned and purely additive (valid logic, but
doesn't match the hardware) the effective rate lands within 40 Hz of
the target for the affected table entries
Valid logic, but the hardware won't actually do that at all and be completely off (as it'll subtract for anything above 32767, as the s16 MSB will be 1).
- If k is treated as signed and subtracted from the next higher m
(similarly valid logic, matches what the TRM says and also what the
updated vendor kernel does) the effective rate lands within 40 Hz of
the target for the updated table entries
- If the parameters are mixed and matched, there is a 2 MHz delta
(five orders of magnitude difference)
So I don't believe that splitting the table updates across commits is helpful.
I agree, because it currently is sound logic but result in broken hardware. I honestly understood the commit log as "the register is s16, so I'll just switch the kernel driver to use that to better match the hardware" (i.e., a cosmetic change). Maybe really highlight that the current approach resulted in completely wrong clock rates (as k cannot be > 32767).
3) switch to signed integers wherever applicable, not stable candidate
IMO (but eventually may be backported to facilitate backports of future
fixes),
See above.
Agreed. A reworded commit log would be nice to highlight they are indeed related.
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.
Looks like an oversight on their end.
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.
This change of theirs is not explained in the commit and is not
related to the code changes they are introducing, so I'm reluctant to
blindly copy it. I also suspect that reducing the magnitude of the
negative k will result in overshooting the requested rate (but haven't
checked).
-15730 results in a -41.504 Hz delta vs. requested
-14419 results in a -32.959 Hz delta vs. requested
So yes, this is proper.
Please specify this in the commit log, since you point at Rockchip's vendor kernel and don't actually implement two's complement with the same values in the table.
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?
It's unrelated and unexplained, so I decided not to change it until a
valid rationale is discovered. Their kernel never used 65535 in the
denominator, FWIW.
It actually did use 65535 for some part, c.f. line 1601 (- column in the diff):
n = div64_u64((uint64_t)ppm * 65535 * fbdiv, 100000);
but that is guarded by CONFIG_ROCKCHIP_CLK_COMPENSATION which isn't enabled except for rv1126_defconfig, so I guess we don't care here indeed :)
Looks good to me!
Cheers,
Quentin