| Message ID | 20220126145549.617165-25-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
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. Heiko > RK3568_CLKSEL_CON(39), 10, 2, MFLAGS, 0, 8, DFLAGS, > RK3568_CLKGATE_CON(20), 10, GFLAGS), > - COMPOSITE(DCLK_VOP1, "dclk_vop1", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > + COMPOSITE(DCLK_VOP1, "dclk_vop1", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_NO_REPARENT, > RK3568_CLKSEL_CON(40), 10, 2, MFLAGS, 0, 8, DFLAGS, > RK3568_CLKGATE_CON(20), 11, GFLAGS), > - COMPOSITE(DCLK_VOP2, "dclk_vop2", hpll_vpll_gpll_cpll_p, 0, > + COMPOSITE(DCLK_VOP2, "dclk_vop2", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_NO_REPARENT, > RK3568_CLKSEL_CON(41), 10, 2, MFLAGS, 0, 8, DFLAGS, > RK3568_CLKGATE_CON(20), 12, GFLAGS), > GATE(CLK_VOP_PWM, "clk_vop_pwm", "xin24m", 0, >
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
Am Montag, 31. Januar 2022, 09:10:42 CET schrieb Sascha Hauer: > 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. Ah, right ... the hpll is in the parent list, that changes things as you said. I somehow only noticed the regular PLLs that normally have a constant rate. So never mind ;-) Heiko > 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 > >
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, RK3568_CLKSEL_CON(39), 10, 2, MFLAGS, 0, 8, DFLAGS, RK3568_CLKGATE_CON(20), 10, GFLAGS), - COMPOSITE(DCLK_VOP1, "dclk_vop1", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, + COMPOSITE(DCLK_VOP1, "dclk_vop1", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_NO_REPARENT, RK3568_CLKSEL_CON(40), 10, 2, MFLAGS, 0, 8, DFLAGS, RK3568_CLKGATE_CON(20), 11, GFLAGS), - COMPOSITE(DCLK_VOP2, "dclk_vop2", hpll_vpll_gpll_cpll_p, 0, + COMPOSITE(DCLK_VOP2, "dclk_vop2", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_NO_REPARENT, RK3568_CLKSEL_CON(41), 10, 2, MFLAGS, 0, 8, DFLAGS, RK3568_CLKGATE_CON(20), 12, GFLAGS), GATE(CLK_VOP_PWM, "clk_vop_pwm", "xin24m", 0,
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(-)