[PATCH] clk: rk3308: make ddrphy4x clock critical

[Saravana Kannan]

On Thu, Jul 29, 2021 at 12:06 PM Stephen Boyd <sboyd at kernel.org> wrote:
>
> Quoting Heiko Stübner (2021-07-28 02:53:54)
> > Am Dienstag, 27. Juli 2021, 03:08:10 CEST schrieb Stephen Boyd:
> > > Quoting Yunhao Tian (2021-07-21 05:48:16)
> > > > Currently, no driver support for DDR memory controller (DMC) is present,
> > > > as a result, no driver is explicitly consuming the ddrphy clock. This means
> > > > that VPLL1 (parent of ddr clock) will be shutdown if we enable
> > > > and then disable any child clock of VPLL1 (e.g. SCLK_I2S0_8CH_TX).
> > > > If VPLL1 is disabled, the whole system will freeze, because the DDR
> > > > controller will lose its clock. So, it's necessary to prevent VPLL1 from
> > > > shutting down, by marking the ddrphy4x CLK_IS_CRITICAL.
> > > >
> > > > This bug was discovered when I was porting rockchip_i2s_tdm driver to
> > > > mainline kernel from Rockchip 4.4 kernel. I guess that other Rockchip
> > > > SoCs without DMC driver may need the same patch. If this applies to
> > > > other devices, please let us know.
> > > >
> > > > Signed-off-by: Yunhao Tian <t123yh at outlook.com>
> > > > ---
> > > >  drivers/clk/rockchip/clk-rk3308.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/clk/rockchip/clk-rk3308.c b/drivers/clk/rockchip/clk-rk3308.c
> > > > index 2c3bd0c749f2..6be077166330 100644
> > > > --- a/drivers/clk/rockchip/clk-rk3308.c
> > > > +++ b/drivers/clk/rockchip/clk-rk3308.c
> > > > @@ -564,7 +564,7 @@ static struct rockchip_clk_branch rk3308_clk_branches[] __initdata = {
> > > >         COMPOSITE(SCLK_DDRCLK, "clk_ddrphy4x_src", mux_dpll_vpll0_vpll1_p, CLK_IGNORE_UNUSED,
> > > >                         RK3308_CLKSEL_CON(1), 6, 2, MFLAGS, 0, 3, DFLAGS,
> > > >                         RK3308_CLKGATE_CON(0), 10, GFLAGS),
> > > > -       GATE(0, "clk_ddrphy4x", "clk_ddrphy4x_src", CLK_IGNORE_UNUSED,
> > > > +       GATE(0, "clk_ddrphy4x", "clk_ddrphy4x_src", CLK_IGNORE_UNUSED | CLK_IS_CRITICAL,
> > >
> > > Is it not enabled by default?
> >
> > All gates are enabled by default, but this gate shares a common parent
> > tree down to a pll, so if another leaf-user is disabling their part, this
> > untracked clock would get disabled as well.
>
> Right, this problem is cropping up in different places for various
> drivers.
>
> >
> > On that note, I remember a sort of CLK_HANDOFF was planned way back
> > in the past, meaning clock is critical until a driver picks it up, after this the
> > driver is responsible for it. Did that get any momentum?
> >
>
> Last I saw Saravana sent a patch to sort of connect CLK_HANDOFF to
> device driver sync_state() callback. I think we need to at least stash
> away that a clk is enabled at boot and then figure out how to tie in
> sync_state and/or something else.

Yeah, my clk_sync_state() series should do that. I'll get back on that
patch this week or next.

Yunhao,

Is there at least some DT device that consumes the DDR phy clock? Can
you point me to the DT for this board (not the SoC) so I can take a
look at it later?

Thanks,
Saravana