ARM: OMAP2+: PM: code consolidation for 3.4

Hi

This series consolidates some duplicated PM code across OMAP2, 3, and 4. This saves both binary kernel image size, and lines of source. These functions are just the low-hanging fruit; it looks to me like there's even more consolidation that could be done.

Suspend + UART wakeup was tested on OMAP44xx BeagleBoard. On the OMAP35xx Beagleboard here, v3.3-rc2 doesn't seem to wake up from suspend correctly with or without these patches, so I've only boot-tested them on that platform.

- Paul

---

pm_cleanup_b_3.4
text data bss dec hex filename
6596402 680284 5593820 12870506 c4636a vmlinux.orig
6596230 680292 5593756 12870278 c46286 vmlinux.patched

Paul Walmsley (2):
ARM: OMAP2+: PM: share clkdms_setup() across OMAP2, 3, 4
ARM: OMAP2+: PM: share some suspend-related functions across OMAP2, 3, 4

 arch/arm/mach-omap2/pm.c     |   74 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/pm.h     |    2 +
 arch/arm/mach-omap2/pm24xx.c |   69 +++------------------------------------
 arch/arm/mach-omap2/pm34xx.c |   73 ++++-------------------------------------
 arch/arm/mach-omap2/pm44xx.c |   57 ++------------------------------
 5 files changed, 91 insertions(+), 184 deletions(-)

clkdms_setup() is identical across OMAP2, 3, and 4, so share it.

---
 arch/arm/mach-omap2/pm.c     |   10 ++++++++++
 arch/arm/mach-omap2/pm.h     |    1 +
 arch/arm/mach-omap2/pm24xx.c |   13 +------------
 arch/arm/mach-omap2/pm34xx.c |   17 +----------------
 arch/arm/mach-omap2/pm44xx.c |   18 +-----------------
 5 files changed, 14 insertions(+), 45 deletions(-)

index 1881fe9..0118614 100644
@@ -68,6 +68,16 @@ static void omap2_init_processor_devices(void)
 #define FORCEWAKEUP_SWITCH	0
 #define LOWPOWERSTATE_SWITCH	1
 
+int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
+{
+	if (clkdm->flags & CLKDM_CAN_ENABLE_AUTO)
+		clkdm_allow_idle(clkdm);
+	else if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP &&
+		 atomic_read(&clkdm->usecount) == 0)
+		clkdm_sleep(clkdm);
+	return 0;
+}
+
 /*
  * This sets pwrdm state (other than mpu & core. Currently only ON &
  * RET are supported.

index b737b11..eef67f6 100644
@@ -22,6 +22,7 @@ extern int omap3_can_sleep(void);
 extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
 extern int omap3_idle_init(void);
 extern int omap4_idle_init(void);
+extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
 
 #if defined(CONFIG_PM_OPP)
 extern int omap3_opp_init(void);

index b8822f8..bfd7905 100644
@@ -312,17 +312,6 @@ static const struct platform_suspend_ops omap_pm_ops = {
 static const struct platform_suspend_ops __initdata omap_pm_ops;
 #endif /* CONFIG_SUSPEND */
 
-/* XXX This function should be shareable between OMAP2xxx and OMAP3 */
-static int __init clkdms_setup(struct clockdomain *clkdm, void *unused)
-{
-	if (clkdm->flags & CLKDM_CAN_ENABLE_AUTO)
-		clkdm_allow_idle(clkdm);
-	else if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP &&
-		 atomic_read(&clkdm->usecount) == 0)
-		clkdm_sleep(clkdm);
-	return 0;
-}
-
 static void __init prcm_setup_regs(void)
 {
 	int i, num_mem_banks;

@@ -364,7 +353,7 @@ static void __init prcm_setup_regs(void)
 	clkdm_sleep(gfx_clkdm);
 
 	/* Enable hardware-supervised idle for all clkdms */
-	clkdm_for_each(clkdms_setup, NULL);
+	clkdm_for_each(omap_pm_clkdms_setup, NULL);
 	clkdm_add_wkdep(mpu_clkdm, wkup_clkdm);
 
 	/* REVISIT: Configure number of 32 kHz clock cycles for sys_clk

index fc69875..ae962b5 100644
@@ -743,21 +743,6 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
 }
 
 /*
- * Enable hw supervised mode for all clockdomains if it's
- * supported. Initiate sleep transition for other clockdomains, if
- * they are not used
- */
-static int __init clkdms_setup(struct clockdomain *clkdm, void *unused)
-{
-	if (clkdm->flags & CLKDM_CAN_ENABLE_AUTO)
-		clkdm_allow_idle(clkdm);
-	else if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP &&
-		 atomic_read(&clkdm->usecount) == 0)
-		clkdm_sleep(clkdm);
-	return 0;
-}
-
-/*
  * Push functions to SRAM
  *
  * The minimum set of functions is pushed to SRAM for execution:

@@ -826,7 +811,7 @@ static int __init omap3_pm_init(void)
 		goto err2;
 	}
 
-	(void) clkdm_for_each(clkdms_setup, NULL);
+	(void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
 	mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
 	if (mpu_pwrdm == NULL) {

index c264ef7..a6fb31b 100644
@@ -120,22 +120,6 @@ static const struct platform_suspend_ops omap_pm_ops = {
 };
 #endif /* CONFIG_SUSPEND */
 
-/*
- * Enable hardware supervised mode for all clockdomains if it's
- * supported. Initiate sleep transition for other clockdomains, if
- * they are not used
- */
-static int __init clkdms_setup(struct clockdomain *clkdm, void *unused)
-{
-	if (clkdm->flags & CLKDM_CAN_ENABLE_AUTO)
-		clkdm_allow_idle(clkdm);
-	else if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP &&
-			atomic_read(&clkdm->usecount) == 0)
-		clkdm_sleep(clkdm);
-	return 0;
-}
-
-
 static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
 {
 	struct power_state *pwrst;

@@ -249,7 +233,7 @@ static int __init omap4_pm_init(void)
 		goto err2;
 	}
 
-	(void) clkdm_for_each(clkdms_setup, NULL);
+	(void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
 #ifdef CONFIG_SUSPEND
 	suspend_set_ops(&omap_pm_ops);

The platform_suspend_ops can be shared across OMAP2, 3, and 4, along with all of the functions referenced in that structure. This patch shares them. It also removes the suspend_state file-scoped variable in the OMAP2 and 3 PM code; it does not appear to be actually needed by anything.

---
 arch/arm/mach-omap2/pm.c     |   64 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/pm.h     |    1 +
 arch/arm/mach-omap2/pm24xx.c |   56 +++----------------------------------
 arch/arm/mach-omap2/pm34xx.c |   56 ++++---------------------------------
 arch/arm/mach-omap2/pm44xx.c |   39 +-------------------------
 5 files changed, 77 insertions(+), 139 deletions(-)

index 0118614..35f5ac6 100644
@@ -15,11 +15,13 @@
 #include <linux/err.h>
 #include <linux/opp.h>
 #include <linux/export.h>
+#include <linux/suspend.h>
 
 #include <plat/omap-pm.h>
 #include <plat/omap_device.h>
 #include "common.h"
 
+#include "prcm-common.h"
 #include "voltage.h"
 #include "powerdomain.h"
 #include "clockdomain.h"

@@ -28,6 +30,12 @@
 
 static struct omap_device_pm_latency *pm_lats;
 
+/*
+ * omap_pm_suspend: points to a function that does the SoC-specific
+ * suspend work
+ */
+int (*omap_pm_suspend)(void);
+
 static int _init_omap_device(char *name)
 {
 	struct omap_hwmod *oh;

@@ -139,6 +147,8 @@ err:
 	return ret;
 }
 
+
+
 /*
  * This API is to be called during init to set the various voltage
  * domains to the voltage as per the opp table. Typically we boot up

@@ -206,6 +216,56 @@ exit:
 	return -EINVAL;
 }
 
+#ifdef CONFIG_SUSPEND
+static int omap_pm_enter(suspend_state_t suspend_state)
+{
+	int ret = 0;
+
+	if (!omap_pm_suspend)
+		return -ENOENT; /* XXX doublecheck */
+
+	switch (suspend_state) {
+	case PM_SUSPEND_STANDBY:
+	case PM_SUSPEND_MEM:
+		ret = omap_pm_suspend();
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int omap_pm_begin(suspend_state_t state)
+{
+	disable_hlt();
+	if (cpu_is_omap34xx())
+		omap_prcm_irq_prepare();
+	return 0;
+}
+
+static void omap_pm_end(void)
+{
+	enable_hlt();
+	return;
+}
+
+static void omap_pm_finish(void)
+{
+	if (cpu_is_omap34xx())
+		omap_prcm_irq_complete();
+}
+
+static const struct platform_suspend_ops omap_pm_ops = {
+	.begin		= omap_pm_begin,
+	.end		= omap_pm_end,
+	.enter		= omap_pm_enter,
+	.finish		= omap_pm_finish,
+	.valid		= suspend_valid_only_mem,
+};
+
+#endif /* CONFIG_SUSPEND */
+
 static void __init omap3_init_voltages(void)
 {
 	if (!cpu_is_omap34xx())

@@ -248,6 +308,10 @@ static int __init omap2_common_pm_late_init(void)
 	/* Smartreflex device init */
 	omap_devinit_smartreflex();
 
+#ifdef CONFIG_SUSPEND
+	suspend_set_ops(&omap_pm_ops);
+#endif
+
 	return 0;
 }
 late_initcall(omap2_common_pm_late_init);

index eef67f6..a051431 100644
@@ -23,6 +23,7 @@ extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
 extern int omap3_idle_init(void);
 extern int omap4_idle_init(void);
 extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
+extern int (*omap_pm_suspend)(void);
 
 #if defined(CONFIG_PM_OPP)
 extern int omap3_opp_init(void);

index a6fb31b..82aa352 100644
@@ -83,41 +83,6 @@ static int omap4_pm_suspend(void)
 
 	return 0;
 }
-
-static int omap4_pm_enter(suspend_state_t suspend_state)
-{
-	int ret = 0;
-
-	switch (suspend_state) {
-	case PM_SUSPEND_STANDBY:
-	case PM_SUSPEND_MEM:
-		ret = omap4_pm_suspend();
-		break;
-	default:
-		ret = -EINVAL;
-	}
-
-	return ret;
-}
-
-static int omap4_pm_begin(suspend_state_t state)
-{
-	disable_hlt();
-	return 0;
-}
-
-static void omap4_pm_end(void)
-{
-	enable_hlt();
-	return;
-}
-
-static const struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap4_pm_begin,
-	.end		= omap4_pm_end,
-	.enter		= omap4_pm_enter,
-	.valid		= suspend_valid_only_mem,
-};
 #endif /* CONFIG_SUSPEND */
 
 static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)

@@ -236,8 +201,8 @@ static int __init omap4_pm_init(void)
 	(void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
 #ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	omap_pm_suspend = omap4_pm_suspend;
+#endif
 
 	/* Overwrite the default arch_idle() */
 	pm_idle = omap_default_idle;

Thanks for patches. Tried them on OMAP4 SDP and suspend is fine on it.

On the consolidation front, I found that OMAP4/OMAP5
can almost share most of the PRCM code.
Actually it's too early to talk about OMAP5 patches, which are not even posted yet but just thought of mentioning that on this relevant thread.

I can send you a git link to have early preview on those changes, if you would like to have a look at it.

Regards
Santosh

I assume you meant Panda board?

I also tested idle & suspend + UART wakeup on 3430/n900, 3530/Overo.

Will add these to my cleanup queue for v3.4[1] after adding a Tested-by from Santosh.

Kevin

[1]
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.4/cleanup/pm

Yep

Great. Do you know what the deal is with suspend on 34xx? I enabled ttyO2 wakeup and entered suspend, but serial traffic doesn't bring it out?

- Paul

Just a quick followup; Kevin tracked this down. v3.3-rc needs the MTD suspend patch that Artem posted recently to suspend correctly on OMAP35xx BeagleBoard with omap2plus_defconfig. Thanks Kevin.

- Paul

Hi,

I can't get ttyO2 wakeup to work on my Blaze with 3.3-rc5. It works fine with 3.2. I've been trying to bisect and everything, but it's difficult. To start with, during my bisect, many patches seem to be in a state where the compilation fails (one example is 'OMAP44XX_IRQ_PRCM' undeclared).

Anyway, what I found out is that after fcf6efa3 (ARM: OMAP4: PM: Add WakeupGen module as OMAP gic_arch_extn) wakeup from the console doesn't work anymore for me. Then I tried to comment out the omap_wakeupgen_init() call in gic_init_irq() and that seems to work... for a while. At least until ff819da4 (ARM: OMAP3: CPUidle:
Make use of CPU PM notifiers) commenting out that line seems to work. But it doesn't work if I comment out the line with v3.3-rc5.

And that's when I reckoned I've been using too much time trying to figure this one out. :)

Does anyone know how to solve this?

Cheers,
Luca.

Hi Luciano,

Maybe try this patch:

http://www.spinics.net/lists/linux-omap/msg65786.html

If that doesn't work, could you try to reproduce with omap2plus_defconfig and a boring kernel command line?

- Paul

Hi Luca,

"Coelho, Luciano" <[email protected]> writes:

[...]

I assume you mean wakeup from system-wide suspend (echo mem > /sys/power/state)?

Are you enabling UART wakeups?

They are disabled by default, so you need to:

echo enabled > /sys/devices/platform/omap/omap_uart.2/tty/ttyO2/power/wakeup

It may work in v3.2 due to the fact that the UART runtime PM had the unintended side-effect of essentially disabling MPU and CORE PM. With that bug fixed and MPU & CORE actually hitting low power states, you'll need to ensure UART wakeups are enabled.

If you're already enabling wakeups and this still isn't working, try adding 'no_console_suspend' to the kernel cmdline, try a suspend/resume and post the console output.

Thanks,

Kevin

Hey Kevin,

Thanks for your reply! :)

Yes, that's what I meant.

No, I was actually not enabling the UART wakeups. But I tried it now but it still doesn't work. :(

I have no_console_suspend in bootargs. Without it, it didn't work with 3.2 either.

This is what I get when I suspend with "echo mem > /sys/power/state":

[  128.534271] PM: Syncing filesystems ... done.                                                                        
[  129.948364] PM: Preparing system for mem sleep                                                                       
[  129.957061] Freezing user space processes ...                                                                        
[  129.961761] BUG: sleeping function called from invalid context at include/linux/freezer.h:46                         
[  129.969177] in_atomic(): 0, irqs_disabled(): 128, pid: 770, name: udevd                                              
[  129.977813] INFO: lockdep is turned off.                                                                             
[  129.977813] irq event stamp: 0                                                                                       
[  129.985168] hardirqs last  enabled at (0): [<  (null)>]   (null)                                                     
[  129.991485] hardirqs last disabled at (0): [<c004233c>] copy_process+0x3ec/0x1050                                    
[  129.996520] softirqs last  enabled at (0): [<c004233c>] copy_process+0x3ec/0x1050                                    
[  130.005218] softirqs last disabled at (0): [<  (null)>]   (null)                                                     
[  130.011291] [<c001e204>] (unwind_backtrace+0x0/0x148) from [<c0502fdc>] (dump_stack+0x20/0x24)                       
[  130.018829] [<c0502fdc>] (dump_stack+0x20/0x24) from [<c00779d0>] (__might_sleep+0x130/0x134)                        
[  130.030670] [<c00779d0>] (__might_sleep+0x130/0x134) from [<c0018998>] (do_signal+0x54/0x600)                        
[  130.031738] [<c0018998>] (do_signal+0x54/0x600) from [<c0018fa4>] (do_notify_resume+0x60/0x6c)                       
[  130.049804] [<c0018fa4>] (do_notify_resume+0x60/0x6c) from [<c00154e4>] (work_pending+0x24/0x28)                     
[  130.067138] (elapsed 0.10 seconds) done.                                                                             
[  130.070648] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.                                      
[  130.101165] PM: Entering mem sleep                                                                                   
[  130.120056] PM: suspend of devices complete after 9.646 msecs                                                        
[  130.127716] PM: late suspend of devices complete after 7.662 msecs                                                   
[  130.134246] Disabling non-boot CPUs ...                                                                              
[  130.139953] CPU1: shutdown  

The "sleeping function" BUG is that known bug I already complained about some time ago. I get it all the time and, in general, it doesn't really hurt (I guess I'm lucky most of the times not to hit a race-condition).

Cheers,
Luca.

You definitely need that.

If you want to use no_console_suspend -- which is not recommended -- then you'll need

http://www.spinics.net/lists/linux-omap/msg65786.html

but best simply not to use no_console_suspend.

- Paul

Thanks, Paul! I was just chatting with Kevin about this on IRC. With your patch and no_console_suspend, it works. :)

I needed no_console_suspend because it wouldn't work otherwise on <3.3-rc1.

Now I tried a vanilla 3.3-rc5 without no_console_suspend and enabling the UART wakeup and it works fine!

Thanks for your help, guys!

Cheers,
Luca.

Luciano Coelho <[email protected]> writes:

[...]

Just FYI... I've queued this patch from Paul and it's now queued by Tony for v3.4 in his cleanup-pm branch.

Kevin

Hi Kevin,

Great, thanks for the info!

BTW, you and Paul said that it was not recommended to use no_console_suspend, but Ido (on CC) said that it can be useful for us, because we need to see if our own (wireless) stuff goes into suspend correctly.

Are there any real problems with no_console_suspend besides power consumption? Is it safe for us to simply apply Paul's patch when we need it?

Cheers,
Luca.

no_console_suspend should not be needed there. It is really only intended as a *temporary* debug feature when suspend fails.

Since the console is disabled during suspend, if suspend fails, you might not see the reason for failure. So in this case, you enable no_console_suspend just to see the suspend output, and why it's failing.

If suspend/resume succeed, you will see all the console output after resume because even though the console is disabled, the printks are buffered, so you will see them eventually.

So with a working suspend/resume, you will eventually see all the console output.

So basically, any usage of no_console_suspend should be temporary, until you figure out why suspend if failing.

There are important side effects of leaving the console enabled. Namely, the console UART will stay clocked, and thus prevent it's surrounding powerdomain from reaching a low power state. UARTs can be in either the PER or CORE powerdomains, so preventing them from reaching the low powerstate will affect other drivers in those powerdomains.

For example, if your device is in the same powerdomain as the UART, it will not be clock gated (or power gated) so the context save/restore paths of those devices will not be exercised.

Kevin