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

[Quentin Schulz via U-Boot <u-boot@lists.u-boot-project.org>]

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