[PATCH] clocksource: sh_tmu: Runtime PM support

[PATCH] clocksource: sh_tmu: Runtime PM support

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add Runtime PM support to the TMU driver.

The hardware device is enabled as long as the clocksource
or the clockevent portion of the driver is used.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Tested on the sh7372 Mackerel board with TMU00 and TMU01.

 drivers/clocksource/sh_tmu.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

--- 0010/drivers/clocksource/sh_tmu.c
+++ work/drivers/clocksource/sh_tmu.c	2011-04-25 15:13:20.000000000 +0900
@@ -25,6 +25,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/clk.h>
+#include <linux/pm_runtime.h>
 #include <linux/irq.h>
 #include <linux/err.h>
 #include <linux/clocksource.h>
@@ -109,10 +110,12 @@ static int sh_tmu_enable(struct sh_tmu_p
 {
 	int ret;
 
-	/* enable clock */
+	/* wake up device and enable clock */
+	pm_runtime_get_sync(&p->pdev->dev);
 	ret = clk_enable(p->clk);
 	if (ret) {
 		dev_err(&p->pdev->dev, "cannot enable clock\n");
+		pm_runtime_put_sync(&p->pdev->dev);
 		return ret;
 	}
 
@@ -141,8 +144,9 @@ static void sh_tmu_disable(struct sh_tmu
 	/* disable interrupts in TMU block */
 	sh_tmu_write(p, TCR, 0x0000);
 
-	/* stop clock */
+	/* stop clock and mark device as idle */
 	clk_disable(p->clk);
+	pm_runtime_put_sync(&p->pdev->dev);
 }
 
 static void sh_tmu_set_next(struct sh_tmu_priv *p, unsigned long delta,
@@ -411,6 +415,7 @@ static int __devinit sh_tmu_probe(struct
 
 	if (p) {
 		dev_info(&pdev->dev, "kept as earlytimer\n");
+		pm_runtime_enable(&pdev->dev);
 		return 0;
 	}
 
@@ -425,6 +430,9 @@ static int __devinit sh_tmu_probe(struct
 		kfree(p);
 		platform_set_drvdata(pdev, NULL);
 	}
+
+	if (!is_early_platform_device(pdev))
+		pm_runtime_enable(&pdev->dev);
 	return ret;
 }
 

Re: [PATCH] clocksource: sh_tmu: Runtime PM support

[john stultz]

On Mon, 2011-04-25 at 22:40 +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add Runtime PM support to the TMU driver.
> 
> The hardware device is enabled as long as the clocksource
> or the clockevent portion of the driver is used.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Acked-by: John Stultz <johnstul@us.ibm.com>

Re: [PATCH] clocksource: sh_tmu: Runtime PM support

[Paul Mundt]

On Mon, Apr 25, 2011 at 10:40:26PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add Runtime PM support to the TMU driver.
> 
> The hardware device is enabled as long as the clocksource
> or the clockevent portion of the driver is used.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  Tested on the sh7372 Mackerel board with TMU00 and TMU01.
> 
Blows up on SMP or with spinlock debugging:

 sh_tmu.0: used for periodic clock events
BUG: spinlock trylock failure on UP on CPU#0, swapper/0
 lock: 804d5198, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
Stack: (0x804cde00 to 0x804ce000)
de00: 8038e31c 804cde10 00000000 804d5198 80230142 804cde18 8047b698 80230396
de20: 804cde34 9f000ea0 804d5198 804d51a8 00000000 80391780 804cde40 802807cc
de40: 802807cc 804cde5c 00000000 9f000ea0 00000001 804d50f8 804d5198 00000004
de60: 802ec0c8 804cde74 00000001 00000002 9f000e60 802ec23a 804cde8c 9f000ea0
de80: 00000001 00000002 9f000e60 9f000e60 80499d18 802ec390 804cdeac 8038e4c0
dea0: ffffffff 00000002 9f000e60 8004990e 804cdebc 00000002 9f000ea0 80049f1e
dec0: 804cdecc 000000f0 9f000ea0 00000000 00000000 8004a0f2 804cdef4 00000000
dee0: 00000000 8038e4c0 ffffffff 000000f0 9f000ea0 8003ef84 804cdf14 80036aa0
df00: 00008000 8038e4c0 ffffffff 00000000 ffffffff 804e2530 9f000ea0 00000000
df20: 00000000 00000000 8003f02c 804cdf4c 000000c8 9f803c80 00000000 8004958c
df40: 804e2508 804e2500 00000000 800495b0 804cdf54 8004961e 804cdf5c 8038cebc
df60: 804cdf74 9f000ea0 804d50f0 804d50f8 9f000e60 804d7918 80661046 804cdf98
df80: 00000001 00000000 00000000 00000000 806758a4 804d50f0 00000000 8027c96a
dfa0: 80673850 00000000 00000002 00000002 80453acc 80650f9a 804cdfd4 80223500
dfc0: 806790b0 8066aa90 000000f0 8000376c 8038e4c0 8064f6fc 804cdfdc 8066af30
dfe0: 40002132 804cde10 000001bc 000002c0 ff000100 00000010 00000090 40000198
[    0.000000]
Call trace:
 [<8038e31c>] dump_stack+0x20/0x2c
 [<80230142>] spin_bug+0x8a/0xb8
 [<80230396>] do_raw_spin_trylock+0x3a/0x50
 [<80391780>] _raw_spin_lock_irqsave+0x5c/0xac
 [<802807cc>] __pm_runtime_resume+0x44/0x7c
 [<802807cc>] __pm_runtime_resume+0x44/0x7c
 [<802ec0c8>] sh_tmu_enable+0x2c/0xb8
 [<802ec23a>] sh_tmu_clock_event_start+0x2e/0xbc
 [<802ec390>] sh_tmu_clock_event_mode+0x70/0x9c
 [<8038e4c0>] printk+0x0/0x40
 [<8004990e>] clockevents_set_mode+0x2e/0x4e
 [<80049f1e>] tick_setup_periodic+0x42/0xf4
 [<8004a0f2>] tick_notify+0x122/0x238
 [<8038e4c0>] printk+0x0/0x40
 [<8003ef84>] notifier_call_chain+0x64/0xb8
 [<80036aa0>] func_ptr_is_kernel_text+0x0/0x40
 [<8038e4c0>] printk+0x0/0x40
 [<8003f02c>] raw_notifier_call_chain+0x24/0x30
 [<8004958c>] clockevents_do_notify+0x0/0x34
 [<800495b0>] clockevents_do_notify+0x24/0x34
 [<8004961e>] clockevents_register_device+0x5e/0xb0
 [<8038cebc>] sh_tmu_probe+0x164/0x2a0
 [<80661046>] early_platform_driver_probe+0x15e/0x1f4
 [<8027c96a>] platform_match+0x0/0x78
 [<80650f9a>] sh_late_time_init+0x16/0x2c
 [<80223500>] strlen+0x0/0x58
 [<8066aa90>] boot_command_line+0x0/0x200
 [<8000376c>] arch_local_irq_restore+0x0/0x2a
 [<8038e4c0>] printk+0x0/0x40
 [<8064f6fc>] start_kernel+0x310/0x4e4

> @@ -109,10 +110,12 @@ static int sh_tmu_enable(struct sh_tmu_p
>  {
>  	int ret;
>  
> -	/* enable clock */
> +	/* wake up device and enable clock */
> +	pm_runtime_get_sync(&p->pdev->dev);
>  	ret = clk_enable(p->clk);
>  	if (ret) {
>  		dev_err(&p->pdev->dev, "cannot enable clock\n");
> +		pm_runtime_put_sync(&p->pdev->dev);
>  		return ret;
>  	}
>  
At this point the spinlock hasn't been initialized yet, so any of the
pm_runtime calls are pretty much unsafe. We could manually test
pm_runtime_enabled() before any of the get/put_sync() calls, but that gets to
be a bit ugly.

Re: [PATCH] clocksource: sh_tmu: Runtime PM support

[Paul Mundt]

On Mon, May 23, 2011 at 05:30:06PM +0900, Paul Mundt wrote:
> On Mon, Apr 25, 2011 at 10:40:26PM +0900, Magnus Damm wrote:
> > From: Magnus Damm <damm@opensource.se>
> > 
> > Add Runtime PM support to the TMU driver.
> > 
[snip]
> > 
> > +	/* wake up device and enable clock */
> > +	pm_runtime_get_sync(&p->pdev->dev);
> >  	ret = clk_enable(p->clk);
> >  	if (ret) {
> >  		dev_err(&p->pdev->dev, "cannot enable clock\n");
> > +		pm_runtime_put_sync(&p->pdev->dev);
> >  		return ret;
> >  	}
> >  
> At this point the spinlock hasn't been initialized yet, so any of the
> pm_runtime calls are pretty much unsafe. We could manually test
> pm_runtime_enabled() before any of the get/put_sync() calls, but that gets to
> be a bit ugly.

Note that I will be reverting these patches for -rc2 if no progress is
made here. This is a fundamental ordering issue with regards to locking,
and is completely bogus for every SMP platform we have.

Re: [PATCH] clocksource: sh_tmu: Runtime PM support

[Paul Mundt]

On Tue, May 31, 2011 at 01:04:44PM +0900, Paul Mundt wrote:
> On Mon, May 23, 2011 at 05:30:06PM +0900, Paul Mundt wrote:
> > On Mon, Apr 25, 2011 at 10:40:26PM +0900, Magnus Damm wrote:
> > > From: Magnus Damm <damm@opensource.se>
> > > 
> > > Add Runtime PM support to the TMU driver.
> > > 
> [snip]
> > > 
> > > +	/* wake up device and enable clock */
> > > +	pm_runtime_get_sync(&p->pdev->dev);
> > >  	ret = clk_enable(p->clk);
> > >  	if (ret) {
> > >  		dev_err(&p->pdev->dev, "cannot enable clock\n");
> > > +		pm_runtime_put_sync(&p->pdev->dev);
> > >  		return ret;
> > >  	}
> > >  
> > At this point the spinlock hasn't been initialized yet, so any of the
> > pm_runtime calls are pretty much unsafe. We could manually test
> > pm_runtime_enabled() before any of the get/put_sync() calls, but that gets to
> > be a bit ugly.
> 
> Note that I will be reverting these patches for -rc2 if no progress is
> made here. This is a fundamental ordering issue with regards to locking,
> and is completely bogus for every SMP platform we have.

Furthermore, I'm not convinced there's any sane way to support this
without further help from the runtime PM framework.

We can't even do a pm_runtime_enabled() check in the case of the early
platform devices because that assumes pm_runtime_init() has been run, and
if that were true, the spinlock would have already been initialized.

As it is now there is simply no sane way to have dev->power.lock
initialized and any of these pm_runtime_xxx() calls made safe via the
early timer path.

If pm_runtime_get/put_sync() and friends were modified to be no-ops or
simply toggle some refcount that could later be inherited by the
framework at late initialization time, then we could support this sort of
multiple entry via early and late probe stages that the early platform
devices depend on, but it's completely bogus otherwise.

As it is now this completely breaks SMP, which is not acceptable. It's
obvious this needs a bit more of a think and there's no easy or clean way
to hack around this in the driver without further framework changes, so
I'll be reverting the runtime PM changes for the TMU and CMT drivers.

They can be trivially re-added once the ordering issues have been
resolved (perhaps a good topic for the runtime PM BoF on friday?)