Message-ID: <w3ttg7rmkut44gbys7m7rcwvsa67d4bqyez5fie3cxgbtjs6ib@pyelryb6gth2>
Date: Mon, 27 Oct 2025 14:20:15 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Andy Yan <andyshrk@....com>
Cc: Heiko Stuebner <heiko@...ech.de>, 
	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 27, 2025 at 10:03:57AM +0800, Andy Yan wrote:
> At 2025-10-21 00:00:59, "Sebastian Reichel" <sebastian.reichel@...labora.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@...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 :)
> 
> 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
be possible to assign a board specific fixed frequency like this
and get the frequency initialized without relying on VOP setting
the PLL rate via CLK_SET_RATE_PARENT from the driver:

&vop {
    assigned-clocks = <&cru DCLK_VOP2_SRC>, <&cru PLL_V0PLL>;
    assigned-clock-parents = <&cru PLL_V0PLL>;
    assigned-clock-rates = <0>, <1337>;
};

Greetings,

-- Sebastian

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

Powered by blists - more mailing lists