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

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

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@cherry.de> 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@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 :)
> 
> 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