* [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).