OMAP3: OFF mode fixes

From: Richard Woodruff <[email protected]>

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 <[email protected]>

[[email protected]: ported rkw's proposal to 2.6.37-rc2]

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

Signed-off-by Richard Woodruff <[email protected]>

---

Side note: just dcache needs to be flushed based on inputs from TI internal team

 arch/arm/mach-omap2/sleep34xx.S |   79 ++++++--------------------------------

 1 files changed, 13 insertions(+), 66 deletions(-)

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

index 2fb205a..8f207b2 100644
@@ -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

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

-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

+        *  - 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

arch/arm/mach-omap2/sleep34xx.S +0 -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

Quoted 92 lines · Nishanth Menon

From: Richard Woodruff <[email protected]>

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 <[email protected]>

[[email protected]: ported rkw's proposal to 2.6.37-rc2]

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

Signed-off-by Richard Woodruff <[email protected]>

---

Side note: just dcache needs to be flushed based on inputs from TI internal team

 arch/arm/mach-omap2/sleep34xx.S |   79 ++++++--------------------------------

 1 files changed, 13 insertions(+), 66 deletions(-)

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

index 2fb205a..8f207b2 100644
@@ -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

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

-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

arch/arm/mach-omap2/sleep34xx.S clean_caches: · line 525

Quoted 10 lines

arch/arm/mach-omap2/sleep34xx.S +3 -7

-       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
+        *  - resue that code is better

Typo: 'reuse'

Quoted 7 lines

+        *  - 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

It is simpler and more efficient to use:
bl v7_flush_dcache_all

Quoted 112 lines · ext Jean Pihet

Quoted 92 lines · Nishanth Menon

From: Richard Woodruff <[email protected]>

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 <[email protected]>

[[email protected]: ported rkw's proposal to 2.6.37-rc2]

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

Signed-off-by Richard Woodruff <[email protected]>

---

Side note: just dcache needs to be flushed based on inputs from TI internal team

 arch/arm/mach-omap2/sleep34xx.S |   79 ++++++--------------------------------

 1 files changed, 13 insertions(+), 66 deletions(-)

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

index 2fb205a..8f207b2 100644
@@ -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

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

-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

arch/arm/mach-omap2/sleep34xx.S clean_caches: · line 525

Quoted 10 lines

arch/arm/mach-omap2/sleep34xx.S +3 -7

-       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
+        *  - resue that code is better

Typo: 'reuse'

Quoted 7 lines

+        *  - 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

Quoted 2 lines

It is simpler and more efficient to use:
            bl v7_flush_dcache_all

This doesn't work from SRAM though, because the linker will generate a PC relative branch which is wrong if the code is moved to SRAM at runtime. So the original version needs to stay :)

Cheers,

Peter.

From: Peter 'p2' De Schrijver <[email protected]>

Errata 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 errata. this is a support logic for detecting lockups and attempting to recover where possible and is known to provide stability in multiple platforms.

Signed-off-by Peter 'p2' De Schrijver <[email protected]>

---
 arch/arm/mach-omap2/sleep34xx.S |   52 +++++++++++++++++++++++++++++++++++---

 1 files changed, 47 insertions(+), 5 deletions(-)

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

index 8f207b2..5a4468f 100644
@@ -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:

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

+
+/* 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]

arch/arm/mach-omap2/sleep34xx.S +2 -1

-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

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

+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 */

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

+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:

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

@@ -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 !!! */

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

+kick_counter:
+       .word   0
+wait_dll_lock_counter:
+       .word   0

 ENTRY(omap34xx_cpu_suspend_sz)
       .word   . - omap34xx_cpu_suspend
--
1.6.3.3

Quoted 6 lines

From: Peter 'p2' De Schrijver <[email protected]>

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

Quoted 4 lines

IMPORTANT: this is not a complete workaround implementation as recommended by the silicon errata. this is a support logic for detecting lockups and attempting to recover where possible and is known to provide stability in multiple platforms.

How does this WA work when Core enters off mode? SRAM contents are lost when Core enters off. So how this code is copied to SRAM upon wakeup? Where is this code placed when Core entered off mode?

Vishwa

Quoted 105 lines

Signed-off-by Peter 'p2' De Schrijver <[email protected]>

---
 arch/arm/mach-omap2/sleep34xx.S |   52 +++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 5 deletions(-)

arch/arm/mach-omap2/sleep34xx.S +47 -6

index 8f207b2..5a4468f 100644
@@ -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

Sripathy, Vishwanath had written, on 11/24/2010 10:51 AM, the following:

Quoted 15 lines · Nishanth Menon

Quoted 7 lines

From: Peter 'p2' De Schrijver <[email protected]>

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

Quoted 5 lines

IMPORTANT: this is not a complete workaround implementation as recommended
by the silicon errata. this is a support logic for detecting lockups and attempting to recover where possible and is known to provide stability in multiple platforms.

Quoted 3 lines

How does this WA work when Core enters off mode? SRAM contents are lost when Core enters off. So how this code is copied to SRAM upon wakeup? Where is this code placed when Core entered off mode?

It depends on the location of wait_sdram_ok - ideally speaking yep, you are right the current wait_sdram_ok for OFF makes no sense at all as it wakes off the ddr. For retention though, it makes sense. after the discussion we had on Jean's series http://marc.info/?t=128532555200004&r=1&w=2, I am pretty sure that refactor that Jean is doing will help us clean this mess up.

Maybe we can refactor this as part of Jean's cleanups.

Quoted 109 lines

Vishwa

Quoted 107 lines

Signed-off-by Peter 'p2' De Schrijver <[email protected]>

---
 arch/arm/mach-omap2/sleep34xx.S |   52
+++++++++++++++++++++++++++++++++++---

 1 files changed, 47 insertions(+), 5 deletions(-)

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

index 8f207b2..5a4468f 100644
@@ -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

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

@@ -554,31 +555,67 @@ skip_l2_inval:
 
  /* Make sure SDRC accesses are ok */

 wait_sdrc_ok:

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

+
+/* DPLL3 must be locked before accessing the SDRC. Maybe the HW ensures

this. */
+       ldr     r4, cm_idlest_ckgen

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

+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]

arch/arm/mach-omap2/sleep34xx.S +2 -1

-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

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

+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 */

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

+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:

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

@@ -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 !!! */

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

+kick_counter:
+       .word   0
+wait_dll_lock_counter:
+       .word   0

 ENTRY(omap34xx_cpu_suspend_sz)
      .word   . - omap34xx_cpu_suspend
--

Quoted 3 lines

Quoted 3 lines

1.6.3.3

--
Regards,
Nishanth Menon

Nishant,

Quoted 6 lines · Nishanth Menon

From: Peter 'p2' De Schrijver <[email protected]>

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

Quoted 4 lines

IMPORTANT: this is not a complete workaround implementation as recommended by the silicon errata. this is a support logic for detecting lockups and attempting to recover where possible and is known to provide stability in multiple platforms.

How does this WA work when Core enters off mode? SRAM contents are lost when Core enters off. So how this code is copied to SRAM upon wakeup? Where is this code placed when Core entered off mode?

From: Eduardo Valentin <[email protected]>

Deny MPU idle before save secure ram and allow it after save secure RAM. We want to deny MPU going to low power state because, there is a short time window where a wakeup event would happen around the time the MPU is going to idle. Since the first thing ROM code does after WFI is to read INTCPS register, we could reach a situation where the INTCPS is in idle, but the read is sent to it. That would produce a data abord during the save of secure ram, which will hang the system. we need to prevent clock transitions as well during this timeframe.

[[email protected]: rebased to 2.6.37-rc2, used omap2_clkdm_deny_idle for clock prevention]

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

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

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

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

index 7877f74..f520b38 100644
@@ -194,15 +194,19 @@ int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *data)
 static void omap3_save_secure_ram_context(u32 target_mpu_state)
 {
 	if (!secure_ram_saved && omap_type() != OMAP2_DEVICE_TYPE_GP) {
+		struct clockdomain *clkd = mpu_pwrdm->pwrdm_clkdms[0];
+
 		/*
 		 * MPU next state must be set to POWER_ON temporarily,
 		 * otherwise the WFI executed inside the ROM code
 		 * will hang the system.
 		 */
 		pwrdm_set_next_pwrst(mpu_pwrdm, PWRDM_POWER_ON);
+		omap2_clkdm_deny_idle(clkd);
 		secure_ram_save_status = _omap_save_secure_sram((u32 *)
 				(omap3_secure_ram_storage));
 		pwrdm_set_next_pwrst(mpu_pwrdm, target_mpu_state);
+		omap2_clkdm_allow_idle(clkd);
 		if (!secure_copy_data.save_every_cycle)
 			secure_ram_saved = 1;
 	}

Quoted 10 lines · Nishanth Menon

From: Eduardo Valentin <[email protected]>

Deny MPU idle before save secure ram and allow it after save secure RAM. We want to deny MPU going to low power state because, there is a short time window where a wakeup event would happen around the time the MPU is going to idle. Since the first thing ROM code does after WFI is to read INTCPS register, we could reach a situation where the INTCPS is in idle, but the read is sent to it. That would produce a data abord during the save of secure ram, which will hang the system. we need to prevent clock transitions as well during this timeframe.

This changelog needs to be a bit clearer about exacly why MPU would be going to a low power state during a secure-mode call. IIUC, it's because the ROM code might do a WFI. Since it's completely non-intuitive (and broken, at least to me) that the ROM code would do a WFI, this should be stated explicitly in the changelog, and probably in the code too.

Also, this approach only solves this problem for this particular secure-mode call. Presumably there are other secure-mode calls that might WFI as well, and will have the same problem. As I'm not familiar with secure ROM or PPAs, so I don't know if this is true or not. If it is, then somethen more general should be done.

Also, do we care about other powerdomains (besides MPU) going idle during a secure mode call?

Because of those issues, some other proposals have been floated for this problem. In particular, explicitly setting some of the powerdomain next states (at least MPU & CORE) to ON when we're not in the normal idle path so that would also prevent this problem.

I guess we need some more details on which secure mode calls can trigger this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

Kevin

Quoted 32 lines

[[email protected]: rebased to 2.6.37-rc2, used omap2_clkdm_deny_idle for clock prevention]

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

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

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

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

index 7877f74..f520b38 100644
@@ -194,15 +194,19 @@ int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *data)
 static void omap3_save_secure_ram_context(u32 target_mpu_state)
 {
 	if (!secure_ram_saved && omap_type() != OMAP2_DEVICE_TYPE_GP) {
+		struct clockdomain *clkd = mpu_pwrdm->pwrdm_clkdms[0];
+
 		/*
 		 * MPU next state must be set to POWER_ON temporarily,
 		 * otherwise the WFI executed inside the ROM code
 		 * will hang the system.
 		 */
 		pwrdm_set_next_pwrst(mpu_pwrdm, PWRDM_POWER_ON);
+		omap2_clkdm_deny_idle(clkd);
 		secure_ram_save_status = _omap_save_secure_sram((u32 *)
 				(omap3_secure_ram_storage));
 		pwrdm_set_next_pwrst(mpu_pwrdm, target_mpu_state);
+		omap2_clkdm_allow_idle(clkd);
 		if (!secure_copy_data.save_every_cycle)
 			secure_ram_saved = 1;
 	}

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Friday, November 19, 2010 10:39 PM
To: Nishanth Menon
Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Quoted 3 lines · Nishanth Menon

From: Eduardo Valentin <[email protected]>

Deny MPU idle before save secure ram and allow it after save secure

Quoted 7 lines

We want to deny MPU going to low power state because, there is a short time window where a wakeup event would happen around the time the MPU is going to idle. Since the first thing ROM code does after WFI is to read INTCPS register, we could reach a situation where the INTCPS is in idle, but the read is sent to it. That would produce a data abord during the save of secure ram, which will hang the system. we need to prevent clock transitions as well during this timeframe.

This changelog needs to be a bit clearer about exacly why MPU would be going to a low power state during a secure-mode call. IIUC, it's because the ROM code might do a WFI. Since it's completely non-intuitive (and broken, at least to me) that the ROM code would do a WFI, this should be stated explicitly in the changelog, and probably in the code too.

Also, this approach only solves this problem for this particular secure-mode call. Presumably there are other secure-mode calls that might WFI as well, and will have the same problem. As I'm not familiar with secure ROM or PPAs, so I don't know if this is true or not. If it is, then somethen more general should be done.

Also, do we care about other powerdomains (besides MPU) going idle during a secure mode call?

Because of those issues, some other proposals have been floated for this problem. In particular, explicitly setting some of the powerdomain next states (at least MPU & CORE) to ON when we're not in the normal idle path so that would also prevent this problem.

I guess we need some more details on which secure mode calls can trigger this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

Quoted 4 lines

I guess we need some more details on which secure mode calls can trigger this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

On the related topic I posted a patch some time back.
http://www.spinics.net/lists/linux-omap/msg37907.html
I guess Kevin is referring to the above patch.

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Friday, November 19, 2010 10:55 PM
To: Santosh Shilimkar
Cc: Kevin Hilman; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Santosh Shilimkar had written, on 11/19/2010 11:18 AM, the following:
[..]

Quoted 1 line

Quoted 1 line

I guess we need some more details on which secure mode calls can

trigger

Quoted 6 lines

Quoted 3 lines

this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

On the related topic I posted a patch some time back.
http://www.spinics.net/lists/linux-omap/msg37907.html
I guess Kevin is referring to the above patch.

I believe the fix we are attempting here is for a specific scenario which IMHO is different from the issue solved in the link above.

Quoted 15 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Friday, November 19, 2010 10:55 PM
To: Santosh Shilimkar
Cc: Kevin Hilman; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Santosh Shilimkar had written, on 11/19/2010 11:18 AM, the following:
[..]

Quoted 1 line

Quoted 1 line

I guess we need some more details on which secure mode calls can

trigger

Quoted 3 lines

Quoted 3 lines

this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

Quoted 5 lines

Quoted 3 lines

On the related topic I posted a patch some time back.
http://www.spinics.net/lists/linux-omap/msg37907.html
I guess Kevin is referring to the above patch.

I believe the fix we are attempting here is for a specific scenario which IMHO is different from the issue solved in the link above.

It will also solve the above issue indirectly.

Santosh Shilimkar had written, on 11/19/2010 11:28 AM, the following:

Quoted 18 lines

Quoted 18 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Friday, November 19, 2010 10:55 PM
To: Santosh Shilimkar
Cc: Kevin Hilman; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Santosh Shilimkar had written, on 11/19/2010 11:18 AM, the following:
[..]

Quoted 1 line

Quoted 1 line

I guess we need some more details on which secure mode calls can

trigger

Quoted 6 lines

Quoted 3 lines

this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

On the related topic I posted a patch some time back.
http://www.spinics.net/lists/linux-omap/msg37907.html
I guess Kevin is referring to the above patch.

Quoted 4 lines

Quoted 2 lines

I believe the fix we are attempting here is for a specific scenario which IMHO is different from the issue solved in the link above.

It will also solve the above issue indirectly.

just my dumb brains :) agreed, good one that takes care of the power domain, agreed that pwrdm_set_next_pwrst(which is currently present in the omap3_save_secure_ram_context) would no longer be necessary

- so [1] should also have removed that -

This specific patch controls the clock domain from auto idling around the secure ram save. Apologies on the confusion - but if the [1] patch is fixing it, you can help me understand how it does it.

In addtion, the patch from Santosh needs to better describe what other problems it is solving, since it is clearly not fixing this particular secure mode entry. Therefore, there must be others that are also doing
WFI. That being said, instead of such a generic fix as is done by
Santosh's patch, maybe we need a common secure-mode entry point which does the necessary ROM code prep.

Quoted 4 lines

This specific patch controls the clock domain from auto idling around the secure ram save. Apologies on the confusion - but if the [1] patch is fixing it, you can help me understand how it does it.

Now that I understand the clockdomain part, I'm seeing the problem differently. (side note: A better written changelog could have avoided this confusion by being clear that it was *clockdomain* idle that was being added here and that it was in addition to the existing powerdomain settings.)

Technically, $SUBJECT patch could have replaced the set_next_pwrst with the clkdm_deny_idle. IOW, setting the pwrdm next state to is redundant if you clkdm_deny_idle.

I think this is the key to the confusion:

1) clkdm_deny_idle() implies the powerdomain stays on
2) setting powerdomain to on, does NOT imply clkdm_deny_idle()

Another way of saying it is that setting a powerdomain to on does not prevent it from going inactive. It only prevents retention or off-mode.

Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
[...]

Quoted 6 lines

In addtion, the patch from Santosh needs to better describe what other problems it is solving, since it is clearly not fixing this particular secure mode entry. Therefore, there must be others that are also doing
WFI. That being said, instead of such a generic fix as is done by
Santosh's patch, maybe we need a common secure-mode entry point which does the necessary ROM code prep.

Ideally speaking - save_secure_ram can hit latencies which are pretty bad.. eventually this logic should be moved outside the current boundaries in some manner - unfortunately, I cant at the moment think of a sane mechanism to do that given various proprietary and not-mainlined-but-public security drivers for OMAP3 out there :(. IMHO, the responsibility of secure storage should be with secure drivers, but, at the moment touching that topic is opening up a pandora's box :(

Quoted 15 lines

Quoted 4 lines

This specific patch controls the clock domain from auto idling around the secure ram save. Apologies on the confusion - but if the [1] patch is fixing it, you can help me understand how it does it.

Now that I understand the clockdomain part, I'm seeing the problem differently. (side note: A better written changelog could have avoided this confusion by being clear that it was *clockdomain* idle that was being added here and that it was in addition to the existing powerdomain settings.)

Technically, $SUBJECT patch could have replaced the set_next_pwrst with the clkdm_deny_idle. IOW, setting the pwrdm next state to is redundant if you clkdm_deny_idle.

Quoted 7 lines

I think this is the key to the confusion:

1) clkdm_deny_idle() implies the powerdomain stays on
2) setting powerdomain to on, does NOT imply clkdm_deny_idle()

Another way of saying it is that setting a powerdomain to on does not prevent it from going inactive. It only prevents retention or off-mode.

Agreed and I apologize for the confusion caused by the commit message -
will it be sufficient for the purpose of this series to change the commit log to better describe the patch? - I will leave the power domain control to Santosh's /Tero's series instead.

Is this acceptable option?

Quoted 8 lines · Nishanth Menon

Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
[...]

Quoted 6 lines

In addtion, the patch from Santosh needs to better describe what other problems it is solving, since it is clearly not fixing this particular secure mode entry. Therefore, there must be others that are also doing
WFI. That being said, instead of such a generic fix as is done by
Santosh's patch, maybe we need a common secure-mode entry point which does the necessary ROM code prep.

Quoted 8 lines

Ideally speaking - save_secure_ram can hit latencies which are pretty bad.. eventually this logic should be moved outside the current boundaries in some manner - unfortunately, I cant at the moment think of a sane mechanism to do that given various proprietary and not-mainlined-but-public security drivers for OMAP3 out there :(. IMHO, the responsibility of secure storage should be with secure drivers, but, at the moment touching that topic is opening up a pandora's box :(

Hmm, so the complexity and mess is pushed into the OMAP PM core...

/me no likey

Quoted 27 lines

Quoted 14 lines

Quoted 4 lines

This specific patch controls the clock domain from auto idling around the secure ram save. Apologies on the confusion - but if the [1] patch is fixing it, you can help me understand how it does it.

Now that I understand the clockdomain part, I'm seeing the problem differently. (side note: A better written changelog could have avoided this confusion by being clear that it was *clockdomain* idle that was being added here and that it was in addition to the existing powerdomain settings.)

Technically, $SUBJECT patch could have replaced the set_next_pwrst with the clkdm_deny_idle. IOW, setting the pwrdm next state to is redundant if you clkdm_deny_idle.

Quoted 7 lines

I think this is the key to the confusion:

1) clkdm_deny_idle() implies the powerdomain stays on
2) setting powerdomain to on, does NOT imply clkdm_deny_idle()

Another way of saying it is that setting a powerdomain to on does not prevent it from going inactive. It only prevents retention or off-mode.

Agreed and I apologize for the confusion caused by the commit message -
will it be sufficient for the purpose of this series to change the commit log to better describe the patch? - I will leave the power domain control to Santosh's /Tero's series instead.

Quoted 1 line

Is this acceptable option?

That is a minimum requirement, but...

Based on the rest of this series, I am not at all comfortable with managing this directly in the idle path. The latencies you mentioned above are only part of the reason. I have been trying to remove this

kind of device idle/PM management from the core idle path and I am not enthused about adding stuff back.

I would much rather see a separate, secure-mode driver, which for
starters only manages secure RAM. It doesn't have to manage all of
security stuff, but it will make a clearer (and cleaner)
separation between the idle path and secure RAM management. If
implemented as a driver, it could be much more intelligent about its save/restore and can behave just like any other driver that has to manage context save/restore. If the concern is about trying to have a
general purpose "secure driver", then just call it a secure RAM driver
or something to be clear it has a small, targetted scope.

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Saturday, November 20, 2010 2:45 AM
To: Kevin Hilman
Cc: Santosh Shilimkar; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Kevin Hilman had written, on 11/19/2010 03:06 PM, the following:

Quoted 5 lines

Quoted 3 lines · Nishanth Menon

Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
[...]

Quoted 1 line

In addtion, the patch from Santosh needs to better describe what

Quoted 1 line

Quoted 1 line

Quoted 1 line

problems it is solving, since it is clearly not fixing this

Quoted 1 line

Quoted 1 line

Quoted 1 line

secure mode entry. Therefore, there must be others that are also

doing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

WFI. That being said, instead of such a generic fix as is done by
Santosh's patch, maybe we need a common secure-mode entry point

Quoted 10 lines

Quoted 9 lines

Quoted 1 line

does the necessary ROM code prep.

Ideally speaking - save_secure_ram can hit latencies which are pretty bad.. eventually this logic should be moved outside the current boundaries in some manner - unfortunately, I cant at the moment think of a sane mechanism to do that given various proprietary and not-mainlined-but-public security drivers for OMAP3 out there :(. IMHO, the responsibility of secure storage should be with secure drivers, but, at the moment touching that topic is opening up a pandora's box :(

Quoted 3 lines

Hmm, so the complexity and mess is pushed into the OMAP PM core...

/me no likey

/me neither :(

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This specific patch controls the clock domain from auto idling

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the secure ram save. Apologies on the confusion - but if the [1]

patch

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is fixing it, you can help me understand how it does it.

Now that I understand the clockdomain part, I'm seeing the problem differently. (side note: A better written changelog could have

avoided

Quoted 1 line

Quoted 1 line

Quoted 1 line

this confusion by being clear that it was *clockdomain* idle that

Quoted 1 line

Quoted 1 line

Quoted 1 line

being added here and that it was in addition to the existing

powerdomain

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

settings.)

Technically, $SUBJECT patch could have replaced the set_next_pwrst

with

Quoted 1 line

Quoted 1 line

Quoted 1 line

the clkdm_deny_idle. IOW, setting the pwrdm next state to is

redundant

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

if you clkdm_deny_idle.

I think this is the key to the confusion:

1) clkdm_deny_idle() implies the powerdomain stays on
2) setting powerdomain to on, does NOT imply clkdm_deny_idle()

Another way of saying it is that setting a powerdomain to on does

Quoted 1 line

Quoted 1 line

Quoted 1 line

prevent it from going inactive. It only prevents retention or off-

mode.

Quoted 6 lines

Quoted 6 lines

Agreed and I apologize for the confusion caused by the commit message -
will it be sufficient for the purpose of this series to change the commit log to better describe the patch? - I will leave the power domain control to Santosh's /Tero's series instead.

Quoted 7 lines

Quoted 1 line

Is this acceptable option?

That is a minimum requirement, but...

Based on the rest of this series, I am not at all comfortable with managing this directly in the idle path. The latencies you mentioned above are only part of the reason. I have been trying to remove this

Keep in mind that the latency is incurred by the default settings in this series *only* for the very first off mode currently.

Quoted 12 lines

kind of device idle/PM management from the core idle path and I am not enthused about adding stuff back.

I would much rather see a separate, secure-mode driver, which for starters only manages secure RAM. It doesn't have to manage all of security stuff, but it will make a clearer (and cleaner) separation between the idle path and secure RAM management. If implemented as a driver, it could be much more intelligent about its save/restore and can behave just like any other driver that has to manage context save/restore. If the concern is about trying to have a general purpose "secure driver", then just call it a secure RAM driver or something to be clear it has a small, targetted scope.

There are few other issues with this approach. secure ram save by itself is just a function. it's trigger should ideally be not just one security driver IMHO - there is AES, SHAM, and other ones that will need to implement runtime pm, context save and restore hooks -> E.g. Dimitry's series[1] is trying to introduce an opensource security driver solution for OMAP - this is just a start - it will be some time before these drivers are ready and merged to mainline followed by power management enablement - do we want to keep omap3 broken while a fix is available till then?

Quoted 10 lines

-----Original Message-----
From: Nishanth Menon [mailto:[email protected]]
Sent: Friday, November 19, 2010 10:55 PM
To: Santosh Shilimkar
Cc: Kevin Hilman; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Santosh Shilimkar had written, on 11/19/2010 11:18 AM, the following:
[..]

Quoted 8 lines

Quoted 1 line

Quoted 1 line

I guess we need some more details on which secure mode calls can

trigger

Quoted 6 lines

Quoted 3 lines

this problem. If this is an isolated case, I'm OK with this fix. If it's more general, I'd like to see a more general fix.

On the related topic I posted a patch some time back.
http://www.spinics.net/lists/linux-omap/msg37907.html
I guess Kevin is referring to the above patch.

Quoted 2 lines

I believe the fix we are attempting here is for a specific scenario which IMHO is different from the issue solved in the link above.

It will also solve the above issue indirectly.

Quoted 3 lines

Quoted 2 lines

I believe the fix we are attempting here is for a specific scenario which IMHO is different from the issue solved in the link above.

It will also solve the above issue indirectly.

Yes, it indirectly fixes the issue solved by $SUBJECT patch, but what else does it fix?

This secure mode call is currently the only one I'm aware of that does a WFI. If there are others, then $SUBJECT patch is not enough.

If TI cannot tell us definitively if other secure-mode calls may use WFI, then we have to assume there are others, and $SUBJECT patch has to be NAK'd in favor of a more generic fix like the one from Santosh.

Kevin Hilman had written, on 11/19/2010 01:41 PM, the following:

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

I believe the fix we are attempting here is for a specific scenario which IMHO is different from the issue solved in the link above.

It will also solve the above issue indirectly.

Yes, it indirectly fixes the issue solved by $SUBJECT patch, but what else does it fix?

Please see [1] - highlights:
a) I dont seem to understand how clock domain idle is being denied by the patch
b) the mentioned patch[1] should have removed the pwr_domain control around the secure ram operation - which is the only weakness in that patch, for power domain control, I like the mentioned patch - but pwrdomain control is not what is being introduced here.

Quoted 1 line

Quoted 6 lines

This secure mode call is currently the only one I'm aware of that does a WFI. If there are others, then $SUBJECT patch is not enough.

If TI cannot tell us definitively if other secure-mode calls may use WFI, then we have to assume there are others, and $SUBJECT patch has to be NAK'd in favor of a more generic fix like the one from Santosh.

Thanks on the unfair beating IMHO,

but, I believe I confirmed this
here[2] - Quote: "After a long review, the impacted section is this logic alone." if you do have other specific SMIs in doubt(the ones we have verified to my knowledge are the ones around sleep34xx code), please list them out and I can get confirmation on behalf of TI if WFI is in use or not.

AFAIK, SMIs can be supported by ROMcode and by
PPA. is WFI is being used by PPA - there is no guarantee on the custom implementations.
For example:
CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE -> has a service ID which could be implemented by PPA -> TI cannot guarentee that implementations will never use WFI, from a kernel context, we'd say - dont use wfi - let the kernel control it, though the actual implementation is upto the developer of PPA.

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

Now, based on what you say below, it seems like there is no way to guarantee that SMIs don't do this, so I guess we have no choice but to protect them all.

Kevin Hilman had written, on 11/19/2010 02:55 PM, the following:
[...]

Quoted 3 lines

Now, based on what you say below, it seems like there is no way to guarantee that SMIs don't do this, so I guess we have no choice but to protect them all.

In that way, I do like the patch from Santosh - with the relevant changes we will need to incorporate. Do keep in mind that SMI is a secure service - In theory, there could be(and to my knowledge, are) custom secure services to do all kind of other things that are not power management related[1] -

The only area we can protect is the one in idle/ power paths.

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Saturday, November 20, 2010 2:40 AM
To: Nishanth Menon
Cc: Santosh Shilimkar; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

Quoted 4 lines · Nishanth Menon

Kevin Hilman had written, on 11/19/2010 02:55 PM, the following:
[...]

Quoted 2 lines

Now, based on what you say below, it seems like there is no way to guarantee that SMIs don't do this, so I guess we have no choice but

Quoted 6 lines

Quoted 1 line

protect them all.

In that way, I do like the patch from Santosh - with the relevant changes we will need to incorporate. Do keep in mind that SMI is a secure service - In theory, there could be(and to my knowledge, are) custom secure services to do all kind of other things that are not power management related[1] -

All the more reason that this secure mode code should be moved out of the core idle path into its own driver.

From: Eduardo Valentin <[email protected]>

We need to disable the autoidle bit from MPU INTC, otherwise INTC would get stall, and we would never come out of WFI. This must be done before save secure ram as well because save secure ram also does WFI.

This patch is just a addition to the current W/A we have for i540, in order to cover the WFI inside the save secure ram.

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

---
 arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--

 1 files changed, 8 insertions(+), 2 deletions(-)

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

index f520b38..c7e2db0 100644

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

+       omap3_intc_prepare_idle();
+
        /* CORE */
        if (core_next_state < PWRDM_POWER_ON) {
                omap_uart_prepare_idle(0);

@@ -433,8 +441,6 @@ void omap_sram_idle(void)
                }
        }
 
-       omap3_intc_prepare_idle();
-
        /*
        * On EMU/HS devices ROM code restores a SRDC value
        * from scratchpad which has automatic self refresh on timeout
--

1.6.3.3

Quoted 8 lines

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

index f520b38..c7e2db0 100644
@@ -422,6 +422,14 @@ void omap_sram_idle(void)
                                omap3_per_save_context();
        }
 

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

Quoted 6 lines

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

+       /*
+        * We need to disable the autoidle bit from MPU INTC,
+        * otherwise INTC would get stall, and we would never
+        * come out of WFI. This is done here because
+        * save secure ram also does WFI.
+        */

The comment should mention the errata ID i540, for easier readability and maintenance.

Jean Pihet wrote, on 11/19/2010 04:09 AM:
[...]

Quoted 9 lines

Quoted 9 lines

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

index f520b38..c7e2db0 100644
@@ -422,6 +422,14 @@ void omap_sram_idle(void)
                                omap3_per_save_context();
        }
 

Quoted 9 lines

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

Quoted 6 lines

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

+       /*
+        * We need to disable the autoidle bit from MPU INTC,
+        * otherwise INTC would get stall, and we would never
+        * come out of WFI. This is done here because
+        * save secure ram also does WFI.
+        */

The comment should mention the errata ID i540, for easier readability and maintenance.

Thanks for your review. I agree + I was given an offline feedback:

1 bug = Erratum
1+ bug = Errata

;) I should be fixing my grammar throughout all patches - so I believe they all become Erratum ID: etc.. will try to fix them as well in my v2.

--
Regards,
Nishanth Menon

From: Eduardo Valentin <[email protected]>

We need to disable the autoidle bit from MPU INTC, otherwise INTC would get stall, and we would never come out of WFI. This must be done before save secure ram as well because save secure ram also does WFI.

This patch is just a addition to the current W/A we have for i540, in order to cover the WFI inside the save secure ram.

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

---
 arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

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

index f520b38..c7e2db0 100644
@@ -422,6 +422,14 @@ void omap_sram_idle(void)
 				omap3_per_save_context();
 	}
 
+	/*
+	 * We need to disable the autoidle bit from MPU INTC,
+	 * otherwise INTC would get stall, and we would never
+	 * come out of WFI. This is done here because
+	 * save secure ram also does WFI.
+	 */
+	omap3_intc_prepare_idle();
+
 	/* CORE */
 	if (core_next_state < PWRDM_POWER_ON) {
 		omap_uart_prepare_idle(0);

@@ -433,8 +441,6 @@ void omap_sram_idle(void)
 		}
 	}
 
-	omap3_intc_prepare_idle();
-
 	/*
 	* On EMU/HS devices ROM code restores a SRDC value
 	* from scratchpad which has automatic self refresh on timeout

Quoted 7 lines · Nishanth Menon

From: Eduardo Valentin <[email protected]>

We need to disable the autoidle bit from MPU INTC, otherwise INTC would get stall, and we would never come out of WFI. This must be done before save secure ram as well because save secure ram also does WFI.

Quoted 2 lines

This patch is just a addition to the current W/A we have for i540, in order to cover the WFI inside the save secure ram.

This 'in addition' doesn't really make sense to me. This doesn't add anything to the current workaround, it just changes the order of operations.

Kevin

Quoted 33 lines

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

---
 arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

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

index f520b38..c7e2db0 100644
@@ -422,6 +422,14 @@ void omap_sram_idle(void)
 				omap3_per_save_context();
 	}
 
+	/*
+	 * We need to disable the autoidle bit from MPU INTC,
+	 * otherwise INTC would get stall, and we would never
+	 * come out of WFI. This is done here because
+	 * save secure ram also does WFI.
+	 */
+	omap3_intc_prepare_idle();
+
 	/* CORE */
 	if (core_next_state < PWRDM_POWER_ON) {
 		omap_uart_prepare_idle(0);

@@ -433,8 +441,6 @@ void omap_sram_idle(void)
 		}
 	}
 
-	omap3_intc_prepare_idle();
-
 	/*
 	* On EMU/HS devices ROM code restores a SRDC value
 	* from scratchpad which has automatic self refresh on timeout

Kevin Hilman had written, on 11/19/2010 11:15 AM, the following:

Quoted 9 lines

Quoted 7 lines · Nishanth Menon

From: Eduardo Valentin <[email protected]>

We need to disable the autoidle bit from MPU INTC, otherwise INTC would get stall, and we would never come out of WFI. This must be done before save secure ram as well because save secure ram also does WFI.

Quoted 6 lines

Quoted 2 lines

This patch is just a addition to the current W/A we have for i540, in order to cover the WFI inside the save secure ram.

This 'in addition' doesn't really make sense to me. This doesn't add anything to the current workaround, it just changes the order of operations.

yes - the "in addition" part is as follows:

ideally speaking you just need the omap3_intc_prepare_idle just before wfi. we "in addition" need to protect the save_secure_ram call as well because ROM code's WFI hits the same window of the bug that we do in the kernel. I believe the "in addition" was meant to state that we have to protect the logic of romcode as well..

Quoted 35 lines

Kevin

Quoted 31 lines

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

---
 arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

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

index f520b38..c7e2db0 100644
@@ -422,6 +422,14 @@ void omap_sram_idle(void)
 				omap3_per_save_context();
 	}
 +	/*
+	 * We need to disable the autoidle bit from MPU INTC,
+	 * otherwise INTC would get stall, and we would never
+	 * come out of WFI. This is done here because
+	 * save secure ram also does WFI.
+	 */
+	omap3_intc_prepare_idle();
+
 	/* CORE */
 	if (core_next_state < PWRDM_POWER_ON) {
 		omap_uart_prepare_idle(0);

@@ -433,8 +441,6 @@ void omap_sram_idle(void)
 		}
 	}
 -	omap3_intc_prepare_idle();
-
 	/*
 	* On EMU/HS devices ROM code restores a SRDC value
 	* from scratchpad which has automatic self refresh on timeout

Quoted 15 lines · Nishanth Menon

Kevin Hilman had written, on 11/19/2010 11:15 AM, the following:

Quoted 9 lines

Quoted 7 lines · Nishanth Menon

From: Eduardo Valentin <[email protected]>

We need to disable the autoidle bit from MPU INTC, otherwise INTC would get stall, and we would never come out of WFI. This must be done before save secure ram as well because save secure ram also does WFI.

Quoted 5 lines

Quoted 2 lines

This patch is just a addition to the current W/A we have for i540, in order to cover the WFI inside the save secure ram.

This 'in addition' doesn't really make sense to me. This doesn't add anything to the current workaround, it just changes the order of operations.

Quoted 7 lines

yes - the "in addition" part is as follows:

ideally speaking you just need the omap3_intc_prepare_idle just before wfi. we "in addition" need to protect the save_secure_ram call as well because ROM code's WFI hits the same window of the bug that we do in the kernel. I believe the "in addition" was meant to state that we have to protect the logic of romcode as well..

OK, then it needs to be rephrased. It doesn't need to (re)summarize the i540 workaround, since it's already in the kernel. You could just state:

The existing workaround for erratum i540 (disable MPU INTC auto-idle) needs to be done before saving secure RAM since secure-mode call can also do WFI.

From: Archana Sriram <[email protected]>

Errata Id:i582

PER Domain reset issue after Domain-OFF/OSWR Wakeup.

Issue:
When Peripheral Power Domain (PER-PD) is woken up from OFF / OSWR state while Core Power Domain (CORE-PD) is kept active, write or read access to some internal memory (e.g., UART3 FIFO) does not work correctly.

Workaround :
* Prevent PER from going off when CORE has not gone to off.

* When both CORE-PD and PER-PD goes into OSWR/OFF, PER-PD should be brought to active before CORE-PD.This can be done by configuring a wakeup dependency so that CORE-PD and PER-PD will wake up at the same time.

Acked-by Tero Kristo <[email protected]>

Tested-by Eduardo Valentin <[email protected]>

Tested-by Westerberg Mika <[email protected]>

[[email protected]: initial code and suggestions]

Signed-off-by Vishwanath BS <[email protected]>

[[email protected]: forward ported to 2.6.37-rc2 and suggestions]

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

Signed-off-by Archana Sriram <[email protected]>

---
 arch/arm/mach-omap2/pm34xx.c             |   41 +++++++++++++++-
 arch/arm/mach-omap2/serial.c             |   80 ++++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/serial.h |    4 ++
 3 files changed, 124 insertions(+), 1 deletions(-)

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

index c7e2db0..218d0b0 100644
@@ -84,6 +84,10 @@ static struct powerdomain *cam_pwrdm;
 static int secure_ram_save_status;
 static int secure_ram_saved;
 
+#define PER_WAKEUP_ERRATA_i582	(1 << 0)
+static u16 pm34xx_errata;
+#define IS_PM34XX_ERRATA(id)	(pm34xx_errata & (id))
+
 static inline void omap3_per_save_context(void)
 {
 	omap_gpio_save_context();

@@ -371,7 +375,8 @@ void omap_sram_idle(void)
 	int mpu_next_state = PWRDM_POWER_ON;
 	int per_next_state = PWRDM_POWER_ON;
 	int core_next_state = PWRDM_POWER_ON;
-	int core_prev_state, per_prev_state;
+	int core_prev_state = PWRDM_POWER_ON;
+	int per_prev_state = PWRDM_POWER_ON;
 	u32 sdrc_pwr = 0;
 
 	if (!_omap_sram_idle)

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

+			/*
+			 * We dont seem to have a real recovery other than reset
+			 * Errata i582:Alternative available here is to do a
+			 * reboot OR go to per off/core off, we will just print
+			 * and cause uart to be in an unstable state and
+			 * continue on till we hit the next off transition.
+			 * Reboot of the device due to this corner case is
+			 * undesirable.
+			 */
+			if (omap_uart_per_errata())
+				pr_err("%s: PER UART hit with Errata i582 "
+					"Corner case.\n", __func__);
+		}
 	}
 
 	/* Disable IO-PAD and IO-CHAIN wakeup */

@@ -1021,15 +1043,27 @@ void omap_push_sram_idle(void)
 				save_secure_ram_context_sz);
 }
 
+static void pm_errata_configure(void)
+{
+	if (cpu_is_omap34xx()) {
+		pm34xx_errata |= PER_WAKEUP_ERRATA_i582;
+		if (cpu_is_omap3630() && (omap_rev() > OMAP3630_REV_ES1_1))
+			pm34xx_errata &= ~PER_WAKEUP_ERRATA_i582;
+	}
+}
+
 static int __init omap3_pm_init(void)
 {
 	struct power_state *pwrst, *tmp;
 	struct clockdomain *neon_clkdm, *per_clkdm, *mpu_clkdm, *core_clkdm;
+	struct clockdomain *wkup_clkdm;
 	int ret;
 
 	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

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

+	if (IS_PM34XX_ERRATA(PER_WAKEUP_ERRATA_i582) && cpu_is_omap34xx())
+		clkdm_add_wkdep(per_clkdm, wkup_clkdm);
+
 	clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
 
 	omap3_save_scratchpad_contents();

arch/arm/mach-omap2/serial.c +80 -0

index becf0e3..43c2ec4 100644
@@ -273,6 +273,86 @@ static void omap_uart_restore_context(struct omap_uart_state *uart)
 		/* UART 16x mode */
 		serial_write_reg(uart, UART_OMAP_MDR1, 0x00);
 }
+
+static inline int _is_per_uart(struct omap_uart_state *uart)
+{
+	if (cpu_is_omap34xx() && (uart->num == 2 || uart->num == 3))
+		return 1;
+	return 0;
+}
+
+int omap_uart_check_per_uarts_used(void)
+{
+	struct omap_uart_state *uart;
+
+	list_for_each_entry(uart, &uart_list, node) {
+		if (_is_per_uart(uart))
+			return 1;
+	}
+	return 0;
+}
+
+/*
+ * Errata i582 affects PER UARTS..Loop back test is done to
+ * check the UART state when the corner case is encountered
+ */
+static int omap_uart_loopback_test(struct omap_uart_state *uart)
+{
+	u8 loopbk_rhr = 0;
+
+	omap_uart_save_context(uart);
+	serial_write_reg(uart, UART_OMAP_MDR1, 0x7);
+	serial_write_reg(uart, UART_LCR, 0xBF); /* Config B mode */
+	serial_write_reg(uart, UART_DLL, uart->dll);
+	serial_write_reg(uart, UART_DLM, uart->dlh);
+	serial_write_reg(uart, UART_LCR, 0x0); /* Operational mode */
+	/* configure uart3 in UART mode */
+	serial_write_reg(uart, UART_OMAP_MDR1, 0x00); /* UART 16x mode */
+	serial_write_reg(uart, UART_LCR, 0x80);
+	/* Enable internal loop back mode by setting MCR_REG[4].LOOPBACK_EN */
+	serial_write_reg(uart, UART_MCR, 0x10);
+
+	/* write to THR,read RHR and compare */
+	/* Tx output is internally looped back to Rx input in loop back mode */
+	serial_write_reg(uart, UART_LCR_DLAB, 0x00);
+	/* Enables write to THR and read from RHR */
+	serial_write_reg(uart, UART_TX, 0xCC); /* writing data to THR */
+	/* reading data from RHR */
+	loopbk_rhr = (serial_read_reg(uart, UART_RX) & 0xFF);
+	if (loopbk_rhr == 0xCC) {
+		/* compare passes,try comparing with different data */
+		serial_write_reg(uart, UART_TX, 0x69);
+		loopbk_rhr = (serial_read_reg(uart, UART_RX) & 0xFF);
+		if (loopbk_rhr == 0x69) {
+			/* compare passes,reset UART3 and re-configure module */
+			omap_uart_reset(uart);
+			omap_uart_restore_context(uart);
+			return 0;
+		}
+	} else {	/* requires warm reset */
+		return -ENODEV;
+	}
+	return 0;
+}
+
+int omap_uart_per_errata(void)
+{
+	struct omap_uart_state *uart;
+
+	/* For all initialised UART modules that are in PER domain
+	 * do loopback test
+	 */
+	list_for_each_entry(uart, &uart_list, node) {
+		if (_is_per_uart(uart)) {
+			if (omap_uart_loopback_test(uart))
+				return -ENODEV;
+			else
+				return 0;
+		}
+	}
+	return 0;
+}
+
 #else
 static inline void omap_uart_save_context(struct omap_uart_state *uart) {}
 static inline void omap_uart_restore_context(struct omap_uart_state *uart) {}

arch/arm/plat-omap/include/plat/serial.h +0 -0

index 19145f5..81169b2 100644

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

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

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

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

---
 arch/arm/mach-omap2/control.c   |    5 ++++- arch/arm/mach-omap2/control.h   |    5 +++++ arch/arm/mach-omap2/pm34xx.c    |   12 ++++++++++++ arch/arm/mach-omap2/sleep34xx.S |   25 +++++++++++++++++++++++++

 4 files changed, 46 insertions(+), 1 deletions(-)

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

index 5a4468f..7259541 100644

       bne     copy_to_sram
       ldr     r1, sram_base
       blx     r1
+

+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*/

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

@@ -650,6 +673,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

Quoted 4 lines

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

index 5a4468f..7259541 100644

...

arch/arm/mach-omap2/sleep34xx.S copy_to_sram:

Quoted 2 lines

/* Function call to get the restore pointer for for ES3 to resume from OFF */

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

@@ -246,6 +256,19 @@ copy_to_sram:

This code is from 'restore_es3' which ends with a pass-through to 'restore:'...

arch/arm/mach-omap2/sleep34xx.S copy_to_sram: · line 259

Quoted 4 lines

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

        bne     copy_to_sram
        ldr     r1, sram_base
        blx     r1
+

.. and so adding restore_3630 here has the result that is also run by 34xx>= ES3.x.
Is that a concern? Was it tested on 34xx>= ES3.x?

Quoted 25 lines

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

+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 +673,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 !!! */

Quoted 4 lines

--
1.6.3.3

A more general remark: since pm34xx.c and sleep34xx.S now contain 34xx and 36xx specific code, should those (and possibly other files) be renamed to e.g. pm3xxx.c and sleep3xxx.S?

From: Eduardo Valentin <[email protected]>

Limitation i583: Self_Refresh Exit issue after OFF mode

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

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

This patch takes option #3 as recommended by the Silicon errata:
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.

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

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

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

---
 arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++

 1 files changed, 14 insertions(+), 0 deletions(-)

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

index 0102d60..1890e49 100644

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

  static u16 pm34xx_errata;
  #define IS_PM34XX_ERRATA(id)   (pm34xx_errata & (id))
 

@@ -437,6 +438,17 @@ void omap_sram_idle(void)
        omap3_intc_prepare_idle();
 
        /* CORE */
+       /*
+        * Errata 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_ERRATA(SDRC_WAKEUP_ERRATA_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);

@@ -1050,6 +1062,8 @@ static void pm_errata_configure(void)
                pm34xx_errata |= PER_WAKEUP_ERRATA_i582;
                if (cpu_is_omap3630() && (omap_rev() > OMAP3630_REV_ES1_1))
                        pm34xx_errata &= ~PER_WAKEUP_ERRATA_i582;
+               if (cpu_is_omap3630() && (omap_rev() < OMAP3630_REV_ES1_2))
+                       pm34xx_errata |= SDRC_WAKEUP_ERRATA_i583;
                if (cpu_is_omap3630())
                        pm34xx_errata |= RTA_ERRATA_i608;
                /* Enable the l2 cache toggling in sleep logic */
--

1.6.3.3

arch/arm/mach-omap2/pm34xx.c static int secure_ram_saved; · line 89

Quoted 9 lines

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

index 0102d60..1890e49 100644
@@ -86,6 +86,7 @@ static int secure_ram_saved;
 
  #define PER_WAKEUP_ERRATA_i582 (1<<  0)
  #define RTA_ERRATA_i608                (1<<  1)
+#define SDRC_WAKEUP_ERRATA_i583        (1<<  0)

This value should be (1<< 2).

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP & EMU (Es1.1, ES1.2)
SDP3430 - GP & EMU (ES3.1)

These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent.

Request for testing this series for comparison between master and this series requested for additional platforms where available.

Archana Sriram (1):
 OMAP3: PM: Errata i582: per domain reset issue: uart

Eduardo Valentin (3):
 OMAP3: PM: Deny MPU idle while saving secure RAM
 OMAP3: PM: Apply errata i540 before save secure ram
 OMAP3630: PM: Errata i583: disable coreoff if < ES1.2

Nishanth Menon (3):
 OMAP3: PM: make secure ram save size configurable
 OMAP3: PM: Fix secure save size for OMAP3
 OMAP3630: PM: Errata i608: disable RTA

Peter 'p2' De Schrijver (2):
 OMAP3: PM: Errata i581 suppport: 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

Tero Kristo (3):
 OMAP3: PM: Save secure RAM context before entering WFI
 OMAP3: PM: optional save secure RAM context every core off cycle
 OMAP3: PM: allocate secure RAM context memory from low-mem

 arch/arm/mach-omap2/control.c            |    5 +- arch/arm/mach-omap2/control.h            |    5 + arch/arm/mach-omap2/io.c                 |   11 ++ arch/arm/mach-omap2/pm.h                 |   40 +++++++ arch/arm/mach-omap2/pm34xx.c             |  184 ++++++++++++++++++++++++----- arch/arm/mach-omap2/serial.c             |   80 +++++++++++++ arch/arm/mach-omap2/sleep34xx.S          |  187 ++++++++++++++++++----------- arch/arm/plat-omap/include/plat/serial.h |    4 +

 8 files changed, 412 insertions(+), 104 deletions(-)

Regards,
Nishanth Menon

Hi,

Quoted 57 lines · Nishanth Menon

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode on OMAP3630,
CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP& EMU (Es1.1, ES1.2)
SDP3430 - GP& EMU (ES3.1)

These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent.

Request for testing this series for comparison between master and this series requested for additional platforms where available.

Archana Sriram (1):
OMAP3: PM: Errata i582: per domain reset issue: uart

Eduardo Valentin (3):
OMAP3: PM: Deny MPU idle while saving secure RAM
OMAP3: PM: Apply errata i540 before save secure ram
OMAP3630: PM: Errata i583: disable coreoff if< ES1.2

Nishanth Menon (3):
OMAP3: PM: make secure ram save size configurable
OMAP3: PM: Fix secure save size for OMAP3
OMAP3630: PM: Errata i608: disable RTA

Peter 'p2' De Schrijver (2):
OMAP3: PM: Errata i581 suppport: 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

Tero Kristo (3):
OMAP3: PM: Save secure RAM context before entering WFI
OMAP3: PM: optional save secure RAM context every core off cycle
OMAP3: PM: allocate secure RAM context memory from low-mem

  arch/arm/mach-omap2/control.c            |    5 +-
  arch/arm/mach-omap2/control.h            |    5 +
  arch/arm/mach-omap2/io.c                 |   11 ++
  arch/arm/mach-omap2/pm.h                 |   40 +++++++
  arch/arm/mach-omap2/pm34xx.c             |  184 ++++++++++++++++++++++++-----
  arch/arm/mach-omap2/serial.c             |   80 +++++++++++++
  arch/arm/mach-omap2/sleep34xx.S          |  187 ++++++++++++++++++-----------
  arch/arm/plat-omap/include/plat/serial.h |    4 +
  8 files changed, 412 insertions(+), 104 deletions(-)

Quoted 3 lines

Regards,
Nishanth Menon

This is a nice set of changes, thanks! I sent my remarks.

After having reviewed the patches I think the best is to merge those in, then apply the clean-up and possibly the move to DDR patches on top of that.

What do you think?

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode on OMAP3630,
CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP & EMU (Es1.1, ES1.2)
SDP3430 - GP & EMU (ES3.1)

These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent.

Request for testing this series for comparison between master and this series requested for additional platforms where available.

Archana Sriram (1):
OMAP3: PM: Errata i582: per domain reset issue: uart

Eduardo Valentin (3):
OMAP3: PM: Deny MPU idle while saving secure RAM
OMAP3: PM: Apply errata i540 before save secure ram
OMAP3630: PM: Errata i583: disable coreoff if < ES1.2

Nishanth Menon (3):
OMAP3: PM: make secure ram save size configurable
OMAP3: PM: Fix secure save size for OMAP3
OMAP3630: PM: Errata i608: disable RTA

Peter 'p2' De Schrijver (2):
OMAP3: PM: Errata i581 suppport: 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

Tero Kristo (3):
OMAP3: PM: Save secure RAM context before entering WFI
OMAP3: PM: optional save secure RAM context every core off cycle
OMAP3: PM: allocate secure RAM context memory from low-mem

 arch/arm/mach-omap2/control.c            |    5 +-
 arch/arm/mach-omap2/control.h            |    5 +
 arch/arm/mach-omap2/io.c                 |   11 ++
 arch/arm/mach-omap2/pm.h                 |   40 +++++++
 arch/arm/mach-omap2/pm34xx.c             |  184 ++++++++++++++++++++++++-----
 arch/arm/mach-omap2/serial.c             |   80 +++++++++++++
 arch/arm/mach-omap2/sleep34xx.S          |  187 ++++++++++++++++++-----------
 arch/arm/plat-omap/include/plat/serial.h |    4 +
 8 files changed, 412 insertions(+), 104 deletions(-)

Regards,
Nishanth Menon

Quoted 2 lines

Request for testing this series for comparison between master and this series requested for additional platforms where available.

I haven't yet been through the entire series, but some general comments to share before the weekend:

The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file
(secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call
into secure code as needed.

Even better (and already suggested in some comments on patch 8), now that there are board-configurable knobs, this should be set up as a very simple platform driver/device so boards can pass configuration in a standard way instead of having new APIs for use by board code to set configuration settings.

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Nishanth Menon
Sent: Saturday, November 20, 2010 3:07 AM
To: Kevin Hilman
Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

Kevin Hilman had written, on 11/19/2010 03:20 PM, the following:

Quoted 1 line

Quoted 1 line

Request for testing this series for comparison between master and

Quoted 3 lines

Quoted 1 line

series requested for additional platforms where available.

I haven't yet been through the entire series, but some general

Quoted 1 line

to share before the weekend:

thanks for comments so far. I will wait for the complete series to be reviewed before reposting a v2.

Quoted 5 lines

The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file
(secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call
into secure code as needed.

Kevin Hilman had written, on 11/19/2010 03:20 PM, the following:

Quoted 5 lines

Quoted 2 lines

Request for testing this series for comparison between master and this series requested for additional platforms where available.

I haven't yet been through the entire series, but some general comments to share before the weekend:

thanks for comments so far. I will wait for the complete series to be reviewed before reposting a v2.

Quoted 5 lines

The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file
(secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call
into secure code as needed.

IMHO - we should do that set of cleanups as part of Jean's patch series where we transition to sdram where possible - that will allow us to have C code replacement for sleep34xx.S and optimize better in conjunction with sram_idle function and helpers.

Quoted 5 lines

Even better (and already suggested in some comments on patch 8), now that there are board-configurable knobs, this should be set up as a very simple platform driver/device so boards can pass configuration in a standard way instead of having new APIs for use by board code to set configuration settings.

in this specific context, having a platform data device is more of an overkill - 90% of the HS platforms (just a guess based on the limited devices I know of and is not a hard statistics) use the TI defaults - we do have exceptional customers who do their own PPA - and having a device-driver model IMHO is an overkill for that flexibility.

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode on OMAP3630,
CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP & EMU (Es1.1, ES1.2)
SDP3430 - GP & EMU (ES3.1)

These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent.

Request for testing this series for comparison between master and this series requested for additional platforms where available.

Archana Sriram (1):
OMAP3: PM: Errata i582: per domain reset issue: uart

Eduardo Valentin (3):
OMAP3: PM: Deny MPU idle while saving secure RAM
OMAP3: PM: Apply errata i540 before save secure ram
OMAP3630: PM: Errata i583: disable coreoff if < ES1.2

Nishanth Menon (3):
OMAP3: PM: make secure ram save size configurable
OMAP3: PM: Fix secure save size for OMAP3
OMAP3630: PM: Errata i608: disable RTA

Peter 'p2' De Schrijver (2):
OMAP3: PM: Errata i581 suppport: 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

Tero Kristo (3):
OMAP3: PM: Save secure RAM context before entering WFI
OMAP3: PM: optional save secure RAM context every core off cycle
OMAP3: PM: allocate secure RAM context memory from low-mem

 arch/arm/mach-omap2/control.c            |    5 +-
 arch/arm/mach-omap2/control.h            |    5 +
 arch/arm/mach-omap2/io.c                 |   11 ++
 arch/arm/mach-omap2/pm.h                 |   40 +++++++
 arch/arm/mach-omap2/pm34xx.c             |  184 ++++++++++++++++++++++++-----
 arch/arm/mach-omap2/serial.c             |   80 +++++++++++++
 arch/arm/mach-omap2/sleep34xx.S          |  187 ++++++++++++++++++-----------
 arch/arm/plat-omap/include/plat/serial.h |    4 +
 8 files changed, 412 insertions(+), 104 deletions(-)

Regards,
Nishanth Menon

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Tuesday, November 23, 2010 12:46 AM
To: Nishanth Menon
Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 1 line · Nishanth Menon

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS

Quoted 3 lines

for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode on

OMAP3630,

Quoted 2 lines

CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the

correct

Quoted 10 lines

service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP & EMU (Es1.1, ES1.2)
SDP3430 - GP & EMU (ES3.1)

These are fixes for corner case bugs seen, so tests of off and ret

with

Quoted 1 line

wakeup timer - behavior between 2.6.37-rc2 before and after applying

patch

Quoted 4 lines

seen consistent.

Request for testing this series for comparison between master and this series requested for additional platforms where available.

After some more thought and review, here's what I think should be the approach moving this forward:

This can be broken up into 3 independent series as follows:

1) fix for UART erratum (patch 10)
2) fixes for idle path errata (patches 1, 2, 11, 12, 13)
3) secure ram save path (the rest)

For (3), I'd like to see the secure ram management moved out of the PM core, and into it's own library/driver. Strictly speaking, context
save/restore for secure ram is not a function of the PM idle core. As
with every other device, context save/restore is the responsibility of the driver(s) using that device.

For secure RAM, the restore is handled by ROM code, but the save should be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved out into a library/driver which can be called by secure drivers in their runtime PM hooks.

Quoted 11 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Tuesday, November 23, 2010 12:46 AM
To: Nishanth Menon
Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 1 line · Nishanth Menon

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS

devices

Quoted 17 lines

Quoted 3 lines

for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode on

OMAP3630,

Quoted 2 lines

CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the

correct

Quoted 10 lines

service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP & EMU (Es1.1, ES1.2)
SDP3430 - GP & EMU (ES3.1)

These are fixes for corner case bugs seen, so tests of off and ret

done

Quoted 23 lines

with

Quoted 1 line

wakeup timer - behavior between 2.6.37-rc2 before and after applying

patch

Quoted 4 lines

seen consistent.

Request for testing this series for comparison between master and this series requested for additional platforms where available.

After some more thought and review, here's what I think should be the approach moving this forward:

This can be broken up into 3 independent series as follows:

1) fix for UART erratum (patch 10)
2) fixes for idle path errata (patches 1, 2, 11, 12, 13)
3) secure ram save path (the rest)

For (3), I'd like to see the secure ram management moved out of the PM core, and into it's own library/driver. Strictly speaking, context
save/restore for secure ram is not a function of the PM idle core. As
with every other device, context save/restore is the responsibility of the driver(s) using that device.

Quoted 8 lines

For secure RAM, the restore is handled by ROM code, but the save should be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved out into a library/driver which can be called by secure drivers in their runtime PM hooks.

I agree with you Kevin here except one point. The secure RAM contents are not just secure driver data but the ROM code infrastructure as well. And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code.
Ofcourse this not related to GP device because we never enter Secure world again after the boot-up.
So moving this code to a separate file is fine but it still related to CPU.

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Wednesday, November 24, 2010 2:06 AM
To: Santosh Shilimkar
Cc: Nishanth Menon; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 15 lines · Santosh Shilimkar

Quoted 11 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Tuesday, November 23, 2010 12:46 AM
To: Nishanth Menon
Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 1 line · Nishanth Menon

Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS

devices

Quoted 3 lines

Quoted 3 lines

for OFF mode logic.

It is important to note - for proper functionality of HS OFF mode

Quoted 13 lines

Quoted 13 lines

OMAP3630,

Quoted 2 lines

CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the

correct

Quoted 9 lines

service that the security PPA supports on the platform.

Based on kernel.org 2.6.37-rc2 tag

Smoke tested on:
SDP3630 -GP
Zoom3 (3630): GP & EMU (Es1.1, ES1.2)
SDP3430 - GP & EMU (ES3.1)

Quoted 4 lines

Quoted 1 line

Quoted 1 line

These are fixes for corner case bugs seen, so tests of off and ret

done

Quoted 2 lines

with

Quoted 1 line

wakeup timer - behavior between 2.6.37-rc2 before and after

Quoted 4 lines

Quoted 4 lines

patch

Quoted 3 lines

seen consistent.

Request for testing this series for comparison between master and

this

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

series requested for additional platforms where available.

After some more thought and review, here's what I think should be the approach moving this forward:

This can be broken up into 3 independent series as follows:

1) fix for UART erratum (patch 10)
2) fixes for idle path errata (patches 1, 2, 11, 12, 13)
3) secure ram save path (the rest)

For (3), I'd like to see the secure ram management moved out of the

Quoted 2 lines

Quoted 2 lines

core, and into it's own library/driver. Strictly speaking, context save/restore for secure ram is not a function of the PM idle core.

Quoted 1 line

Quoted 1 line

with every other device, context save/restore is the responsibility

Quoted 3 lines

Quoted 3 lines

the driver(s) using that device.

For secure RAM, the restore is handled by ROM code, but the save

Quoted 4 lines

Quoted 4 lines

be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved

Quoted 5 lines

Quoted 3 lines

into a library/driver which can be called by secure drivers in their runtime PM hooks.

I agree with you Kevin here except one point. The secure RAM contents are not just secure driver data but the ROM code infrastructure as

Quoted 9 lines

And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code.
Ofcourse this not related to GP device because we never enter Secure world again after the boot-up.
So moving this code to a separate file is fine but it still related to CPU.

Sure, it's still *related* to the CPU, but what I am arguing is that it should not be related to CPU *idle*.

My undersanding is this (please correct me):

Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.) Therefore, any driver/device usage that has a side effect of changing secure RAM should be responsible for updating secure RAM.

The approach taken in $SUBJECT series is basically: since we don't know who is using/changing secure RAM, we better save it (or have the option to) during every off-mode transition. This approach is what I do not like. It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong.

Rather, I want to follow the same approach we follow for every other device driver. Drivers must assume they can lose context. Therefore it's up to them to save it.

IOW, the drivers that *change* secure RAM should be responsible for ensuring that any of the changes they make are saved.

-----Original Message-----
From: Santosh Shilimkar [mailto:[email protected]]
Sent: Wednesday, November 24, 2010 11:04 AM
To: 'Kevin Hilman'
Cc: Nishanth Menon; 'linux-omap'; 'Jean Pihet'; Vishwanath Sripathy; 'Tony'
Subject: RE: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 8 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Wednesday, November 24, 2010 2:06 AM
To: Santosh Shilimkar
Cc: Nishanth Menon; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 11 lines

Quoted 9 lines

And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code.
Ofcourse this not related to GP device because we never enter Secure world again after the boot-up.
So moving this code to a separate file is fine but it still related to CPU.

Sure, it's still *related* to the CPU, but what I am arguing is that

Quoted 7 lines

should not be related to CPU *idle*.

My undersanding is this (please correct me):

Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.) Therefore, any driver/device usage that has a side effect of changing secure RAM

Quoted 2 lines

be responsible for updating secure RAM.

This assumption holds true largely but not completely. There are more Secure APIs which are outside of any secure driver usage which can also alter the state of secure RAM. OMAP4, we have more APIs apart from

RAM where the secure HW registers, firewalls, cache controllers,

controllers are managed using secure APIs. All of this is must for

CPU OFF functioning.

Quoted 1 line

The approach taken in $SUBJECT series is basically: since we don't

Quoted 1 line

who is using/changing secure RAM, we better save it (or have the

Quoted 4 lines

to) during every off-mode transition. This approach is what I do not like. It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong.

The problem is because the secure RAM is not portioned per device basis but managed as a whole. If we had per secure device portioning then the respective device drivers saving it's context would have worked

And the fact is it's not the secure device driver context, but it's a Secure software context which runs in parallel with HLOS on HS devices.

Quoted 7 lines

Rather, I want to follow the same approach we follow for every other device driver. Drivers must assume they can lose context. Therefore it's up to them to save it.

IOW, the drivers that *change* secure RAM should be responsible for ensuring that any of the changes they make are saved.

As I mentioned above its not just the driver context but the whole secure software context. I will check with ROM team if it can be made more granular for future OMAPs so that we can have the usual strategy of respective components taking care of there save/restore. This will also save huge latency we incure while saving whole RAM on every MPU OFF transition.

(Sorry. You will see two replies from me on your email)

Quoted 16 lines

-----Original Message-----
From: Santosh Shilimkar [mailto:[email protected]]
Sent: Wednesday, November 24, 2010 11:04 AM
To: 'Kevin Hilman'
Cc: Nishanth Menon; 'linux-omap'; 'Jean Pihet'; Vishwanath Sripathy; 'Tony'
Subject: RE: [PATCH 00/13] OMAP3: OFF mode fixes

Quoted 8 lines

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Wednesday, November 24, 2010 2:06 AM
To: Santosh Shilimkar
Cc: Nishanth Menon; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony
Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes

[...]

Quoted 11 lines

Quoted 11 lines

Quoted 9 lines

And we should treat ROM code as a hardware. Secure services don't give  you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code.
Ofcourse this not related to GP device because we never enter Secure world again after the boot-up.
So moving this code to a separate file is fine but it still related to CPU.

Sure, it's still *related* to the CPU, but what I am arguing is that

it

Quoted 7 lines

Quoted 7 lines

should not be related to CPU *idle*.

My undersanding is this (please correct me):

Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.)  Therefore, any driver/device usage that has a side effect of changing secure RAM

should

Quoted 5 lines

Quoted 2 lines

be responsible for updating secure RAM.

This assumption holds true largely but not completely. There are more Secure APIs which are outside of any secure driver usage which can also alter the state of secure RAM. OMAP4, we have more APIs apart from

secure

Quoted 1 line

RAM where the secure HW registers, firewalls, cache controllers,

interrupt

Quoted 1 line

controllers are managed using secure APIs. All of this is must for

correct

Quoted 3 lines

CPU OFF functioning.

Quoted 1 line

The approach taken in $SUBJECT series is basically: since we don't

know

Quoted 1 line

Quoted 1 line

who is using/changing secure RAM, we better save it (or have the

option

Quoted 7 lines

Quoted 4 lines

to) during every off-mode transition.  This approach is what I do not like.  It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong.

The problem is because the secure RAM is not portioned per device basis but managed as a whole. If we had per secure device portioning then the respective device drivers saving it's context would have worked

perfectly.

Quoted 10 lines

And the fact is it's not the secure device driver context, but it's a Secure software context which runs in parallel with HLOS on HS devices.

Quoted 7 lines

Rather, I want to follow the same approach we follow for every other device driver.  Drivers must assume they can lose context.  Therefore it's up to them to save it.

IOW, the drivers that *change* secure RAM should be responsible for ensuring that any of the changes they make are saved.

Quoted 7 lines

As I mentioned above its not just the driver context but the whole secure software context. I will check with ROM team if it can be made more granular for future OMAPs so that we can have the usual strategy of respective components taking care of there save/restore. This will also save huge latency we incure while saving whole RAM on every MPU OFF transition.

I had a discussion with ROM team and they conformed that the secure context can get changed with protected applications (PA for FW, secure keys) as well as with secure drivers like crypto, aes etc.

This can be centralized and some APIs can be provided by the secure middleware to know whether the context needs to be save or not. Secure middleware manages all the secure driver interfaces as well as PA's. This is the centralized place where all state knowledge is available. This component also can take care of doing a secure context save job based on needs. The only concern from them was that which security middleware to be used is not fixed and its upto customers to decide. Few of them develop this on there own.

So with clarity now, I concur with your idea of moving out the secure context save from CPUIDLE code.
The only problem I see is how do we support multiple security middleware's with a common infrastructure.

Regards
Santosh