On 7/21/26 5:21 PM, Alexey Charkov wrote:
On Tue, Jul 21, 2026 at 6:34 PM Quentin Schulz <[email protected]> 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 <[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?
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
