* [PATCH] cpufreq: imx6q: Fix clock enable balance @ 2013-08-26 11:48 Sascha Hauer 2013-08-26 14:05 ` Viresh Kumar 2013-08-29 14:01 ` Shawn Guo 0 siblings, 2 replies; 6+ messages in thread From: Sascha Hauer @ 2013-08-26 11:48 UTC (permalink / raw) To: linux-arm-kernel For changing the cpu frequency the i.MX6q has to be switched to some intermediate clock during the PLL reprogramming. The driver tries to be clever to keep the enable count correct but gets it wrong. If the cpufreq is increased it calls clk_disable_unprepare twice on pll2_pfd2_396m. This puts all other devices which get their clock from pll2_pfd2_396m into a nonworking state. Fix this by removing the clk enabling/disabling altogether since the clk core will do this automatically during a reparent. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/cpufreq/imx6q-cpufreq.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index e37cdae..2971d12 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -117,28 +117,11 @@ static int imx6q_set_target(struct cpufreq_policy *policy, * - Reprogram pll1_sys_clk and reparent pll1_sw_clk back to it * - Disable pll2_pfd2_396m_clk */ - clk_prepare_enable(pll2_pfd2_396m_clk); clk_set_parent(step_clk, pll2_pfd2_396m_clk); clk_set_parent(pll1_sw_clk, step_clk); if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) { clk_set_rate(pll1_sys_clk, freqs.new * 1000); - /* - * If we are leaving 396 MHz set-point, we need to enable - * pll1_sys_clk and disable pll2_pfd2_396m_clk to keep - * their use count correct. - */ - if (freqs.old * 1000 <= clk_get_rate(pll2_pfd2_396m_clk)) { - clk_prepare_enable(pll1_sys_clk); - clk_disable_unprepare(pll2_pfd2_396m_clk); - } clk_set_parent(pll1_sw_clk, pll1_sys_clk); - clk_disable_unprepare(pll2_pfd2_396m_clk); - } else { - /* - * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient - * to provide the frequency. - */ - clk_disable_unprepare(pll1_sys_clk); } /* Ensure the arm clock divider is what we expect */ -- 1.8.4.rc3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] cpufreq: imx6q: Fix clock enable balance 2013-08-26 11:48 [PATCH] cpufreq: imx6q: Fix clock enable balance Sascha Hauer @ 2013-08-26 14:05 ` Viresh Kumar 2013-08-29 14:01 ` Shawn Guo 1 sibling, 0 replies; 6+ messages in thread From: Viresh Kumar @ 2013-08-26 14:05 UTC (permalink / raw) To: linux-arm-kernel On 26 August 2013 17:18, Sascha Hauer <s.hauer@pengutronix.de> wrote: > For changing the cpu frequency the i.MX6q has to be switched to some > intermediate clock during the PLL reprogramming. The driver tries > to be clever to keep the enable count correct but gets it wrong. If > the cpufreq is increased it calls clk_disable_unprepare twice > on pll2_pfd2_396m. This puts all other devices which get their clock > from pll2_pfd2_396m into a nonworking state. > > Fix this by removing the clk enabling/disabling altogether since the > clk core will do this automatically during a reparent. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/cpufreq/imx6q-cpufreq.c | 17 ----------------- > 1 file changed, 17 deletions(-) Applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cpufreq: imx6q: Fix clock enable balance 2013-08-26 11:48 [PATCH] cpufreq: imx6q: Fix clock enable balance Sascha Hauer 2013-08-26 14:05 ` Viresh Kumar @ 2013-08-29 14:01 ` Shawn Guo 2013-08-30 19:00 ` Sascha Hauer 1 sibling, 1 reply; 6+ messages in thread From: Shawn Guo @ 2013-08-29 14:01 UTC (permalink / raw) To: linux-arm-kernel On Mon, Aug 26, 2013 at 01:48:36PM +0200, Sascha Hauer wrote: > For changing the cpu frequency the i.MX6q has to be switched to some > intermediate clock during the PLL reprogramming. The driver tries > to be clever to keep the enable count correct but gets it wrong. If > the cpufreq is increased it calls clk_disable_unprepare twice > on pll2_pfd2_396m. This puts all other devices which get their clock > from pll2_pfd2_396m into a nonworking state. So you're running into a problem in real? The clk_disable_unprepare on pll2_pfd2_396m below will only be executed when are leaving 396MHz set-point. It's there to balance the clk_prepare_enable on pll2_pfd2_396m when we enters 396MHz set-point. if (freqs.old * 1000 <= clk_get_rate(pll2_pfd2_396m_clk)) { clk_prepare_enable(pll1_sys_clk); clk_disable_unprepare(pll2_pfd2_396m_clk); } > > Fix this by removing the clk enabling/disabling altogether since the > clk core will do this automatically during a reparent. It seems clk core will only enable the parent clock during the clk_set_parent() call, and only in case that the child clock is prepared. For example, I do not think pll2_pfd2_396m_clk and step_clk will be altered to ON state. Or am I missing something? clk_set_parent(step_clk, pll2_pfd2_396m_clk); clk_set_parent(pll1_sw_clk, step_clk); Shawn > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/cpufreq/imx6q-cpufreq.c | 17 ----------------- > 1 file changed, 17 deletions(-) > > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c > index e37cdae..2971d12 100644 > --- a/drivers/cpufreq/imx6q-cpufreq.c > +++ b/drivers/cpufreq/imx6q-cpufreq.c > @@ -117,28 +117,11 @@ static int imx6q_set_target(struct cpufreq_policy *policy, > * - Reprogram pll1_sys_clk and reparent pll1_sw_clk back to it > * - Disable pll2_pfd2_396m_clk > */ > - clk_prepare_enable(pll2_pfd2_396m_clk); > clk_set_parent(step_clk, pll2_pfd2_396m_clk); > clk_set_parent(pll1_sw_clk, step_clk); > if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) { > clk_set_rate(pll1_sys_clk, freqs.new * 1000); > - /* > - * If we are leaving 396 MHz set-point, we need to enable > - * pll1_sys_clk and disable pll2_pfd2_396m_clk to keep > - * their use count correct. > - */ > - if (freqs.old * 1000 <= clk_get_rate(pll2_pfd2_396m_clk)) { > - clk_prepare_enable(pll1_sys_clk); > - clk_disable_unprepare(pll2_pfd2_396m_clk); > - } > clk_set_parent(pll1_sw_clk, pll1_sys_clk); > - clk_disable_unprepare(pll2_pfd2_396m_clk); > - } else { > - /* > - * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient > - * to provide the frequency. > - */ > - clk_disable_unprepare(pll1_sys_clk); > } > > /* Ensure the arm clock divider is what we expect */ > -- > 1.8.4.rc3 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cpufreq: imx6q: Fix clock enable balance 2013-08-29 14:01 ` Shawn Guo @ 2013-08-30 19:00 ` Sascha Hauer 2013-09-02 15:02 ` Shawn Guo 0 siblings, 1 reply; 6+ messages in thread From: Sascha Hauer @ 2013-08-30 19:00 UTC (permalink / raw) To: linux-arm-kernel On Thu, Aug 29, 2013 at 10:01:25PM +0800, Shawn Guo wrote: > On Mon, Aug 26, 2013 at 01:48:36PM +0200, Sascha Hauer wrote: > > For changing the cpu frequency the i.MX6q has to be switched to some > > intermediate clock during the PLL reprogramming. The driver tries > > to be clever to keep the enable count correct but gets it wrong. If > > the cpufreq is increased it calls clk_disable_unprepare twice > > on pll2_pfd2_396m. This puts all other devices which get their clock > > from pll2_pfd2_396m into a nonworking state. > > So you're running into a problem in real? The clk_disable_unprepare on > pll2_pfd2_396m below will only be executed when are leaving 396MHz > set-point. And that's when my SD card stops working. On my board the SD clock is derived from pll2_pfd2_396m. I used the userspace cpufreq governor and scaled down to 396MHz. When I scale up again the SDHC driver times out while waiting for interrupts. This is because the cpufreq driver disables the clock twice. > It's there to balance the clk_prepare_enable on > pll2_pfd2_396m when we enters 396MHz set-point. > > if (freqs.old * 1000 <= clk_get_rate(pll2_pfd2_396m_clk)) { > clk_prepare_enable(pll1_sys_clk); > clk_disable_unprepare(pll2_pfd2_396m_clk); > } > > > > > Fix this by removing the clk enabling/disabling altogether since the > > clk core will do this automatically during a reparent. > > It seems clk core will only enable the parent clock during the > clk_set_parent() call, and only in case that the child clock is > prepared. For example, I do not think pll2_pfd2_396m_clk and step_clk > will be altered to ON state. Or am I missing something? > It seems so, yes. Reparenting takes the prepare/enable state of a clock with it. See __clk_set_parent, it starts with: if (clk->prepare_count) { __clk_prepare(parent); clk_enable(parent); clk_enable(clk); } So if the clock to be reparent is enabled then the new parent gets enabled aswell. And it ends with: if (clk->prepare_count) { clk_disable(clk); clk_disable(old_parent); __clk_unprepare(old_parent); } So if the clock is enabled the old parent now decreases its enable count. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cpufreq: imx6q: Fix clock enable balance 2013-08-30 19:00 ` Sascha Hauer @ 2013-09-02 15:02 ` Shawn Guo 2013-09-02 18:27 ` Sascha Hauer 0 siblings, 1 reply; 6+ messages in thread From: Shawn Guo @ 2013-09-02 15:02 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 30, 2013 at 09:00:19PM +0200, Sascha Hauer wrote: > On Thu, Aug 29, 2013 at 10:01:25PM +0800, Shawn Guo wrote: > > On Mon, Aug 26, 2013 at 01:48:36PM +0200, Sascha Hauer wrote: > > > For changing the cpu frequency the i.MX6q has to be switched to some > > > intermediate clock during the PLL reprogramming. The driver tries > > > to be clever to keep the enable count correct but gets it wrong. If > > > the cpufreq is increased it calls clk_disable_unprepare twice > > > on pll2_pfd2_396m. This puts all other devices which get their clock > > > from pll2_pfd2_396m into a nonworking state. > > > > So you're running into a problem in real? The clk_disable_unprepare on > > pll2_pfd2_396m below will only be executed when are leaving 396MHz > > set-point. > > And that's when my SD card stops working. On my board the SD clock is > derived from pll2_pfd2_396m. I used the userspace cpufreq governor > and scaled down to 396MHz. When I scale up again the SDHC driver times > out while waiting for interrupts. This is because the cpufreq driver > disables the clock twice. So when you scale down to 396MHz, the following function sequence is all what you will call. clk_prepare_enable(pll2_pfd2_396m_clk); clk_set_parent(step_clk, pll2_pfd2_396m_clk); clk_set_parent(pll1_sw_clk, step_clk); if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) { ... } else { clk_disable_unprepare(pll1_sys_clk); } You will leave imx6q_set_target() with use count of pll2_pfd2_396m_clk increased. Then when you scale up, you call clk_prepare_enable() once and clk_disable_unprepare() twice on pll2_pfd2_396m_clk, and you get the use count balanced in the end. Isn't that the case for you? The question is not important though, since with your explanation below, I agree the changes you propose make more sense. <snip> > > > Fix this by removing the clk enabling/disabling altogether since the > > > clk core will do this automatically during a reparent. > > > > It seems clk core will only enable the parent clock during the > > clk_set_parent() call, and only in case that the child clock is > > prepared. For example, I do not think pll2_pfd2_396m_clk and step_clk > > will be altered to ON state. Or am I missing something? > > > > It seems so, yes. Reparenting takes the prepare/enable state > of a clock with it. See __clk_set_parent, it starts with: > > if (clk->prepare_count) { > __clk_prepare(parent); > clk_enable(parent); > clk_enable(clk); > } > > So if the clock to be reparent is enabled then the new parent > gets enabled aswell. And it ends with: > > if (clk->prepare_count) { > clk_disable(clk); > clk_disable(old_parent); > __clk_unprepare(old_parent); > } > > So if the clock is enabled the old parent now decreases its enable > count. Ah, yes, I forgot the fact that clock pll1_sw_clk must be always prepared/enabled in this case. So when clk_set_parent(pll1_sw_clk, step_clk) happens, both step_clk and pll2_pfd2_396m_clk will be enabled. Thanks. Shawn ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cpufreq: imx6q: Fix clock enable balance 2013-09-02 15:02 ` Shawn Guo @ 2013-09-02 18:27 ` Sascha Hauer 0 siblings, 0 replies; 6+ messages in thread From: Sascha Hauer @ 2013-09-02 18:27 UTC (permalink / raw) To: linux-arm-kernel On Mon, Sep 02, 2013 at 11:02:58PM +0800, Shawn Guo wrote: > On Fri, Aug 30, 2013 at 09:00:19PM +0200, Sascha Hauer wrote: > > On Thu, Aug 29, 2013 at 10:01:25PM +0800, Shawn Guo wrote: > > > On Mon, Aug 26, 2013 at 01:48:36PM +0200, Sascha Hauer wrote: > > > > For changing the cpu frequency the i.MX6q has to be switched to some > > > > intermediate clock during the PLL reprogramming. The driver tries > > > > to be clever to keep the enable count correct but gets it wrong. If > > > > the cpufreq is increased it calls clk_disable_unprepare twice > > > > on pll2_pfd2_396m. This puts all other devices which get their clock > > > > from pll2_pfd2_396m into a nonworking state. > > > > > > So you're running into a problem in real? The clk_disable_unprepare on > > > pll2_pfd2_396m below will only be executed when are leaving 396MHz > > > set-point. > > > > And that's when my SD card stops working. On my board the SD clock is > > derived from pll2_pfd2_396m. I used the userspace cpufreq governor > > and scaled down to 396MHz. When I scale up again the SDHC driver times > > out while waiting for interrupts. This is because the cpufreq driver > > disables the clock twice. > > So when you scale down to 396MHz, the following function sequence is all > what you will call. > > clk_prepare_enable(pll2_pfd2_396m_clk); > clk_set_parent(step_clk, pll2_pfd2_396m_clk); > clk_set_parent(pll1_sw_clk, step_clk); > if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) { > ... > } else { > clk_disable_unprepare(pll1_sys_clk); > } > > You will leave imx6q_set_target() with use count of pll2_pfd2_396m_clk > increased. Then when you scale up, you call clk_prepare_enable() once > and clk_disable_unprepare() twice on pll2_pfd2_396m_clk, and you get > the use count balanced in the end. Isn't that the case for you? I see what you mean. I looked a bit deeper. The issue seems to be that my board comes up with 396MHz. The code assumes that it switches to 396MHz and back again. When imx6q_set_target is entered with 396MHz for the first time after startup then it calls clk_disable_unprepare twice without having called clk_prepare_enable twice before. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-02 18:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-26 11:48 [PATCH] cpufreq: imx6q: Fix clock enable balance Sascha Hauer 2013-08-26 14:05 ` Viresh Kumar 2013-08-29 14:01 ` Shawn Guo 2013-08-30 19:00 ` Sascha Hauer 2013-09-02 15:02 ` Shawn Guo 2013-09-02 18:27 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).