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 <[email protected]> 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 <[email protected]>
---
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
