Re: [PATCH 24/27] clk: rk3568: drop CLK_SET_RATE_PARENT from dclk_vop*

[Sascha Hauer <s.hauer@pengutronix.de>]

On Sat, Jan 29, 2022 at 06:48:13PM +0100, Heiko Stübner wrote:
> Am Mittwoch, 26. Januar 2022, 15:55:46 CET schrieb Sascha Hauer:
> > The pixel clocks dclk_vop[012] can be clocked from hpll, vpll, gpll or
> > cpll. gpll and cpll also drive many other clocks, so changing the
> > dclk_vop[012] clocks could change these other clocks as well. Drop
> > CLK_SET_RATE_PARENT to fix that. With this change the VOP2 driver can
> > only adjust the pixel clocks with the divider between the PLL and the
> > dclk_vop[012] which means the user may have to adjust the PLL clock to a
> > suitable rate using the assigned-clock-rate device tree property.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/clk/rockchip/clk-rk3568.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/rockchip/clk-rk3568.c b/drivers/clk/rockchip/clk-rk3568.c
> > index 9d889fc46811..7687c62d1fa8 100644
> > --- a/drivers/clk/rockchip/clk-rk3568.c
> > +++ b/drivers/clk/rockchip/clk-rk3568.c
> > @@ -1044,13 +1044,13 @@ static struct rockchip_clk_branch rk3568_clk_branches[] __initdata = {
> >  			RK3568_CLKGATE_CON(20), 8, GFLAGS),
> >  	GATE(HCLK_VOP, "hclk_vop", "hclk_vo", 0,
> >  			RK3568_CLKGATE_CON(20), 9, GFLAGS),
> > -	COMPOSITE(DCLK_VOP0, "dclk_vop0", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
> > +	COMPOSITE(DCLK_VOP0, "dclk_vop0", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_NO_REPARENT,
> 
> hmm, I'm wondering about the use of having CLK_SET_RATE_NO_REPARENT here
> (and even adding it below).
> 
> Using SET_RATE_PARENT in the following patch for the hdmi-pll, should give
> us at least a suitable rate for the hdmi output, so the vop using that
> should already find a nice rate to use.
> 
> The normal system-PLLs don't normally don't change their rate at runtime,
> so I think we should liberate the dclks to select a PLL that best matches
> their target rate - so drop the CLK_SET_RATE_NO_REPARENT as well.
> 
> That way the DCLKs can change to another PLL source if that provides
> a rate nearer to their target.

The HDMI reference clock has the CLK_SET_RATE_PARENT flag set and we
need that to program the HPLL clock to suitable rates for the HDMI
output. Now any other display choosing HPLL as parent, because that
provides the best rate in that point of time, hangs on a PLL which
changes its rate whenever the resolution is changed on the HDMI output.

Changing parents on rate changes only works when all possible parents of
all the children involved have a constant rate. IMO allowing reparenting
on rate changes is a poorly chosen default because it's very unsafe. We
should rather have a CLK_SET_RATE_ALLOW_REPARENT flag.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |