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

From: 张晴

Date: Mon Oct 26 2026 - 20:57:47 EST

在 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.

Best regards,
Cunhao