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

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

[Heiko Stuebner]

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
of garbled output for a while at least.

Shared PLLs should not be changed by individual users, so drop these
flags from dclk_vop2_src and make the flags the same as on dclk_vop0
and dclk_vop1.

Fixes: f1c506d152ff ("clk: rockchip: add clock controller for the RK3588")
Cc: stable@vger.kernel.org
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3588.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3588.c b/drivers/clk/rockchip/clk-rk3588.c
index 1694223f4f84..cf83242d1726 100644
--- a/drivers/clk/rockchip/clk-rk3588.c
+++ b/drivers/clk/rockchip/clk-rk3588.c
@@ -2094,7 +2094,7 @@ static struct rockchip_clk_branch rk3588_early_clk_branches[] __initdata = {
 	COMPOSITE(DCLK_VOP1_SRC, "dclk_vop1_src", gpll_cpll_v0pll_aupll_p, 0,
 			RK3588_CLKSEL_CON(111), 14, 2, MFLAGS, 9, 5, DFLAGS,
 			RK3588_CLKGATE_CON(52), 11, GFLAGS),
-	COMPOSITE(DCLK_VOP2_SRC, "dclk_vop2_src", gpll_cpll_v0pll_aupll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
+	COMPOSITE(DCLK_VOP2_SRC, "dclk_vop2_src", gpll_cpll_v0pll_aupll_p, 0,
 			RK3588_CLKSEL_CON(112), 5, 2, MFLAGS, 0, 5, DFLAGS,
 			RK3588_CLKGATE_CON(52), 12, GFLAGS),
 	COMPOSITE_NODIV(DCLK_VOP0, "dclk_vop0", dclk_vop0_p,
-- 
2.47.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Quentin Schulz]

Hi Heiko,

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

So,

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3588 Tiger w/ 
DP carrierboard

> of garbled output for a while at least.
> 
> Shared PLLs should not be changed by individual users, so drop these
> flags from dclk_vop2_src and make the flags the same as on dclk_vop0
> and dclk_vop1.
> 

I hope there isn't a hardware reason for CLK_SET_RATE_NO_REPARENT which 
we remove here. But there's only one consumer of this clock (dclk_vop2) 
so this would be an isolated problem if there ever is one and now we 
match vop0 and vop1 behavior and I like consistency :)

Thanks!
Quentin

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Heiko Stübner]

Am Mittwoch, 15. Oktober 2025, 14:58:46 Mitteleuropäische Sommerzeit schrieb Quentin Schulz:
> Hi Heiko,
> 
> 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

Thanks for testing
Heiko



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1972 bytes --]

Hi,

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:
> > Hi Heiko,
> > 
> > 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.

Greetings,

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Heiko Stuebner]

Am Donnerstag, 16. Oktober 2025, 00:57:15 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> Hi,
> 
> 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:
> > > Hi Heiko,
> > > 
> > > 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.

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.


Heiko



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 4089 bytes --]

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@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?

> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Andy Yan]

Hello,

At 2025-10-21 00:00:59, "Sebastian Reichel" <sebastian.reichel@collabora.com> wrote:
>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@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?
>
>> 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.



>
>Greetings,
>
>-- Sebastian
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 5845 bytes --]

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?
> >
> >> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[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?
>> >
>> >> 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>;
>};


For a fixed screen, this solution is feasible. But what about pluggable interfaces like HDMI or DP? 
For example, if a board has two HDMI ports and one DP port, the two HDMI ports might support
any resolution using the HDMI PHY PLL, but in that case, the DP port might only be able to use the V0PLL. 
Under such circumstances, it's necessary to be able to dynamically adjust the PLL frequency.

I'm not sure whether the clock framework can enforce restrictions such that the frequencies of shared PLLs like CPLL and GPLL, 
which are used by other peripherals, cannot be modified, while allowing the frequency of an exclusive PLL like V0PLL, 
which is dedicated to display, to be adjustable.


>
>Greetings,
>
>-- Sebastian
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip