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

On Wed, Jul 22, 2026 at 4:59 PM Quentin Schulz <[email protected]> wrote:
>
> On 7/22/26 1:00 PM, Alexey Charkov wrote:
> > Hi Quentin,
> >
> > On Wed, Jul 22, 2026 at 2:35 PM Quentin Schulz <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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 :)

Maybe... I can try to shout loudly when/if I get the email from Greg's
autopicker bot :)

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

Yes, exactly. But at least that logic was internally consistent for
the parallel universe in which k would have been truly unsigned and
treated by the hardware as such, and for that universe the existing
PLL table makes sense. Not for our actual universe though.

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

Fair enough, I will highlight it in the commit description. Thanks for
pointing it out!

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

Ack

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

Ack

> >>>        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 :)

Yes, well that part is unrelated, so it doesn't count :)

> Looks good to me!

Thank you for looking into it and for your feedback!

Best regards,
Alexey