On Tue, Jul 21, 2026 at 8:34 PM Quentin Schulz <[email protected]> wrote: > > 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.....
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
