[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Paul Walmsley]

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(-)

[PATCH 1/2] ARM: OMAP2+: PM: share clkdms_setup() across OMAP2, 3, 4

[Paul Walmsley]

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

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 1881fe9..0118614 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -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.
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index b737b11..eef67f6 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -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);
diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
index b8822f8..bfd7905 100644
--- a/arch/arm/mach-omap2/pm24xx.c
+++ b/arch/arm/mach-omap2/pm24xx.c
@@ -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
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index fc69875..ae962b5 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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) {
diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
index c264ef7..a6fb31b 100644
--- a/arch/arm/mach-omap2/pm44xx.c
+++ b/arch/arm/mach-omap2/pm44xx.c
@@ -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);

[PATCH 2/2] ARM: OMAP2+: PM: share some suspend-related functions across OMAP2, 3, 4

[Paul Walmsley]

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.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 0118614..35f5ac6 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -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);
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index eef67f6..a051431 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -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);
diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
index bfd7905..538cd15 100644
--- a/arch/arm/mach-omap2/pm24xx.c
+++ b/arch/arm/mach-omap2/pm24xx.c
@@ -53,19 +53,6 @@
 #include "powerdomain.h"
 #include "clockdomain.h"
 
-#ifdef CONFIG_SUSPEND
-static suspend_state_t suspend_state = PM_SUSPEND_ON;
-static inline bool is_suspending(void)
-{
-	return (suspend_state != PM_SUSPEND_ON);
-}
-#else
-static inline bool is_suspending(void)
-{
-	return false;
-}
-#endif
-
 static void (*omap2_sram_idle)(void);
 static void (*omap2_sram_suspend)(u32 dllctrl, void __iomem *sdrc_dlla_ctrl,
 				  void __iomem *sdrc_power);
@@ -253,13 +240,6 @@ out:
 }
 
 #ifdef CONFIG_SUSPEND
-static int omap2_pm_begin(suspend_state_t state)
-{
-	disable_hlt();
-	suspend_state = state;
-	return 0;
-}
-
 static int omap2_pm_suspend(void)
 {
 	u32 wken_wkup, mir1;
@@ -279,37 +259,6 @@ static int omap2_pm_suspend(void)
 
 	return 0;
 }
-
-static int omap2_pm_enter(suspend_state_t state)
-{
-	int ret = 0;
-
-	switch (state) {
-	case PM_SUSPEND_STANDBY:
-	case PM_SUSPEND_MEM:
-		ret = omap2_pm_suspend();
-		break;
-	default:
-		ret = -EINVAL;
-	}
-
-	return ret;
-}
-
-static void omap2_pm_end(void)
-{
-	suspend_state = PM_SUSPEND_ON;
-	enable_hlt();
-}
-
-static const struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap2_pm_begin,
-	.enter		= omap2_pm_enter,
-	.end		= omap2_pm_end,
-	.valid		= suspend_valid_only_mem,
-};
-#else
-static const struct platform_suspend_ops __initdata omap_pm_ops;
 #endif /* CONFIG_SUSPEND */
 
 static void __init prcm_setup_regs(void)
@@ -356,6 +305,10 @@ static void __init prcm_setup_regs(void)
 	clkdm_for_each(omap_pm_clkdms_setup, NULL);
 	clkdm_add_wkdep(mpu_clkdm, wkup_clkdm);
 
+#ifdef CONFIG_SUSPEND
+	omap_pm_suspend = omap2_pm_suspend;
+#endif
+
 	/* REVISIT: Configure number of 32 kHz clock cycles for sys_clk
 	 * stabilisation */
 	omap2_prm_write_mod_reg(15 << OMAP_SETUP_TIME_SHIFT, OMAP24XX_GR_MOD,
@@ -456,7 +409,6 @@ static int __init omap2_pm_init(void)
 						    omap24xx_cpu_suspend_sz);
 	}
 
-	suspend_set_ops(&omap_pm_ops);
 	pm_idle = omap2_pm_idle;
 
 	return 0;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index ae962b5..0e0a2af 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -50,10 +50,6 @@
 #include "sdrc.h"
 #include "control.h"
 
-#ifdef CONFIG_SUSPEND
-static suspend_state_t suspend_state = PM_SUSPEND_ON;
-#endif
-
 /* pm34xx errata defined in pm.h */
 u16 pm34xx_errata;
 
@@ -479,50 +475,6 @@ restore:
 	return ret;
 }
 
-static int omap3_pm_enter(suspend_state_t unused)
-{
-	int ret = 0;
-
-	switch (suspend_state) {
-	case PM_SUSPEND_STANDBY:
-	case PM_SUSPEND_MEM:
-		ret = omap3_pm_suspend();
-		break;
-	default:
-		ret = -EINVAL;
-	}
-
-	return ret;
-}
-
-/* Hooks to enable / disable UART interrupts during suspend */
-static int omap3_pm_begin(suspend_state_t state)
-{
-	disable_hlt();
-	suspend_state = state;
-	omap_prcm_irq_prepare();
-	return 0;
-}
-
-static void omap3_pm_end(void)
-{
-	suspend_state = PM_SUSPEND_ON;
-	enable_hlt();
-	return;
-}
-
-static void omap3_pm_finish(void)
-{
-	omap_prcm_irq_complete();
-}
-
-static const struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap3_pm_begin,
-	.end		= omap3_pm_end,
-	.enter		= omap3_pm_enter,
-	.finish		= omap3_pm_finish,
-	.valid		= suspend_valid_only_mem,
-};
 #endif /* CONFIG_SUSPEND */
 
 
@@ -813,6 +765,10 @@ static int __init omap3_pm_init(void)
 
 	(void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
+#ifdef CONFIG_SUSPEND
+	omap_pm_suspend = omap3_pm_suspend;
+#endif
+
 	mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
 	if (mpu_pwrdm == NULL) {
 		printk(KERN_ERR "Failed to get mpu_pwrdm\n");
@@ -830,8 +786,8 @@ static int __init omap3_pm_init(void)
 	core_clkdm = clkdm_lookup("core_clkdm");
 
 #ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	omap_pm_suspend = omap3_pm_suspend;
+#endif
 
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();
diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
index a6fb31b..82aa352 100644
--- a/arch/arm/mach-omap2/pm44xx.c
+++ b/arch/arm/mach-omap2/pm44xx.c
@@ -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;

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Shilimkar, Santosh]

On Thu, Feb 2, 2012 at 3:08 PM, Paul Walmsley <paul@pwsan.com> wrote:
> 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.
>
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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> 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.  

I assume you meant Panda board?

> 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.

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Paul Walmsley]

On Thu, 2 Feb 2012, Kevin Hilman wrote:

> Paul Walmsley <paul@pwsan.com> writes:
> 
> > Suspend + UART wakeup was tested on OMAP44xx BeagleBoard.  
> 
> I assume you meant Panda board?

Yep

> > 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.
> 
> 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.

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Paul Walmsley]

On Thu, 2 Feb 2012, Paul Walmsley wrote:

> 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?

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Coelho, Luciano]

Hi,

On Thu, Feb 2, 2012 at 10:29 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Thu, 2 Feb 2012, Paul Walmsley wrote:
>
>> 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?
>
> 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.

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.

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Paul Walmsley]

Hi Luciano,

On Thu, 8 Mar 2012, Coelho, Luciano wrote:

> 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.

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Kevin Hilman]

Hi Luca,

"Coelho, Luciano" <coelho@ti.com> writes:

[...]

> I can't get ttyO2 wakeup to work on my Blaze with 3.3-rc5.  It works
> fine with 3.2.  

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Luciano Coelho]

On Thu, 2012-03-08 at 10:27 -0800, Kevin Hilman wrote: 
> Hi Luca,

Hey Kevin,

Thanks for your reply! :)


> "Coelho, Luciano" <coelho@ti.com> writes:
> 
> [...]
> 
> > I can't get ttyO2 wakeup to work on my Blaze with 3.3-rc5.  It works
> > fine with 3.2.  
> 
> I assume you mean wakeup from system-wide suspend (echo mem > /sys/power/state)?

Yes, that's what I meant.

> 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

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


> 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.

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.

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Paul Walmsley]

On Thu, 8 Mar 2012, Luciano Coelho wrote:

> On Thu, 2012-03-08 at 10:27 -0800, Kevin Hilman wrote: 
> 
> > 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
> 
> No, I was actually not enabling the UART wakeups.  But I tried it now
> but it still doesn't work. :(

You definitely need that.

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

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Luciano Coelho]

On Thu, 2012-03-08 at 11:55 -0700, Paul Walmsley wrote: 
> On Thu, 8 Mar 2012, Luciano Coelho wrote:
> 
> > On Thu, 2012-03-08 at 10:27 -0800, Kevin Hilman wrote: 
> > 
> > > 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
> > 
> > No, I was actually not enabling the UART wakeups.  But I tried it now
> > but it still doesn't work. :(
> 
> You definitely need that.
> 
> > I have no_console_suspend in bootargs.  Without it, it didn't work with
> > 3.2 either.
> 
> 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.

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.

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Kevin Hilman]

Luciano Coelho <coelho@ti.com> writes:

[...]

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

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

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Luciano Coelho]

Hi Kevin,

On Thu, 2012-03-08 at 13:21 -0800, Kevin Hilman wrote: 
> Luciano Coelho <coelho@ti.com> writes:
> 
> [...]
> 
> > Thanks, Paul! I was just chatting with Kevin about this on IRC.  With
> > your patch and no_console_suspend, it works. :)
> 
> Just FYI... I've queued this patch from Paul and it's now queued by Tony
> for v3.4 in his cleanup-pm branch.

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.

[PATCH 0/2] ARM: OMAP2+: PM: code consolidation for 3.4

[Kevin Hilman]

Luciano Coelho <coelho@ti.com> writes:

> Hi Kevin,
>
> On Thu, 2012-03-08 at 13:21 -0800, Kevin Hilman wrote: 
>> Luciano Coelho <coelho@ti.com> writes:
>> 
>> [...]
>> 
>> > Thanks, Paul! I was just chatting with Kevin about this on IRC.  With
>> > your patch and no_console_suspend, it works. :)
>> 
>> Just FYI... I've queued this patch from Paul and it's now queued by Tony
>> for v3.4 in his cleanup-pm branch.
>
> 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.

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. 

> 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?

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