Re: [PATCH] clk: rockchip: Fractional PLL coefficient on RK3588/RK3576 is two's complement

[Heiko Stübner]

Am Mittwoch, 22. Juli 2026, 13:00:47 Mitteleuropäische Sommerzeit schrieb Alexey Charkov:
> Hi Quentin,
> 
> On Wed, Jul 22, 2026 at 2:35 PM Quentin Schulz <quentin.schulz@xxxxxxxxx> wrote:
> >
> > Hi Alexey,
> >
> > On 7/21/26 9:17 PM, Alexey Charkov wrote:
> > > When the PLL rates table was first committed for RK3588 (and later reused
> > > for RK3576), the fractional PLL coefficient was defined as an unsigned
> > > value, while the TRM clearly states that it is a two's complement 16-bit
> > > value.
> > >
> > > Rockchip's downstream kernel later revised the fractional PLL code [1] to
> > > account for the two's complement nature of the coefficient, but that
> > > change wasn't upstreamed.
> > >
> > > Change the PLL table definition to use two's complement for the
> > > fractional coefficient and update its users accordingly.
> > >
> > > Note that a negative fractional coefficient is meant to be subtracted from
> > > the next larger integer multiplier, so the _m values in the table are
> > > also adjusted accordingly for the two negative-k entries.
> > >
> > > While at it, fix the denominator of the fractional PLL calculation to use
> > > 65536 instead of 65535, as per the TRM (RK3576 TRM Part 1 V1.2, Section
> > > 2.13.1.4 Setting Guide on P, M, S, and K):
> > >
> > > Fout = ((m + k/65536) * Fin) / (p * 2^s)
> > >
> > > Link: https://github.com/flipperdevices/rockchip-linux/commit/7a72bc05dcc3a51e85ae531749e6270bf9b9212d [1]
> > > Fixes: f1c506d152ff ("clk: rockchip: add clock controller for the RK3588")
> > > Fixes: cc40f5baa91b ("clk: rockchip: Add clock controller for the RK3576")
> > > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxxxx>
> > > ---
> > > Not adding Cc stable, because while this fixes a real bug it's not a
> > > regression, as the issue was introduced in the same commit that added the
> > > RK3576/RK3588 support.
> > >
> >
> > I don't think this is a valid reason :)
> 
> I believe Linus frowns upon changes like "it never worked, but we've
> fixed it now" being submitted as fixes. It's been broken for years,
> and since nobody complained yet, going via the normal development path
> (i.e. -next) seems perfectly fine to me.

Personally, I also don't think this _needs_ a stable tag.
The whole if it ain't broke thing ....

If stable-maintainers do think it valuable on their own (without stable tag)
then by all means, but changing how the PLL rate stuff works has the
_ability_ to introduce other behaviour _somewhere_.


> > In any case, this patch is doing too many things at once. I see the
> > following things that would warrant individual patches:
> >
> > 1) fix the wrong denominator, stable candidate IMO,
> 
> I could split this one out, but since it's a trivial one-liner, I'd
> like to hear what Heiko prefers.

yes please.
- Subject: Foo and Bar
- Commit message with "while at it"
are clear indicators that the change wants to get split ;-)

So please split out that "while at it" part. The rest can stay
together though.


Thanks
Heiko