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