[PATCH 0/5 v3] OMAP: idle path errata fixes

[PATCH 0/5 v3] OMAP: idle path errata fixes

[Nishanth Menon]

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 <charu@ti.com>
Cc: Jean Pihet <jean.pihet@newoldbits.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Tao Hu <tghk48@motorola.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Regards,
Nishanth Menon

[PATCH 1/5 v2] OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all

[Nishanth Menon]

From: Richard Woodruff <r-woodruff2@ti.com>

Analysis in TI kernel with ETM showed that using cache mapped flush
in kernel instead of SO mapped flush cost drops by 65% (3.39mS down
to 1.17mS) for clean_l2 which is used during sleep sequences.
Overall:
	- speed up
	- unfortunately there isn't a good alternative flush method today
	- code reduction and less maintenance and potential bug in
	  unmaintained code

This also fixes the bug with the clean_l2 function usage.

Reported-by: Tony Lindgren <tony@atomide.com>

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Tony Lindgren <tony@atomide.com>

[nm@ti.com: ported rkw's proposal to 2.6.37-rc2]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
---
(no change in this series, posted for completeness)
v2: https://patchwork.kernel.org/patch/365222/
v1: http://marc.info/?l=linux-omap&m=129013171325210&w=2
 arch/arm/mach-omap2/sleep34xx.S |   79 ++++++--------------------------------
 1 files changed, 13 insertions(+), 66 deletions(-)

diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 2fb205a..2c20fcf 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -520,72 +520,17 @@ clean_caches:
 	cmp	r9, #1 /* Check whether L2 inval is required or not*/
 	bne	skip_l2_inval
 clean_l2:
-	/* read clidr */
-	mrc     p15, 1, r0, c0, c0, 1
-	/* extract loc from clidr */
-	ands    r3, r0, #0x7000000
-	/* left align loc bit field */
-	mov     r3, r3, lsr #23
-	/* if loc is 0, then no need to clean */
-	beq     finished
-	/* start clean at cache level 0 */
-	mov     r10, #0
-loop1:
-	/* work out 3x current cache level */
-	add     r2, r10, r10, lsr #1
-	/* extract cache type bits from clidr*/
-	mov     r1, r0, lsr r2
-	/* mask of the bits for current cache only */
-	and     r1, r1, #7
-	/* see what cache we have at this level */
-	cmp     r1, #2
-	/* skip if no cache, or just i-cache */
-	blt     skip
-	/* select current cache level in cssr */
-	mcr     p15, 2, r10, c0, c0, 0
-	/* isb to sych the new cssr&csidr */
-	isb
-	/* read the new csidr */
-	mrc     p15, 1, r1, c0, c0, 0
-	/* extract the length of the cache lines */
-	and     r2, r1, #7
-	/* add 4 (line length offset) */
-	add     r2, r2, #4
-	ldr     r4, assoc_mask
-	/* find maximum number on the way size */
-	ands    r4, r4, r1, lsr #3
-	/* find bit position of way size increment */
-	clz     r5, r4
-	ldr     r7, numset_mask
-	/* extract max number of the index size*/
-	ands    r7, r7, r1, lsr #13
-loop2:
-	mov     r9, r4
-	/* create working copy of max way size*/
-loop3:
-	/* factor way and cache number into r11 */
-	orr     r11, r10, r9, lsl r5
-	/* factor index number into r11 */
-	orr     r11, r11, r7, lsl r2
-	/*clean & invalidate by set/way */
-	mcr     p15, 0, r11, c7, c10, 2
-	/* decrement the way*/
-	subs    r9, r9, #1
-	bge     loop3
-	/*decrement the index */
-	subs    r7, r7, #1
-	bge     loop2
-skip:
-	add     r10, r10, #2
-	/* increment cache number */
-	cmp     r3, r10
-	bgt     loop1
-finished:
-	/*swith back to cache level 0 */
-	mov     r10, #0
-	/* select current cache level in cssr */
-	mcr     p15, 2, r10, c0, c0, 0
-	isb
+	/*
+	 * Jump out to kernel flush routine
+	 *  - reuse that code is better
+	 *  - it executes in a cached space so is faster than refetch per-block
+	 *  - should be faster and will change with kernel
+	 *  - 'might' have to copy address, load and jump to it
+	 */
+	ldr r1, kernel_flush
+	mov lr, pc
+	bx  r1
+
 skip_l2_inval:
 	/* Data memory barrier and Data sync barrier */
 	mov     r1, #0
@@ -668,5 +613,7 @@ cache_pred_disable_mask:
 	.word	0xFFFFE7FB
 control_stat:
 	.word	CONTROL_STAT
+kernel_flush:
+	.word v7_flush_dcache_all
 ENTRY(omap34xx_cpu_suspend_sz)
 	.word	. - omap34xx_cpu_suspend
-- 
1.6.3.3

[PATCH 2/5 v2] OMAP3: PM: Erratum i581 support: dll kick strategy

[Nishanth Menon]

From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>

Erratum i581 impacts OMAP3 platforms.
PRCM DPLL control FSM removes SDRC_IDLEREQ before DPLL3 locks causing
the DPLL not to be locked at times.

IMPORTANT:
*) This is not a complete workaround implementation as recommended
by the silicon erratum. this is a support logic for detecting lockups and
attempting to recover where possible and is known to provide stability
in multiple platforms.
*) This code is mostly important for inactive and retention. The ROM code
waits for the maximum dll lock time when resuming from off mode. So for
off mode this code isn't really needed.

This should eventually get refactored as part of cleanups to sleep34xx.S

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Tony Lindgren <tony@atomide.com>

Signed-off-by: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
---
(no change done, posting for completeness of the series)
v2: https://patchwork.kernel.org/patch/365252/
	typo correction- erratum, support, added comment from Peter from the
	thread to commit message
v1: http://marc.info/?l=linux-omap&m=129013172525234&w=2
 arch/arm/mach-omap2/sleep34xx.S |   52 +++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 2c20fcf..3fbd1e5 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -42,6 +42,7 @@
 				OMAP3430_PM_PREPWSTST)
 #define PM_PWSTCTRL_MPU_P	OMAP3430_PRM_BASE + MPU_MOD + OMAP2_PM_PWSTCTRL
 #define CM_IDLEST1_CORE_V	OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
+#define CM_IDLEST_CKGEN_V	OMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST)
 #define SRAM_BASE_P		0x40200000
 #define CONTROL_STAT		0x480022F0
 #define SCRATCHPAD_MEM_OFFS	0x310 /* Move this as correct place is
@@ -554,31 +555,67 @@ skip_l2_inval:
 
 /* Make sure SDRC accesses are ok */
 wait_sdrc_ok:
+
+/* DPLL3 must be locked before accessing the SDRC. Maybe the HW ensures this. */
+	ldr	r4, cm_idlest_ckgen
+wait_dpll3_lock:
+	ldr	r5, [r4]
+	tst	r5, #1
+	beq	wait_dpll3_lock
+
         ldr     r4, cm_idlest1_core
+wait_sdrc_ready:
         ldr     r5, [r4]
-        and     r5, r5, #0x2
-        cmp     r5, #0
-        bne     wait_sdrc_ok
+        tst     r5, #0x2
+        bne     wait_sdrc_ready
+	/* allow DLL powerdown upon hw idle req */
         ldr     r4, sdrc_power
         ldr     r5, [r4]
         bic     r5, r5, #0x40
         str     r5, [r4]
-wait_dll_lock:
+is_dll_in_lock_mode:
+
         /* Is dll in lock mode? */
         ldr     r4, sdrc_dlla_ctrl
         ldr     r5, [r4]
         tst     r5, #0x4
         bxne    lr
         /* wait till dll locks */
-        ldr     r4, sdrc_dlla_status
+wait_dll_lock_timed:
+	ldr	r4, wait_dll_lock_counter
+	add	r4, r4, #1
+	str	r4, wait_dll_lock_counter
+	ldr	r4, sdrc_dlla_status
+        mov	r6, #8		/* Wait 20uS for lock */
+wait_dll_lock:
+	subs	r6, r6, #0x1
+	beq	kick_dll
         ldr     r5, [r4]
         and     r5, r5, #0x4
         cmp     r5, #0x4
         bne     wait_dll_lock
         bx      lr
 
+	/* disable/reenable DLL if not locked */
+kick_dll:
+	ldr	r4, sdrc_dlla_ctrl
+	ldr	r5, [r4]
+	mov	r6, r5
+	bic	r6, #(1<<3)	/* disable dll */
+	str	r6, [r4]
+	dsb
+	orr	r6, r6, #(1<<3)	/* enable dll */
+	str	r6, [r4]
+	dsb
+	ldr	r4, kick_counter
+	add	r4, r4, #1
+	str	r4, kick_counter
+	b	wait_dll_lock_timed
+
 cm_idlest1_core:
 	.word	CM_IDLEST1_CORE_V
+cm_idlest_ckgen:
+	.word	CM_IDLEST_CKGEN_V
 sdrc_dlla_status:
 	.word	SDRC_DLLA_STATUS_V
 sdrc_dlla_ctrl:
@@ -615,5 +652,10 @@ control_stat:
 	.word	CONTROL_STAT
 kernel_flush:
 	.word v7_flush_dcache_all
+	/* these 2 words need to be at the end !!! */
+kick_counter:
+	.word	0
+wait_dll_lock_counter:
+	.word	0
 ENTRY(omap34xx_cpu_suspend_sz)
 	.word	. - omap34xx_cpu_suspend
-- 
1.6.3.3

[PATCH 3/5 v3] OMAP3630: PM: Erratum i608: disable RTA

[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 <khilman@deeprootsystems.com>
Cc: Tony Lindgren <tony@atomide.com>

[ambresh@ti.com: co-developer]
Signed-off-by: Ambresh K <ambresh@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
v3: additional comment to explain Ambresh's contrib
	removed the redundant check for cpu_is34xx - it is already
		done by pm_init
	pm_errata_configure is __init
v2: https://patchwork.kernel.org/patch/365242/
	fixed missing b restore for 3430 es3.1 code.
	introduced erratum handling logic here splitting it out of uart errata
	typo fixes for erratum
v1: http://marc.info/?l=linux-omap&m=129013172825240&w=2

 arch/arm/mach-omap2/control.c   |    5 ++++-
 arch/arm/mach-omap2/control.h   |    5 +++++
 arch/arm/mach-omap2/pm34xx.c    |   21 +++++++++++++++++++++
 arch/arm/mach-omap2/sleep34xx.S |   26 ++++++++++++++++++++++++++
 4 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 1fa3294..728f268 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -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());
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index b6c6b7c..d7911c5 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -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,6 +351,7 @@ 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);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0ec8a04..a89ffc7 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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,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);
+
 	clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
 	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
 		omap3_secure_ram_storage =
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 3fbd1e5..cc3507b 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -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 !!! */
-- 
1.6.3.3

[PATCH 4/5 v3] OMAP3630: PM: Disable L2 cache while invalidating L2 cache

[Nishanth Menon]

From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>

This disables L2 cache before invalidating it and reenables it afterwards.
This is be done according to ARM documentation. Currently this is identified
as being needed on OMAP3630 as the disable/enable is done from "public side"
while, on OMAP3430, this is done in the "secure side".

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Tony Lindgren <tony@atomide.com>

[nm@ti.com: ported to 2.6.37-rc2, added hooks to enable the logic only on 3630]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
Signed-off-by: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
---
v3: collate all silicon specific errata under a single cpu detection code
	making it elegant and more maintainable.
v2: https://patchwork.kernel.org/patch/365232/
	rebased out to this series independent of HS bugfixes
v1: http://marc.info/?l=linux-omap&m=129013171125204&w=2
 arch/arm/mach-omap2/pm.h        |    6 ++++++
 arch/arm/mach-omap2/pm34xx.c    |    5 ++++-
 arch/arm/mach-omap2/sleep34xx.S |   30 ++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 0d75bfd..aff39d0 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -85,4 +85,10 @@ extern unsigned int save_secure_ram_context_sz;
 extern unsigned int omap24xx_cpu_suspend_sz;
 extern unsigned int omap34xx_cpu_suspend_sz;
 
+#if defined(CONFIG_PM)
+extern void enable_omap3630_toggle_l2_on_restore(void);
+#else
+static inline void enable_omap3630_toggle_l2_on_restore(void) { }
+#endif
+
 #endif
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index a89ffc7..b49e02b 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -995,8 +995,11 @@ void omap_push_sram_idle(void)
 
 static void __init pm_errata_configure(void)
 {
-	if (cpu_is_omap3630())
+	if (cpu_is_omap3630()) {
 		pm34xx_errata |= RTA_ERRATUM_i608;
+		/* Enable the l2 cache toggling in sleep logic */
+		enable_omap3630_toggle_l2_on_restore();
+	}
 }
 
 static int __init omap3_pm_init(void)
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index cc3507b..d2eda01 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -111,6 +111,19 @@ ENTRY(get_omap3630_restore_pointer_sz)
         .word   . - get_omap3630_restore_pointer
 
 	.text
+/*
+ * L2 cache needs to be toggled for stable OFF mode functionality on 3630.
+ * This function sets up a fflag that will allow for this toggling to take
+ * place on 3630. Hopefully some version in the future maynot need this
+ */
+ENTRY(enable_omap3630_toggle_l2_on_restore)
+        stmfd   sp!, {lr}     @ save registers on stack
+	/* Setup so that we will disable and enable l2 */
+	mov	r1, #0x1
+	str	r1, l2dis_3630
+        ldmfd   sp!, {pc}     @ restore regs and return
+
+	.text
 /* Function call to get the restore pointer for for ES3 to resume from OFF */
 ENTRY(get_es3_restore_pointer)
 	stmfd	sp!, {lr}	@ save registers on stack
@@ -283,6 +296,14 @@ restore:
         moveq   r9, #0x3        @ MPU OFF => L1 and L2 lost
 	movne	r9, #0x1	@ Only L1 and L2 lost => avoid L2 invalidation
 	bne	logic_l1_restore
+
+	ldr	r0, l2dis_3630
+	cmp	r0, #0x1	@ should we disable L2 on 3630?
+	bne	skipl2dis
+	mrc	p15, 0, r0, c1, c0, 1
+	bic	r0, r0, #2	@ disable L2 cache
+	mcr	p15, 0, r0, c1, c0, 1
+skipl2dis:
 	ldr	r0, control_stat
 	ldr	r1, [r0]
 	and	r1, #0x700
@@ -343,6 +364,13 @@ smi:    .word 0xE1600070		@ Call SMI monitor (smieq)
 	mov	r12, #0x2
 	.word 0xE1600070	@ Call SMI monitor (smieq)
 logic_l1_restore:
+	ldr	r1, l2dis_3630
+	cmp	r1, #0x1	@ Do we need to re-enable L2 on 3630?
+	bne	skipl2reen
+	mrc	p15, 0, r1, c1, c0, 1
+	orr	r1, r1, #2	@ re-enable L2 cache
+	mcr	p15, 0, r1, c1, c0, 1
+skipl2reen:
 	mov	r1, #0
 	/* Invalidate all instruction caches to PoU
 	 * and flush branch target cache */
@@ -678,6 +706,8 @@ control_mem_rta:
 	.word	CONTROL_MEM_RTA_CTRL
 kernel_flush:
 	.word v7_flush_dcache_all
+l2dis_3630:
+	.word 0
 	/* these 2 words need to be at the end !!! */
 kick_counter:
 	.word	0
-- 
1.6.3.3

[PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

From: Eduardo Valentin <eduardo.valentin@nokia.com>

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 <khilman@deeprootsystems.com>
Cc: Tony Lindgren <tony@atomide.com>

[nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code changed a bit]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
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(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index b49e02b..ba3c0d6 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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 */
+	/*
+	 * 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;
+	}
+
 	if (core_next_state < PWRDM_POWER_ON) {
 		omap_uart_prepare_idle(0);
 		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

Re: [PATCH 0/5 v3] OMAP: idle path errata fixes

[Nishanth Menon]

Nishanth Menon had written, on 12/03/2010 11:03 AM, the following:
> 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 <charu@ti.com>
> Cc: Jean Pihet <jean.pihet@newoldbits.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Tao Hu <tghk48@motorola.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Gentle ping on this series..

Regards,
Nishanth Menon

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy]

Nishant,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] 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 <eduardo.valentin@nokia.com>
>
> 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 <khilman@deeprootsystems.com>
> Cc: Tony Lindgren <tony@atomide.com>
>
> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code changed
> a bit]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
> 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(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> omap2/pm34xx.c
> index b49e02b..ba3c0d6 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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 */
> +	/*
> +	 * 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?

Vishwa
> +
>  	if (core_next_state < PWRDM_POWER_ON) {
>  		omap_uart_prepare_idle(0);
>  		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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Vishwanath Sripathy had written, on 12/13/2010 07:35 AM, the following:
> Nishant,
> 
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] 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 <eduardo.valentin@nokia.com>
>>
>> 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 <khilman@deeprootsystems.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>>
>> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code changed
>> a bit]
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
>> ---
>> 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(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>> omap2/pm34xx.c
>> index b49e02b..ba3c0d6 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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 */
>> +	/*
>> +	 * 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?

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

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy]

Nishant,

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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:
> > Nishant,
> >
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] 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 <eduardo.valentin@nokia.com>
> >>
> >> 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 <khilman@deeprootsystems.com>
> >> Cc: Tony Lindgren <tony@atomide.com>
> >>
> >> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code
> changed
> >> a bit]
> >> Signed-off-by: Nishanth Menon <nm@ti.com>
> >> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> >> omap2/pm34xx.c
> >> index b49e02b..ba3c0d6 100644
> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> @@ -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 */
> >> +	/*
> >> +	 * 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?
>
> 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.

Vishwa
>
> --
> Regards,
> Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Vishwanath Sripathy had written, on 12/13/2010 07:54 AM, the following:
> Nishant,
> 
>> -----Original Message-----
>> From: Nishanth Menon [mailto:nm@ti.com]
>> 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:
>>> Nishant,
>>>
>>>> -----Original Message-----
>>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>>>> owner@vger.kernel.org] 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 <eduardo.valentin@nokia.com>
>>>>
>>>> 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 <khilman@deeprootsystems.com>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>>
>>>> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code
>> changed
>>>> a bit]
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>>>> omap2/pm34xx.c
>>>> index b49e02b..ba3c0d6 100644
>>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>>> @@ -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 */
>>>> +	/*
>>>> +	 * 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?
>> 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.
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

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy]

Nishant,

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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:
> > Nishant,
> >
> >> -----Original Message-----
> >> From: Nishanth Menon [mailto:nm@ti.com]
> >> 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:
> >>> Nishant,
> >>>
> >>>> -----Original Message-----
> >>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >>>> owner@vger.kernel.org] 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 <eduardo.valentin@nokia.com>
> >>>>
> >>>> 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 <khilman@deeprootsystems.com>
> >>>> Cc: Tony Lindgren <tony@atomide.com>
> >>>>
> >>>> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code
> >> changed
> >>>> a bit]
> >>>> Signed-off-by: Nishanth Menon <nm@ti.com>
> >>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> >>>> ---
> >>>> 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(-)
> >>>>
> >>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> >>>> omap2/pm34xx.c
> >>>> index b49e02b..ba3c0d6 100644
> >>>> --- a/arch/arm/mach-omap2/pm34xx.c
> >>>> +++ b/arch/arm/mach-omap2/pm34xx.c
> >>>> @@ -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 */
> >>>> +	/*
> >>>> +	 * 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?
> >> 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.
> 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)?
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.

Vishwa

>
> --
> Regards,
> Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the following:
[...]
>>>>>> +	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?
>>>> 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.
>> 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)?
> 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.
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

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

> -----Original Message-----
> Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the following:
> [...]
> >>>>>> +	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?
> >>>> 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.
> >> 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)?
> > 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.
> 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.

I take that back, we could do something like the following instead:
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index ba3c0d6..74842f1 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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) &&
+                               pwrst->pwrdm == core_pwrdm) {
+                       omap_set_pwrdm_state(pwrst->pwrdm,
PWRDM_POWER_RET);
+                       pr_err("%s: cannot enable Core OFF due to i583\n",
+                               __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

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy]

Nishant,

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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:
> [...]
> >>>>>> +	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?
> >>>> 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.
> >> 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)?
> > 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.
> 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);
	}
}

Vishwa
>
> --
> Regards,
> Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

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

>> 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);
yep, I think our emails crossed wires - I realized the same as well.
-- 
Regards,
Nishanth Menon

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy]

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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
>
> > -----Original Message-----
> > Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the
> following:
> > [...]
> > >>>>>> +	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?
> > >>>> 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.
> > >> 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)?
> > > 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.
> > 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.
>
> I take that back, we could do something like the following instead:
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> omap2/pm34xx.c
> index ba3c0d6..74842f1 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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)
> &&
> +                               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 code in
other email.

Vishwa
> +                       pr_err("%s: cannot enable Core OFF due to
i583\n",
> +                               __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

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Vishwanath Sripathy had written, on 12/13/2010 08:48 AM, the following:
>> -----Original Message-----
>> From: Nishanth Menon [mailto:nm@ti.com]
>> 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
>>
>>> -----Original Message-----
>>> Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the
>> following:
>>> [...]
>>>>>>>>> +	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?
>>>>>>> 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.
>>>>> 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)?
>>>> 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.
>>> 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.
>> I take that back, we could do something like the following instead:
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>> omap2/pm34xx.c
>> index ba3c0d6..74842f1 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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)
>> &&
>> +                               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 code in
> other email.
yep. except, I think we dont need to do string compare. the following 
looks any better?

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index ba3c0d6..da12a56 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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) &&
+                               pwrst->pwrdm == core_pwrdm) {
+                       pwrst->next_state = PWRDM_POWER_RET;
+                       pr_err("%s: cannot enable Core OFF due to i583\n",
+                               __func__);
+               } else {
+                       pwrst->next_state = state;
+               }
+               omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
         }
  }

-- 
Regards,
Nishanth Menon

RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy]

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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:
> >> -----Original Message-----
> >> From: Nishanth Menon [mailto:nm@ti.com]
> >> 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
> >>
> >>> -----Original Message-----
> >>> Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the
> >> following:
> >>> [...]
> >>>>>>>>> +	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?
> >>>>>>> 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.
> >>>>> 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)?
> >>>> 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.
> >>> 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.
> >> I take that back, we could do something like the following instead:
> >> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> >> omap2/pm34xx.c
> >> index ba3c0d6..74842f1 100644
> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> @@ -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)
> >> &&
> >> +                               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
code
> in
> > other email.
> yep. except, I think we dont need to do string compare. the following
> looks any better?
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> omap2/pm34xx.c
> index ba3c0d6..da12a56 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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)
> &&
> +                               pwrst->pwrdm == core_pwrdm) {
> +                       pwrst->next_state = PWRDM_POWER_RET;
> +                       pr_err("%s: cannot enable Core OFF due to
i583\n",
> +                               __func__);
		You probably need to throw up this warning only if state
== PWRDM_POWER_OFF. Otherwise this code looks fine to me.

Vishwa
> +               } else {
> +                       pwrst->next_state = state;
> +               }
> +               omap_set_pwrdm_state(pwrst->pwrdm, pwrst-
> >next_state);
>          }
>   }
>
> --
> Regards,
> Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
[...]
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>> omap2/pm34xx.c
>> index ba3c0d6..da12a56 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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)
>> &&
>> +                               pwrst->pwrdm == core_pwrdm) {
>> +                       pwrst->next_state = PWRDM_POWER_RET;
>> +                       pr_err("%s: cannot enable Core OFF due to
> i583\n",
>> +                               __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.

-- 
Regards,
Nishanth Menon

Re: [PATCH 3/5 v3] OMAP3630: PM: Erratum i608: disable RTA

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> 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 <khilman@deeprootsystems.com>
> Cc: Tony Lindgren <tony@atomide.com>
>
> [ambresh@ti.com: co-developer]
> Signed-off-by: Ambresh K <ambresh@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

[...]

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

Thanks,

Kevin

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
> [...]
>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>>> omap2/pm34xx.c
>>> index ba3c0d6..da12a56 100644
>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>> @@ -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)
>>> &&
>>> +                               pwrst->pwrdm == core_pwrdm) {
>>> +                       pwrst->next_state = PWRDM_POWER_RET;
>>> +                       pr_err("%s: cannot enable Core OFF due to
>> i583\n",
>>> +                               __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.

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

Re: [PATCH 0/5 v3] OMAP: idle path errata fixes

[Kevin Hilman]

Hi Nishanth,

Nishanth Menon <nm@ti.com> writes:

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

I had a couple small comments on individual patches.

In addition, in the next series, can you report the platforms it was
tested on, and how it was tested (retention idle/suspend, off
idle/suspend, CPUidle enabled?, etc.)

I tested this series (and Jean's cleanup patch) on 34xx/n900 with
retention idle & suspend and off idle & suspend with and without CPUidle
enabled.)

Also, when posting an updated series, can you update the version of all
patches in the series, even if they are unchanged?  This makes more
more explicit versioning, keeps things clearer in patchwork and avoids
problems with dumb mailers who thread by subject only.

Also, please Cc linux-arm-kernel when posting the next version. 

Thanks,

Kevin

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Kevin Hilman had written, on 12/13/2010 09:42 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
>> [...]
>>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>>>> omap2/pm34xx.c
>>>> index ba3c0d6..da12a56 100644
>>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>>> @@ -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)
>>>> &&
>>>> +                               pwrst->pwrdm == core_pwrdm) {
>>>> +                       pwrst->next_state = PWRDM_POWER_RET;
>>>> +                       pr_err("%s: cannot enable Core OFF due to
>>> i583\n",
>>>> +                               __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.
> 
> 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?

-- 
Regards,
Nishanth Menon

Re: [PATCH 0/5 v3] OMAP: idle path errata fixes

[Nishanth Menon]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

Kevin Hilman had written, on 12/13/2010 09:49 PM, the following:
> Hi Nishanth,
> 
> Nishanth Menon <nm@ti.com> writes:
> 
>> 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.
> 
> I had a couple small comments on individual patches.
> 
> In addition, in the next series, can you report the platforms it was
> tested on, and how it was tested (retention idle/suspend, off
> idle/suspend, CPUidle enabled?, etc.)
> 
> I tested this series (and Jean's cleanup patch) on 34xx/n900 with
> retention idle & suspend and off idle & suspend with and without CPUidle
> enabled.)
> 
> Also, when posting an updated series, can you update the version of all
> patches in the series, even if they are unchanged?  This makes more
> more explicit versioning, keeps things clearer in patchwork and avoids
> problems with dumb mailers who thread by subject only.
> 
> Also, please Cc linux-arm-kernel when posting the next version. 
ok will do. for reference, I wrote a script to make things easy for all 
- attached.

With the pm-fixes being merged to master, I tested today with latest 
kernel.org master commit: 0fcdcfb against omap2plus_defconfig without 
any of my patches applied:

Results:
SDP3630:
Log: http://pastebin.mozilla.org/889642
Summary:
SUSPEND:OFF test | PASS | OFF: 0->1| RET:0 ->0 (8 sec) 

SUSPEND:RET test | PASS | OFF: 1->1| RET:0 ->1 (8 sec) 

IDLE:OFF test | PASS | OFF: 1->24| RET:1 ->1 (21 sec) 

IDLE:RET test | PASS | OFF: 24->| RET:1 ->23 (21 sec)

SDP3430 (ES3.1):
Log: http://pastebin.mozilla.org/889643
Summary:
SUSPEND:OFF test | FAIL | OFF: 0->0| RET:0 ->0 (7 sec) 

SUSPEND:RET test | FAIL | OFF: 0->0| RET:0 ->0 (6 sec) 

IDLE:OFF test | FAIL | OFF: 0->0| RET:0 ->0 (21 sec) 

IDLE:RET test | FAIL | OFF: 0->0| RET:0 ->0 (21 sec)

Core never hits OFF/retention.

-- 
Regards,
Nishanth Menon

[-- Attachment #2: suspend-idle.sh --]
[-- Type: application/x-sh, Size: 3664 bytes --]

Re: [PATCH 3/5 v3] OMAP3630: PM: Erratum i608: disable RTA

[Nishanth Menon]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

Kevin Hilman had written, on 12/13/2010 09:28 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> 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 <khilman@deeprootsystems.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>>
>> [ambresh@ti.com: co-developer]
>> Signed-off-by: Ambresh K <ambresh@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
> 
> [...]
> 
>> @@ -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.

-- 
Regards,
Nishanth Menon

[-- Attachment #2: 0001-OMAP3630-PM-Erratum-i608-disable-RTA.patch --]
[-- Type: text/x-patch, Size: 6864 bytes --]

>From 5cf295fa3fe8d423323500fb8ddb49650f797edd Mon Sep 17 00:00:00 2001
From: Nishanth Menon <nm@ti.com>
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 <ambresh@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 1fa3294..7c415bc 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -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());
@@ -474,4 +477,12 @@ void omap3_control_restore_context(void)
 	omap_ctrl_writel(control_context.csi, OMAP343X_CONTROL_CSI);
 	return;
 }
+
+void omap3630_disable_rta(void)
+{
+	if (!cpu_is_omap36xx())
+		return;
+	omap_ctrl_writel(OMAP36XX_RTA_DISABLE, OMAP36XX_CONTROL_MEM_RTA_CTRL);
+}
+
 #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index b6c6b7c..13771cc 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -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
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0ec8a04..859c452 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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 =
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 3fbd1e5..cc3507b 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -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 !!! */
-- 
1.6.3.3

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 12/13/2010 09:42 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following:
>>> [...]
>>>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
>>>>> omap2/pm34xx.c
>>>>> index ba3c0d6..da12a56 100644
>>>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>>>> @@ -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)
>>>>> &&
>>>>> +                               pwrst->pwrdm == core_pwrdm) {
>>>>> +                       pwrst->next_state = PWRDM_POWER_RET;
>>>>> +                       pr_err("%s: cannot enable Core OFF due to
>>>> i583\n",
>>>>> +                               __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.
>>
>> 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?

Yes.  More precisely, dynamically set the 'valid' bit of C7 based on
this condition.

> 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

Re: [PATCH 3/5 v3] OMAP3630: PM: Erratum i608: disable RTA

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 12/13/2010 09:28 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> 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 <khilman@deeprootsystems.com>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>>
>>> [ambresh@ti.com: co-developer]
>>> Signed-off-by: Ambresh K <ambresh@ti.com>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>
>> [...]
>>
>>> @@ -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? 

Yes, but minor comment below...

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

The above is currently only in Paul's queue, which is included in my
pm-core, so you can base there for testing if you prefer.



> -- 
> Regards,
> Nishanth Menon
> From 5cf295fa3fe8d423323500fb8ddb49650f797edd Mon Sep 17 00:00:00 2001
> From: Nishanth Menon <nm@ti.com>
> 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 <ambresh@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  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(-)
>
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index 1fa3294..7c415bc 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -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());
> @@ -474,4 +477,12 @@ void omap3_control_restore_context(void)
>  	omap_ctrl_writel(control_context.csi, OMAP343X_CONTROL_CSI);
>  	return;
>  }
> +
> +void omap3630_disable_rta(void)

Let's call this omap3630_control_disable_rta() to match the naming that
Paul was using in his patch too.

Thanks,

Kevin

> +{
> +	if (!cpu_is_omap36xx())
> +		return;
> +	omap_ctrl_writel(OMAP36XX_RTA_DISABLE, OMAP36XX_CONTROL_MEM_RTA_CTRL);
> +}
> +
>  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index b6c6b7c..13771cc 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -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
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 0ec8a04..859c452 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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 =
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index 3fbd1e5..cc3507b 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -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 !!! */

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

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

>> 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).
Thx. it is clear now. let me see how to clean this up.

-- 
Regards,
Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
> Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
> 
>>> 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).
> 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    |
--------+-------+-------+-------+-------+--------+

-- 
Regards,
Nishanth Menon

[-- Attachment #2: 0001-OMAP3-PM-make-omap3_cpuidle_update_states-independen.patch --]
[-- Type: text/x-patch, Size: 3189 bytes --]

>From bd3d8decf922d7425b9bc9025c7709a9414ad380 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <nm@ti.com>
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 <nm@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 0d50b45..f80d3f6 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -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;
 		}
 	}
 }
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index aff39d0..3597591 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -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)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 21707c9..84ef71b 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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) {
-- 
1.6.3.3


[-- Attachment #3: 0002-OMAP3630-PM-Erratum-i583-disable-coreoff-if-ES1.2.patch --]
[-- Type: text/x-patch, Size: 2945 bytes --]

>From e085afb8523ca52e52b7a631c2b63e2bd6d91661 Mon Sep 17 00:00:00 2001
From: Eduardo Valentin <eduardo.valentin@nokia.com>
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.

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

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 84ef71b..ad60105 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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;
 	}
 }
 
-- 
1.6.3.3

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
>> Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
>>
>>>> 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).
>> 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


> 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    |
> --------+-------+-------+-------+-------+--------+
>
> -- 
> Regards,
> Nishanth Menon
> From bd3d8decf922d7425b9bc9025c7709a9414ad380 Mon Sep 17 00:00:00 2001
> From: Nishanth Menon <nm@ti.com>
> 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 <nm@ti.com>
> ---
>  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(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 0d50b45..f80d3f6 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -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;
>  		}
>  	}
>  }
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index aff39d0..3597591 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -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)
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 21707c9..84ef71b 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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) {
> -- 
> 1.6.3.3
>
> From e085afb8523ca52e52b7a631c2b63e2bd6d91661 Mon Sep 17 00:00:00 2001
> From: Eduardo Valentin <eduardo.valentin@nokia.com>
> 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.
>
> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code changed a bit]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |   25 ++++++++++++++++++++++---
>  1 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 84ef71b..ad60105 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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;
>  	}
>  }

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
>>> Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
>>>
>>>>> 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).
>>> 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?
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?

-- 
Regards,
Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
>>>> Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
>>>>
>>>>>> 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).
>>>> 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?
> 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?

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

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 81b0a90..92873b4 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -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;
+
+	/*
+	 * 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;
+	    
 }
 

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Nishanth Menon]

Kevin Hilman had written, on 12/17/2010 04:54 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:
>>> Nishanth Menon <nm@ti.com> writes:
>>>
>>>> Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
>>>>> Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
>>>>>
>>>>>>> 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).
>>>>> 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?
>> 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?
> 
> 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
> 
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 81b0a90..92873b4 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -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;
> +
> +	/*
> +	 * 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?


-- 
Regards,
Nishanth Menon

Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 12/17/2010 04:54 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:
>>>> Nishanth Menon <nm@ti.com> writes:
>>>>
>>>>> Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
>>>>>> Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
>>>>>>
>>>>>>>> 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).
>>>>>> 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?
>>> 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?
>>
>> 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
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
>> index 81b0a90..92873b4 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -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;
>> +
>> +	/*
>> +	 * 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?

You're right.

I see you've done both in the updated series.  Will have a closer look
shortly, but looks ok for now.

Kevin