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

[Alexey Charkov]

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.

> 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++).

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
- 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.

> 3) switch to signed integers wherever applicable, not stable candidate
> IMO (but eventually may be backported to facilitate backports of future
> fixes),

See above.

> > 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.

> >       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.

Best regards,
Alexey