OMAP: idle path errata fixes

Erratum id: i608
RTA (Retention Till Access) feature is not supported and leads to device stability issues when enabled. This impacts modules with embedded memories on OMAP3630

Workaround is to disable RTA on boot and coming out of core off. For disabling rta coming out of off mode, we do this by overriding the restore pointer for 3630 to allow us restore handler as the first point of entry before caches are touched and is common for GP and HS devices. to disable earlier than this could be possible by modifying the ppa for HS devices, but not for GP devices.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

[[email protected]: co-developer]

Signed-off-by Ambresh K <[email protected]>

Signed-off-by Nishanth Menon <[email protected]>

Quoted 18 lines · Nishanth Menon

Erratum id: i608
RTA (Retention Till Access) feature is not supported and leads to device stability issues when enabled. This impacts modules with embedded memories on OMAP3630

Workaround is to disable RTA on boot and coming out of core off. For disabling rta coming out of off mode, we do this by overriding the restore pointer for 3630 to allow us restore handler as the first point of entry before caches are touched and is common for GP and HS devices. to disable earlier than this could be possible by modifying the ppa for HS devices, but not for GP devices.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

[[email protected]: co-developer]

Signed-off-by Ambresh K <[email protected]>

Signed-off-by Nishanth Menon <[email protected]>

[...]

Quoted 4 lines

(quoted diff) +0 -0

@@ -1045,6 +1057,15 @@ static int __init omap3_pm_init(void)
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();
 

Review static int __init omap3_pm_init(void) · line 1068

Quoted 9 lines

(quoted diff) +9 -0

+	/*
+	 * RTA is disabled during initialization as per erratum i608
+	 * it is safer to disable rta by the bootloader, but we would like
+	 * to be doubly sure here and prevent any mishaps.
+	 */
+	if (IS_PM34XX_ERRATUM(RTA_ERRATUM_i608))
+		omap_ctrl_writel(OMAP36XX_RTA_DISABLE,
+			OMAP36XX_CONTROL_MEM_RTA_CTRL);
+

Minor nit: we've been trying to clean up control module access. So, rather than directly writing control module registers, could you create an API for this like was done for omap3_ctrl_write_boot_mode().

From 5cf295fa3fe8d423323500fb8ddb49650f797edd Mon Sep 17 00:00:00 2001

Kevin Hilman had written, on 12/13/2010 09:28 PM, the following:

Quoted 23 lines

Quoted 18 lines · Nishanth Menon

Erratum id: i608
RTA (Retention Till Access) feature is not supported and leads to device stability issues when enabled. This impacts modules with embedded memories on OMAP3630

Workaround is to disable RTA on boot and coming out of core off. For disabling rta coming out of off mode, we do this by overriding the restore pointer for 3630 to allow us restore handler as the first point of entry before caches are touched and is common for GP and HS devices. to disable earlier than this could be possible by modifying the ppa for HS devices, but not for GP devices.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

[[email protected]: co-developer]

Signed-off-by Ambresh K <[email protected]>

Signed-off-by Nishanth Menon <[email protected]>

[...]

Quoted 16 lines

arch/arm/mach-omap2/pm34xx.c static int __init omap3_pm_init(void) · line 1067

Quoted 12 lines

arch/arm/mach-omap2/pm34xx.c +8 -0

@@ -1045,6 +1057,15 @@ static int __init omap3_pm_init(void)
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();
 +	/*
+	 * RTA is disabled during initialization as per erratum i608
+	 * it is safer to disable rta by the bootloader, but we would like
+	 * to be doubly sure here and prevent any mishaps.
+	 */
+	if (IS_PM34XX_ERRATUM(RTA_ERRATUM_i608))
+		omap_ctrl_writel(OMAP36XX_RTA_DISABLE,
+			OMAP36XX_CONTROL_MEM_RTA_CTRL);
+

Minor nit: we've been trying to clean up control module access. So, rather than directly writing control module registers, could you create an API for this like was done for omap3_ctrl_write_boot_mode().

looks like the cleanups are somewhere in -next and not in k.org tree. basing the change similar to
http://marc.info/?l=linux-omap&m=129168623011464&w=2 does the attached(based on 2.6.37-rc5) looks like how you'd like to see it?

If i need to rebase to any particular tree which already has this change instead of k.org, do let me know.

Signature · 27 lines

Regards,
Nishanth Menon
From 5cf295fa3fe8d423323500fb8ddb49650f797edd Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Mon, 6 Sep 2010 10:26:08 +0530
Subject: [PATCH] OMAP3630: PM: Erratum i608: disable RTA

Erratum id: i608
RTA (Retention Till Access) feature is not supported and leads to device
stability issues when enabled. This impacts modules with embedded memories
on OMAP3630

Workaround is to disable RTA on boot and coming out of core off.
For disabling rta coming out of off mode, we do this by overriding the
restore pointer for 3630 to allow us restore handler as the first point of
entry before caches are touched and is common for GP and HS devices.
to disable earlier than this could be possible by modifying the ppa for HS
devices, but not for GP devices.

Signed-off-by: Ambresh K <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
 arch/arm/mach-omap2/control.c   |   13 ++++++++++++-
 arch/arm/mach-omap2/control.h   |    7 ++++++-
 arch/arm/mach-omap2/pm34xx.c    |   20 ++++++++++++++++++++
 arch/arm/mach-omap2/sleep34xx.S |   26 ++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/control.c +4 -1

index 1fa3294..7c415bc 100644
@@ -241,7 +241,10 @@ void omap3_save_scratchpad_contents(void)
 
 	/* Populate the Scratchpad contents */
 	scratchpad_contents.boot_config_ptr = 0x0;
-	if (omap_rev() != OMAP3430_REV_ES3_0 &&
+	if (cpu_is_omap3630())
+		scratchpad_contents.public_restore_ptr =
+			virt_to_phys(get_omap3630_restore_pointer());
+	else if (omap_rev() != OMAP3430_REV_ES3_0 &&
 					omap_rev() != OMAP3430_REV_ES3_1)
 		scratchpad_contents.public_restore_ptr =
 			virt_to_phys(get_restore_pointer());

arch/arm/mach-omap2/control.c +6 -0

+{
+	if (!cpu_is_omap36xx())
+		return;
+	omap_ctrl_writel(OMAP36XX_RTA_DISABLE, OMAP36XX_CONTROL_MEM_RTA_CTRL);
+}
+
 #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */

arch/arm/mach-omap2/control.h +6 -1

index b6c6b7c..13771cc 100644
@@ -204,6 +204,10 @@
 #define OMAP343X_CONTROL_WKUP_DEBOBS3 (OMAP343X_CONTROL_GENERAL_WKUP + 0x014)
 #define OMAP343X_CONTROL_WKUP_DEBOBS4 (OMAP343X_CONTROL_GENERAL_WKUP + 0x018)
 
+/* 36xx-only RTA - Retention till Accesss control registers and bits */
+#define OMAP36XX_CONTROL_MEM_RTA_CTRL	0x40C
+#define OMAP36XX_RTA_DISABLE		0x0
+
 /* 34xx D2D idle-related pins, handled by PM core */
 #define OMAP3_PADCONF_SAD2D_MSTANDBY   0x250
 #define OMAP3_PADCONF_SAD2D_IDLEACK    0x254

@@ -347,10 +351,11 @@ extern void omap3_save_scratchpad_contents(void);
 extern void omap3_clear_scratchpad_contents(void);
 extern u32 *get_restore_pointer(void);
 extern u32 *get_es3_restore_pointer(void);
+extern u32 *get_omap3630_restore_pointer(void);
 extern u32 omap3_arm_context[128];
 extern void omap3_control_save_context(void);
 extern void omap3_control_restore_context(void);
-
+extern void omap3630_disable_rta(void);
 #else
 #define omap_ctrl_base_get()		0
 #define omap_ctrl_readb(x)		0

arch/arm/mach-omap2/pm34xx.c +20 -0

index 0ec8a04..859c452 100644
@@ -55,6 +55,10 @@
 #define OMAP343X_TABLE_VALUE_OFFSET	   0xc0
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
+#define RTA_ERRATUM_i608		(1 << 0)
+static u16 pm34xx_errata;
+#define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
+
 struct power_state {
 	struct powerdomain *pwrdm;
 	u32 next_state;

@@ -989,6 +993,12 @@ void omap_push_sram_idle(void)
 				save_secure_ram_context_sz);
 }
 
+static void __init pm_errata_configure(void)
+{
+	if (cpu_is_omap3630())
+		pm34xx_errata |= RTA_ERRATUM_i608;
+}
+
 static int __init omap3_pm_init(void)
 {
 	struct power_state *pwrst, *tmp;

@@ -998,6 +1008,8 @@ static int __init omap3_pm_init(void)
 	if (!cpu_is_omap34xx())
 		return -ENODEV;
 
+	pm_errata_configure();
+
 	printk(KERN_ERR "Power Management for TI OMAP3.\n");
 
 	/* XXX prcm_setup_regs needs to be before enabling hw

@@ -1045,6 +1057,14 @@ static int __init omap3_pm_init(void)
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();
 
+	/*
+	 * RTA is disabled during initialization as per erratum i608
+	 * it is safer to disable rta by the bootloader, but we would like
+	 * to be doubly sure here and prevent any mishaps.
+	 */
+	if (IS_PM34XX_ERRATUM(RTA_ERRATUM_i608))
+		omap3630_disable_rta();
+
 	clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
 	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
 		omap3_secure_ram_storage =

arch/arm/mach-omap2/sleep34xx.S +26 -0

index 3fbd1e5..cc3507b 100644
@@ -45,6 +45,8 @@
 #define CM_IDLEST_CKGEN_V	OMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST)
 #define SRAM_BASE_P		0x40200000
 #define CONTROL_STAT		0x480022F0
+#define CONTROL_MEM_RTA_CTRL	(OMAP343X_CTRL_BASE\
+					+ OMAP36XX_CONTROL_MEM_RTA_CTRL)
 #define SCRATCHPAD_MEM_OFFS	0x310 /* Move this as correct place is
 				       * available */
 #define SCRATCHPAD_BASE_P	(OMAP343X_CTRL_BASE + OMAP343X_CONTROL_MEM_WKUP\

@@ -99,6 +101,14 @@ ENTRY(get_restore_pointer)
         ldmfd   sp!, {pc}     @ restore regs and return
 ENTRY(get_restore_pointer_sz)
         .word   . - get_restore_pointer
+	.text
+/* Function call to get the restore pointer for 3630 resume from OFF */
+ENTRY(get_omap3630_restore_pointer)
+        stmfd   sp!, {lr}     @ save registers on stack
+	adr	r0, restore_3630
+        ldmfd   sp!, {pc}     @ restore regs and return
+ENTRY(get_omap3630_restore_pointer_sz)
+        .word   . - get_omap3630_restore_pointer
 
 	.text
 /* Function call to get the restore pointer for for ES3 to resume from OFF */

@@ -246,6 +256,20 @@ copy_to_sram:
 	bne	copy_to_sram
 	ldr	r1, sram_base
 	blx	r1
+	b	restore
+
+restore_3630:
+	/*b restore_es3630*/		@ Enable to debug restore code
+	ldr	r1, pm_prepwstst_core_p
+	ldr	r2, [r1]
+	and	r2, r2, #0x3
+	cmp	r2, #0x0	@ Check if previous power state of CORE is OFF
+	bne	restore
+	/* Disable rta before giving control */
+	ldr	r1, control_mem_rta
+	mov	r2, #OMAP36XX_RTA_DISABLE
+	str	r2, [r1]
+	/* Fall thru for the remaining logic */
 restore:
 	/* b restore*/  @ Enable to debug restore code
         /* Check what was the reason for mpu reset and store the reason in r9*/

@@ -650,6 +674,8 @@ cache_pred_disable_mask:
 	.word	0xFFFFE7FB
 control_stat:
 	.word	CONTROL_STAT
+control_mem_rta:
+	.word	CONTROL_MEM_RTA_CTRL
 kernel_flush:
 	.word v7_flush_dcache_all
 	/* these 2 words need to be at the end !!! */

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Nishanth Menon
Sent: Friday, December 03, 2010 10:34 PM
To: linux-omap
Cc: Eduardo Valentin; Kevin Hilman; Tony Lindgren; Nishanth Menon
Subject: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

From: Eduardo Valentin <[email protected]>

Limitation i583: Self_Refresh Exit issue after OFF mode

Issue:
When device is waking up from OFF mode, then SDRC state machine sends
inappropriate sequence violating JEDEC standards.

Impact:
OMAP3630 < ES1.2 is impacted as follows depending on the platform:
CS0: for 38.4MHz as internal sysclk, DDR content seen to be stable, while
for all other sysclk frequencies, varied levels of instability seen based on varied parameters.
CS1: impacted

This patch takes option #3 as recommended by the Silicon erratum:
Avoid core power domain transitioning to OFF mode. Power consumption impact is expected in this case.
To do this, we route OFF requests to RET request on the impacted revisions
of silicon.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

[[email protected]: rebased the code to 2.6.37-rc2- short circuit code changed a bit]

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Eduardo Valentin <[email protected]>

---
v3: no functional change in erratum wa implementation, just registration of
erratum is collated to a single cpu detection and version check
v2: https://patchwork.kernel.org/patch/365262/
rebased to this patch series instead of depending on hs changes fix typo for macro definition
v1: http://marc.info/?l=linux-omap&m=129013173425266&w=2

 arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index b49e02b..ba3c0d6 100644

arch/arm/mach-omap2/pm34xx.c +2 -0

@@ -56,6 +56,7 @@
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
 #define RTA_ERRATUM_i608		(1 << 0)
+#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
 

@@ -406,6 +407,17 @@ void omap_sram_idle(void)
 	}
 
 	/* CORE */
+	/*

arch/arm/mach-omap2/pm34xx.c +1 -0

+
 	if (core_next_state < PWRDM_POWER_ON) {
 		omap_uart_prepare_idle(0);

arch/arm/mach-omap2/pm34xx.c +2 -1

 		omap_uart_prepare_idle(1);

@@ -999,6 +1011,8 @@ static void __init pm_errata_configure(void)
 		pm34xx_errata |= RTA_ERRATUM_i608;
 		/* Enable the l2 cache toggling in sleep logic */
 		enable_omap3630_toggle_l2_on_restore();
+		if (omap_rev() < OMAP3630_REV_ES1_2)
+			pm34xx_errata |= SDRC_WAKEUP_ERRATUM_i583;
 	}
 }
 
--

1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nishant,

Quoted 69 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Nishanth Menon
Sent: Friday, December 03, 2010 10:34 PM
To: linux-omap
Cc: Eduardo Valentin; Kevin Hilman; Tony Lindgren; Nishanth Menon
Subject: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

From: Eduardo Valentin <[email protected]>

Limitation i583: Self_Refresh Exit issue after OFF mode

Issue:
When device is waking up from OFF mode, then SDRC state machine sends
inappropriate sequence violating JEDEC standards.

Impact:
OMAP3630 < ES1.2 is impacted as follows depending on the platform:
CS0: for 38.4MHz as internal sysclk, DDR content seen to be stable, while
for all other sysclk frequencies, varied levels of instability seen based on varied parameters.
CS1: impacted

This patch takes option #3 as recommended by the Silicon erratum:
Avoid core power domain transitioning to OFF mode. Power consumption impact is expected in this case.
To do this, we route OFF requests to RET request on the impacted revisions
of silicon.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

[[email protected]: rebased the code to 2.6.37-rc2- short circuit code changed a bit]

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Eduardo Valentin <[email protected]>

---
v3: no functional change in erratum wa implementation, just registration of
erratum is collated to a single cpu detection and version check
v2: https://patchwork.kernel.org/patch/365262/
rebased to this patch series instead of depending on hs changes fix typo for macro definition
v1: http://marc.info/?l=linux-omap&m=129013173425266&w=2

 arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index b49e02b..ba3c0d6 100644

arch/arm/mach-omap2/pm34xx.c +2 -0

@@ -56,6 +56,7 @@
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
 #define RTA_ERRATUM_i608		(1 << 0)
+#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
 

@@ -406,6 +407,17 @@ void omap_sram_idle(void)
 	}
 
 	/* CORE */
+	/*

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 420

Quoted 10 lines

arch/arm/mach-omap2/pm34xx.c +9 -0

+	 * Erratum i583: implementation for ES rev < Es1.2 on 3630
+	 * We cannot enable OFF mode in a stable form for previous
+	 * revisions, transition instead to RET
+	 */
+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583) &&
+			(core_next_state == PWRDM_POWER_OFF)) {
+		pwrdm_set_next_pwrst(core_pwrdm,
 PWRDM_POWER_RET);
+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is throwing up an error "Powerdomain (core_pwrdm) didn't enter target state 0" when you off mode is enabled for ES1.1 or lesser (in suspend path). It's not really true that Core has not entered target state. It has entered Retention state which is the actual target state set in omap_sram_idle. However it did not enter the state that was passed by omap3_pm_suspend. Is this expected behavior?

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 7:13 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Vishwanath Sripathy had written, on 12/13/2010 07:35 AM, the following:

Quoted 9 lines

Nishant,

Quoted 7 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Nishanth Menon
Sent: Friday, December 03, 2010 10:34 PM
To: linux-omap
Cc: Eduardo Valentin; Kevin Hilman; Tony Lindgren; Nishanth Menon
Subject: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable

coreoff if

Quoted 6 lines

Quoted 6 lines

< ES1.2

From: Eduardo Valentin <[email protected]>

Limitation i583: Self_Refresh Exit issue after OFF mode

Quoted 7 lines

Quoted 7 lines

Issue:
When device is waking up from OFF mode, then SDRC state machine sends
inappropriate sequence violating JEDEC standards.

Impact:
OMAP3630 < ES1.2 is impacted as follows depending on the

platform:

Quoted 8 lines

Quoted 8 lines

CS0: for 38.4MHz as internal sysclk, DDR content seen to be stable, while
for all other sysclk frequencies, varied levels of instability seen based on varied parameters.
CS1: impacted

This patch takes option #3 as recommended by the Silicon erratum:
Avoid core power domain transitioning to OFF mode. Power

consumption

Quoted 8 lines

Quoted 8 lines

impact is expected in this case.
To do this, we route OFF requests to RET request on the impacted revisions
of silicon.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

Quoted 1 line

Quoted 1 line

[[email protected]: rebased the code to 2.6.37-rc2- short circuit code

changed

Quoted 5 lines

Quoted 5 lines

a bit]

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Eduardo Valentin <[email protected]>

---
v3: no functional change in erratum wa implementation, just

registration

Quoted 39 lines

Quoted 27 lines

of
erratum is collated to a single cpu detection and version check
v2: https://patchwork.kernel.org/patch/365262/
rebased to this patch series instead of depending on hs changes fix typo for macro definition
v1: http://marc.info/?l=linux-omap&m=129013173425266&w=2

 arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index b49e02b..ba3c0d6 100644

arch/arm/mach-omap2/pm34xx.c +2 -0

@@ -56,6 +56,7 @@
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
 #define RTA_ERRATUM_i608		(1 << 0)
+#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
 

@@ -406,6 +407,17 @@ void omap_sram_idle(void)
 	}
 
 	/* CORE */
+	/*

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 420

Quoted 10 lines

arch/arm/mach-omap2/pm34xx.c +9 -0

+	 * Erratum i583: implementation for ES rev < Es1.2 on 3630
+	 * We cannot enable OFF mode in a stable form for previous
+	 * revisions, transition instead to RET
+	 */
+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583) &&
+			(core_next_state == PWRDM_POWER_OFF)) {
+		pwrdm_set_next_pwrst(core_pwrdm,
 PWRDM_POWER_RET);
+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target state 0"

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path). It's

Quoted 2 lines

really true that Core has not entered target state. It has entered Retention state which is the actual target state set in

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed) indicate that the transition failed on <ES1.2, or we could make it entirely transparent(by modifying the the pwrst_list - claim we achieved off, while not really hitting off - I personally dont think that is correct.

--
Regards,
Nishanth Menon

Nishant,

Quoted 51 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 7:13 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Vishwanath Sripathy had written, on 12/13/2010 07:35 AM, the following:

Quoted 9 lines

Nishant,

Quoted 7 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Nishanth Menon
Sent: Friday, December 03, 2010 10:34 PM
To: linux-omap
Cc: Eduardo Valentin; Kevin Hilman; Tony Lindgren; Nishanth Menon
Subject: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable

coreoff if

Quoted 6 lines

Quoted 6 lines

< ES1.2

From: Eduardo Valentin <[email protected]>

Limitation i583: Self_Refresh Exit issue after OFF mode

Quoted 7 lines

Quoted 7 lines

Issue:
When device is waking up from OFF mode, then SDRC state machine sends
inappropriate sequence violating JEDEC standards.

Impact:
OMAP3630 < ES1.2 is impacted as follows depending on the

platform:

Quoted 8 lines

Quoted 8 lines

CS0: for 38.4MHz as internal sysclk, DDR content seen to be stable, while
for all other sysclk frequencies, varied levels of instability seen based on varied parameters.
CS1: impacted

This patch takes option #3 as recommended by the Silicon erratum:
Avoid core power domain transitioning to OFF mode. Power

consumption

Quoted 8 lines

Quoted 8 lines

impact is expected in this case.
To do this, we route OFF requests to RET request on the impacted revisions
of silicon.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

Quoted 46 lines

Quoted 1 line

Quoted 1 line

[[email protected]: rebased the code to 2.6.37-rc2- short circuit code

changed

Quoted 5 lines

Quoted 5 lines

a bit]

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Eduardo Valentin <[email protected]>

---
v3: no functional change in erratum wa implementation, just

registration

Quoted 38 lines

Quoted 27 lines

of
erratum is collated to a single cpu detection and version check
v2: https://patchwork.kernel.org/patch/365262/
rebased to this patch series instead of depending on hs changes fix typo for macro definition
v1: http://marc.info/?l=linux-omap&m=129013173425266&w=2

 arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index b49e02b..ba3c0d6 100644

arch/arm/mach-omap2/pm34xx.c +2 -0

@@ -56,6 +56,7 @@
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
 #define RTA_ERRATUM_i608		(1 << 0)
+#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
 

@@ -406,6 +407,17 @@ void omap_sram_idle(void)
 	}
 
 	/* CORE */
+	/*

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 420

Quoted 10 lines

arch/arm/mach-omap2/pm34xx.c +9 -0

+	 * Erratum i583: implementation for ES rev < Es1.2 on 3630
+	 * We cannot enable OFF mode in a stable form for previous
+	 * revisions, transition instead to RET
+	 */
+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583) &&
+			(core_next_state == PWRDM_POWER_OFF)) {
+		pwrdm_set_next_pwrst(core_pwrdm,
 PWRDM_POWER_RET);
+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is

throwing

Quoted 3 lines

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target state 0"

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path). It's

not

Quoted 2 lines

Quoted 2 lines

really true that Core has not entered target state. It has entered Retention state which is the actual target state set in

omap_sram_idle.

Quoted 3 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

Quoted 4 lines

we could go both ways on this - this patch will(as you noticed) indicate that the transition failed on <ES1.2, or we could make it entirely transparent(by modifying the the pwrst_list - claim we achieved off, while not really hitting off - I personally dont think that is correct.

The point I am making is that you cannot distinguish between genuine off /retention failure since this message is thrown for both pass and fail. May be an additional trace message indicating that system entered retention instead of off (for ES<1.2) will be useful.

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 7:35 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Vishwanath Sripathy had written, on 12/13/2010 07:54 AM, the following:

Quoted 20 lines

Nishant,

Quoted 18 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 7:13 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Vishwanath Sripathy had written, on 12/13/2010 07:35 AM, the following:

Quoted 8 lines

Nishant,

Quoted 6 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Nishanth Menon
Sent: Friday, December 03, 2010 10:34 PM
To: linux-omap
Cc: Eduardo Valentin; Kevin Hilman; Tony Lindgren; Nishanth

Menon

Quoted 10 lines

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Subject: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable

coreoff if

Quoted 8 lines

Quoted 8 lines

< ES1.2

From: Eduardo Valentin <[email protected]>

Limitation i583: Self_Refresh Exit issue after OFF mode

Issue:
When device is waking up from OFF mode, then SDRC state

machine

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

sends
inappropriate sequence violating JEDEC standards.

Impact:
OMAP3630 < ES1.2 is impacted as follows depending on the

platform:

Quoted 1 line

Quoted 1 line

CS0: for 38.4MHz as internal sysclk, DDR content seen to be

stable,

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

while
for all other sysclk frequencies, varied levels of instability seen based on varied parameters.
CS1: impacted

This patch takes option #3 as recommended by the Silicon

erratum:

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

Quoted 1 line

Avoid core power domain transitioning to OFF mode. Power

consumption

Quoted 9 lines

Quoted 9 lines

impact is expected in this case.
To do this, we route OFF requests to RET request on the impacted revisions
of silicon.

Cc Kevin Hilman <[email protected]>

Cc Tony Lindgren <[email protected]>

[[email protected]: rebased the code to 2.6.37-rc2- short circuit code

changed

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

a bit]

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Eduardo Valentin <[email protected]>

---
v3: no functional change in erratum wa implementation, just

registration

Quoted 2 lines

Quoted 2 lines

of
erratum is collated to a single cpu detection and version

check

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

v2: https://patchwork.kernel.org/patch/365262/
rebased to this patch series instead of depending on hs

changes

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

fix typo for macro definition
v1: http://marc.info/?l=linux-omap&m=129013173425266&w=2

 arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index b49e02b..ba3c0d6 100644

arch/arm/mach-omap2/pm34xx.c +1 -0

@@ -56,6 +56,7 @@
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
 #define RTA_ERRATUM_i608		(1 << 0)
+#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 411

Quoted 6 lines

arch/arm/mach-omap2/pm34xx.c +2 -0

@@ -406,6 +407,17 @@ void omap_sram_idle(void)
 	}
 
 	/* CORE */
+	/*
+	 * Erratum i583: implementation for ES rev < Es1.2 on

3630

Quoted 1 line

Quoted 1 line

Quoted 1 line

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 412

Quoted 1 line

arch/arm/mach-omap2/pm34xx.c +1 -0

+	 * We cannot enable OFF mode in a stable form for

previous

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void)

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c +3 -0

+	 * revisions, transition instead to RET
+	 */
+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 416

Quoted 1 line

arch/arm/mach-omap2/pm34xx.c +1 -0

+			(core_next_state == PWRDM_POWER_OFF))

{

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 419

Quoted 4 lines

arch/arm/mach-omap2/pm34xx.c +3 -0

+		pwrdm_set_next_pwrst(core_pwrdm,
 PWRDM_POWER_RET);
+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target state

0"

Quoted 3 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path). It's

not

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

really true that Core has not entered target state. It has entered Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed)

indicate

Quoted 3 lines

Quoted 3 lines

that the transition failed on <ES1.2, or we could make it entirely transparent(by modifying the the pwrst_list - claim we achieved off, while not really hitting off - I personally dont think that is

Quoted 1 line

The point I am making is that you cannot distinguish between genuine

off

Quoted 1 line

/retention failure since this message is thrown for both pass and

Quoted 2 lines

May be an additional trace message indicating that system entered retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was achieved as
well in the error message (trivial fix).
b) how do we notify users that it was due to SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in omap3_pm_suspend(when user attempts actual OFF) OR omap3_pm_off_mode_enable(when user attempts to
enable OFF mode)?

Any suggestions to allow the same uImage boot on all silicon + allow cpu_idle + suspend paths not to spew pr_info messages(aka cant add prints in sram_idle)?

--
Regards,
Nishanth Menon

Quoted 15 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+			(core_next_state == PWRDM_POWER_OFF))

{

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 419

Quoted 4 lines

(quoted diff) +1 -0

+		pwrdm_set_next_pwrst(core_pwrdm,

PWRDM_POWER_RET);

arch/arm/mach-omap2/pm34xx.c +2 -0

+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target state

0"

Quoted 3 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path). It's

not

Quoted 11 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

really true that Core has not entered target state. It has entered Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed)

indicate

Quoted 3 lines

Quoted 3 lines

that the transition failed on <ES1.2, or we could make it entirely transparent(by modifying the the pwrst_list - claim we achieved off, while not really hitting off - I personally dont think that is

correct.

Quoted 3 lines

Quoted 1 line

The point I am making is that you cannot distinguish between genuine

off

Quoted 1 line

/retention failure since this message is thrown for both pass and

fail.

Quoted 14 lines

Quoted 2 lines

May be an additional trace message indicating that system entered retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was achieved as
well in the error message (trivial fix).
b) how do we notify users that it was due to SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in omap3_pm_suspend(when user attempts actual OFF) OR omap3_pm_off_mode_enable(when user attempts to
enable OFF mode)?

Quoted 3 lines

Any suggestions to allow the same uImage boot on all silicon + allow cpu_idle + suspend paths not to spew pr_info messages(aka cant add prints in sram_idle)?

I vote for denying off mode for Core (for ES<1.2) in omap3_pm_off_mode_enable and throw up a message saying that Core off is not enabled. Then we will not get this failure message in suspend path since pwrst_list will have the right state.

-----Original Message-----
Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the following:
[...]

Quoted 14 lines

Quoted 14 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+			(core_next_state == PWRDM_POWER_OFF))

{

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+		pwrdm_set_next_pwrst(core_pwrdm,

PWRDM_POWER_RET);

(quoted diff) +2 -0

+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target state

0"

Quoted 2 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path).

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

not

Quoted 2 lines

Quoted 2 lines

really true that Core has not entered target state. It has entered Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed)

indicate

Quoted 2 lines

Quoted 2 lines

that the transition failed on <ES1.2, or we could make it entirely transparent(by modifying the the pwrst_list - claim we achieved

Quoted 20 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

while not really hitting off - I personally dont think that is

correct.

Quoted 3 lines

Quoted 1 line

The point I am making is that you cannot distinguish between genuine

off

Quoted 1 line

/retention failure since this message is thrown for both pass and

fail.

Quoted 14 lines

Quoted 2 lines

May be an additional trace message indicating that system entered retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was achieved as
well in the error message (trivial fix).
b) how do we notify users that it was due to SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in omap3_pm_suspend(when user attempts actual OFF) OR omap3_pm_off_mode_enable(when user attempts to
enable OFF mode)?

Quoted 5 lines

Quoted 3 lines

Any suggestions to allow the same uImage boot on all silicon + allow cpu_idle + suspend paths not to spew pr_info messages(aka cant add prints in sram_idle)?

I vote for denying off mode for Core (for ES<1.2) in omap3_pm_off_mode_enable and throw up a message saying that Core off

Quoted 2 lines

not enabled. Then we will not get this failure message in suspend path since pwrst_list will have the right state.

Keep in mind - if we disable it in omap3_pm_off_mode_enable - we will deny OFF wholesale if I understand the logic right- not just core-off - I kind of think that is extreme.

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 8:14 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Quoted 2 lines

-----Original Message-----
Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the

following:

Quoted 2 lines

[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+	if

(IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+			(core_next_state ==

PWRDM_POWER_OFF))

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

{

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+		pwrdm_set_next_pwrst(core_pwrdm,

PWRDM_POWER_RET);

(quoted diff) +2 -0

+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target

state

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

0"

Quoted 2 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path).

It's

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

not

Quoted 1 line

Quoted 1 line

really true that Core has not entered target state. It has

entered

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed)

indicate

Quoted 1 line

Quoted 1 line

that the transition failed on <ES1.2, or we could make it

entirely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

transparent(by modifying the the pwrst_list - claim we achieved

off,

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

while not really hitting off - I personally dont think that is

correct.

Quoted 1 line

Quoted 1 line

The point I am making is that you cannot distinguish between

genuine

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

off

Quoted 1 line

/retention failure since this message is thrown for both pass and

fail.

Quoted 1 line

Quoted 1 line

May be an additional trace message indicating that system

entered

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was

achieved

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

as
well in the error message (trivial fix).
b) how do we notify users that it was due to SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in omap3_pm_suspend(when

user

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

attempts actual OFF) OR omap3_pm_off_mode_enable(when user attempts to
enable OFF mode)?

Any suggestions to allow the same uImage boot on all silicon +

allow

Quoted 1 line

Quoted 1 line

Quoted 1 line

cpu_idle + suspend paths not to spew pr_info messages(aka cant

add

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

prints in sram_idle)?

I vote for denying off mode for Core (for ES<1.2) in omap3_pm_off_mode_enable and throw up a message saying that

Core off
is

Quoted 1 line

Quoted 1 line

not enabled. Then we will not get this failure message in suspend

path

Quoted 2 lines

Quoted 1 line

since pwrst_list will have the right state.

Keep in mind - if we disable it in omap3_pm_off_mode_enable - we

will

Quoted 1 line

deny OFF wholesale if I understand the logic right- not just core-off

Quoted 1 line

I kind of think that is extreme.

I take that back, we could do something like the following instead:

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..74842f1 100644

arch/arm/mach-omap2/pm34xx.c +1 -1

@@ -933,7 +933,14 @@ void omap3_pm_off_mode_enable(int enable)
 
        list_for_each_entry(pwrst, &pwrst_list, node) {
                pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

arch/arm/mach-omap2/pm34xx.c +2 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       omap_set_pwrdm_state(pwrst->pwrdm,

PWRDM_POWER_RET);

+ pr_err("%s: cannot enable Core OFF due to

arch/arm/mach-omap2/pm34xx.c +4 -0

+                               __func__);
+               } else {
+                       omap_set_pwrdm_state(pwrst->pwrdm, state);
+               }
        }
 }

Will wait to see if there are additional opinions on this approach -if none,
Will post a v4 for this patch alone.
Regards,
Nishanth Menon

Quoted 10 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 8:14 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Quoted 2 lines

-----Original Message-----
Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the

Quoted 13 lines

following:

Quoted 2 lines

[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+	if

(IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+			(core_next_state ==

PWRDM_POWER_OFF))

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

{

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+		pwrdm_set_next_pwrst(core_pwrdm,

PWRDM_POWER_RET);

(quoted diff) +2 -0

+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this

is

Quoted 52 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target

state

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

0"

Quoted 2 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path).

It's

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

not

Quoted 1 line

Quoted 1 line

really true that Core has not entered target state. It has

entered

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed)

indicate

Quoted 1 line

Quoted 1 line

that the transition failed on <ES1.2, or we could make it

entirely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

transparent(by modifying the the pwrst_list - claim we achieved

off,

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

while not really hitting off - I personally dont think that is

correct.

Quoted 1 line

Quoted 1 line

The point I am making is that you cannot distinguish between

genuine

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

off

Quoted 1 line

/retention failure since this message is thrown for both pass and

fail.

Quoted 1 line

Quoted 1 line

May be an additional trace message indicating that system

entered

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was

achieved

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

as
well in the error message (trivial fix).
b) how do we notify users that it was due to SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in omap3_pm_suspend(when

user

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

attempts actual OFF) OR omap3_pm_off_mode_enable(when user attempts to
enable OFF mode)?

Any suggestions to allow the same uImage boot on all silicon +

allow

Quoted 1 line

Quoted 1 line

Quoted 1 line

cpu_idle + suspend paths not to spew pr_info messages(aka cant

add

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

prints in sram_idle)?

I vote for denying off mode for Core (for ES<1.2) in omap3_pm_off_mode_enable and throw up a message saying that

Quoted 8 lines

Core off
is

Quoted 1 line

Quoted 1 line

not enabled. Then we will not get this failure message in suspend

path

Quoted 2 lines

Quoted 1 line

since pwrst_list will have the right state.

Keep in mind - if we disable it in omap3_pm_off_mode_enable - we

will

Quoted 1 line

deny OFF wholesale if I understand the logic right- not just core-off

-

Quoted 16 lines

Quoted 1 line

I kind of think that is extreme.

I take that back, we could do something like the following instead:

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..74842f1 100644

arch/arm/mach-omap2/pm34xx.c +1 -1

@@ -933,7 +933,14 @@ void omap3_pm_off_mode_enable(int enable)
 
        list_for_each_entry(pwrst, &pwrst_list, node) {
                pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

arch/arm/mach-omap2/pm34xx.c +2 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       omap_set_pwrdm_state(pwrst->pwrdm,

Quoted 1 line

PWRDM_POWER_RET);

			You need to update pwrst->next_state as well.

Otherwise suspend will still throw the same error. I just sent the code in other email.

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 8:23 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Vishwanath Sripathy had written, on 12/13/2010 08:48 AM, the following:

Quoted 17 lines

Quoted 17 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 8:14 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Quoted 2 lines

-----Original Message-----
Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the

following:

Quoted 2 lines

[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+	if

(IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+			(core_next_state ==

PWRDM_POWER_OFF))

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

{

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+		pwrdm_set_next_pwrst(core_pwrdm,

PWRDM_POWER_RET);

(quoted diff) +2 -0

+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this

is

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target

state

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

0"

Quoted 2 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend

path).

Quoted 10 lines

Quoted 10 lines

It's

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

not

Quoted 1 line

Quoted 1 line

really true that Core has not entered target state. It has

entered

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you

noticed)

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

indicate

Quoted 1 line

Quoted 1 line

that the transition failed on <ES1.2, or we could make it

entirely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

transparent(by modifying the the pwrst_list - claim we

achieved

Quoted 7 lines

Quoted 7 lines

off,

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

while not really hitting off - I personally dont think that is

correct.

Quoted 1 line

Quoted 1 line

The point I am making is that you cannot distinguish between

genuine

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

off

Quoted 1 line

/retention failure since this message is thrown for both pass

and

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

fail.

Quoted 1 line

Quoted 1 line

May be an additional trace message indicating that system

entered

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was

Quoted 6 lines

Quoted 6 lines

achieved

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

as
well in the error message (trivial fix).
b) how do we notify users that it was due to
SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in

omap3_pm_suspend(when

Quoted 2 lines

Quoted 2 lines

user

Quoted 1 line

Quoted 1 line

Quoted 1 line

attempts actual OFF) OR omap3_pm_off_mode_enable(when

user

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

attempts to
enable OFF mode)?

Any suggestions to allow the same uImage boot on all silicon +

Quoted 2 lines

Quoted 2 lines

allow

Quoted 1 line

Quoted 1 line

Quoted 1 line

cpu_idle + suspend paths not to spew pr_info messages(aka

cant

Quoted 4 lines

Quoted 4 lines

add

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

prints in sram_idle)?

I vote for denying off mode for Core (for ES<1.2) in omap3_pm_off_mode_enable and throw up a message saying

that

Quoted 8 lines

Quoted 8 lines

Core off
is

Quoted 1 line

Quoted 1 line

not enabled. Then we will not get this failure message in suspend

path

Quoted 2 lines

Quoted 1 line

since pwrst_list will have the right state.

Keep in mind - if we disable it in omap3_pm_off_mode_enable - we

will

Quoted 1 line

deny OFF wholesale if I understand the logic right- not just

Quoted 9 lines

-

arch/arm/mach-omap2/pm34xx.c void omap3_pm_off_mode_enable(int

Quoted 8 lines

Quoted 1 line

I kind of think that is extreme.

I take that back, we could do something like the following instead:

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..74842f1 100644

arch/arm/mach-omap2/pm34xx.c +0 -0

@@ -933,7 +933,14 @@ void omap3_pm_off_mode_enable(int

enable)

Quoted 1 line

Quoted 1 line

Quoted 4 lines

arch/arm/mach-omap2/pm34xx.c

Quoted 4 lines

        list_for_each_entry(pwrst, &pwrst_list, node) {
                pwrst->next_state = state;

arch/arm/mach-omap2/pm34xx.c +1 -1

-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if

(IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

Quoted 6 lines

Quoted 4 lines

&&

arch/arm/mach-omap2/pm34xx.c +2 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       omap_set_pwrdm_state(pwrst->pwrdm,

PWRDM_POWER_RET);

			You need to update pwrst->next_state as well.

Otherwise suspend will still throw the same error. I just sent the

in

Quoted 1 line

other email.

yep. except, I think we dont need to do string compare. the following looks any better?

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..da12a56 100644

arch/arm/mach-omap2/pm34xx.c +1 -2

@@ -932,8 +932,15 @@ void omap3_pm_off_mode_enable(int enable)
  #endif
 
         list_for_each_entry(pwrst, &pwrst_list, node) {
-               pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

+ __func__);

arch/arm/mach-omap2/pm34xx.c +4 -0

+               } else {
+                       pwrst->next_state = state;
+               }
+               omap_set_pwrdm_state(pwrst->pwrdm, pwrst-

Quoted 1 line

next_state);

         }

}

--
Regards,
Nishanth Menon

Quoted 13 lines

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..da12a56 100644

arch/arm/mach-omap2/pm34xx.c +1 -2

@@ -932,8 +932,15 @@ void omap3_pm_off_mode_enable(int enable)
  #endif
 
         list_for_each_entry(pwrst, &pwrst_list, node) {
-               pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

arch/arm/mach-omap2/pm34xx.c void omap3_pm_off_mode_enable(int enable)

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c +3 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       pwrst->next_state = PWRDM_POWER_RET;
+                       pr_err("%s: cannot enable Core OFF due to

i583\n",

Quoted 1 line

+ __func__);

You probably need to throw up this warning only if state
== PWRDM_POWER_OFF. Otherwise this code looks fine to me.

Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
[...]

Quoted 17 lines

Quoted 13 lines

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..da12a56 100644

arch/arm/mach-omap2/pm34xx.c +1 -2

@@ -932,8 +932,15 @@ void omap3_pm_off_mode_enable(int enable)
  #endif
 
         list_for_each_entry(pwrst, &pwrst_list, node) {
-               pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

arch/arm/mach-omap2/pm34xx.c void omap3_pm_off_mode_enable(int enable)

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c +3 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       pwrst->next_state = PWRDM_POWER_RET;
+                       pr_err("%s: cannot enable Core OFF due to

i583\n",

Quoted 3 lines

Quoted 1 line

+ __func__);

You probably need to throw up this warning only if state
== PWRDM_POWER_OFF. Otherwise this code looks fine to me.

Thanks for the review. added it. will post a v4 later today if no one cribs with this approach. I will retain the logic in sram_idle as well as a backup measure.

Quoted 22 lines · Nishanth Menon

Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
[...]

Quoted 20 lines

Quoted 13 lines

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..da12a56 100644

arch/arm/mach-omap2/pm34xx.c +1 -2

@@ -932,8 +932,15 @@ void omap3_pm_off_mode_enable(int enable)
  #endif
 
         list_for_each_entry(pwrst, &pwrst_list, node) {
-               pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

arch/arm/mach-omap2/pm34xx.c void omap3_pm_off_mode_enable(int enable)

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c +3 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       pwrst->next_state = PWRDM_POWER_RET;
+                       pr_err("%s: cannot enable Core OFF due to

i583\n",

Quoted 1 line

+ __func__);

You probably need to throw up this warning only if state
== PWRDM_POWER_OFF. Otherwise this code looks fine to me.

Quoted 3 lines

Thanks for the review. added it. will post a v4 later today if no one cribs with this approach. I will retain the logic in sram_idle as well as a backup measure.

This logic doesn't belong in SRAM idle. To handle the idle case, you should also disable the 'valid' bit for any C-state that has CORE off (I think there's only one.)

Kevin Hilman had written, on 12/13/2010 09:42 PM, the following:

Quoted 28 lines

Quoted 25 lines · Nishanth Menon

Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
[...]

Quoted 17 lines

Quoted 13 lines

arch/arm/mach- renamed from arch/arm/mach-omap2/pm34xx.c +0 -0

omap2/pm34xx.c
index ba3c0d6..da12a56 100644

arch/arm/mach-omap2/pm34xx.c +1 -2

@@ -932,8 +932,15 @@ void omap3_pm_off_mode_enable(int enable)
  #endif
 
         list_for_each_entry(pwrst, &pwrst_list, node) {
-               pwrst->next_state = state;
-               omap_set_pwrdm_state(pwrst->pwrdm, state);
+               if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

arch/arm/mach-omap2/pm34xx.c void omap3_pm_off_mode_enable(int enable)

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c +3 -0

+                               pwrst->pwrdm == core_pwrdm) {
+                       pwrst->next_state = PWRDM_POWER_RET;
+                       pr_err("%s: cannot enable Core OFF due to

i583\n",

Quoted 3 lines

Quoted 1 line

+ __func__);

You probably need to throw up this warning only if state
== PWRDM_POWER_OFF. Otherwise this code looks fine to me.

Thanks for the review. added it. will post a v4 later today if no one cribs with this approach. I will retain the logic in sram_idle as well as a backup measure.

Quoted 3 lines

This logic doesn't belong in SRAM idle. To handle the idle case, you should also disable the 'valid' bit for any C-state that has CORE off (I think there's only one.)

Apologies, but I dont think I get your point. Do you intend to state that we dynamically add the C7 state in cpuidle34xx.c if this condition is met?

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Setting the next states only sets the default states, but CPUidle changes them.

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:

Quoted 4 lines

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Quoted 7 lines

Setting the next states only sets the default states, but CPUidle changes them.

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Thx. it is clear now. let me see how to clean this up.

From bd3d8decf922d7425b9bc9025c7709a9414ad380 Mon Sep 17 00:00:00 2001

From e085afb8523ca52e52b7a631c2b63e2bd6d91661 Mon Sep 17 00:00:00 2001

Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:

Quoted 9 lines

Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:

Quoted 7 lines

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Setting the next states only sets the default states, but CPUidle changes them.

Quoted 5 lines

Quoted 4 lines

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Thx. it is clear now. let me see how to clean this up.

k. Does the attached look any better now :)?

I have removed the check
logic from sram_idle path instead made omap3_cpuidle_update_states little more generic, updated validity of C state based on errata. This now disables just those C states with core OFF on 3630 <ES1.2

in a map, this will now look as follows:
--------+-------+-------+-------+-------+--------+

        | MPU   | Core  | OFF   | OFF Enable-36xx|
        | Dom   | Dom   |       +-------+--------+
C state | State | State | Dis   | ES1.1 | ES 1.2 |
--------+-------+-------+-------+-------+--------+
1       | ON    | ON    | YES   | YES   | YES    |
2       | ON    | ON    | YES   | YES   | YES    |
3       | RET   | ON    | YES   | YES   | YES    |
4       | OFF   | ON    | NO    | YES   | YES    |
5       | RET   | RET   | YES   | YES   | YES    |
6       | OFF   | RET   | NO    | YES   | YES    |
7       | OFF   | OFF   | NO    | NO    | YES    |

--------+-------+-------+-------+-------+--------+

Signature · 19 lines

Regards,
Nishanth Menon
From bd3d8decf922d7425b9bc9025c7709a9414ad380 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Wed, 15 Dec 2010 18:40:29 -0600
Subject: [PATCH 1/2] OMAP3: PM: make omap3_cpuidle_update_states independent of enable_off_mode

Currently omap3_cpuidle_update_states makes whole sale decision
on which C states to update based on enable_off_mode variable
Instead, achieve the same functionality by independently providing
mpu and core deepest states the system is allowed to achieve and
update the idle states accordingly.

Signed-off-by: Nishanth Menon <[email protected]>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   19 ++++++++++---------
 arch/arm/mach-omap2/pm.h          |    3 ++-
 arch/arm/mach-omap2/pm34xx.c      |    2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

arch/arm/mach-omap2/cpuidle34xx.c +10 -9

index 0d50b45..f80d3f6 100644
@@ -293,25 +293,26 @@ select_state:
 DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
 
 /**
- * omap3_cpuidle_update_states - Update the cpuidle states.
+ * omap3_cpuidle_update_states() - Update the cpuidle states
+ * @mpu_deepest_state:	Enable states upto and including this for mpu domain
+ * @core_deepest_state:	Enable states upto and including this for core domain
  *
- * Currently, this function toggles the validity of idle states based upon
- * the flag 'enable_off_mode'. When the flag is set all states are valid.
- * Else, states leading to OFF state set to be invalid.
+ * This goes through the list of states available and enables and disables the
+ * validity of C states based on deepest state that can be achieved for the
+ * variable domain
  */
-void omap3_cpuidle_update_states(void)
+void omap3_cpuidle_update_states(u32 mpu_deepest_state, u32 core_deepest_state)
 {
 	int i;
 
 	for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
 		struct omap3_processor_cx *cx = &omap3_power_states[i];
 
-		if (enable_off_mode) {
+		if ((cx->mpu_state >= mpu_deepest_state) &&
+		    (cx->core_state >= core_deepest_state)) {
 			cx->valid = 1;
 		} else {
-			if ((cx->mpu_state == PWRDM_POWER_OFF) ||
-				(cx->core_state	== PWRDM_POWER_OFF))
-				cx->valid = 0;
+			cx->valid = 0;
 		}
 	}
 }

arch/arm/mach-omap2/pm.h +2 -1

index aff39d0..3597591 100644
@@ -58,7 +58,8 @@ extern u32 sleep_while_idle;
 #endif
 
 #if defined(CONFIG_CPU_IDLE)
-extern void omap3_cpuidle_update_states(void);
+extern void omap3_cpuidle_update_states(u32 core_deepest_state,
+		u32 core_deepest_state);
 #endif
 
 #if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)

arch/arm/mach-omap2/pm34xx.c +1 -1

index 21707c9..84ef71b 100644
@@ -916,7 +916,7 @@ void omap3_pm_off_mode_enable(int enable)
 		state = PWRDM_POWER_RET;
 
 #ifdef CONFIG_CPU_IDLE
-	omap3_cpuidle_update_states();
+	omap3_cpuidle_update_states(state, state);
 #endif
 
 	list_for_each_entry(pwrst, &pwrst_list, node) {

Signature · 32 lines

1.6.3.3

From e085afb8523ca52e52b7a631c2b63e2bd6d91661 Mon Sep 17 00:00:00 2001
From: Eduardo Valentin <[email protected]>
Date: Wed, 17 Nov 2010 21:46:01 -0600
Subject: [PATCH 2/2] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Limitation i583: Self_Refresh Exit issue after OFF mode

Issue:
When device is waking up from OFF mode, then SDRC state machine sends
inappropriate sequence violating JEDEC standards.

Impact:
OMAP3630 < ES1.2 is impacted as follows depending on the platform:
CS0: for 38.4MHz as internal sysclk, DDR content seen to be stable, while
	for all other sysclk frequencies, varied levels of instability
	seen based on varied parameters.
CS1: impacted

This patch takes option #3 as recommended by the Silicon erratum:
Avoid core power domain transitioning to OFF mode. Power consumption
impact is expected in this case.
To do this, we route core OFF requests to RET request on the impacted
revisions of silicon.

[[email protected]: rebased the code to 2.6.37-rc2- short circuit code changed a bit]
Signed-off-by: Nishanth Menon <[email protected]>
Signed-off-by: Eduardo Valentin <[email protected]>
---
 arch/arm/mach-omap2/pm34xx.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

arch/arm/mach-omap2/pm34xx.c +22 -3

index 84ef71b..ad60105 100644
@@ -56,6 +56,7 @@
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
 #define RTA_ERRATUM_i608		(1 << 0)
+#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
 

@@ -916,12 +917,28 @@ void omap3_pm_off_mode_enable(int enable)
 		state = PWRDM_POWER_RET;
 
 #ifdef CONFIG_CPU_IDLE
-	omap3_cpuidle_update_states(state, state);
+	/*
+	 * Erratum i583: implementation for ES rev < Es1.2 on 3630. We cannot
+	 * enable OFF mode in a stable form for previous revisions, restrict
+	 * instead to RET
+	 */
+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583))
+		omap3_cpuidle_update_states(state, PWRDM_POWER_RET);
+	else
+		omap3_cpuidle_update_states(state, state);
 #endif
 
 	list_for_each_entry(pwrst, &pwrst_list, node) {
-		pwrst->next_state = state;
-		omap_set_pwrdm_state(pwrst->pwrdm, state);
+		if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583) &&
+				pwrst->pwrdm == core_pwrdm &&
+				state == PWRDM_POWER_OFF) {
+			pwrst->next_state = PWRDM_POWER_RET;
+			pr_err("%s: Core OFF disabled due to errata i583\n",
+				__func__);
+		} else {
+			pwrst->next_state = state;
+		}
+		omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
 	}
 }
 

@@ -999,6 +1016,8 @@ static void __init pm_errata_configure(void)
 		pm34xx_errata |= RTA_ERRATUM_i608;
 		/* Enable the l2 cache toggling in sleep logic */
 		enable_omap3630_toggle_l2_on_restore();
+		if (omap_rev() < OMAP3630_REV_ES1_2)
+			pm34xx_errata |= SDRC_WAKEUP_ERRATUM_i583;
 	}
 }

Quoted 9 lines · Nishanth Menon

Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:

Quoted 8 lines

Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:

Quoted 6 lines

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Setting the next states only sets the default states, but CPUidle changes them.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Thx. it is clear now. let me see how to clean this up.

k. Does the attached look any better now :)?

Yes, but, I still don't quite like it. Basically, I'm not crazy about
the errata knowledge being centralized in pm34xx.c. How about this:

Move the Errata handling core code (pm_errata_* PM_ERRATTA_*) to pm.[ch] as a single patch. Then both pm34xx.c and cpuidle34xx.c would be free to use it.

This would allow CPUidle handle the errata itself in the 'update_states' function. Or even better, if CPUidle core can check this errata, it should probably just never register C7 in the first place, because it is *never* a valid C-state.

Make sense?

Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:

Quoted 25 lines

Quoted 9 lines · Nishanth Menon

Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:

Quoted 8 lines

Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:

Quoted 6 lines

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Setting the next states only sets the default states, but CPUidle changes them.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Thx. it is clear now. let me see how to clean this up.

k. Does the attached look any better now :)?

Yes, but, I still don't quite like it. Basically, I'm not crazy about
the errata knowledge being centralized in pm34xx.c. How about this:

Move the Errata handling core code (pm_errata_* PM_ERRATTA_*) to pm.[ch] as a single patch. Then both pm34xx.c and cpuidle34xx.c would be free to use it.

Quoted 6 lines

This would allow CPUidle handle the errata itself in the 'update_states' function. Or even better, if CPUidle core can check this errata, it should probably just never register C7 in the first place, because it is *never* a valid C-state.

Make sense?

hmm.. IMHO at the problems themselves:
a) cpuidle_params_table -> this needs to become dynamically generated if you'd like cpuidle to not know about the existance of the invalid states at all(C7 has to disappear from it's radar - but extending it to a generic solution where an inbetween C state might be disabled as well)

b) extending the problem further - cpu idle state latencies by themselves might vary between boards(PMIC variances causing delta in voltage setup times as an example).. I suppose we have some way to plug that data in as well? but irrelevant to this discussion. or maybe some board would like to have a customized additional c state which is not really practical for other platforms for what ever reasons..

c) if cpuidle where to get pm errata info, it is nice thing to do, but, dont you think it is an overkill atm for just one errata?

d) omap_init_power_states may need some cleanups as well.. too many = assignments IMHO..

e) On the topic of argument that the states controlled by enable_off_mode is dynamic, though even though enable_off_mode is a variable, it is in debugfs - not really a dynamic variant in a product.. with that in mind, we may want to have off OR not have off mode in a product board file and folks would probably call omap3_pm_off_mode_enable or set the variable and set it to 1. That makes it even more crazy IMHO.

f) Finally, where do we detect the erratas? it is more handy to have them in one place - pm34xx.c was chosen to make it independent of silicon type - pm44xx.c might endup having different set with it's own requirements - so not all together convinced it should be in pm.[ch]. I have tried to restrict the detection and usage purely to the file that needs it - pm34xx.c

I think I agree to your overall thought that C state by itself should'nt have been registered, but would'nt it be better to do the cpuidle cleanups in a different context?

Quoted 65 lines · Nishanth Menon

Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:

Quoted 28 lines

Quoted 9 lines · Nishanth Menon

Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:

Quoted 8 lines

Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:

Quoted 6 lines

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Setting the next states only sets the default states, but CPUidle changes them.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Thx. it is clear now. let me see how to clean this up.

k. Does the attached look any better now :)?

Yes, but, I still don't quite like it. Basically, I'm not crazy about
the errata knowledge being centralized in pm34xx.c. How about this:

Move the Errata handling core code (pm_errata_* PM_ERRATTA_*) to pm.[ch] as a single patch. Then both pm34xx.c and cpuidle34xx.c would be free to use it.
This would allow CPUidle handle the errata itself in the 'update_states' function. Or even better, if CPUidle core can check this errata, it should probably just never register C7 in the first place, because it is *never* a valid C-state.

Quoted 1 line

Make sense?

hmm.. IMHO at the problems themselves:
a) cpuidle_params_table -> this needs to become dynamically generated if you'd like cpuidle to not know about the existance of the invalid states at all(C7 has to disappear from it's radar - but extending it to a generic solution where an inbetween C state might be disabled as well)

b) extending the problem further - cpu idle state latencies by themselves might vary between boards(PMIC variances causing delta in voltage setup times as an example).. I suppose we have some way to plug that data in as well? but irrelevant to this discussion. or maybe some board would like to have a customized additional c state which is not really practical for other platforms for what ever reasons..

c) if cpuidle where to get pm errata info, it is nice thing to do, but, dont you think it is an overkill atm for just one errata?

d) omap_init_power_states may need some cleanups as well.. too many = assignments IMHO..

e) On the topic of argument that the states controlled by enable_off_mode is dynamic, though even though enable_off_mode is a variable, it is in debugfs - not really a dynamic variant in a product.. with that in mind, we may want to have off OR not have off mode in a product board file and folks would probably call omap3_pm_off_mode_enable or set the variable and set it to 1. That makes it even more crazy IMHO.

f) Finally, where do we detect the erratas? it is more handy to have them in one place - pm34xx.c was chosen to make it independent of silicon type - pm44xx.c might endup having different set with it's own requirements - so not all together convinced it should be in pm.[ch]. I have tried to restrict the detection and usage purely to the file that needs it - pm34xx.c

Quoted 3 lines

I think I agree to your overall thought that C state by itself should'nt have been registered, but would'nt it be better to do the cpuidle cleanups in a different context?

If you want to do all those cleanups, feel free. They all are valid.

However, your patch targets an isolated problem, and I'm OK with an isolated fix.

All I was suggesting is moving the PM errata detection/macros etc. into pm.h, and doing somthing simple (below) in CPUidle.

Kevin

arch/arm/mach-omap2/cpuidle34xx.c +1 -0

index 81b0a90..92873b4 100644
@@ -452,6 +452,15 @@ void omap_init_power_states(void)
 	omap3_power_states[OMAP3_STATE_C7].core_state = PWRDM_POWER_OFF;
 	omap3_power_states[OMAP3_STATE_C7].flags = CPUIDLE_FLAG_TIME_VALID |
 				CPUIDLE_FLAG_CHECK_BM;
+

Kevin Hilman had written, on 12/17/2010 04:54 PM, the following:

Quoted 89 lines

Quoted 65 lines · Nishanth Menon

Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:

Quoted 28 lines

Quoted 9 lines · Nishanth Menon

Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:

Quoted 8 lines

Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:

Quoted 6 lines

Quoted 3 lines

I agree that this additional check in sram_idle should be removed, but as long as I handle it in omap3_pm_off_mode_enable where the next states are configured, is'nt that enough or am I missing something?

Setting the next states only sets the default states, but CPUidle changes them.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).

Thx. it is clear now. let me see how to clean this up.

k. Does the attached look any better now :)?

Yes, but, I still don't quite like it. Basically, I'm not crazy about
the errata knowledge being centralized in pm34xx.c. How about this:

Move the Errata handling core code (pm_errata_* PM_ERRATTA_*) to pm.[ch] as a single patch. Then both pm34xx.c and cpuidle34xx.c would be free to use it.
This would allow CPUidle handle the errata itself in the 'update_states' function. Or even better, if CPUidle core can check this errata, it should probably just never register C7 in the first place, because it is *never* a valid C-state.

Quoted 1 line

Make sense?

hmm.. IMHO at the problems themselves:
a) cpuidle_params_table -> this needs to become dynamically generated if you'd like cpuidle to not know about the existance of the invalid states at all(C7 has to disappear from it's radar - but extending it to a generic solution where an inbetween C state might be disabled as well)

b) extending the problem further - cpu idle state latencies by themselves might vary between boards(PMIC variances causing delta in voltage setup times as an example).. I suppose we have some way to plug that data in as well? but irrelevant to this discussion. or maybe some board would like to have a customized additional c state which is not really practical for other platforms for what ever reasons..

c) if cpuidle where to get pm errata info, it is nice thing to do, but, dont you think it is an overkill atm for just one errata?

d) omap_init_power_states may need some cleanups as well.. too many = assignments IMHO..

e) On the topic of argument that the states controlled by enable_off_mode is dynamic, though even though enable_off_mode is a variable, it is in debugfs - not really a dynamic variant in a product.. with that in mind, we may want to have off OR not have off mode in a product board file and folks would probably call omap3_pm_off_mode_enable or set the variable and set it to 1. That makes it even more crazy IMHO.

f) Finally, where do we detect the erratas? it is more handy to have them in one place - pm34xx.c was chosen to make it independent of silicon type - pm44xx.c might endup having different set with it's own requirements - so not all together convinced it should be in pm.[ch]. I have tried to restrict the detection and usage purely to the file that needs it - pm34xx.c

Quoted 3 lines

I think I agree to your overall thought that C state by itself should'nt have been registered, but would'nt it be better to do the cpuidle cleanups in a different context?

If you want to do all those cleanups, feel free. They all are valid.

However, your patch targets an isolated problem, and I'm OK with an isolated fix.

All I was suggesting is moving the PM errata detection/macros etc. into pm.h, and doing somthing simple (below) in CPUidle.

Kevin

arch/arm/mach-omap2/cpuidle34xx.c +0 -0

index 81b0a90..92873b4 100644
@@ -452,6 +452,15 @@ void omap_init_power_states(void)
 	omap3_power_states[OMAP3_STATE_C7].core_state = PWRDM_POWER_OFF;
 	omap3_power_states[OMAP3_STATE_C7].flags = CPUIDLE_FLAG_TIME_VALID |
 				CPUIDLE_FLAG_CHECK_BM;

arch/arm/mach-omap2/cpuidle34xx.c void omap_init_power_states(void) · line 464

Quoted 10 lines

arch/arm/mach-omap2/cpuidle34xx.c +9 -0

+
+	/*
+	 * Erratum i583: implementation for ES rev < Es1.2 on 3630
+	 * We cannot enable OFF mode in a stable form for previous
+	 * revisions, transition instead to RET
+	 */
+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)
+	    omap3_power_states[OMAP3_STATE_C7].valid = 0;
+	     }
 

Look at the following sequence:
a) omap_init_power_states is called at init omap3_power_states[OMAP3_STATE_C7].valid = 0
lets say we make cpuidle_params_table[OMAP3_STATE_C7].valid = 0 as well here.

b) user enables enable_off_mode omap3_pm_off_mode_enable(pm34xx.c) -> calls omap3_cpuidle_update_states

for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
        struct omap3_processor_cx *cx = &omap3_power_states[i];

        if (enable_off_mode) {

                cx->valid = 1;
---> valid bit is back to 1.
        } else {
                if ((cx->mpu_state == PWRDM_POWER_OFF) ||
                        (cx->core_state == PWRDM_POWER_OFF))
                        cx->valid = 0;
        }
}

c) idle will not hit the low state coz cpuidle_params_table[OMAP3_STATE_C7].valid will not be set

d) you try suspend path, well, we dont check the cpuidle_params_table.. we will attempt to hit off.

This approach of selective disable of omap3_power_states will not work without modifying omap3_cpuidle_update_states. am I wrong?

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Monday, December 13, 2010 8:07 PM
To: Vishwanath Sripathy
Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the following:
[...]

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)

&&

Quoted 1 line

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+			(core_next_state == PWRDM_POWER_OFF))

{

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+		pwrdm_set_next_pwrst(core_pwrdm,

PWRDM_POWER_RET);

(quoted diff) +2 -0

+		core_next_state = PWRDM_POWER_RET;
+	}

Since next_state in pwrst_list (for core) is not updated, this is

throwing

Quoted 1 line

Quoted 1 line

up an error "Powerdomain (core_pwrdm) didn't enter target

state

Quoted 3 lines

Quoted 3 lines

0"

Quoted 2 lines

Quoted 2 lines

when

Quoted 1 line

you off mode is enabled for ES1.1 or lesser (in suspend path).

It's

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

not

Quoted 2 lines

Quoted 2 lines

really true that Core has not entered target state. It has entered Retention state which is the actual target state set in

omap_sram_idle.

Quoted 4 lines

Quoted 1 line

However it did not enter the state that was passed by

omap3_pm_suspend. Is

Quoted 1 line

this expected behavior?

we could go both ways on this - this patch will(as you noticed)

indicate

Quoted 2 lines

Quoted 2 lines

that the transition failed on <ES1.2, or we could make it entirely transparent(by modifying the the pwrst_list - claim we achieved

off,

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

while not really hitting off - I personally dont think that is

correct.

Quoted 1 line

Quoted 1 line

The point I am making is that you cannot distinguish between

genuine

Quoted 8 lines

Quoted 2 lines

off

Quoted 1 line

/retention failure since this message is thrown for both pass and

fail.

Quoted 5 lines

Quoted 2 lines

May be an additional trace message indicating that system entered retention instead of off (for ES<1.2) will be useful.

hmm... good point there.
two issues here:
a) omap3_pm_suspend should probably state which state was

achieved

Quoted 5 lines

Quoted 5 lines

as
well in the error message (trivial fix).
b) how do we notify users that it was due to SDRC_WAKEUP_ERRATUM_i583
that core-off was denied. -> do this in omap3_pm_suspend(when

user

Quoted 6 lines

Quoted 6 lines

attempts actual OFF) OR omap3_pm_off_mode_enable(when user attempts to
enable OFF mode)?

Any suggestions to allow the same uImage boot on all silicon + allow cpu_idle + suspend paths not to spew pr_info messages(aka cant

add

Quoted 1 line

Quoted 1 line

prints in sram_idle)?

Quoted 2 lines

I vote for denying off mode for Core (for ES<1.2) in omap3_pm_off_mode_enable and throw up a message saying that

Core off is

Quoted 2 lines

not enabled. Then we will not get this failure message in suspend path since pwrst_list will have the right state.

Keep in mind - if we disable it in omap3_pm_off_mode_enable - we will deny OFF wholesale if I understand the logic right- not just core-off - I kind of think that is extreme.

--
Regards,
Nishanth Menon

Quoted 3 lines

Keep in mind - if we disable it in omap3_pm_off_mode_enable - we will deny OFF wholesale if I understand the logic right- not just core-off - I kind of think that is extreme.

No, I am not saying that deny idle for all power domains. Deny it only for Core domain, something like this.
void omap3_pm_off_mode_enable(int enable)

{
	struct power_state *pwrst;
	u32 state;

	if (enable)
		state = PWRDM_POWER_OFF;
	else
		state = PWRDM_POWER_RET;

#ifdef CONFIG_CPU_IDLE
	omap3_cpuidle_update_states();

#endif

	list_for_each_entry(pwrst, &pwrst_list, node) {
		pwrst->next_state = state;
		if (strcmp("core_pwrdm", pwrst->pwrdm->name)==0) {
		if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)
&& state ==PWRDM_POWER_OFF)
		pwrst->next_state = PWRDM_POWER_RET;
		}
		omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);

Hi,
as discussed in [1], here is step 2 - idle path errata fixes. this is the next rev incorporating comments from V2 post of this series.

V2: http://marc.info/?l=linux-omap&m=129106200408109&w=2

Major change in V3:
Erratas are now handled per silicon - it is much cleaner :) no more redundant cpu_is_omap34xx check anymore errata configure is __init as it should be

Eduardo Valentin (1):
OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

Nishanth Menon (1):
OMAP3630: PM: Erratum i608: disable RTA

Peter 'p2' De Schrijver (2):
OMAP3: PM: Erratum i581 support: dll kick strategy
OMAP3630: PM: Disable L2 cache while invalidating L2 cache

Richard Woodruff (1):
OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all

 arch/arm/mach-omap2/control.c   |    5 +-
 arch/arm/mach-omap2/control.h   |    5 +
 arch/arm/mach-omap2/pm.h        |    6 ++
 arch/arm/mach-omap2/pm34xx.c    |   38 ++++++++
 arch/arm/mach-omap2/sleep34xx.S |  187 ++++++++++++++++++++++++---------------
 5 files changed, 169 insertions(+), 72 deletions(-)

bloat-o-meter report Vs 2.6.37-rc4
add/remove: 1/0 grow/shrink: 6/2 up/down: 220/-12 (208)
function old new delta

omap3_pm_init                               1776    1868     +92
omap_sram_idle                              1040    1104     +64
omap3_save_scratchpad_contents               732     760     +28
vermagic                                      45      60     +15
linux_banner                                 131     146     +15
omap2_init_mmc                              1032    1036      +4
pm34xx_errata                                  -       2      +2
omap_serial_init_port                       1120    1116      -4
prcm_interrupt_handler                       276     268      -8

[1] http://marc.info/?l=linux-omap&m=129045338806957&w=2

Cc Charulatha Varadarajan <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Tao Hu <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Vishwanath Sripathy <[email protected]>