Re: [PATCH] clk: rockchip: rk3588: Don't change PLL rates when setting dclk_vop2_src

[Heiko Stuebner <heiko@sntech.de>]

Am Dienstag, 28. Oktober 2025, 02:25:14 Mitteleuropäische Normalzeit schrieb Andy Yan:
> 
> Hello，
> 在 2025-10-27 21:20:15，"Sebastian Reichel" <sebastian.reichel@collabora.com> 写道：
> >Hi,
> >
> >On Mon, Oct 27, 2025 at 10:03:57AM +0800, Andy Yan wrote:
> >> At 2025-10-21 00:00:59, "Sebastian Reichel" <sebastian.reichel@collabora.com> wrote:
> >> >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@cherry.de>
> >> >> > > > Tested-by: Quentin Schulz <quentin.schulz@cherry.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?

I guess the actual bug is, that VP2 cannot set its rate with the same
flexibility as the other VPs. I guess I can split that in two patches.

One dropping the SET_RATE_PARENT, to fix the actual bug, the
other dropping the NO_REPARENT flag to give VP2 the same
possiblities to find a rate.


> >> >> 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 :)
> >> 
> >> The main limitation is that there are not enough PLLs on the SoC
> >> to be used for the display side. In our downstream code
> >> implementation, we usually exclusively assign V0PLL to a certain
> >> VP. Other VPs generally need to share the PLL with other
> >> peripherals , or use the HDMI PHY PLL.
> >> 
> >> For GPLL and CPLL,  they will be set to a fixed frequency during
> >> the system startup stage, and they should not be modified again as
> >> these two PLL always shared by other peripherals.
> >> 
> >> When shared with other peripherals,  we can not do
> >> CLK_SET_RATE_PARENT,. However, when we need a relatively precise
> >> frequency in certain scenarios, such as driving an eDP or DSI
> >> panel（see what we do for eDP on rk3588s-evb1-v10.dts and
> >> rk3588-coolpi-cm5-genbook.dts ）, we tend to use V0PLL. But since
> >> V0PLL does not proper initializated at system startup, we then
> >> need CLK_SET_RATE_PARENT. This does indeed seem to be a
> >> contradiction.
> >
> >I suppose for eDP and DSI, which are more or less fixed, it would

While DSI is fixed, eDP is not. While the Analogix-DP cannot support
DP+ (DP converted to HDMI), it can support a real DP connection just fine.


Heiko