Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

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

>                         RK3308_CLKGATE_CON(0), 11, GFLAGS),
>         FACTOR_GATE(0, "clk_ddr_stdby_div4", "clk_ddrphy4x", CLK_IGNORE_UNUSED, 1, 4,
>                         RK3308_CLKGATE_CON(0), 13, GFLAGS),

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

[Heiko Stübner]

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

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?

Heiko

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

[Stephen Boyd]

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

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

[Saravana Kannan]

On Thu, Jul 29, 2021 at 12:06 PM Stephen Boyd <sboyd@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@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

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

[Heiko Stübner]

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


The whole memory handling is a blank slate on the kernel side for Rockchip
boards still. The bootloader sets up the memory and nobody has found the
time to modell things like memory scaling in a nice way yet.


Heiko

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

[Saravana Kannan]

On Mon, Aug 2, 2021 at 12:26 PM Heiko Stübner <heiko@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@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@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

Re: [PATCH] clk: rk3308: make ddrphy4x clock critical

[Heiko Stuebner]

On Wed, 21 Jul 2021 20:48:16 +0800, Yunhao Tian wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] clk: rk3308: make ddrphy4x clock critical
      commit: c0c81245dac7caaef4db627fb7043495d1afe662

Though I moved the clock to the pre-existing
list of critical clocks.

I'm still hoping for that handoff mechanism before
we sprinkle CLK_IS_CRITICAL throughout the clock trees.


Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>