[PATCH 0/6] clk: rockchip: pll: fixes and simplification of maths in RK3588 frac PLL

[PATCH 0/6] clk: rockchip: pll: fixes and simplification of maths in RK3588 frac PLL

[Alexey Charkov]

Current fractional PLL code for RK3588 (also applies to RK3576) has two
issues causing it to overshoot the requested frequency for some negative
values of the fractional coefficient: one due to rounding, the other due
to treating -32768 as an invalid value where it is actually valid. It
also overflows the calculation of the fractional PLL rate when reading
it back, causing the function to return wrong PLL frequencies for non-
trivial values of the fractional coefficient.

Fix all three, and also use native signed integer arithmetics to avoid
manual bitwise operations on the unsigned representation of the fractional
coefficient, which simplifies the code and makes it easier to read.

The commits are deliberately split into excruciating granularity to
ensure each step is explained and easy to review. The final diff is not
too large though, so I'd be happy to squash as required.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
Alexey Charkov (6):
      clk: rockchip: pll: drop misleading fout in rockchip_rk3588_pll_k_get()
      clk: rockchip: pll: fix rounding of negative k in RK3588 frac PLL
      clk: rockchip: pll: fix RK3588 frac PLL result for k=-32768
      clk: rockchip: pll: let rockchip_rk3588_pll_k_get update m directly
      clk: rockchip: pll: fractional PLL coefficient is two's complement
      clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

 arch/arm/include/asm/arch-rockchip/clock.h |  2 +-
 drivers/clk/rockchip/clk_pll.c             | 70 ++++++++++++------------------
 2 files changed, 29 insertions(+), 43 deletions(-)
---
base-commit: 6741b0dfb41dc82a284ab1cff4c58af6ef2f3f9c
change-id: 20260713-rk3588-fracpll-7b3e1f817c7d

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

[PATCH 1/6] clk: rockchip: pll: drop misleading fout in rockchip_rk3588_pll_k_get()

[Alexey Charkov]

The calculation in rockchip_rk3588_pll_k_get() uses an intermediate
variable misleadingly named fout, which is not the output frequency.

Drop it and inline its calculation into the k assignment, which also
improves clarity. While at that, also drop the redundant reassignment of
fref which doesn't change within this function.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/clk/rockchip/clk_pll.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index 9dec40b1fe83..d0df3b8fb49d 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -169,18 +169,15 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
 static u32
 rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
 {
-	u64 fref, fout, ffrac;
+	u64 fref, ffrac;
 	u32 k = 0;
 
 	fref = fin_hz / p;
 	ffrac = fvco - (m * fref);
-	fout = ffrac * 65536;
-	k = fout / fref;
+	k = ffrac * 65536 / fref;
 	if (k > 32767) {
-		fref = fin_hz / p;
 		ffrac = ((m + 1) * fref) - fvco;
-		fout = ffrac * 65536;
-		k = ((fout * 10 / fref) + 7) / 10;
+		k = ((ffrac * 65536 * 10 / fref) + 7) / 10;
 		if (k > 32767)
 			k = 0;
 		else

-- 
2.53.0

[PATCH 2/6] clk: rockchip: pll: fix rounding of negative k in RK3588 frac PLL

[Alexey Charkov]

Current code uses magical constants when rounding up the magnitude of
negative fractional PLL component k. This leads to overshooting the
requested rate when the calculated fractional part has less than 0.3 in
its decimal part due to failure to round up the fractional part.

Use a proper rounding up function to avoid overshooting the requested
rate and make the calculation more readable.

Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll calculation")
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/clk/rockchip/clk_pll.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index d0df3b8fb49d..69d2d182dcb5 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -11,6 +11,7 @@
 #include <asm/arch-rockchip/hardware.h>
 #include <div64.h>
 #include <linux/delay.h>
+#include <linux/math64.h>
 
 static struct rockchip_pll_rate_table rockchip_auto_table;
 
@@ -177,7 +178,10 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
 	k = ffrac * 65536 / fref;
 	if (k > 32767) {
 		ffrac = ((m + 1) * fref) - fvco;
-		k = ((ffrac * 65536 * 10 / fref) + 7) / 10;
+		/*
+		 * Round up to avoid overshooting requested rate for negative k
+		 */
+		k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
 		if (k > 32767)
 			k = 0;
 		else

-- 
2.53.0

[PATCH 3/6] clk: rockchip: pll: fix RK3588 frac PLL result for k=-32768

[Alexey Charkov]

Current code needlessly sets the k value to 0 when it is calculated as
-32768, which is a valid value for the RK3588 frac PLL. This results in
the PLL output frequency being higher than requested when the requested
frequency is exactly halfway between two integer-multiplier PLL output
frequencies.

Negative k values can never go below -32768 either, because that case is
handled just above this code, so the check for k > 32767 is redundant.

What remains of the if statement is a hand-rolled two's complement
negation of the result, so write it out as such for clarity, and return
the true S16 type of k as specified in the TRM.

Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll calculation")
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/clk/rockchip/clk_pll.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index 69d2d182dcb5..c6fbeb71c77a 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -167,11 +167,11 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
 	return rate_table;
 }
 
-static u32
+static s16
 rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
 {
 	u64 fref, ffrac;
-	u32 k = 0;
+	int k;
 
 	fref = fin_hz / p;
 	ffrac = fvco - (m * fref);
@@ -181,11 +181,7 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
 		/*
 		 * Round up to avoid overshooting requested rate for negative k
 		 */
-		k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
-		if (k > 32767)
-			k = 0;
-		else
-			k = ~k + 1;
+		k = -(int)DIV64_U64_ROUND_UP(ffrac * 65536, fref);
 	}
 	return k;
 }

-- 
2.53.0

[PATCH 4/6] clk: rockchip: pll: let rockchip_rk3588_pll_k_get update m directly

[Alexey Charkov]

Selecting the next integer multiplier m is coupled to setting a negative
fractional coefficient k. The current code checks for negative k in two
separate places, which is error-prone.

Let rockchip_rk3588_pll_k_get update m directly, to make it the single
source of truth for the final value of the integer multiplier m, which
also reduces the number of scattered conditional branches in the code.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/clk/rockchip/clk_pll.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index c6fbeb71c77a..6324c11091af 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -168,16 +168,21 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
 }
 
 static s16
-rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
+rockchip_rk3588_pll_k_get(u32 *m, u32 p, u32 s, u64 fin_hz, u64 fvco)
 {
 	u64 fref, ffrac;
 	int k;
 
 	fref = fin_hz / p;
-	ffrac = fvco - (m * fref);
+	ffrac = fvco - (*m) * fref;
 	k = ffrac * 65536 / fref;
 	if (k > 32767) {
-		ffrac = ((m + 1) * fref) - fvco;
+		/*
+		 * The requested rate is closer to the next integer multiplier
+		 * m, so pick it and use a negative fractional coefficient k
+		 */
+		*m += 1;
+		ffrac = (*m) * fref - fvco;
 		/*
 		 * Round up to avoid overshooting requested rate for negative k
 		 */
@@ -201,7 +206,10 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
 			for (m = 64; m <= 1023; m++) {
 				if ((fvco >= m * fin_hz / p) &&
 				    (fvco < (m + 1) * fin_hz / p)) {
-					k = rockchip_rk3588_pll_k_get(m, p, s,
+					u32 m_tmp = m;
+
+					k = rockchip_rk3588_pll_k_get(&m_tmp,
+								      p, s,
 								      fin_hz,
 								      fvco);
 					if (!k)
@@ -209,10 +217,7 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
 					rate_table->p = p;
 					rate_table->s = s;
 					rate_table->k = k;
-					if (k > 32767)
-						rate_table->m = m + 1;
-					else
-						rate_table->m = m;
+					rate_table->m = m_tmp;
 					return rate_table;
 				}
 			}

-- 
2.53.0

[PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Alexey Charkov]

The TRM defines the fractional PLL adjustment coefficient as a signed
two's complement number, 16 bits wide, so store it as such to avoid
confusion.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
 drivers/clk/rockchip/clk_pll.c             | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
index 95b08bfd046f..f9bfdfb8a6a3 100644
--- a/arch/arm/include/asm/arch-rockchip/clock.h
+++ b/arch/arm/include/asm/arch-rockchip/clock.h
@@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
 	unsigned int m;
 	unsigned int p;
 	unsigned int s;
-	unsigned int k;
+	int k;
 };
 
 enum rockchip_pll_type {
diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index 6324c11091af..ebe90b55ea06 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -195,8 +195,9 @@ static struct rockchip_pll_rate_table *
 rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
 {
 	struct rockchip_pll_rate_table *rate_table = &rockchip_auto_table;
-	u32 p, m, s, k;
+	u32 p, m, s;
 	u64 fvco;
+	s16 k;
 
 	for (s = 0; s <= 6; s++) {
 		fvco = (u64)fout_hz << s;

-- 
2.53.0

[PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Alexey Charkov]

Current code calculates the fractional component in 32 bits before
assigning it to a 64-bit holding variable, causing overflow for real-world
values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
massaging of an unsigned representation of what is actually a two's
complement signed value, which is confusing and makes the code harder to
read.

Read k into a properly signed type and promote operands to avoid overflow,
which also enables the use of div_s64() to express the math more clearly.

Fixes: b851c006a150 ("clk: rockchip: pll: Add pll_rk3588 type for rk3588")
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/clk/rockchip/clk_pll.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
index ebe90b55ea06..9d529242be31 100644
--- a/drivers/clk/rockchip/clk_pll.c
+++ b/drivers/clk/rockchip/clk_pll.c
@@ -559,9 +559,10 @@ static int rk3588_pll_set_rate(struct rockchip_pll_clock *pll,
 static ulong rk3588_pll_get_rate(struct rockchip_pll_clock *pll,
 				 void __iomem *base, ulong pll_id)
 {
-	u32 m, p, s, k;
+	u32 m, p, s;
 	u32 con = 0, shift, mode;
-	u64 rate, postdiv;
+	u64 rate;
+	s16 k;
 
 	con = readl(base + pll->mode_offset);
 	shift = pll->mode_shift;
@@ -588,25 +589,7 @@ static ulong rk3588_pll_get_rate(struct rockchip_pll_clock *pll,
 
 		rate = OSC_HZ / p;
 		rate *= m;
-		if (k & BIT(15)) {
-			/* fractional mode */
-			u64 frac_rate64;
-
-			k = (~(k - 1)) & RK3588_PLLCON2_K_MASK;
-			frac_rate64 = OSC_HZ * k;
-			postdiv = p;
-			postdiv *= 65536;
-			do_div(frac_rate64, postdiv);
-			rate -= frac_rate64;
-		} else {
-			/* fractional mode */
-			u64 frac_rate64 = OSC_HZ * k;
-
-			postdiv = p;
-			postdiv *= 65536;
-			do_div(frac_rate64, postdiv);
-			rate += frac_rate64;
-		}
+		rate += div_s64((s64)OSC_HZ * k, p * 65536);
 		rate = rate >> s;
 		return rate;
 	case RKCLK_PLL_MODE_DEEP:

-- 
2.53.0

Re: [PATCH 1/6] clk: rockchip: pll: drop misleading fout in rockchip_rk3588_pll_k_get()

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> The calculation in rockchip_rk3588_pll_k_get() uses an intermediate
> variable misleadingly named fout, which is not the output frequency.
> 
> Drop it and inline its calculation into the k assignment, which also
> improves clarity. While at that, also drop the redundant reassignment of
> fref which doesn't change within this function.
> 

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 3/6] clk: rockchip: pll: fix RK3588 frac PLL result for k=-32768

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> Current code needlessly sets the k value to 0 when it is calculated as
> -32768, which is a valid value for the RK3588 frac PLL. This results in
> the PLL output frequency being higher than requested when the requested
> frequency is exactly halfway between two integer-multiplier PLL output
> frequencies.
> 
> Negative k values can never go below -32768 either, because that case is
> handled just above this code, so the check for k > 32767 is redundant.
> 

and because we add 1 to m, which is eventually multiplied by 65536 and 
thus if k is >32767 before adding 1 to m, it can only be <=32768 after 
adding 1 to m as we also invert the sign of k.

> What remains of the if statement is a hand-rolled two's complement
> negation of the result, so write it out as such for clarity, and return
> the true S16 type of k as specified in the TRM.
> 
> Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll calculation")
> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
>   drivers/clk/rockchip/clk_pll.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
> index 69d2d182dcb5..c6fbeb71c77a 100644
> --- a/drivers/clk/rockchip/clk_pll.c
> +++ b/drivers/clk/rockchip/clk_pll.c
> @@ -167,11 +167,11 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
>   	return rate_table;
>   }
>   
> -static u32
> +static s16
>   rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
>   {
>   	u64 fref, ffrac;
> -	u32 k = 0;
> +	int k;
>   
>   	fref = fin_hz / p;
>   	ffrac = fvco - (m * fref);
> @@ -181,11 +181,7 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
>   		/*
>   		 * Round up to avoid overshooting requested rate for negative k
>   		 */
> -		k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
> -		if (k > 32767)
> -			k = 0;
> -		else
> -			k = ~k + 1;
> +		k = -(int)DIV64_U64_ROUND_UP(ffrac * 65536, fref);

I don't like migrating k to s16 in multiple commits, especially since 
there's also a mix of int/s16 in there. It's quite confusing.

It's kinda bad to store the return value of this function and then do 
some additional based on its value and set the rate_table based on it. 
Considering the next patch, I think it also itches you :)
I'm thinking to move the whole rate_table->X assignment within 
rockchip_rk3588_pll_k_get(), also pass rate_table pointer as argument 
and simply return 0 if it worked, 1 (or -EINVAL or whatever) otherwise 
and have the for-loop return rate_table if rockchip_rk3588_pll_k_get() 
returns 0.

Keep the current type when moving the assignments into the function, 
then migrate rate_table->k to be an s16 (including the struct 
definition) in another commit, then fix the k=-32768 case. I think it's 
clearer that way and avoid implicit casts and a mix of signed and 
unsigned types.

Cheers,
Quentin

Re: [PATCH 4/6] clk: rockchip: pll: let rockchip_rk3588_pll_k_get update m directly

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> Selecting the next integer multiplier m is coupled to setting a negative
> fractional coefficient k. The current code checks for negative k in two
> separate places, which is error-prone.
> 

Yeah and it's kinda ugly :)

> Let rockchip_rk3588_pll_k_get update m directly, to make it the single
> source of truth for the final value of the integer multiplier m, which
> also reduces the number of scattered conditional branches in the code.
> 
> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
>   drivers/clk/rockchip/clk_pll.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
> index c6fbeb71c77a..6324c11091af 100644
> --- a/drivers/clk/rockchip/clk_pll.c
> +++ b/drivers/clk/rockchip/clk_pll.c
> @@ -168,16 +168,21 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
>   }
>   
>   static s16
> -rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
> +rockchip_rk3588_pll_k_get(u32 *m, u32 p, u32 s, u64 fin_hz, u64 fvco)
>   {
>   	u64 fref, ffrac;
>   	int k;
>   
>   	fref = fin_hz / p;
> -	ffrac = fvco - (m * fref);
> +	ffrac = fvco - (*m) * fref;
>   	k = ffrac * 65536 / fref;
>   	if (k > 32767) {
> -		ffrac = ((m + 1) * fref) - fvco;
> +		/*
> +		 * The requested rate is closer to the next integer multiplier
> +		 * m, so pick it and use a negative fractional coefficient k
> +		 */
> +		*m += 1;
> +		ffrac = (*m) * fref - fvco;
>   		/*
>   		 * Round up to avoid overshooting requested rate for negative k
>   		 */
> @@ -201,7 +206,10 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
>   			for (m = 64; m <= 1023; m++) {
>   				if ((fvco >= m * fin_hz / p) &&
>   				    (fvco < (m + 1) * fin_hz / p)) {
> -					k = rockchip_rk3588_pll_k_get(m, p, s,
> +					u32 m_tmp = m;
> +
> +					k = rockchip_rk3588_pll_k_get(&m_tmp,
> +								      p, s,
>   								      fin_hz,
>   								      fvco);
>   					if (!k)
> @@ -209,10 +217,7 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
>   					rate_table->p = p;
>   					rate_table->s = s;
>   					rate_table->k = k;
> -					if (k > 32767)
> -						rate_table->m = m + 1;
> -					else
> -						rate_table->m = m;
> +					rate_table->m = m_tmp;
>   					return rate_table;

I don't like this much more, it's a halfway-solution to me. I suggest to 
pass rate_table pointer directly, instead of only m, and update 
rate_table m, p, s and k if we can find a k that works, otherwise leave 
the pointer untouched. Change the function to return 0 on success, 1 (or 
-EINVAL) otherwise and return rate_table in the for-loop if 0 is 
returned, otherwise continue.

Cheers,
Quentin

Re: [PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> The TRM defines the fractional PLL adjustment coefficient as a signed
> two's complement number, 16 bits wide, so store it as such to avoid
> confusion.
> 

Yet...

> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
>   arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
>   drivers/clk/rockchip/clk_pll.c             | 3 ++-
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
> index 95b08bfd046f..f9bfdfb8a6a3 100644
> --- a/arch/arm/include/asm/arch-rockchip/clock.h
> +++ b/arch/arm/include/asm/arch-rockchip/clock.h
> @@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
>   	unsigned int m;
>   	unsigned int p;
>   	unsigned int s;
> -	unsigned int k;
> +	int k;

... you use int here instead of s16, any specific reason?

Cheers,
Quentin

Re: [PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Alexey Charkov via U-Boot]

Hi Quentin,

On Tue, Jul 21, 2026 at 4:20 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> > The TRM defines the fractional PLL adjustment coefficient as a signed
> > two's complement number, 16 bits wide, so store it as such to avoid
> > confusion.
> >
>
> Yet...
>
> > Signed-off-by: Alexey Charkov <alchark@flipper.net>
> > ---
> >   arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
> >   drivers/clk/rockchip/clk_pll.c             | 3 ++-
> >   2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
> > index 95b08bfd046f..f9bfdfb8a6a3 100644
> > --- a/arch/arm/include/asm/arch-rockchip/clock.h
> > +++ b/arch/arm/include/asm/arch-rockchip/clock.h
> > @@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
> >       unsigned int m;
> >       unsigned int p;
> >       unsigned int s;
> > -     unsigned int k;
> > +     int k;
>
> ... you use int here instead of s16, any specific reason?

Yes. What matters here is the signedness. The table value never gets
written to or read from the hardware without accessor functions, which
mask on writes and sign-extend on reads anyway. A generic 'int'
usually performs better than a fixed-width type because it aligns
better and requires fewer instructions for arithmetic.

It also reduces potential churn if this table definition is ever
reused for another SoC with a different width for the k coefficient,
but this latter point is more theoretical.

Shall I reword the commit description accordingly?

Happy to set the type to s16 if you believe it's more expressive,
though; we aren't doing much arithmetic on the table values anyway.

Best regards,
Alexey

Re: [PATCH 4/6] clk: rockchip: pll: let rockchip_rk3588_pll_k_get update m directly

[Alexey Charkov via U-Boot]

Hi Quentin,

On Tue, Jul 21, 2026 at 4:19 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> > Selecting the next integer multiplier m is coupled to setting a negative
> > fractional coefficient k. The current code checks for negative k in two
> > separate places, which is error-prone.
> >
>
> Yeah and it's kinda ugly :)
>
> > Let rockchip_rk3588_pll_k_get update m directly, to make it the single
> > source of truth for the final value of the integer multiplier m, which
> > also reduces the number of scattered conditional branches in the code.
> >
> > Signed-off-by: Alexey Charkov <alchark@flipper.net>
> > ---
> >   drivers/clk/rockchip/clk_pll.c | 21 +++++++++++++--------
> >   1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
> > index c6fbeb71c77a..6324c11091af 100644
> > --- a/drivers/clk/rockchip/clk_pll.c
> > +++ b/drivers/clk/rockchip/clk_pll.c
> > @@ -168,16 +168,21 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
> >   }
> >
> >   static s16
> > -rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
> > +rockchip_rk3588_pll_k_get(u32 *m, u32 p, u32 s, u64 fin_hz, u64 fvco)
> >   {
> >       u64 fref, ffrac;
> >       int k;
> >
> >       fref = fin_hz / p;
> > -     ffrac = fvco - (m * fref);
> > +     ffrac = fvco - (*m) * fref;
> >       k = ffrac * 65536 / fref;
> >       if (k > 32767) {
> > -             ffrac = ((m + 1) * fref) - fvco;
> > +             /*
> > +              * The requested rate is closer to the next integer multiplier
> > +              * m, so pick it and use a negative fractional coefficient k
> > +              */
> > +             *m += 1;
> > +             ffrac = (*m) * fref - fvco;
> >               /*
> >                * Round up to avoid overshooting requested rate for negative k
> >                */
> > @@ -201,7 +206,10 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
> >                       for (m = 64; m <= 1023; m++) {
> >                               if ((fvco >= m * fin_hz / p) &&
> >                                   (fvco < (m + 1) * fin_hz / p)) {
> > -                                     k = rockchip_rk3588_pll_k_get(m, p, s,
> > +                                     u32 m_tmp = m;
> > +
> > +                                     k = rockchip_rk3588_pll_k_get(&m_tmp,
> > +                                                                   p, s,
> >                                                                     fin_hz,
> >                                                                     fvco);
> >                                       if (!k)
> > @@ -209,10 +217,7 @@ rockchip_rk3588_pll_frac_by_auto(unsigned long fin_hz, unsigned long fout_hz)
> >                                       rate_table->p = p;
> >                                       rate_table->s = s;
> >                                       rate_table->k = k;
> > -                                     if (k > 32767)
> > -                                             rate_table->m = m + 1;
> > -                                     else
> > -                                             rate_table->m = m;
> > +                                     rate_table->m = m_tmp;
> >                                       return rate_table;
>
> I don't like this much more, it's a halfway-solution to me. I suggest to
> pass rate_table pointer directly, instead of only m, and update
> rate_table m, p, s and k if we can find a k that works, otherwise leave
> the pointer untouched. Change the function to return 0 on success, 1 (or
> -EINVAL) otherwise and return rate_table in the for-loop if 0 is
> returned, otherwise continue.

Indeed, can do that. Will reshuffle in v2. Thanks for the idea!

Best regards,
Alexey

Re: [PATCH 3/6] clk: rockchip: pll: fix RK3588 frac PLL result for k=-32768

[Alexey Charkov via U-Boot]

Hi Quentin,

On Tue, Jul 21, 2026 at 4:16 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> > Current code needlessly sets the k value to 0 when it is calculated as
> > -32768, which is a valid value for the RK3588 frac PLL. This results in
> > the PLL output frequency being higher than requested when the requested
> > frequency is exactly halfway between two integer-multiplier PLL output
> > frequencies.
> >
> > Negative k values can never go below -32768 either, because that case is
> > handled just above this code, so the check for k > 32767 is redundant.
> >
>
> and because we add 1 to m, which is eventually multiplied by 65536 and
> thus if k is >32767 before adding 1 to m, it can only be <=32768 after
> adding 1 to m as we also invert the sign of k.
>
> > What remains of the if statement is a hand-rolled two's complement
> > negation of the result, so write it out as such for clarity, and return
> > the true S16 type of k as specified in the TRM.
> >
> > Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll calculation")
> > Signed-off-by: Alexey Charkov <alchark@flipper.net>
> > ---
> >   drivers/clk/rockchip/clk_pll.c | 10 +++-------
> >   1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c
> > index 69d2d182dcb5..c6fbeb71c77a 100644
> > --- a/drivers/clk/rockchip/clk_pll.c
> > +++ b/drivers/clk/rockchip/clk_pll.c
> > @@ -167,11 +167,11 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz,
> >       return rate_table;
> >   }
> >
> > -static u32
> > +static s16
> >   rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
> >   {
> >       u64 fref, ffrac;
> > -     u32 k = 0;
> > +     int k;
> >
> >       fref = fin_hz / p;
> >       ffrac = fvco - (m * fref);
> > @@ -181,11 +181,7 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco)
> >               /*
> >                * Round up to avoid overshooting requested rate for negative k
> >                */
> > -             k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
> > -             if (k > 32767)
> > -                     k = 0;
> > -             else
> > -                     k = ~k + 1;
> > +             k = -(int)DIV64_U64_ROUND_UP(ffrac * 65536, fref);
>
> I don't like migrating k to s16 in multiple commits, especially since
> there's also a mix of int/s16 in there. It's quite confusing.

It is somewhat. I did it primarily to make the derivations of all the
maths churn easy to follow, and I've double-checked that all the sign
extension and signed->unsigned promotion works out correctly. It might
be easier to squash the whole thing though :)

> It's kinda bad to store the return value of this function and then do
> some additional based on its value and set the rate_table based on it.
> Considering the next patch, I think it also itches you :)
> I'm thinking to move the whole rate_table->X assignment within
> rockchip_rk3588_pll_k_get(), also pass rate_table pointer as argument
> and simply return 0 if it worked, 1 (or -EINVAL or whatever) otherwise
> and have the for-loop return rate_table if rockchip_rk3588_pll_k_get()
> returns 0.
>
> Keep the current type when moving the assignments into the function,
> then migrate rate_table->k to be an s16 (including the struct
> definition) in another commit, then fix the k=-32768 case. I think it's
> clearer that way and avoid implicit casts and a mix of signed and
> unsigned types.

Sounds good, let me try that in v2. Thanks for the suggestion!

Best regards,
Alexey

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> Current code calculates the fractional component in 32 bits before
> assigning it to a 64-bit holding variable, causing overflow for real-world
> values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
> massaging of an unsigned representation of what is actually a two's
> complement signed value, which is confusing and makes the code harder to
> read.
> 
> Read k into a properly signed type and promote operands to avoid overflow,

I'm not sure this is correct? Converting an unsigned integer (readl 
returns an u32 and con is a u32) to signed (k is s16) is 
implementation-defined as far as I understood (c.f. 
https://en.cppreference.com/c/language/conversion). I'm sure I 
misunderstood the spec but considering signed integer overflow is 
undefined (and I guess one could understand casting a u32 storing a 
number bigger than S16_MAX into an s16 to be some kind of overflow), I'm 
a bit concerned here. I probably forgot important stuff I learned a 
decade ago :) Can you point me where/what I misunderstood?

Cheers,
Quentin

Re: [PATCH 2/6] clk: rockchip: pll: fix rounding of negative k in RK3588 frac PLL

[Quentin Schulz via U-Boot]

Hi Alexey,

Resending so it hits the ML because I once again triggered the spam 
filter by using the "wrong" mail address.

On 7/13/26 8:35 PM, Alexey Charkov wrote:
 > Current code uses magical constants when rounding up the magnitude of
 > negative fractional PLL component k. This leads to overshooting the
 > requested rate when the calculated fractional part has less than 0.3 in
 > its decimal part due to failure to round up the fractional part.
 >

The choice of magic numbers sure is odd.

 > Use a proper rounding up function to avoid overshooting the requested
 > rate and make the calculation more readable.
 >

Just for posterity, ffrac is guaranteed to not underflow in the if 
condition because the caller of rockchip_rk3588_pll_k_get() actually 
check that fvco < ffrac (computed with m+1).

ffrac * 65536 won't overflow because ffrac can be roughly max 1024 * 
fin_hz, so fin_hz needs to be U64_MAX / 1024 / 65536 which is 256GHz.

Therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

 > Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll 
calculation")
 > Signed-off-by: Alexey Charkov <alchark@flipper.net>
 > ---
 >   drivers/clk/rockchip/clk_pll.c | 6 +++++-
 >   1 file changed, 5 insertions(+), 1 deletion(-)
 >
 > diff --git a/drivers/clk/rockchip/clk_pll.c 
b/drivers/clk/rockchip/clk_pll.c
 > index d0df3b8fb49d..69d2d182dcb5 100644
 > --- a/drivers/clk/rockchip/clk_pll.c
 > +++ b/drivers/clk/rockchip/clk_pll.c
 > @@ -11,6 +11,7 @@
 >   #include <asm/arch-rockchip/hardware.h>
 >   #include <div64.h>
 >   #include <linux/delay.h>
 > +#include <linux/math64.h>
 >     static struct rockchip_pll_rate_table rockchip_auto_table;
 >   @@ -177,7 +178,10 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, 
u64 fin_hz, u64 fvco)
 >       k = ffrac * 65536 / fref;
 >       if (k > 32767) {
 >           ffrac = ((m + 1) * fref) - fvco;
 > -        k = ((ffrac * 65536 * 10 / fref) + 7) / 10;
 > +        /*
 > +         * Round up to avoid overshooting requested rate for negative k
 > +         */
 > +        k = DIV64_U64_ROUND_UP(ffrac * 65536, fref);
 >           if (k > 32767)
 >               k = 0;
 >           else
 >

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Alexey Charkov via U-Boot]

Hi Quentin,

On Tue, Jul 21, 2026 at 4:56 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> > Current code calculates the fractional component in 32 bits before
> > assigning it to a 64-bit holding variable, causing overflow for real-world
> > values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
> > massaging of an unsigned representation of what is actually a two's
> > complement signed value, which is confusing and makes the code harder to
> > read.
> >
> > Read k into a properly signed type and promote operands to avoid overflow,
>
> I'm not sure this is correct? Converting an unsigned integer (readl
> returns an u32 and con is a u32) to signed (k is s16) is
> implementation-defined as far as I understood (c.f.
> https://en.cppreference.com/c/language/conversion). I'm sure I
> misunderstood the spec but considering signed integer overflow is
> undefined (and I guess one could understand casting a u32 storing a
> number bigger than S16_MAX into an s16 to be some kind of overflow), I'm
> a bit concerned here. I probably forgot important stuff I learned a
> decade ago :) Can you point me where/what I misunderstood?

It is indeed implementation defined in the C standard, but given that
U-Boot enforces the gnu11 convention, it's defined to be reduction
modulo 2^16 [1] along with two's complement representation, which is
exactly what we need here.

[1] https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html

Best regards,
Alexey

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/21/26 3:34 PM, Alexey Charkov wrote:
> Hi Quentin,
> 
> On Tue, Jul 21, 2026 at 4:56 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Alexey,
>>
>> On 7/13/26 8:35 PM, Alexey Charkov wrote:
>>> Current code calculates the fractional component in 32 bits before
>>> assigning it to a 64-bit holding variable, causing overflow for real-world
>>> values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
>>> massaging of an unsigned representation of what is actually a two's
>>> complement signed value, which is confusing and makes the code harder to
>>> read.
>>>
>>> Read k into a properly signed type and promote operands to avoid overflow,
>>
>> I'm not sure this is correct? Converting an unsigned integer (readl
>> returns an u32 and con is a u32) to signed (k is s16) is
>> implementation-defined as far as I understood (c.f.
>> https://en.cppreference.com/c/language/conversion). I'm sure I
>> misunderstood the spec but considering signed integer overflow is
>> undefined (and I guess one could understand casting a u32 storing a
>> number bigger than S16_MAX into an s16 to be some kind of overflow), I'm
>> a bit concerned here. I probably forgot important stuff I learned a
>> decade ago :) Can you point me where/what I misunderstood?
> 
> It is indeed implementation defined in the C standard, but given that
> U-Boot enforces the gnu11 convention, it's defined to be reduction
> modulo 2^16 [1] along with two's complement representation, which is
> exactly what we need here.
> 

That is helpful thank you! I was sure I was missing something as I don't 
think we explicitly handle this anywhere in U-Boot or the Linux kernel 
:) A few more related questions though if you don't mind :)

I'm assuming we're simply masking the bits 16+ to be 0 (modulo 2**16) as 
stored in the u32 (which already carries an s16 value just "as" u32) and 
not care about signedness when doing that (otherwise we would have an 
issue since the MSB of the u32 is necessarily a 0, thus a positive 
value, since the register returns 0 for [31:16] according to the TRM). 
Is that what you meant by "along with two's complement representation"?

We also build with clang, but I'm assuming it respects the GNU 
implementation with the -std=gnu11 argument we have in KBUILD_CFLAGS. 
May I ask how you know this implementation is part of gnu11 from that 
webpage?

Cheers,
Quentin

Re: [PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Quentin Schulz via U-Boot]

Hi Alexey,

On 7/21/26 2:46 PM, Alexey Charkov wrote:
> Hi Quentin,
> 
> On Tue, Jul 21, 2026 at 4:20 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Alexey,
>>
>> On 7/13/26 8:35 PM, Alexey Charkov wrote:
>>> The TRM defines the fractional PLL adjustment coefficient as a signed
>>> two's complement number, 16 bits wide, so store it as such to avoid
>>> confusion.
>>>
>>
>> Yet...
>>
>>> Signed-off-by: Alexey Charkov <alchark@flipper.net>
>>> ---
>>>    arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
>>>    drivers/clk/rockchip/clk_pll.c             | 3 ++-
>>>    2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
>>> index 95b08bfd046f..f9bfdfb8a6a3 100644
>>> --- a/arch/arm/include/asm/arch-rockchip/clock.h
>>> +++ b/arch/arm/include/asm/arch-rockchip/clock.h
>>> @@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
>>>        unsigned int m;
>>>        unsigned int p;
>>>        unsigned int s;
>>> -     unsigned int k;
>>> +     int k;
>>
>> ... you use int here instead of s16, any specific reason?
> 
> Yes. What matters here is the signedness. The table value never gets
> written to or read from the hardware without accessor functions, which
> mask on writes and sign-extend on reads anyway. A generic 'int'
> usually performs better than a fixed-width type because it aligns
> better and requires fewer instructions for arithmetic.
> 

Is the performance gain worth the potential confusion around int vs s16?

> It also reduces potential churn if this table definition is ever
> reused for another SoC with a different width for the k coefficient,
> but this latter point is more theoretical.
> 

The kernel uses a union in rockchip_pll_rate_table, maybe we should be 
doing the same (totally unrelated to your patch though)? It also uses an 
unsigned int k (still, but maybe you're working on that? haven't seen 
patches on the ML at a quick glance though). In general, I like to not 
differ tooooo much from the kernel as ideally it would allow to backport 
patches from the kernel and more eyes have read the code.

If all we care is signedness, I would rather have all s16 or int, not a 
mix. But if the kernel keeps using an unsigned int for k, maybe we 
should wait for them to change or just stay with what we have here?

I understand using an unsigned int for what is effectively an s16 to be 
quite confusing, at the very least we can add a comment in the struct.

> Shall I reword the commit description accordingly?
> 
> Happy to set the type to s16 if you believe it's more expressive,
> though; we aren't doing much arithmetic on the table values anyway.
> 

I'm undecided whether we should diverge from how the kernel represents 
the rate table (that is, switch away from unsigned int for k), but if we 
do, I think we really should be consistent and avoid optimization at the 
cost of readability/confusion (except if gains are substantial).

I see that the kernel only has a table of rates, and doesn't do maths to 
figure out k and it seems they store negative k's in their 
representation in a unsigned form (that is, values above 32767 to 
represent negative k's). I'm guessing we cannot store tables in U-Boot 
because they would take too much space.

I'm sorry this mail is a bit all over the place, but I think there's 
another issue in the driver. I believe we shouldn't check for rate->k 
before calling rk_clrsetreg(base + pll->con_offset + 
RK3588_PLLCON(2),...) otherwise we may not clear an existing non-zero k 
when setting a new rate. Is that correct? Not required for this series, 
but I think it should be fixed (if I'm indeed right).

I'm wondering also if we couldn't merge the loops in 
rockchip_rk3588_pll_frac_by_auto() and rk3588_pll_clk_set_by_auto(). The 
only difference I see is that p cannot be 1 when we have an exact match 
(the for-loop in rk3588_pll_clk_set_by_auto()), but if we modify 
rockchip_rk3588_pll_k_get() as suggested in another patch in this series 
to return 0 on success, we could have the function set k to 0 and still 
be valid. What do you think? This is further improvement and is not 
required for this series.

Cheers,
Quentin

Re: [PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Alexey Charkov via U-Boot]

On Tue, Jul 21, 2026 at 6:34 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/21/26 2:46 PM, Alexey Charkov wrote:
> > Hi Quentin,
> >
> > On Tue, Jul 21, 2026 at 4:20 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Alexey,
> >>
> >> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> >>> The TRM defines the fractional PLL adjustment coefficient as a signed
> >>> two's complement number, 16 bits wide, so store it as such to avoid
> >>> confusion.
> >>>
> >>
> >> Yet...
> >>
> >>> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> >>> ---
> >>>    arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
> >>>    drivers/clk/rockchip/clk_pll.c             | 3 ++-
> >>>    2 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
> >>> index 95b08bfd046f..f9bfdfb8a6a3 100644
> >>> --- a/arch/arm/include/asm/arch-rockchip/clock.h
> >>> +++ b/arch/arm/include/asm/arch-rockchip/clock.h
> >>> @@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
> >>>        unsigned int m;
> >>>        unsigned int p;
> >>>        unsigned int s;
> >>> -     unsigned int k;
> >>> +     int k;
> >>
> >> ... you use int here instead of s16, any specific reason?
> >
> > Yes. What matters here is the signedness. The table value never gets
> > written to or read from the hardware without accessor functions, which
> > mask on writes and sign-extend on reads anyway. A generic 'int'
> > usually performs better than a fixed-width type because it aligns
> > better and requires fewer instructions for arithmetic.
> >
>
> Is the performance gain worth the potential confusion around int vs s16?

I'm not particularly attached to either approach. I just prefer using
general types when there is no hardware-dictated reason to use
forced-width, and there isn't one in this case. The other coefficients
use general unsigned types too, so mixing in a single fixed-width felt
ugly to me. But again, it doesn't matter much, so happy to switch if
you prefer.

> > It also reduces potential churn if this table definition is ever
> > reused for another SoC with a different width for the k coefficient,
> > but this latter point is more theoretical.
> >
>
> The kernel uses a union in rockchip_pll_rate_table, maybe we should be
> doing the same (totally unrelated to your patch though)? It also uses an
> unsigned int k (still, but maybe you're working on that? haven't seen
> patches on the ML at a quick glance though). In general, I like to not
> differ tooooo much from the kernel as ideally it would allow to backport
> patches from the kernel and more eyes have read the code.

The kernel also uses a wrong denominator for the fractional component
(65535 instead of 65536). I have a local patch to fix that, but
haven't yet sent it out (mostly due to the fact that the kernel
doesn't use rate auto-calculation anyway, and writing out a table
value directly to the register doesn't care if it's signed or not).

> If all we care is signedness, I would rather have all s16 or int, not a
> mix. But if the kernel keeps using an unsigned int for k, maybe we
> should wait for them to change or just stay with what we have here?
>
> I understand using an unsigned int for what is effectively an s16 to be
> quite confusing, at the very least we can add a comment in the struct.
>
> > Shall I reword the commit description accordingly?
> >
> > Happy to set the type to s16 if you believe it's more expressive,
> > though; we aren't doing much arithmetic on the table values anyway.
> >
>
> I'm undecided whether we should diverge from how the kernel represents
> the rate table (that is, switch away from unsigned int for k), but if we
> do, I think we really should be consistent and avoid optimization at the
> cost of readability/confusion (except if gains are substantial).

Happy to convert the whole struct to fixed-width values for
consistency then, and submit a corresponding change to Linux along the
way.

> I see that the kernel only has a table of rates, and doesn't do maths to
> figure out k and it seems they store negative k's in their
> representation in a unsigned form (that is, values above 32767 to
> represent negative k's). I'm guessing we cannot store tables in U-Boot
> because they would take too much space.

We do store them (drivers/clk/rockchip/clk_rk3576.c:22 and
drivers/clk/rockchip/clk_rk3588.c:22), but our table is smaller than
the kernel's.

> I'm sorry this mail is a bit all over the place, but I think there's
> another issue in the driver. I believe we shouldn't check for rate->k
> before calling rk_clrsetreg(base + pll->con_offset +
> RK3588_PLLCON(2),...) otherwise we may not clear an existing non-zero k
> when setting a new rate. Is that correct? Not required for this series,
> but I think it should be fixed (if I'm indeed right).

The check looks wrong: it should be conditional on the given PLL
supporting the fractional coefficient, not on the rate having a
fractional component. PLLCON(2) register is not defined for
integer-only PLLs such as LPLL or BPLL, so I'm not sure if they react
nicely to a k being written there, be it zero or not.

> I'm wondering also if we couldn't merge the loops in
> rockchip_rk3588_pll_frac_by_auto() and rk3588_pll_clk_set_by_auto(). The
> only difference I see is that p cannot be 1 when we have an exact match
> (the for-loop in rk3588_pll_clk_set_by_auto()), but if we modify
> rockchip_rk3588_pll_k_get() as suggested in another patch in this series
> to return 0 on success, we could have the function set k to 0 and still
> be valid. What do you think? This is further improvement and is not
> required for this series.

I think it would be tricky to explicitly prefer integer matches if we
try both integer and fractional in the same loop. And we want to
prefer all-integer settings wherever possible, as it avoids the whole
sigma-delta block, along with the additional clock jitter it can
introduce.

What we can do is just inline the rockchip_rk3588_pll_frac_by_auto()
helper into the "else" branch of rk3588_pll_clk_set_by_auto(). Having
the two loops next to each other can make them easier to compare
visually. Let me know what you think - happy to rearrange those in v2.

Best regards,
Alexey

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Alexey Charkov via U-Boot]

On Tue, Jul 21, 2026 at 6:05 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/21/26 3:34 PM, Alexey Charkov wrote:
> > Hi Quentin,
> >
> > On Tue, Jul 21, 2026 at 4:56 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Alexey,
> >>
> >> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> >>> Current code calculates the fractional component in 32 bits before
> >>> assigning it to a 64-bit holding variable, causing overflow for real-world
> >>> values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
> >>> massaging of an unsigned representation of what is actually a two's
> >>> complement signed value, which is confusing and makes the code harder to
> >>> read.
> >>>
> >>> Read k into a properly signed type and promote operands to avoid overflow,
> >>
> >> I'm not sure this is correct? Converting an unsigned integer (readl
> >> returns an u32 and con is a u32) to signed (k is s16) is
> >> implementation-defined as far as I understood (c.f.
> >> https://en.cppreference.com/c/language/conversion). I'm sure I
> >> misunderstood the spec but considering signed integer overflow is
> >> undefined (and I guess one could understand casting a u32 storing a
> >> number bigger than S16_MAX into an s16 to be some kind of overflow), I'm
> >> a bit concerned here. I probably forgot important stuff I learned a
> >> decade ago :) Can you point me where/what I misunderstood?
> >
> > It is indeed implementation defined in the C standard, but given that
> > U-Boot enforces the gnu11 convention, it's defined to be reduction
> > modulo 2^16 [1] along with two's complement representation, which is
> > exactly what we need here.
> >
>
> That is helpful thank you! I was sure I was missing something as I don't
> think we explicitly handle this anywhere in U-Boot or the Linux kernel
> :) A few more related questions though if you don't mind :)
>
> I'm assuming we're simply masking the bits 16+ to be 0 (modulo 2**16) as
> stored in the u32 (which already carries an s16 value just "as" u32) and
> not care about signedness when doing that (otherwise we would have an
> issue since the MSB of the u32 is necessarily a 0, thus a positive
> value, since the register returns 0 for [31:16] according to the TRM).
> Is that what you meant by "along with two's complement representation"?

The masking that the driver does is technically redundant, but doesn't
hurt. It's the same modulo operation that the compiler does upon
assignment of a u32 value to a narrower type.

Two's complement representation ensures that bit(15) is the sign bit
and that 0x8000..0xffff map to -32768..-1, so assigning a masked value
to an s16 variable correctly interprets the value as what the hardware
implies for both positive and negative values.

> We also build with clang, but I'm assuming it respects the GNU
> implementation with the -std=gnu11 argument we have in KBUILD_CFLAGS.
> May I ask how you know this implementation is part of gnu11 from that
> webpage?

I guess I stand corrected here: the webpage defines the specific
"implementation-defined" behavior for this case for the specific
implementation in GNU GCC, although it's not one of the GNU extensions
to the C11 per se (and shouldn't be, given that C11 explicitly leaves
this part to the implementation). Clang tries to maintain
compatibility with GCC, so I assume it handles it the same way, but I
haven't found any official documentation to confirm this.

For a two's complement signed representation this is perhaps the only
sane approach IMO, so I'd be surprised if any mainstream compilers
chose to do it differently :)

FWIW, Exynos does a similar thing in drivers/clk/exynos/clk-pll.c:88

Best regards,
Alexey

Re: [PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Quentin Schulz via U-Boot]

On 7/21/26 5:21 PM, Alexey Charkov wrote:
> On Tue, Jul 21, 2026 at 6:34 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Alexey,
>>
>> On 7/21/26 2:46 PM, Alexey Charkov wrote:
>>> Hi Quentin,
>>>
>>> On Tue, Jul 21, 2026 at 4:20 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Alexey,
>>>>
>>>> On 7/13/26 8:35 PM, Alexey Charkov wrote:
>>>>> The TRM defines the fractional PLL adjustment coefficient as a signed
>>>>> two's complement number, 16 bits wide, so store it as such to avoid
>>>>> confusion.
>>>>>
>>>>
>>>> Yet...
>>>>
>>>>> Signed-off-by: Alexey Charkov <alchark@flipper.net>
>>>>> ---
>>>>>     arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
>>>>>     drivers/clk/rockchip/clk_pll.c             | 3 ++-
>>>>>     2 files changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
>>>>> index 95b08bfd046f..f9bfdfb8a6a3 100644
>>>>> --- a/arch/arm/include/asm/arch-rockchip/clock.h
>>>>> +++ b/arch/arm/include/asm/arch-rockchip/clock.h
>>>>> @@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
>>>>>         unsigned int m;
>>>>>         unsigned int p;
>>>>>         unsigned int s;
>>>>> -     unsigned int k;
>>>>> +     int k;
>>>>
>>>> ... you use int here instead of s16, any specific reason?
>>>
>>> Yes. What matters here is the signedness. The table value never gets
>>> written to or read from the hardware without accessor functions, which
>>> mask on writes and sign-extend on reads anyway. A generic 'int'
>>> usually performs better than a fixed-width type because it aligns
>>> better and requires fewer instructions for arithmetic.
>>>
>>
>> Is the performance gain worth the potential confusion around int vs s16?
> 
> I'm not particularly attached to either approach. I just prefer using
> general types when there is no hardware-dictated reason to use
> forced-width, and there isn't one in this case. The other coefficients
> use general unsigned types too, so mixing in a single fixed-width felt
> ugly to me. But again, it doesn't matter much, so happy to switch if
> you prefer.
> 
>>> It also reduces potential churn if this table definition is ever
>>> reused for another SoC with a different width for the k coefficient,
>>> but this latter point is more theoretical.
>>>
>>
>> The kernel uses a union in rockchip_pll_rate_table, maybe we should be
>> doing the same (totally unrelated to your patch though)? It also uses an
>> unsigned int k (still, but maybe you're working on that? haven't seen
>> patches on the ML at a quick glance though). In general, I like to not
>> differ tooooo much from the kernel as ideally it would allow to backport
>> patches from the kernel and more eyes have read the code.
> 
> The kernel also uses a wrong denominator for the fractional component
> (65535 instead of 65536). I have a local patch to fix that, but
> haven't yet sent it out (mostly due to the fact that the kernel
> doesn't use rate auto-calculation anyway, and writing out a table
> value directly to the register doesn't care if it's signed or not).
> 
>> If all we care is signedness, I would rather have all s16 or int, not a
>> mix. But if the kernel keeps using an unsigned int for k, maybe we
>> should wait for them to change or just stay with what we have here?
>>
>> I understand using an unsigned int for what is effectively an s16 to be
>> quite confusing, at the very least we can add a comment in the struct.
>>
>>> Shall I reword the commit description accordingly?
>>>
>>> Happy to set the type to s16 if you believe it's more expressive,
>>> though; we aren't doing much arithmetic on the table values anyway.
>>>
>>
>> I'm undecided whether we should diverge from how the kernel represents
>> the rate table (that is, switch away from unsigned int for k), but if we
>> do, I think we really should be consistent and avoid optimization at the
>> cost of readability/confusion (except if gains are substantial).
> 
> Happy to convert the whole struct to fixed-width values for
> consistency then, and submit a corresponding change to Linux along the
> way.
> 

Let's see what the kernel has to say and then we can do the same in 
U-Boot once there's some agreement there.

>> I see that the kernel only has a table of rates, and doesn't do maths to
>> figure out k and it seems they store negative k's in their
>> representation in a unsigned form (that is, values above 32767 to
>> represent negative k's). I'm guessing we cannot store tables in U-Boot
>> because they would take too much space.
> 
> We do store them (drivers/clk/rockchip/clk_rk3576.c:22 and
> drivers/clk/rockchip/clk_rk3588.c:22), but our table is smaller than
> the kernel's.
> 

I'm really starting to wonder if it makes sense to have both a static 
table and a way to figure out the dividers (including fractional) 
dynamically, especially since we fallback to the latter if we cannot 
find the desired rate in the static table. And then we ask to find a 
rate with an input frequency of 24MHz but this expects the PLL to have 
its parent configured to use the 24MHz (xin_osc0) input clock... which 
isn't guaranteed as far as I could tell (yes, it's the register reset 
value but we better hope nothing changes it before U-Boot runs this 
code). While looking for this, I started to read the clock driver a bit 
more and oh boy... it doesn't look good. We **really** shouldn't be 
using anything rk3588 for rk3576 because many assumptions are made that 
it is only rk3588 and not anything else (e.g. it uses pll_id **integer** 
value instead of enum (because it cannot, otherwise it wouldn't compile 
on non-rk3588 devices which don't define the enum) to identify PLLs and 
decide to which offsets to write, etc... and of course, the enum integer 
values are different between rk3576 and rk3588.... I wish I hadn't read 
this code.....

>> I'm sorry this mail is a bit all over the place, but I think there's
>> another issue in the driver. I believe we shouldn't check for rate->k
>> before calling rk_clrsetreg(base + pll->con_offset +
>> RK3588_PLLCON(2),...) otherwise we may not clear an existing non-zero k
>> when setting a new rate. Is that correct? Not required for this series,
>> but I think it should be fixed (if I'm indeed right).
> 
> The check looks wrong: it should be conditional on the given PLL
> supporting the fractional coefficient, not on the rate having a
> fractional component. PLLCON(2) register is not defined for
> integer-only PLLs such as LPLL or BPLL, so I'm not sure if they react
> nicely to a k being written there, be it zero or not.
> 

Yeah, better not try to write to undocumented registers if the vendor 
BSP isn't doing it as well. I don't think we store which PLL is an 
integer PLL and we cannot simply use the PLL ID as they aren't all 
defined for all SoCs (e.g. RK3576 doesn't have NPLL, B0PLL, B1PLL and 
RK3588 doesn't have BPLL)... I guess we may need a new member in the 
struct to reflect that.

At the same time, the kernel doesn't seem to bother checking that and 
simply writes to PLLCON(2) regardless of the type? c.f. 
https://elixir.bootlin.com/linux/v7.1.4/source/drivers/clk/rockchip/clk-pll.c#L967 
so maybe we shouldn't as well (yes I said the opposite in the previous 
paragraph :) ).

We also don't check whether the PLL actually supports fractional 
dividing, we only check whether the input and output frequency are 
multiples of 1MHz, but we could have something based on an integer PLL 
try to get a frequency that wouldn't match this requirement no? Or do we 
have a mechanism in place to make sure this cannot happen?

>> I'm wondering also if we couldn't merge the loops in
>> rockchip_rk3588_pll_frac_by_auto() and rk3588_pll_clk_set_by_auto(). The
>> only difference I see is that p cannot be 1 when we have an exact match
>> (the for-loop in rk3588_pll_clk_set_by_auto()), but if we modify
>> rockchip_rk3588_pll_k_get() as suggested in another patch in this series
>> to return 0 on success, we could have the function set k to 0 and still
>> be valid. What do you think? This is further improvement and is not
>> required for this series.
> 
> I think it would be tricky to explicitly prefer integer matches if we
> try both integer and fractional in the same loop. And we want to
> prefer all-integer settings wherever possible, as it avoids the whole
> sigma-delta block, along with the additional clock jitter it can
> introduce.
> 
> What we can do is just inline the rockchip_rk3588_pll_frac_by_auto()
> helper into the "else" branch of rk3588_pll_clk_set_by_auto(). Having
> the two loops next to each other can make them easier to compare
> visually. Let me know what you think - happy to rearrange those in v2.
> 

Meh. Less appealing now :) I wouldn't bother but you do you :)

Cheers,
Quentin

Re: [PATCH 5/6] clk: rockchip: pll: fractional PLL coefficient is two's complement

[Alexey Charkov via U-Boot]

On Tue, Jul 21, 2026 at 8:34 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> On 7/21/26 5:21 PM, Alexey Charkov wrote:
> > On Tue, Jul 21, 2026 at 6:34 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Alexey,
> >>
> >> On 7/21/26 2:46 PM, Alexey Charkov wrote:
> >>> Hi Quentin,
> >>>
> >>> On Tue, Jul 21, 2026 at 4:20 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>>>
> >>>> Hi Alexey,
> >>>>
> >>>> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> >>>>> The TRM defines the fractional PLL adjustment coefficient as a signed
> >>>>> two's complement number, 16 bits wide, so store it as such to avoid
> >>>>> confusion.
> >>>>>
> >>>>
> >>>> Yet...
> >>>>
> >>>>> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> >>>>> ---
> >>>>>     arch/arm/include/asm/arch-rockchip/clock.h | 2 +-
> >>>>>     drivers/clk/rockchip/clk_pll.c             | 3 ++-
> >>>>>     2 files changed, 3 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
> >>>>> index 95b08bfd046f..f9bfdfb8a6a3 100644
> >>>>> --- a/arch/arm/include/asm/arch-rockchip/clock.h
> >>>>> +++ b/arch/arm/include/asm/arch-rockchip/clock.h
> >>>>> @@ -104,7 +104,7 @@ struct rockchip_pll_rate_table {
> >>>>>         unsigned int m;
> >>>>>         unsigned int p;
> >>>>>         unsigned int s;
> >>>>> -     unsigned int k;
> >>>>> +     int k;
> >>>>
> >>>> ... you use int here instead of s16, any specific reason?
> >>>
> >>> Yes. What matters here is the signedness. The table value never gets
> >>> written to or read from the hardware without accessor functions, which
> >>> mask on writes and sign-extend on reads anyway. A generic 'int'
> >>> usually performs better than a fixed-width type because it aligns
> >>> better and requires fewer instructions for arithmetic.
> >>>
> >>
> >> Is the performance gain worth the potential confusion around int vs s16?
> >
> > I'm not particularly attached to either approach. I just prefer using
> > general types when there is no hardware-dictated reason to use
> > forced-width, and there isn't one in this case. The other coefficients
> > use general unsigned types too, so mixing in a single fixed-width felt
> > ugly to me. But again, it doesn't matter much, so happy to switch if
> > you prefer.
> >
> >>> It also reduces potential churn if this table definition is ever
> >>> reused for another SoC with a different width for the k coefficient,
> >>> but this latter point is more theoretical.
> >>>
> >>
> >> The kernel uses a union in rockchip_pll_rate_table, maybe we should be
> >> doing the same (totally unrelated to your patch though)? It also uses an
> >> unsigned int k (still, but maybe you're working on that? haven't seen
> >> patches on the ML at a quick glance though). In general, I like to not
> >> differ tooooo much from the kernel as ideally it would allow to backport
> >> patches from the kernel and more eyes have read the code.
> >
> > The kernel also uses a wrong denominator for the fractional component
> > (65535 instead of 65536). I have a local patch to fix that, but
> > haven't yet sent it out (mostly due to the fact that the kernel
> > doesn't use rate auto-calculation anyway, and writing out a table
> > value directly to the register doesn't care if it's signed or not).
> >
> >> If all we care is signedness, I would rather have all s16 or int, not a
> >> mix. But if the kernel keeps using an unsigned int for k, maybe we
> >> should wait for them to change or just stay with what we have here?
> >>
> >> I understand using an unsigned int for what is effectively an s16 to be
> >> quite confusing, at the very least we can add a comment in the struct.
> >>
> >>> Shall I reword the commit description accordingly?
> >>>
> >>> Happy to set the type to s16 if you believe it's more expressive,
> >>> though; we aren't doing much arithmetic on the table values anyway.
> >>>
> >>
> >> I'm undecided whether we should diverge from how the kernel represents
> >> the rate table (that is, switch away from unsigned int for k), but if we
> >> do, I think we really should be consistent and avoid optimization at the
> >> cost of readability/confusion (except if gains are substantial).
> >
> > Happy to convert the whole struct to fixed-width values for
> > consistency then, and submit a corresponding change to Linux along the
> > way.
> >
>
> Let's see what the kernel has to say and then we can do the same in
> U-Boot once there's some agreement there.
>
> >> I see that the kernel only has a table of rates, and doesn't do maths to
> >> figure out k and it seems they store negative k's in their
> >> representation in a unsigned form (that is, values above 32767 to
> >> represent negative k's). I'm guessing we cannot store tables in U-Boot
> >> because they would take too much space.
> >
> > We do store them (drivers/clk/rockchip/clk_rk3576.c:22 and
> > drivers/clk/rockchip/clk_rk3588.c:22), but our table is smaller than
> > the kernel's.
> >
>
> I'm really starting to wonder if it makes sense to have both a static
> table and a way to figure out the dividers (including fractional)
> dynamically, especially since we fallback to the latter if we cannot
> find the desired rate in the static table. And then we ask to find a
> rate with an input frequency of 24MHz but this expects the PLL to have
> its parent configured to use the 24MHz (xin_osc0) input clock... which
> isn't guaranteed as far as I could tell (yes, it's the register reset
> value but we better hope nothing changes it before U-Boot runs this
> code). While looking for this, I started to read the clock driver a bit
> more and oh boy... it doesn't look good. We **really** shouldn't be
> using anything rk3588 for rk3576 because many assumptions are made that
> it is only rk3588 and not anything else (e.g. it uses pll_id **integer**
> value instead of enum (because it cannot, otherwise it wouldn't compile
> on non-rk3588 devices which don't define the enum) to identify PLLs and
> decide to which offsets to write, etc... and of course, the enum integer
> values are different between rk3576 and rk3588.... I wish I hadn't read
> this code.....

Yeah, storing a precalculated rate table and never using it is an
interesting implementation choice :) As are some others here.

> >> I'm sorry this mail is a bit all over the place, but I think there's
> >> another issue in the driver. I believe we shouldn't check for rate->k
> >> before calling rk_clrsetreg(base + pll->con_offset +
> >> RK3588_PLLCON(2),...) otherwise we may not clear an existing non-zero k
> >> when setting a new rate. Is that correct? Not required for this series,
> >> but I think it should be fixed (if I'm indeed right).
> >
> > The check looks wrong: it should be conditional on the given PLL
> > supporting the fractional coefficient, not on the rate having a
> > fractional component. PLLCON(2) register is not defined for
> > integer-only PLLs such as LPLL or BPLL, so I'm not sure if they react
> > nicely to a k being written there, be it zero or not.
> >
>
> Yeah, better not try to write to undocumented registers if the vendor
> BSP isn't doing it as well. I don't think we store which PLL is an
> integer PLL and we cannot simply use the PLL ID as they aren't all
> defined for all SoCs (e.g. RK3576 doesn't have NPLL, B0PLL, B1PLL and
> RK3588 doesn't have BPLL)... I guess we may need a new member in the
> struct to reflect that.
>
> At the same time, the kernel doesn't seem to bother checking that and
> simply writes to PLLCON(2) regardless of the type? c.f.
> https://elixir.bootlin.com/linux/v7.1.4/source/drivers/clk/rockchip/clk-pll.c#L967
> so maybe we shouldn't as well (yes I said the opposite in the previous
> paragraph :) ).

A struct member won't hurt IMO. Not sure if the mainline Linux source
is a very good reference in this regard, as I suspect no-one ever
tested fractional rates there, given they are recalculated with a
wrong denominator and without accounting for the two's complement
value of k.

> We also don't check whether the PLL actually supports fractional
> dividing, we only check whether the input and output frequency are
> multiples of 1MHz, but we could have something based on an integer PLL
> try to get a frequency that wouldn't match this requirement no? Or do we
> have a mechanism in place to make sure this cannot happen?

The check for multiples of 1MHz looks bogus to me, too. Some
(10/7)*OSC_HZ is a perfectly fine integer PLL config, and it's not a
multiple of 1MHz. On the other hand, some 1151 MHz is impossible to
obtain from a 24 MHz source with integer operations alone.

> >> I'm wondering also if we couldn't merge the loops in
> >> rockchip_rk3588_pll_frac_by_auto() and rk3588_pll_clk_set_by_auto(). The
> >> only difference I see is that p cannot be 1 when we have an exact match
> >> (the for-loop in rk3588_pll_clk_set_by_auto()), but if we modify
> >> rockchip_rk3588_pll_k_get() as suggested in another patch in this series
> >> to return 0 on success, we could have the function set k to 0 and still
> >> be valid. What do you think? This is further improvement and is not
> >> required for this series.
> >
> > I think it would be tricky to explicitly prefer integer matches if we
> > try both integer and fractional in the same loop. And we want to
> > prefer all-integer settings wherever possible, as it avoids the whole
> > sigma-delta block, along with the additional clock jitter it can
> > introduce.
> >
> > What we can do is just inline the rockchip_rk3588_pll_frac_by_auto()
> > helper into the "else" branch of rk3588_pll_clk_set_by_auto(). Having
> > the two loops next to each other can make them easier to compare
> > visually. Let me know what you think - happy to rearrange those in v2.
> >
>
> Meh. Less appealing now :) I wouldn't bother but you do you :)

Alright, let me leave it for dessert then :) Looks like we're not yet
done with the main course here.

Best regards,
Alexey

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Quentin Schulz]

Hi Alexey,

On 7/13/26 8:35 PM, Alexey Charkov wrote:
> Current code calculates the fractional component in 32 bits before
> assigning it to a 64-bit holding variable, causing overflow for real-world
> values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
> massaging of an unsigned representation of what is actually a two's
> complement signed value, which is confusing and makes the code harder to
> read.
> 
> Read k into a properly signed type and promote operands to avoid overflow,
> which also enables the use of div_s64() to express the math more clearly.
> 

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

I've quickly looked at the rest of the threads in this series and I 
think there's no open questions left for me? You sent patches to the 
kernel for making k an s16 so I'm assuming we'll go for that in U-Boot 
as well. The PLLCON(2) thing for integer PLLs will need to be fixed one 
way or another, but I'm assuming this is also something we need to fix 
in the kernel so a similar approach would be nice. I don't think we need 
to make this a big series fixing everything in one go, so feel free to 
send smallish series whenever you're ready. Anything I'm missing?

Thanks!
Quentin

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Alexey Charkov]

On Thu, Jul 23, 2026 at 1:23 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Alexey,
>
> On 7/13/26 8:35 PM, Alexey Charkov wrote:
> > Current code calculates the fractional component in 32 bits before
> > assigning it to a 64-bit holding variable, causing overflow for real-world
> > values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
> > massaging of an unsigned representation of what is actually a two's
> > complement signed value, which is confusing and makes the code harder to
> > read.
> >
> > Read k into a properly signed type and promote operands to avoid overflow,
> > which also enables the use of div_s64() to express the math more clearly.
> >
>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>
> I've quickly looked at the rest of the threads in this series and I
> think there's no open questions left for me? You sent patches to the
> kernel for making k an s16 so I'm assuming we'll go for that in U-Boot
> as well. The PLLCON(2) thing for integer PLLs will need to be fixed one
> way or another, but I'm assuming this is also something we need to fix
> in the kernel so a similar approach would be nice. I don't think we need
> to make this a big series fixing everything in one go, so feel free to
> send smallish series whenever you're ready. Anything I'm missing?

Thanks Quentin!

I'll rework the helper to take a pointer to the table entry instead,
as we've discussed in the other sub-thread, and update the whole thing
to match the types etc. of what I submitted to Linux. Hopefully the
unsigned->signed conversion will then get localized in a single
commit.

Looks like https://lore.kernel.org/u-boot/ doesn't pick up new emails
from the list since July 20, so `b4 trailers -u` doesn't work either.
I've picked up your review tags manually, hope I haven't missed
anything in process.

Best regards,
Alexey

Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Quentin Schulz]

Hi Alexey,

On 7/23/26 2:25 PM, Alexey Charkov wrote:
> On Thu, Jul 23, 2026 at 1:23 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Alexey,
>>
>> On 7/13/26 8:35 PM, Alexey Charkov wrote:
>>> Current code calculates the fractional component in 32 bits before
>>> assigning it to a 64-bit holding variable, causing overflow for real-world
>>> values of k, given that OSC_HZ is 24000000U. It also does bitwise manual
>>> massaging of an unsigned representation of what is actually a two's
>>> complement signed value, which is confusing and makes the code harder to
>>> read.
>>>
>>> Read k into a properly signed type and promote operands to avoid overflow,
>>> which also enables the use of div_s64() to express the math more clearly.
>>>
>>
>> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> I've quickly looked at the rest of the threads in this series and I
>> think there's no open questions left for me? You sent patches to the
>> kernel for making k an s16 so I'm assuming we'll go for that in U-Boot
>> as well. The PLLCON(2) thing for integer PLLs will need to be fixed one
>> way or another, but I'm assuming this is also something we need to fix
>> in the kernel so a similar approach would be nice. I don't think we need
>> to make this a big series fixing everything in one go, so feel free to
>> send smallish series whenever you're ready. Anything I'm missing?
> 
> Thanks Quentin!
> 
> I'll rework the helper to take a pointer to the table entry instead,
> as we've discussed in the other sub-thread, and update the whole thing
> to match the types etc. of what I submitted to Linux. Hopefully the
> unsigned->signed conversion will then get localized in a single
> commit.
> 

We're on the same page then :)

> Looks like https://lore.kernel.org/u-boot/ doesn't pick up new emails
> from the list since July 20, so `b4 trailers -u` doesn't work either.
> I've picked up your review tags manually, hope I haven't missed
> anything in process.
> 

Yes that's unfortunate. We know, the LF (handling lore.kernel.org) 
knows. The main IT was on holidays and just came back a few days ago, 
and he's overloaded at the moment. I'm guessing there are more urgent 
fires for him to put out at the moment :)

Cheers,
Quentin