linux-kernel - Re: [PATCH] clk: rockchip: rk3588: Don't change PLL rates when setting dclk_vop2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <j6ondk5xnwbm36isdoni5vtdq5mf5ak4kp63ratqlnpwsgrqj2@paw5lzwqa2ze>
Date: Mon, 20 Oct 2025 17:59:29 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Heiko Stuebner <heiko@...ech.de>
Cc: Quentin Schulz <quentin.schulz@...rry.de>, mturquette@...libre.com, 
	sboyd@...nel.org, zhangqing@...k-chips.com, linux-clk@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, Andy Yan <andy.yan@...k-chips.com>
Subject: Re: [PATCH] clk: rockchip: rk3588: Don't change PLL rates when
 setting dclk_vop2_src

Hi,

On Mon, Oct 20, 2025 at 02:49:10PM +0200, Heiko Stuebner wrote:
> Am Donnerstag, 16. Oktober 2025, 00:57:15 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> > On Wed, Oct 15, 2025 at 03:27:12PM +0200, Heiko Stübner wrote:
> > > Am Mittwoch, 15. Oktober 2025, 14:58:46 Mitteleuropäische Sommerzeit schrieb Quentin Schulz:
> > > > On 10/8/25 3:31 PM, Heiko Stuebner wrote:
> > > > > dclk_vop2_src currently has CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT
> > > > > flags set, which is vastly different than dclk_vop0_src or dclk_vop1_src,
> > > > > which have none of those.
> > > > > 
> > > > > With these flags in dclk_vop2_src, actually setting the clock then results
> > > > > in a lot of other peripherals breaking, because setting the rate results
> > > > > in the PLL source getting changed:
> > > > > 
> > > > > [   14.898718] clk_core_set_rate_nolock: setting rate for dclk_vop2 to 152840000
> > > > > [   15.155017] clk_change_rate: setting rate for pll_gpll to 1680000000
> > > > > [ clk adjusting every gpll user ]
> > > > > 
> > > > > This includes possibly the other vops, i2s, spdif and even the uarts.
> > > > > Among other possible things, this breaks the uart console on a board
> > > > > I use. Sometimes it recovers later on, but there will be a big block
> > > > 
> > > > I can reproduce on the same board as yours and this fixes the issue 
> > > > indeed (note I can only reproduce for now when display the modetest 
> > > > pattern, otherwise after boot the console seems fine to me).
> > > 
> > > I boot into a Debian rootfs with fbcon on my system, and the serial
> > > console produces garbled output when the vop adjusts the clock
> > > 
> > > Sometimes it recovers after a bit, but other times it doesn't
> > > 
> > > > Reviewed-by: Quentin Schulz <quentin.schulz@...rry.de>
> > > > Tested-by: Quentin Schulz <quentin.schulz@...rry.de> # RK3588 Tiger w/DP carrierboard
> > 
> > I'm pretty sure I've seen this while playing with USB-C DP AltMode
> > on Rock 5B. So far I had no time to investigate further.
> > 
> > What I'm missing in the commit message is the impact on VOP. Also
> > it might be a good idea to have Andy in Cc, so I've added him.
> 
> Hmm, it brings VP2 in line with the other two VPs, only VP2 had this
> special setting - even right from the start, so it could very well
> have been left there accidentially during submission.

I did the initial upstream submission based on downstream (the TRM
is quite bad regading describing the clock trees, so not much
validation has been done by me). The old vendor kernel tree had it
like this, but that also changed a bit over time afterwards and no
longer has any special handling for VP2. OTOH it does set
CLK_SET_RATE_NO_REPARENT for all dclk_vop<number>_src, which you
are now removing for VP2.

FWIW these are the two flags:

#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up one level */
#define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */

So by removing CLK_SET_RATE_NO_REPARENT you are allowing dclk_vop2_src
to be switched to a different PLL when a different rate is being
requested. That change is completley unrelated to the bug you are
seeing right now?

> So in the end VP2 will have to deal with this, because when the VP
> causes a rate change in the GPLL, this changes so many clocks of
> other possibly running devices. Not only the uart, but also emmc
> and many more. And all those devices do not like if their clock gets
> changed under them I think.

It's certainly weird, that VP2 was (and still is in upstream) handled
special. Note that GPLL being changed is not really necessary.
dclk_vop2_src parent can be GPLL, CPLL, V0PLL or AUPLL. Effects on
other hardware IP very much depends on the parent setup. What I try
to understand is if there is also a bug in the rockchipdrm driver
and/or if removing CLK_SET_RATE_NO_REPARENT is a good idea. That's
why I hoped Andy could chime in and provide some background :)

Greetings,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)