[PATCH 1/3] plat-nomadik: MTU timer

[PATCH 1/3] plat-nomadik: MTU timer

[Jonas Aaberg]

timer0 to 3 are all on mtu block 0, so don't calculate the clock event
rate based upon mtu block 1's clock speed.

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Alessandro Rubini <rubini@unipv.it>
Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
---
 arch/arm/plat-nomadik/timer.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/arch/arm/plat-nomadik/timer.c b/arch/arm/plat-nomadik/timer.c
index ea3ca86..d673888 100644
--- a/arch/arm/plat-nomadik/timer.c
+++ b/arch/arm/plat-nomadik/timer.c
@@ -131,17 +131,12 @@ void __init nmdk_timer_init(void)
 {
 	unsigned long rate;
 	struct clk *clk0;
-	struct clk *clk1;
 	u32 cr;
 
 	clk0 = clk_get_sys("mtu0", NULL);
 	BUG_ON(IS_ERR(clk0));
 
-	clk1 = clk_get_sys("mtu1", NULL);
-	BUG_ON(IS_ERR(clk1));
-
 	clk_enable(clk0);
-	clk_enable(clk1);
 
 	/*
 	 * Tick rate is 2.4MHz for Nomadik and 110MHz for ux500:
@@ -170,15 +165,8 @@ void __init nmdk_timer_init(void)
 		pr_err("timer: failed to initialize clock source %s\n",
 		       nmdk_clksrc.name);
 
-	/* Timer 1 is used for events, fix according to rate */
-	cr = MTU_CRn_32BITS;
-	rate = clk_get_rate(clk1);
-	if (rate > 16 << 20) {
-		rate /= 16;
-		cr |= MTU_CRn_PRESCALE_16;
-	} else {
-		cr |= MTU_CRn_PRESCALE_1;
-	}
+	/* Timer 1 is used for events */
+
 	clockevents_calc_mult_shift(&nmdk_clkevt, rate, MTU_MIN_RANGE);
 
 	writel(cr | MTU_CRn_ONESHOT, mtu_base + MTU_CR(1)); /* off, currently */
-- 
1.6.3.3

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Jonas Aaberg]

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Alessandro Rubini <rubini@unipv.it>
Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
---
 arch/arm/plat-nomadik/timer.c |   80 ++++++++++++++++++++++++++++++-----------
 1 files changed, 59 insertions(+), 21 deletions(-)

diff --git a/arch/arm/plat-nomadik/timer.c b/arch/arm/plat-nomadik/timer.c
index d673888..7d2d965 100644
--- a/arch/arm/plat-nomadik/timer.c
+++ b/arch/arm/plat-nomadik/timer.c
@@ -1,5 +1,5 @@
 /*
- *  linux/arch/arm/mach-nomadik/timer.c
+ *  linux/arch/arm/plat-nomadik/timer.c
  *
  * Copyright (C) 2008 STMicroelectronics
  * Copyright (C) 2010 Alessandro Rubini
@@ -75,7 +75,7 @@ static void nmdk_clkevt_mode(enum clock_event_mode mode,
 		cr = readl(mtu_base + MTU_CR(1));
 		writel(0, mtu_base + MTU_LR(1));
 		writel(cr | MTU_CRn_ENA, mtu_base + MTU_CR(1));
-		writel(0x2, mtu_base + MTU_IMSC);
+		writel(1 << 1, mtu_base + MTU_IMSC);
 		break;
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	case CLOCK_EVT_MODE_UNUSED:
@@ -127,36 +127,78 @@ static struct irqaction nmdk_timer_irq = {
 	.dev_id		= &nmdk_clkevt,
 };
 
+static void nmdk_timer_reset(void)
+{
+	u32 cr = MTU_CRn_32BITS | MTU_CRn_ENA;
+	struct clk *clk0;
+
+	clk0 = clk_get_sys("mtu0", NULL);
+
+	clk_enable(clk0);
+
+	if (clk_get_rate(clk0) > (16 << 20))
+		cr |= MTU_CRn_PRESCALE_16;
+	else
+		cr |= MTU_CRn_PRESCALE_1;
+
+	/* Timer 0, the free running clocksource */
+	writel(0, mtu_base + MTU_CR(0));
+	writel(0, mtu_base + MTU_LR(0));
+	writel(0, mtu_base + MTU_BGLR(0));
+	writel(cr, mtu_base + MTU_CR(0));
+
+	/* Timer 1, clock event */
+	writel(0, mtu_base + MTU_CR(1));
+	writel(0, mtu_base + MTU_LR(1));
+	writel(1 << 1, mtu_base + MTU_IMSC);
+	writel(cr | MTU_CRn_ONESHOT, mtu_base + MTU_CR(1));
+}
+
+#ifdef CONFIG_PM
+void nmdk_timer_suspend(void)
+{
+	struct clk *clk0;
+
+	/* No interrupts */
+	writel(0, mtu_base + MTU_IMSC);
+	/* Off */
+	writel(0, mtu_base + MTU_CR(0));
+	writel(0, mtu_base + MTU_CR(1));
+
+	clk0 = clk_get_sys("mtu0", NULL);
+
+	clk_disable(clk0);
+}
+
+void nmdk_timer_resume(void)
+{
+	nmdk_timer_reset();
+}
+#endif
+
 void __init nmdk_timer_init(void)
 {
 	unsigned long rate;
 	struct clk *clk0;
-	u32 cr;
 
+	/*
+	 * On MTU block 0: timer0 is used as source,
+	 * timer1 is used for events.
+	 */
 	clk0 = clk_get_sys("mtu0", NULL);
 	BUG_ON(IS_ERR(clk0));
 
-	clk_enable(clk0);
-
 	/*
-	 * Tick rate is 2.4MHz for Nomadik and 110MHz for ux500:
+	 * Tick rate is 2.4MHz for Nomadik and 100 or 133MHz for ux500:
 	 * use a divide-by-16 counter if it's more than 16MHz
 	 */
-	cr = MTU_CRn_32BITS;;
 	rate = clk_get_rate(clk0);
-	if (rate > 16 << 20) {
+	if (rate > (16 << 20))
 		rate /= 16;
-		cr |= MTU_CRn_PRESCALE_16;
-	} else {
-		cr |= MTU_CRn_PRESCALE_1;
-	}
+
 	clocksource_calc_mult_shift(&nmdk_clksrc, rate, MTU_MIN_RANGE);
 
-	/* Timer 0 is the free running clocksource */
-	writel(cr, mtu_base + MTU_CR(0));
-	writel(0, mtu_base + MTU_LR(0));
-	writel(0, mtu_base + MTU_BGLR(0));
-	writel(cr | MTU_CRn_ENA, mtu_base + MTU_CR(0));
+	nmdk_timer_reset();
 
 	/* Now the scheduling clock is ready */
 	nmdk_clksrc.read = nmdk_read_timer;
@@ -165,12 +207,8 @@ void __init nmdk_timer_init(void)
 		pr_err("timer: failed to initialize clock source %s\n",
 		       nmdk_clksrc.name);
 
-	/* Timer 1 is used for events */
-
 	clockevents_calc_mult_shift(&nmdk_clkevt, rate, MTU_MIN_RANGE);
 
-	writel(cr | MTU_CRn_ONESHOT, mtu_base + MTU_CR(1)); /* off, currently */
-
 	nmdk_clkevt.max_delta_ns =
 		clockevent_delta2ns(0xffffffff, &nmdk_clkevt);
 	nmdk_clkevt.min_delta_ns =
-- 
1.6.3.3

[PATCH 3/3] mach-ux500: Added nomadik mtu timer suspend/resume hooks.

[Jonas Aaberg]

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Acked-by: Alessandro Rubini <rubini@unipv.it>
Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
---
 arch/arm/mach-ux500/cpu.c                |    6 +++++-
 arch/arm/mach-ux500/include/mach/setup.h |    2 ++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-ux500/cpu.c b/arch/arm/mach-ux500/cpu.c
index e0fd747..86e95db 100644
--- a/arch/arm/mach-ux500/cpu.c
+++ b/arch/arm/mach-ux500/cpu.c
@@ -101,5 +101,9 @@ static void __init ux500_timer_init(void)
 }
 
 struct sys_timer ux500_timer = {
-	.init	= ux500_timer_init,
+	.init	 = ux500_timer_init,
+#ifdef CONFIG_PM
+	.suspend = nmdk_timer_suspend,
+	.resume  = nmdk_timer_resume,
+#endif
 };
diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
index 54bbe64..10e3511 100644
--- a/arch/arm/mach-ux500/include/mach/setup.h
+++ b/arch/arm/mach-ux500/include/mach/setup.h
@@ -25,6 +25,8 @@ extern void __init u8500_init_devices(void);
 extern void __init ux500_init_irq(void);
 /* We re-use nomadik_timer for this platform */
 extern void nmdk_timer_init(void);
+extern void nmdk_timer_suspend(void);
+extern void nmdk_timer_resume(void);
 
 extern void __init amba_add_devices(struct amba_device *devs[], int num);
 
-- 
1.6.3.3

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Russell King - ARM Linux]

"Added suspend/resume support for MTU timer."

Present tense, as it is describing what the patch is doing rather than
what has been done in the past?

On Thu, Sep 02, 2010 at 09:30:32AM +0200, Jonas Aaberg wrote:
> +static void nmdk_timer_reset(void)
> +{
> +	u32 cr = MTU_CRn_32BITS | MTU_CRn_ENA;
> +	struct clk *clk0;
> +
> +	clk0 = clk_get_sys("mtu0", NULL);
> +
> +	clk_enable(clk0);
> +
> +	if (clk_get_rate(clk0) > (16 << 20))

16 * 1048576 = 16.777216MHz

> +}
> +
> +#ifdef CONFIG_PM
> +void nmdk_timer_suspend(void)
> +{
> +	struct clk *clk0;
...
> +	clk0 = clk_get_sys("mtu0", NULL);
> +
> +	clk_disable(clk0);
> +}
>  void __init nmdk_timer_init(void)
>  {
>  	unsigned long rate;
>  	struct clk *clk0;
> -	u32 cr;
>  
> +	/*
> +	 * On MTU block 0: timer0 is used as source,
> +	 * timer1 is used for events.
> +	 */
>  	clk0 = clk_get_sys("mtu0", NULL);
>  	BUG_ON(IS_ERR(clk0));
>  
> -	clk_enable(clk0);
> -

Given all these calls to clk_get_sys() vs the 4 bytes consumed by having
a static clk0 pointer, is it really worth repeatedly using clk_get_sys() ?
Wouldn't the static pointer be more efficient?

>  	/*
> -	 * Tick rate is 2.4MHz for Nomadik and 110MHz for ux500:
> +	 * Tick rate is 2.4MHz for Nomadik and 100 or 133MHz for ux500:
>  	 * use a divide-by-16 counter if it's more than 16MHz
>  	 */
> -	cr = MTU_CRn_32BITS;;
>  	rate = clk_get_rate(clk0);
> -	if (rate > 16 << 20) {
> +	if (rate > (16 << 20))

n << 20 doesn't give you MHz, so this code doesn't match the comments.
Do you mean 16MHz or 16.777216MHz ?

[PATCH 3/3] mach-ux500: Added nomadik mtu timer suspend/resume hooks.

[Russell King - ARM Linux]

On Thu, Sep 02, 2010 at 09:30:33AM +0200, Jonas Aaberg wrote:
>  struct sys_timer ux500_timer = {
> -	.init	= ux500_timer_init,
> +	.init	 = ux500_timer_init,
> +#ifdef CONFIG_PM
> +	.suspend = nmdk_timer_suspend,
> +	.resume  = nmdk_timer_resume,
> +#endif

It would be better to use the callbacks from the clocksource/clockevent
code so that if your timers aren't being used, they can be shutdown for
power saving.

In any case, with clockevents support enabled, these suspend/resume
callbacks will not be called.

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Linus Walleij]

2010/9/2 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Thu, Sep 02, 2010 at 09:30:32AM +0200, Jonas Aaberg wrote:
>> (...)
>> ? ? ? /*
>> - ? ? ?* Tick rate is 2.4MHz for Nomadik and 110MHz for ux500:
>> + ? ? ?* Tick rate is 2.4MHz for Nomadik and 100 or 133MHz for ux500:
>> ? ? ? ?* use a divide-by-16 counter if it's more than 16MHz
>> ? ? ? ?*/
>> - ? ? cr = MTU_CRn_32BITS;;
>> ? ? ? rate = clk_get_rate(clk0);
>> - ? ? if (rate > 16 << 20) {
>> + ? ? if (rate > (16 << 20))
>
> n << 20 doesn't give you MHz, so this code doesn't match the comments.
> Do you mean 16MHz or 16.777216MHz ?

That comes from Alessandros oneshot mode a while back actually,
Jonas is only factoring it around.

Alessandro why do we kick in a prescaler if rate > (16 << 20)?
Seems weird to me too when I look@it, more like it'd be
if rate (1 << 20) which makes sense, if the counter will flip
around 20 bits in 1 second we prescale it by 16.

Yours,
Linus Walleij

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Linus Walleij]

2010/9/2 Jonas Aaberg <jonas.aberg@stericsson.com>:

> Acked-by: Linus Walleij <linus.walleij@stericsson.com>

... and then I saw this:

> (...)
> diff --git a/arch/arm/plat-nomadik/timer.c b/arch/arm/plat-nomadik/timer.c
> (...)
> +static void nmdk_timer_reset(void)
> +{
> + ? ? ? u32 cr = MTU_CRn_32BITS | MTU_CRn_ENA;

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Linus Walleij]

2010/9/2 Jonas Aaberg <jonas.aberg@stericsson.com>:

> Acked-by: Linus Walleij <linus.walleij@stericsson.com>

... and then I saw this:

> (...)
> diff --git a/arch/arm/plat-nomadik/timer.c b/arch/arm/plat-nomadik/timer.c
> (...)
> +static void nmdk_timer_reset(void)
> +{
> + ? ? ? u32 cr = MTU_CRn_32BITS | MTU_CRn_ENA;

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Alessandro Rubini]

Linus about the prescaler on the Nomadik MTU:
> That comes from Alessandros oneshot mode a while back actually,
> Jonas is only factoring it around.

Yes. 8815 has 2.5MHz and the U8500 had 110MHz according to this line:

>>> -      * Tick rate is 2.4MHz for Nomadik and 110MHz for ux500:

Now changed to:

>>> +      * Tick rate is 2.4MHz for Nomadik and 100 or 133MHz for ux5

So I chose to add a prescaler if the frequency is too high. A
non-prescaled 110MHz overflows in 39 seconds.  But the threshold is
subjective.  I think I choise ~16MHz to have no more than 1usec per
tick, for timeevent precision.

Russell:
>> n << 20 doesn't give you MHz, so this code doesn't match the comments.
>> Do you mean 16MHz or 16.777216MHz ?

Well, it's not really important which one. But the comment is wrong,
my fault.

> Alessandro why do we kick in a prescaler if rate > (16 << 20)?
> Seems weird to me too when I look at it, more like it'd be
> if rate (1 << 20) which makes sense, if the counter will flip
> around 20 bits in 1 second we prescale it by 16.

Your "makes sense" is as subjective as mine. But on 8815, with 2.4MHz
we overflow in around 1790 seconds. I feel it reasonable. If you
prescale we get at 150kHz, which is 6.6 usec per tick: I don't like
that figure too much.

/alessandro

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Linus Walleij]

2010/9/3 Alessandro Rubini <rubini-list@gnudd.com>:
> [Me]
>> Alessandro why do we kick in a prescaler if rate > (16 << 20)?
>> Seems weird to me too when I look at it, more like it'd be
>> if rate (1 << 20) which makes sense, if the counter will flip
>> around 20 bits in 1 second we prescale it by 16.
>
> Your "makes sense" is as subjective as mine. But on 8815, with 2.4MHz
> we overflow in around 1790 seconds. I feel it reasonable. If you
> prescale we get at 150kHz, which is 6.6 usec per tick: I don't like
> that figure too much.

Ah I got it backwards, actually I was thinking along the lines
that since (16 << 20) == (1 << 25) the latter would be more
apropriate thats what I think makes sense, it's so binary...
not (1 << 20) sorry for that.

So (1 << 25) means if bit 25 will switch in one second then
the remaining 7 bits will count less than 127 seconds
so roughly 2 minutes sleep (nice).

On a side note I believe we should then set the minimum range
for mul+div definied@the top of the file to something similar,
like 128.

I'll leave it to Jonas to come up with the best figure(s)!

Yours,
Linus Walleij

[PATCH 2/3] plat-nomadik: Added suspend/resume support for MTU timer.

[Armando Visconti]

Linus Walleij wrote:
> 2010/9/3 Alessandro Rubini <rubini-list@gnudd.com>:
>   
>> [Me]
>>     
>>> Alessandro why do we kick in a prescaler if rate > (16 << 20)?
>>> Seems weird to me too when I look at it, more like it'd be
>>> if rate (1 << 20) which makes sense, if the counter will flip
>>> around 20 bits in 1 second we prescale it by 16.
>>>       
>> Your "makes sense" is as subjective as mine. But on 8815, with 2.4MHz
>> we overflow in around 1790 seconds. I feel it reasonable. If you
>> prescale we get at 150kHz, which is 6.6 usec per tick: I don't like
>> that figure too much.
>>     
>
> Ah I got it backwards, actually I was thinking along the lines
> that since (16 << 20) == (1 << 25) the latter would be more
> apropriate thats what I think makes sense, it's so binary...
> not (1 << 20) sorry for that.
>   
Sorry,

I'm not following the discussion, but just for the sake
of precision, should be

(16 << 20) == (1 << 24)

Is it right?

Sorry for intruding,
Arm

[PATCH 3/3] mach-ux500: Added nomadik mtu timer suspend/resume hooks.

[Jonas Aaberg]

On 09/02/2010 06:07 PM, Russell King - ARM Linux wrote:
> On Thu, Sep 02, 2010 at 09:30:33AM +0200, Jonas Aaberg wrote:
>>  struct sys_timer ux500_timer = {
>> -	.init	= ux500_timer_init,
>> +	.init	 = ux500_timer_init,
>> +#ifdef CONFIG_PM
>> +	.suspend = nmdk_timer_suspend,
>> +	.resume  = nmdk_timer_resume,
>> +#endif
> 
> It would be better to use the callbacks from the clocksource/clockevent
> code so that if your timers aren't being used, they can be shutdown for
> power saving.

I agree. For clockevents, I suppose you get CLOCK_EVT_MODE_SHUTDOWN/UNUSED
when the clockevent isn't used, but for clocksource, I don't get how it is supposed
to work. When a clocksource is registered or changed, clocksource.c:clock_select() 
is called, but it does not look like the clocksources involved get informed about 
the change, if there is any change.

> 
> In any case, with clockevents support enabled, these suspend/resume
> callbacks will not be called.

Ok. Thanks for your comments!

BR,
 Jonas