[PATCH] sh_tmu: compute mult and shift before registration

[PATCH] sh_tmu: compute mult and shift before registration

[Aurelien Jarno]

Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
clocksource wrapping during idle"), the CPU of an R2D board never goes
to idle. This commit assumes that mult and shift are assigned before
the clocksource is registered. As a consequence the safe maximum sleep
time is negative and the CPU never goes into idle.

This patch fixes the problem by moving mult and shift initialization
from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 drivers/clocksource/sh_tmu.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index 8e44e14..e9e35aa 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -199,16 +199,8 @@ static cycle_t sh_tmu_clocksource_read(struct clocksource *cs)
 static int sh_tmu_clocksource_enable(struct clocksource *cs)
 {
 	struct sh_tmu_priv *p = cs_to_sh_tmu(cs);
-	int ret;
-
-	ret = sh_tmu_enable(p);
-	if (ret)
-		return ret;
 
-	/* TODO: calculate good shift from rate and counter bit width */
-	cs->shift = 10;
-	cs->mult = clocksource_hz2mult(p->rate, cs->shift);
-	return 0;
+	return sh_tmu_enable(p);
 }
 
 static void sh_tmu_clocksource_disable(struct clocksource *cs)
@@ -228,6 +220,10 @@ static int sh_tmu_register_clocksource(struct sh_tmu_priv *p,
 	cs->disable = sh_tmu_clocksource_disable;
 	cs->mask = CLOCKSOURCE_MASK(32);
 	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+	/* TODO: calculate good shift from rate and counter bit width */
+	cs->shift = 10;
+	/* channel will be configured at parent clock / 4 */
+	cs->mult = clocksource_hz2mult(clk_get_rate(p->clk) / 4, cs->shift);
 	dev_info(&p->pdev->dev, "used as clock source\n");
 	clocksource_register(cs);
 	return 0;
-- 
1.7.1

Re: [PATCH] sh_tmu: compute mult and shift before registration

[Magnus Damm]

Hey Aurelien,

Thanks for your patch!

On Mon, May 31, 2010 at 6:09 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
> clocksource wrapping during idle"), the CPU of an R2D board never goes
> to idle. This commit assumes that mult and shift are assigned before
> the clocksource is registered. As a consequence the safe maximum sleep
> time is negative and the CPU never goes into idle.
>
> This patch fixes the problem by moving mult and shift initialization
> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  drivers/clocksource/sh_tmu.c |   14 +++++---------
>  1 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
> index 8e44e14..e9e35aa 100644
> --- a/drivers/clocksource/sh_tmu.c
> +++ b/drivers/clocksource/sh_tmu.c
> @@ -199,16 +199,8 @@ static cycle_t sh_tmu_clocksource_read(struct clocksource *cs)
>  static int sh_tmu_clocksource_enable(struct clocksource *cs)
>  {
>        struct sh_tmu_priv *p = cs_to_sh_tmu(cs);
> -       int ret;
> -
> -       ret = sh_tmu_enable(p);
> -       if (ret)
> -               return ret;
>
> -       /* TODO: calculate good shift from rate and counter bit width */
> -       cs->shift = 10;
> -       cs->mult = clocksource_hz2mult(p->rate, cs->shift);
> -       return 0;
> +       return sh_tmu_enable(p);
>  }
>
>  static void sh_tmu_clocksource_disable(struct clocksource *cs)
> @@ -228,6 +220,10 @@ static int sh_tmu_register_clocksource(struct sh_tmu_priv *p,
>        cs->disable = sh_tmu_clocksource_disable;
>        cs->mask = CLOCKSOURCE_MASK(32);
>        cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +       /* TODO: calculate good shift from rate and counter bit width */
> +       cs->shift = 10;
> +       /* channel will be configured at parent clock / 4 */
> +       cs->mult = clocksource_hz2mult(clk_get_rate(p->clk) / 4, cs->shift);
>        dev_info(&p->pdev->dev, "used as clock source\n");
>        clocksource_register(cs);
>        return 0;

Hm, are you sure the clock is enabled at this point?

The file include/linux/clk.h says that the clock has to be enabled
before clk_get_rate() is called.

/ magnus

Re: [PATCH] sh_tmu: compute mult and shift before registration

[Aurelien Jarno]

Magnus Damm a écrit :
> Hey Aurelien,
> 
> Thanks for your patch!
> 
> On Mon, May 31, 2010 at 6:09 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
>> clocksource wrapping during idle"), the CPU of an R2D board never goes
>> to idle. This commit assumes that mult and shift are assigned before
>> the clocksource is registered. As a consequence the safe maximum sleep
>> time is negative and the CPU never goes into idle.
>>
>> This patch fixes the problem by moving mult and shift initialization
>> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>  drivers/clocksource/sh_tmu.c |   14 +++++---------
>>  1 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
>> index 8e44e14..e9e35aa 100644
>> --- a/drivers/clocksource/sh_tmu.c
>> +++ b/drivers/clocksource/sh_tmu.c
>> @@ -199,16 +199,8 @@ static cycle_t sh_tmu_clocksource_read(struct clocksource *cs)
>>  static int sh_tmu_clocksource_enable(struct clocksource *cs)
>>  {
>>        struct sh_tmu_priv *p = cs_to_sh_tmu(cs);
>> -       int ret;
>> -
>> -       ret = sh_tmu_enable(p);
>> -       if (ret)
>> -               return ret;
>>
>> -       /* TODO: calculate good shift from rate and counter bit width */
>> -       cs->shift = 10;
>> -       cs->mult = clocksource_hz2mult(p->rate, cs->shift);
>> -       return 0;
>> +       return sh_tmu_enable(p);
>>  }
>>
>>  static void sh_tmu_clocksource_disable(struct clocksource *cs)
>> @@ -228,6 +220,10 @@ static int sh_tmu_register_clocksource(struct sh_tmu_priv *p,
>>        cs->disable = sh_tmu_clocksource_disable;
>>        cs->mask = CLOCKSOURCE_MASK(32);
>>        cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>> +       /* TODO: calculate good shift from rate and counter bit width */
>> +       cs->shift = 10;
>> +       /* channel will be configured at parent clock / 4 */
>> +       cs->mult = clocksource_hz2mult(clk_get_rate(p->clk) / 4, cs->shift);
>>        dev_info(&p->pdev->dev, "used as clock source\n");
>>        clocksource_register(cs);
>>        return 0;
> 
> Hm, are you sure the clock is enabled at this point?
>
> The file include/linux/clk.h says that the clock has to be enabled
> before clk_get_rate() is called.
> 

In practice yes, but as you said it seems that theoretically it is not
guaranteed, so it may break again in the future.

It probably means that if the clock is not already started we have to
start it, call clk_get_rate() and stop it.

That starts to ressemble to my first patch, that started and stoped the
clock before registering it:

diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index 8e44e14..6f0fcfd 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -228,6 +228,8 @@ static int sh_tmu_register_clocksource(struct
sh_tmu_priv *p,
        cs->disable = sh_tmu_clocksource_disable;
        cs->mask = CLOCKSOURCE_MASK(32);
        cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+       sh_tmu_clocksource_enable(cs);
+       sh_tmu_clocksource_disable(cs);
        dev_info(&p->pdev->dev, "used as clock source\n");
        clocksource_register(cs);
        return 0;


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] sh_tmu: compute mult and shift before registration

[Paul Mundt]

On Mon, May 31, 2010 at 01:19:51PM +0200, Aurelien Jarno wrote:
> Magnus Damm a ?crit :
> > Hm, are you sure the clock is enabled at this point?
> >
> > The file include/linux/clk.h says that the clock has to be enabled
> > before clk_get_rate() is called.
> > 
> 
> In practice yes, but as you said it seems that theoretically it is not
> guaranteed, so it may break again in the future.
> 
It's a bit risky this early on, board code has ultimate control over what
sort of external oscillator is actually hooked up and promptly kicks a
rate propagation down the chain from the top down in order to reflect
those settings (some boards will have variable input clocks where we need
to test a pin to work out which is actually hooked up).

We've been lucky for the simple cases largely because few boards have
deviated from the defaults, but that's not behaviour we want to rely on.
The alternative is tying in a notifier chain, as we do with the serial
and cpufreq cases.

> It probably means that if the clock is not already started we have to
> start it, call clk_get_rate() and stop it.
> 
The clock yes, but not the timer channel. You can simply use a
clk_enable()/disable() pair around the get_rate and you'll be fine, this
is what most drivers do when the rate information is needed well before
there is any intent to keep the block clocked for any lengthy duration.
This still won't help for the case where the board code kicks down a rate
change, so that still needs a bit of a think.

Re: [PATCH] sh_tmu: compute mult and shift before registration

[Aurelien Jarno]

On Mon, May 31, 2010 at 08:36:54PM +0900, Paul Mundt wrote:
> On Mon, May 31, 2010 at 01:19:51PM +0200, Aurelien Jarno wrote:
> > Magnus Damm a ?crit :
> > > Hm, are you sure the clock is enabled at this point?
> > >
> > > The file include/linux/clk.h says that the clock has to be enabled
> > > before clk_get_rate() is called.
> > > 
> > 
> > In practice yes, but as you said it seems that theoretically it is not
> > guaranteed, so it may break again in the future.
> > 
> It's a bit risky this early on, board code has ultimate control over what
> sort of external oscillator is actually hooked up and promptly kicks a
> rate propagation down the chain from the top down in order to reflect
> those settings (some boards will have variable input clocks where we need
> to test a pin to work out which is actually hooked up).
> 
> We've been lucky for the simple cases largely because few boards have
> deviated from the defaults, but that's not behaviour we want to rely on.
> The alternative is tying in a notifier chain, as we do with the serial
> and cpufreq cases.
> 
> > It probably means that if the clock is not already started we have to
> > start it, call clk_get_rate() and stop it.
> > 
> The clock yes, but not the timer channel. You can simply use a
> clk_enable()/disable() pair around the get_rate and you'll be fine, this
> is what most drivers do when the rate information is needed well before
> there is any intent to keep the block clocked for any lengthy duration.
> This still won't help for the case where the board code kicks down a rate
> change, so that still needs a bit of a think.
> 

The current code gives a maximum idle period of 287 seconds, so it is
unlikely that this valued is reached, even with a clock that get
multiplied by 10 or 20. I am not really sure the nohz change has 
actually an effect on most clock sources, I guess it has been written 
for a very special case. I still wonder why it has been applied in 
stable updates.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net