Message ID | BYAPR20MB24886765F888A9705CBEB70789E39@BYAPR20MB2488.namprd20.prod.outlook.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | clk: rk3308: make ddrphy4x clock critical | expand |
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),
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.
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
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,
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.
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
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
> 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
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
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),
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(-)