Hi Quentin, On Tue, Jul 21, 2026 at 4:16 PM Quentin Schulz <[email protected]> wrote: > > Hi Alexey, > > On 7/13/26 8:35 PM, Alexey Charkov wrote: > > Current code needlessly sets the k value to 0 when it is calculated as > > -32768, which is a valid value for the RK3588 frac PLL. This results in > > the PLL output frequency being higher than requested when the requested > > frequency is exactly halfway between two integer-multiplier PLL output > > frequencies. > > > > Negative k values can never go below -32768 either, because that case is > > handled just above this code, so the check for k > 32767 is redundant. > > > > and because we add 1 to m, which is eventually multiplied by 65536 and > thus if k is >32767 before adding 1 to m, it can only be <=32768 after > adding 1 to m as we also invert the sign of k. > > > What remains of the if statement is a hand-rolled two's complement > > negation of the result, so write it out as such for clarity, and return > > the true S16 type of k as specified in the TRM. > > > > Fixes: 6bfb37e70209 ("clk: rockchip: rk3588: fix up the frac pll > > calculation") > > Signed-off-by: Alexey Charkov <[email protected]> > > --- > > drivers/clk/rockchip/clk_pll.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/clk/rockchip/clk_pll.c b/drivers/clk/rockchip/clk_pll.c > > index 69d2d182dcb5..c6fbeb71c77a 100644 > > --- a/drivers/clk/rockchip/clk_pll.c > > +++ b/drivers/clk/rockchip/clk_pll.c > > @@ -167,11 +167,11 @@ rockchip_pll_clk_set_by_auto(ulong fin_hz, > > return rate_table; > > } > > > > -static u32 > > +static s16 > > rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 fin_hz, u64 fvco) > > { > > u64 fref, ffrac; > > - u32 k = 0; > > + int k; > > > > fref = fin_hz / p; > > ffrac = fvco - (m * fref); > > @@ -181,11 +181,7 @@ rockchip_rk3588_pll_k_get(u32 m, u32 p, u32 s, u64 > > fin_hz, u64 fvco) > > /* > > * Round up to avoid overshooting requested rate for negative > > k > > */ > > - k = DIV64_U64_ROUND_UP(ffrac * 65536, fref); > > - if (k > 32767) > > - k = 0; > > - else > > - k = ~k + 1; > > + k = -(int)DIV64_U64_ROUND_UP(ffrac * 65536, fref); > > I don't like migrating k to s16 in multiple commits, especially since > there's also a mix of int/s16 in there. It's quite confusing.
It is somewhat. I did it primarily to make the derivations of all the maths churn easy to follow, and I've double-checked that all the sign extension and signed->unsigned promotion works out correctly. It might be easier to squash the whole thing though :) > It's kinda bad to store the return value of this function and then do > some additional based on its value and set the rate_table based on it. > Considering the next patch, I think it also itches you :) > I'm thinking to move the whole rate_table->X assignment within > rockchip_rk3588_pll_k_get(), also pass rate_table pointer as argument > and simply return 0 if it worked, 1 (or -EINVAL or whatever) otherwise > and have the for-loop return rate_table if rockchip_rk3588_pll_k_get() > returns 0. > > Keep the current type when moving the assignments into the function, > then migrate rate_table->k to be an s16 (including the struct > definition) in another commit, then fix the k=-32768 case. I think it's > clearer that way and avoid implicit casts and a mix of signed and > unsigned types. Sounds good, let me try that in v2. Thanks for the suggestion! Best regards, Alexey
