Re: [PATCH v3 3/3] clk: rockchip: Add RK3572 clock and reset support

[Heiko Stübner]

Am Montag, 26. Oktober 2026, 15:56:06 Mitteleuropäische Normalzeit schrieb Cunhao Lu:
> Hi zhangqing,
> 
> > This is in clk.h, please submit it separately.
> 
> Thanks for the review. I will move COMPOSITE_MUXTBL to clk.h and add it
> in a separate preparatory patch.
> 
> > This is incorrect here; there should be no negative values. Two's
> > complement is used here, so a conversion is needed. For example,
> > -12955 should be 52581.
> >
> > In clk-pll.c, cur.k & BIT(15) is used to determine the two's complement
> > conversion.
> 
> I agree that the hardware expects a 16-bit two's complement encoding:
> -12955 is encoded as 0xcd65, which is 52581 when read as unsigned. The
> difference here is how that coefficient is represented in the rate
> table on the kernel version this series targets.
> 
> I found the cur.k & BIT(15) branch in Rockchip's downstream develop-6.12
> tree. There, k is unsigned int, so the driver explicitly checks bit 15
> and subtracts the magnitude of a negative fractional coefficient.
> 
> This series uses the upstream implementation. Both v2 and v3 are based
> on a804c5742bff, which already contains commit
> 581478883477 ("clk: rockchip: Fractional PLL coefficient on RK3588/RK3576
> is two's complement"). That commit changed the k member of
> struct rockchip_pll_rate_table to:
> 
>         s16 k; /* fractional part, 16 bit two's complement */
> 
> On this base, rockchip_rk3588_pll_recalc_rate() uses signed arithmetic:
> 
>         s64 frac_rate64 = (s64)prate * cur.k;
>         postdiv = cur.p * 65536;
>         rate64 += div_s64(frac_rate64, postdiv);
> 
> The signed type represents the negative coefficient directly, so this
> implementation does not need a separate cur.k & BIT(15) branch. For
> example, the mainline RK3576 rate table already contains:
> 
>         RK3588_PLL_RATE(955520000, 3, 478, 2, -15730),
>         RK3588_PLL_RATE(785560000, 3, 393, 2, -14419),
> 
> These are exactly the same entries as in this RK3572 patch, including
> p, m and s. Mainline RK3588 uses the same entries as well. Since RK3572
> uses this shared upstream PLL implementation, I will follow the
> mainline RK3576 convention for its rate table.
> 
> Writing 52581 directly into an s16 initializer would require an
> out-of-range implicit conversion; a reduced compiler test reproduces
> Clang's constant-conversion diagnostic. An explicit cast such as
> (s16)52581 would produce the same signed coefficient, but would add a
> conversion that the existing mainline tables avoid. Using -12955
> directly makes the value used in the rate calculation clear and stays
> consistent with RK3576.
> 
> The register encoding is preserved: the write path passes k through
> HIWORD_UPDATE() with a 0xffff mask, so -12955 still writes 0xcd65 into
> the coefficient field. The five initializer changes from v2 to v3 only
> make the signed values explicit; they do not change p, m, s or the
> 16-bit coefficient encodings. In particular, no further adjustment of
> m is needed for these representation changes.
> 
> I will therefore retain the signed coefficients, following mainline
> RK3576.

I definitly agree. For reference the commit fixing rk3588/rk3576 and
containing all the details is
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5814788834774ed6ad1a27ae91b44eeca80cd27f

Heiko