[PATCH] clk: rk3308: make ddrphy4x clock critical

[Saravana Kannan]

On Mon, Aug 2, 2021 at 12:26 PM Heiko Stübner <heiko at sntech.de> wrote:
>
> Hi Saravana,
>
> Am Montag, 2. August 2021, 20:24:56 CEST schrieb 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?
>
> Not for the rk3308. If you're looking for live-examples of handoff clocks,
> I can provide another examples though:
>
>
> rockchip/clk-rk3288.c - pclk_rkpwm (in the separate critical clock list) ... with
> arch/arm/boot/dts/rk3288.dtsi - clock is supplying pwm nodes.
>
> As the comment in the clock driver suggests (line 850), some boards use
> pwm-regulators for central components. The pwm-regulators are configured
> at boot already, so the clock shouldn't be disabled till the pwm-regulator takes
> over.

If you can reproduce the issue on your end if you remove the
pclk_rkpwm clock from the critical clock list, then can you try this
series?
https://lore.kernel.org/lkml/20210407034456.516204-1-saravanak@google.com/

It should keep the pclk_rkpwm clock on till all the consumers of the
clock have probed. And after that it'll actually allow the clock to be
turned off instead of keeping it on forever.

-Saravana