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

[张晴 <zhangqing@rock-chips.com>]

在 2026/10/26 22:56, 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.
OK
>
> Best regards,
> Cunhao