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

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

[Alexey Charkov]

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.

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),
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),
 	RK3588_PLL_RATE(722534400, 8, 963, 2, 24850),
 	RK3588_PLL_RATE(600000000, 2, 200, 2, 0),
 	RK3588_PLL_RATE(594000000, 2, 198, 2, 0),
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 9e3503e2ffc2..72b36bba3152 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -635,10 +635,10 @@ struct rockchip_pll_rate_table {
 		};
 		struct {
 			/* for RK3588 */
-			unsigned int m;
-			unsigned int p;
-			unsigned int s;
-			unsigned int k;
+			unsigned int m;	/* main divider, 10 bit unsigned */
+			unsigned int p;	/* pre-divider, 6 bit unsigned */
+			unsigned int s;	/* scaler, 3 bit unsigned */
+			s16 k;		/* fractional part, 16 bit two's complement */
 		};
 	};
 };

---
base-commit: 290aaf24a551d5a0dce037e3fab30820f9113a10
change-id: 20260721-rk3588-fracpll-f4377cd80983

Best regards,
--  
Alexey Charkov <alchark@flipper.net>

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

[Quentin Schulz]

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

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

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

> 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

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

[Quentin Schulz]

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

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

[Alexey Charkov]

On Wed, Jul 22, 2026 at 4:59 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> 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 :)

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

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

[Sebastian Reichel]

Hello Alexey,

On Wed, Jul 22, 2026 at 02:59:25PM +0200, Quentin Schulz wrote:
> On 7/22/26 1:00 PM, Alexey Charkov wrote:
> > On Wed, Jul 22, 2026 at 2:35 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > > On 7/21/26 9:17 PM, Alexey Charkov wrote:
> > > > 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 :)

FWIW you are mixing up things. Linus does not handle the stable
kernels. He does the release candidates and the initial version of
every kernel release. The stable releases are handled by the stable
maintainers. Linus only said, that he does not want to get these
kind of fixes during late -rc phase. That makes sense considering
fixes also have a chance of introducing regressions.

But if patches end up in a pull request for an -rc kernel or in the
pull request for the next merge window depends on the maintainer
applying the patch (i.e. Heiko). The maintainer can queue a patch
with all those tags into the normal for-next queue targeting the
following merge window. This results in the patch being in
linux-next for a while, then being added to the master branch during
a merge window and then being added to the stable trees (as stable
only picks patches that are in mainline).

The maintainer can also merge a patch to the queue for -rc kernels
without the Cc stable. That is exactly what happens for a regression
from a normal patch in the merge window, since the affected patch
wouldn't be in any stable kernel.

TLDR: The Cc stable tag has nothing to do with Linus.

Greetings,

-- Sebastian

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

[Alexey Charkov]

Hi Sebastian,

On Wed, Jul 22, 2026 at 5:44 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hello Alexey,
>
> On Wed, Jul 22, 2026 at 02:59:25PM +0200, Quentin Schulz wrote:
> > On 7/22/26 1:00 PM, Alexey Charkov wrote:
> > > On Wed, Jul 22, 2026 at 2:35 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > > > On 7/21/26 9:17 PM, Alexey Charkov wrote:
> > > > > 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 :)
>
> FWIW you are mixing up things. Linus does not handle the stable
> kernels. He does the release candidates and the initial version of
> every kernel release. The stable releases are handled by the stable
> maintainers. Linus only said, that he does not want to get these
> kind of fixes during late -rc phase. That makes sense considering
> fixes also have a chance of introducing regressions.
>
> But if patches end up in a pull request for an -rc kernel or in the
> pull request for the next merge window depends on the maintainer
> applying the patch (i.e. Heiko). The maintainer can queue a patch
> with all those tags into the normal for-next queue targeting the
> following merge window. This results in the patch being in
> linux-next for a while, then being added to the master branch during
> a merge window and then being added to the stable trees (as stable
> only picks patches that are in mainline).
>
> The maintainer can also merge a patch to the queue for -rc kernels
> without the Cc stable. That is exactly what happens for a regression
> from a normal patch in the merge window, since the affected patch
> wouldn't be in any stable kernel.
>
> TLDR: The Cc stable tag has nothing to do with Linus.

Indeed, I made a logical leap here. In my view, however, this patch
doesn't need to go into 7.2 fixes (it wasn't broken during the 7.2
merge window), nor necessarily into stable backports (no-one seems to
have used the affected PLL rates in older kernels, so there is nothing
to fix for them either).

However, it will become relevant for driving arbitrary display clocks
for DP video output because several display modes (and consequently,
PLL rates) I needed for those monitors I tested require new table
entries with negative PLL coefficients. Those would be 7.3+ material
anyway.

Best regards,
Alexey

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3619 bytes --]

Hello Alexey,

On Wed, Jul 22, 2026 at 05:54:15PM +0400, Alexey Charkov wrote:
> On Wed, Jul 22, 2026 at 5:44 PM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> > On Wed, Jul 22, 2026 at 02:59:25PM +0200, Quentin Schulz wrote:
> > > On 7/22/26 1:00 PM, Alexey Charkov wrote:
> > > > On Wed, Jul 22, 2026 at 2:35 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > > > > On 7/21/26 9:17 PM, Alexey Charkov wrote:
> > > > > > 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 :)
> >
> > FWIW you are mixing up things. Linus does not handle the stable
> > kernels. He does the release candidates and the initial version of
> > every kernel release. The stable releases are handled by the stable
> > maintainers. Linus only said, that he does not want to get these
> > kind of fixes during late -rc phase. That makes sense considering
> > fixes also have a chance of introducing regressions.
> >
> > But if patches end up in a pull request for an -rc kernel or in the
> > pull request for the next merge window depends on the maintainer
> > applying the patch (i.e. Heiko). The maintainer can queue a patch
> > with all those tags into the normal for-next queue targeting the
> > following merge window. This results in the patch being in
> > linux-next for a while, then being added to the master branch during
> > a merge window and then being added to the stable trees (as stable
> > only picks patches that are in mainline).
> >
> > The maintainer can also merge a patch to the queue for -rc kernels
> > without the Cc stable. That is exactly what happens for a regression
> > from a normal patch in the merge window, since the affected patch
> > wouldn't be in any stable kernel.
> >
> > TLDR: The Cc stable tag has nothing to do with Linus.
> 
> Indeed, I made a logical leap here. In my view, however, this patch
> doesn't need to go into 7.2 fixes (it wasn't broken during the 7.2
> merge window), nor necessarily into stable backports (no-one seems to
> have used the affected PLL rates in older kernels, so there is nothing
> to fix for them either).
> 
> However, it will become relevant for driving arbitrary display clocks
> for DP video output because several display modes (and consequently,
> PLL rates) I needed for those monitors I tested require new table
> entries with negative PLL coefficients. Those would be 7.3+ material
> anyway.

That's a different argument. FWIW I don't see any issues with not
Cc'ing stable. I suggest to put that info into the commit
description. E.g.:

While this patch fixes a few PLL rates, there are no known users of
the affected rates in the mainline kernel. Thus there is no need to
backport this. The fixed rate calculation will become relevant for
driving arbitrary display clocks for DP video output in the future.

That helps anyone to understand what is going on regarding stable
backport (incl. the bot).

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Heiko Stübner]

Am Mittwoch, 22. Juli 2026, 13:00:47 Mitteleuropäische Sommerzeit schrieb Alexey Charkov:
> 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.

Personally, I also don't think this _needs_ a stable tag.
The whole if it ain't broke thing ....

If stable-maintainers do think it valuable on their own (without stable tag)
then by all means, but changing how the PLL rate stuff works has the
_ability_ to introduce other behaviour _somewhere_.


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

yes please.
- Subject: Foo and Bar
- Commit message with "while at it"
are clear indicators that the change wants to get split ;-)

So please split out that "while at it" part. The rest can stay
together though.


Thanks
Heiko