* [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-21 12:48 ` Yunhao Tian 0 siblings, 0 replies; 27+ messages in thread From: Yunhao Tian @ 2021-07-21 12:48 UTC (permalink / raw) To: heiko Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-rockchip 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, 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), -- 2.32.0 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-21 12:48 ` Yunhao Tian 0 siblings, 0 replies; 27+ messages in thread From: Yunhao Tian @ 2021-07-21 12:48 UTC (permalink / raw) To: heiko Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-rockchip 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, 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), -- 2.32.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-07-21 12:48 ` Yunhao Tian (?) @ 2021-07-27 1:08 ` Stephen Boyd -1 siblings, 0 replies; 27+ messages in thread From: Stephen Boyd @ 2021-07-27 1:08 UTC (permalink / raw) To: Yunhao Tian, heiko Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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), ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-27 1:08 ` Stephen Boyd 0 siblings, 0 replies; 27+ messages in thread From: Stephen Boyd @ 2021-07-27 1:08 UTC (permalink / raw) To: Yunhao Tian, heiko Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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), _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-27 1:08 ` Stephen Boyd 0 siblings, 0 replies; 27+ messages in thread From: Stephen Boyd @ 2021-07-27 1:08 UTC (permalink / raw) To: Yunhao Tian, heiko Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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), _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* 回复: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-07-27 1:08 ` Stephen Boyd @ 2021-07-27 1:22 ` Tian Yunhao -1 siblings, 0 replies; 27+ messages in thread From: Tian Yunhao @ 2021-07-27 1:22 UTC (permalink / raw) To: Stephen Boyd, heiko Cc: t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip Quoting Stephen Boyd <sboyd@kernel.org> > Is it not enabled by default? Indeed it's enabled by default upon power-up, but it's not called by any clk_enable (if you look at clk_summary, enable count is 0). The clk framework thinks that it's an unnecessary clock, and decides the parent, VPLL1, can be shutdown any time. If you enable and then disable its siblings (like mclk_i2s0_8ch_in), the clk framework will think that VPLL1 is no longer necessary, thus shutdown this PLL. This will cause DDR to lose clock. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* 回复: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-27 1:22 ` Tian Yunhao 0 siblings, 0 replies; 27+ messages in thread From: Tian Yunhao @ 2021-07-27 1:22 UTC (permalink / raw) To: Stephen Boyd, heiko Cc: t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip Quoting Stephen Boyd <sboyd@kernel.org> > Is it not enabled by default? Indeed it's enabled by default upon power-up, but it's not called by any clk_enable (if you look at clk_summary, enable count is 0). The clk framework thinks that it's an unnecessary clock, and decides the parent, VPLL1, can be shutdown any time. If you enable and then disable its siblings (like mclk_i2s0_8ch_in), the clk framework will think that VPLL1 is no longer necessary, thus shutdown this PLL. This will cause DDR to lose clock. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-07-27 1:08 ` Stephen Boyd (?) @ 2021-07-28 9:53 ` Heiko Stübner -1 siblings, 0 replies; 27+ messages in thread From: Heiko Stübner @ 2021-07-28 9:53 UTC (permalink / raw) To: Yunhao Tian, Stephen Boyd Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-28 9:53 ` Heiko Stübner 0 siblings, 0 replies; 27+ messages in thread From: Heiko Stübner @ 2021-07-28 9:53 UTC (permalink / raw) To: Yunhao Tian, Stephen Boyd Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-28 9:53 ` Heiko Stübner 0 siblings, 0 replies; 27+ messages in thread From: Heiko Stübner @ 2021-07-28 9:53 UTC (permalink / raw) To: Yunhao Tian, Stephen Boyd Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-07-28 9:53 ` Heiko Stübner (?) @ 2021-07-29 19:06 ` Stephen Boyd -1 siblings, 0 replies; 27+ messages in thread From: Stephen Boyd @ 2021-07-29 19:06 UTC (permalink / raw) To: Heiko Stübner, Yunhao Tian, saravanak Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-29 19:06 ` Stephen Boyd 0 siblings, 0 replies; 27+ messages in thread From: Stephen Boyd @ 2021-07-29 19:06 UTC (permalink / raw) To: Heiko Stübner, Yunhao Tian, saravanak Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-29 19:06 ` Stephen Boyd 0 siblings, 0 replies; 27+ messages in thread From: Stephen Boyd @ 2021-07-29 19:06 UTC (permalink / raw) To: Heiko Stübner, Yunhao Tian, saravanak Cc: t123yh.xyz, Yunhao Tian, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-07-29 19:06 ` Stephen Boyd (?) @ 2021-08-02 18:24 ` Saravana Kannan -1 siblings, 0 replies; 27+ messages in thread From: Saravana Kannan @ 2021-08-02 18:24 UTC (permalink / raw) To: Stephen Boyd Cc: Heiko Stübner, Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-02 18:24 ` Saravana Kannan 0 siblings, 0 replies; 27+ messages in thread From: Saravana Kannan @ 2021-08-02 18:24 UTC (permalink / raw) To: Stephen Boyd Cc: Heiko Stübner, Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-02 18:24 ` Saravana Kannan 0 siblings, 0 replies; 27+ messages in thread From: Saravana Kannan @ 2021-08-02 18:24 UTC (permalink / raw) To: Stephen Boyd Cc: Heiko Stübner, Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-08-02 18:24 ` Saravana Kannan (?) @ 2021-08-02 19:25 ` Heiko Stübner -1 siblings, 0 replies; 27+ messages in thread From: Heiko Stübner @ 2021-08-02 19:25 UTC (permalink / raw) To: Stephen Boyd, Saravana Kannan Cc: Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-02 19:25 ` Heiko Stübner 0 siblings, 0 replies; 27+ messages in thread From: Heiko Stübner @ 2021-08-02 19:25 UTC (permalink / raw) To: Stephen Boyd, Saravana Kannan Cc: Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-02 19:25 ` Heiko Stübner 0 siblings, 0 replies; 27+ messages in thread From: Heiko Stübner @ 2021-08-02 19:25 UTC (permalink / raw) To: Stephen Boyd, Saravana Kannan Cc: Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-08-02 19:25 ` Heiko Stübner (?) @ 2021-08-05 21:51 ` Saravana Kannan -1 siblings, 0 replies; 27+ messages in thread From: Saravana Kannan @ 2021-08-05 21:51 UTC (permalink / raw) To: Heiko Stübner Cc: Stephen Boyd, Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-05 21:51 ` Saravana Kannan 0 siblings, 0 replies; 27+ messages in thread From: Saravana Kannan @ 2021-08-05 21:51 UTC (permalink / raw) To: Heiko Stübner Cc: Stephen Boyd, Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-05 21:51 ` Saravana Kannan 0 siblings, 0 replies; 27+ messages in thread From: Saravana Kannan @ 2021-08-05 21:51 UTC (permalink / raw) To: Heiko Stübner Cc: Stephen Boyd, Yunhao Tian, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-08-02 18:24 ` Saravana Kannan @ 2021-08-03 2:23 ` Tian Yunhao -1 siblings, 0 replies; 27+ messages in thread From: Tian Yunhao @ 2021-08-03 2:23 UTC (permalink / raw) To: Saravana Kannan, Stephen Boyd Cc: Heiko Stübner, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip > 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? Hi Saravana, If you don't mind, you can refer to 4.4 kernel modified by Rockchip. This is one of a board DTS: [1] (note line 203: &dmc). This is not the board I'm using, but I think specific board is not related to our problem here. [1]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/arch/arm64/boot/dts/rockchip/rk3308-ai-va-v10.dts _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-08-03 2:23 ` Tian Yunhao 0 siblings, 0 replies; 27+ messages in thread From: Tian Yunhao @ 2021-08-03 2:23 UTC (permalink / raw) To: Saravana Kannan, Stephen Boyd Cc: Heiko Stübner, t123yh.xyz, Michael Turquette, linux-clk, linux-arm-kernel, linux-rockchip > 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? Hi Saravana, If you don't mind, you can refer to 4.4 kernel modified by Rockchip. This is one of a board DTS: [1] (note line 203: &dmc). This is not the board I'm using, but I think specific board is not related to our problem here. [1]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/arch/arm64/boot/dts/rockchip/rk3308-ai-va-v10.dts _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical 2021-07-21 12:48 ` Yunhao Tian (?) @ 2021-07-29 13:19 ` Heiko Stuebner -1 siblings, 0 replies; 27+ messages in thread From: Heiko Stuebner @ 2021-07-29 13:19 UTC (permalink / raw) To: Yunhao Tian Cc: Heiko Stuebner, linux-rockchip, t123yh.xyz, Stephen Boyd, linux-clk, Michael Turquette, linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-29 13:19 ` Heiko Stuebner 0 siblings, 0 replies; 27+ messages in thread From: Heiko Stuebner @ 2021-07-29 13:19 UTC (permalink / raw) To: Yunhao Tian Cc: Heiko Stuebner, linux-rockchip, t123yh.xyz, Stephen Boyd, linux-clk, Michael Turquette, linux-arm-kernel 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> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] clk: rk3308: make ddrphy4x clock critical @ 2021-07-29 13:19 ` Heiko Stuebner 0 siblings, 0 replies; 27+ messages in thread From: Heiko Stuebner @ 2021-07-29 13:19 UTC (permalink / raw) To: Yunhao Tian Cc: Heiko Stuebner, linux-rockchip, t123yh.xyz, Stephen Boyd, linux-clk, Michael Turquette, linux-arm-kernel 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> _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-08-05 21:54 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-21 12:48 [PATCH] clk: rk3308: make ddrphy4x clock critical Yunhao Tian 2021-07-21 12:48 ` Yunhao Tian 2021-07-27 1:08 ` Stephen Boyd 2021-07-27 1:08 ` Stephen Boyd 2021-07-27 1:08 ` Stephen Boyd 2021-07-27 1:22 ` 回复: " Tian Yunhao 2021-07-27 1:22 ` Tian Yunhao 2021-07-28 9:53 ` Heiko Stübner 2021-07-28 9:53 ` Heiko Stübner 2021-07-28 9:53 ` Heiko Stübner 2021-07-29 19:06 ` Stephen Boyd 2021-07-29 19:06 ` Stephen Boyd 2021-07-29 19:06 ` Stephen Boyd 2021-08-02 18:24 ` Saravana Kannan 2021-08-02 18:24 ` Saravana Kannan 2021-08-02 18:24 ` Saravana Kannan 2021-08-02 19:25 ` Heiko Stübner 2021-08-02 19:25 ` Heiko Stübner 2021-08-02 19:25 ` Heiko Stübner 2021-08-05 21:51 ` Saravana Kannan 2021-08-05 21:51 ` Saravana Kannan 2021-08-05 21:51 ` Saravana Kannan 2021-08-03 2:23 ` Tian Yunhao 2021-08-03 2:23 ` Tian Yunhao 2021-07-29 13:19 ` Heiko Stuebner 2021-07-29 13:19 ` Heiko Stuebner 2021-07-29 13:19 ` Heiko Stuebner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.