[PATCH 00/13] OMAP3: OFF mode fixes

[PATCH 00/13] OMAP3: OFF mode fixes

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

Regards,
Nishanth Menon

[PATCH 01/13] OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all

[Nishanth Menon]

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

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

This also fixes the bug with the clean_l2 function usage.

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

[nm@ti.com: ported rkw's proposal to 2.6.37-rc2]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
---

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

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

Re: [PATCH 01/13] OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all

[Jean Pihet]

On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
> From: Richard Woodruff <r-woodruff2@ti.com>
>
> Analysis in TI kernel with ETM showed that using cache mapped flush
> in kernel instead of SO mapped flush cost drops by 65% (3.39mS down
> to 1.17mS) for clean_l2 which is used during sleep sequences.
> Overall:
>        - speed up
>        - unfortunately there isn't a good alternative flush method today
>        - code reduction and less maintenance and potential bug in
>          unmaintained code
>
> This also fixes the bug with the clean_l2 function usage.
>
> Reported-by: Tony Lindgren <tony@atomide.com>
>
> [nm@ti.com: ported rkw's proposal to 2.6.37-rc2]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
> ---
>
> 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(-)
>
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index 2fb205a..8f207b2 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -520,72 +520,17 @@ clean_caches:
>        cmp     r9, #1 /* Check whether L2 inval is required or not*/
>        bne     skip_l2_inval
>  clean_l2:
> -       /* read clidr */
> -       mrc     p15, 1, r0, c0, c0, 1
> -       /* extract loc from clidr */
> -       ands    r3, r0, #0x7000000
> -       /* left align loc bit field */
> -       mov     r3, r3, lsr #23
> -       /* if loc is 0, then no need to clean */
> -       beq     finished
> -       /* start clean at cache level 0 */
> -       mov     r10, #0
> -loop1:
> -       /* work out 3x current cache level */
> -       add     r2, r10, r10, lsr #1
> -       /* extract cache type bits from clidr*/
> -       mov     r1, r0, lsr r2
> -       /* mask of the bits for current cache only */
> -       and     r1, r1, #7
> -       /* see what cache we have at this level */
> -       cmp     r1, #2
> -       /* skip if no cache, or just i-cache */
> -       blt     skip
> -       /* select current cache level in cssr */
> -       mcr     p15, 2, r10, c0, c0, 0
> -       /* isb to sych the new cssr&csidr */
> -       isb
> -       /* read the new csidr */
> -       mrc     p15, 1, r1, c0, c0, 0
> -       /* extract the length of the cache lines */
> -       and     r2, r1, #7
> -       /* add 4 (line length offset) */
> -       add     r2, r2, #4
> -       ldr     r4, assoc_mask
> -       /* find maximum number on the way size */
> -       ands    r4, r4, r1, lsr #3
> -       /* find bit position of way size increment */
> -       clz     r5, r4
> -       ldr     r7, numset_mask
> -       /* extract max number of the index size*/
> -       ands    r7, r7, r1, lsr #13
> -loop2:
> -       mov     r9, r4
> -       /* create working copy of max way size*/
> -loop3:
> -       /* factor way and cache number into r11 */
> -       orr     r11, r10, r9, lsl r5
> -       /* factor index number into r11 */
> -       orr     r11, r11, r7, lsl r2
> -       /*clean & invalidate by set/way */
> -       mcr     p15, 0, r11, c7, c10, 2
> -       /* decrement the way*/
> -       subs    r9, r9, #1
> -       bge     loop3
> -       /*decrement the index */
> -       subs    r7, r7, #1
> -       bge     loop2
> -skip:
> -       add     r10, r10, #2
> -       /* increment cache number */
> -       cmp     r3, r10
> -       bgt     loop1
> -finished:
> -       /*swith back to cache level 0 */
> -       mov     r10, #0
> -       /* select current cache level in cssr */
> -       mcr     p15, 2, r10, c0, c0, 0
> -       isb
> +       /*
> +        * jump out to kernel flush routine
> +        *  - resue that code is better
Typo: 'reuse'

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

> +
>  skip_l2_inval:
>        /* Data memory barrier and Data sync barrier */
>        mov     r1, #0
> @@ -668,5 +613,7 @@ cache_pred_disable_mask:
>        .word   0xFFFFE7FB
>  control_stat:
>        .word   CONTROL_STAT
> +kernel_flush:
> +       .word v7_flush_dcache_all
Remove this if 'bl' is used.

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

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/13] OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all

[Peter 'p2' De Schrijver]

On Fri, Nov 19, 2010 at 10:46:19AM +0100, ext Jean Pihet wrote:
> On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
> > From: Richard Woodruff <r-woodruff2@ti.com>
> >
> > Analysis in TI kernel with ETM showed that using cache mapped flush
> > in kernel instead of SO mapped flush cost drops by 65% (3.39mS down
> > to 1.17mS) for clean_l2 which is used during sleep sequences.
> > Overall:
> >        - speed up
> >        - unfortunately there isn't a good alternative flush method today
> >        - code reduction and less maintenance and potential bug in
> >          unmaintained code
> >
> > This also fixes the bug with the clean_l2 function usage.
> >
> > Reported-by: Tony Lindgren <tony@atomide.com>
> >
> > [nm@ti.com: ported rkw's proposal to 2.6.37-rc2]
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
> > ---
> >
> > 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(-)
> >
> > diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> > index 2fb205a..8f207b2 100644
> > --- a/arch/arm/mach-omap2/sleep34xx.S
> > +++ b/arch/arm/mach-omap2/sleep34xx.S
> > @@ -520,72 +520,17 @@ clean_caches:
> >        cmp     r9, #1 /* Check whether L2 inval is required or not*/
> >        bne     skip_l2_inval
> >  clean_l2:
> > -       /* read clidr */
> > -       mrc     p15, 1, r0, c0, c0, 1
> > -       /* extract loc from clidr */
> > -       ands    r3, r0, #0x7000000
> > -       /* left align loc bit field */
> > -       mov     r3, r3, lsr #23
> > -       /* if loc is 0, then no need to clean */
> > -       beq     finished
> > -       /* start clean at cache level 0 */
> > -       mov     r10, #0
> > -loop1:
> > -       /* work out 3x current cache level */
> > -       add     r2, r10, r10, lsr #1
> > -       /* extract cache type bits from clidr*/
> > -       mov     r1, r0, lsr r2
> > -       /* mask of the bits for current cache only */
> > -       and     r1, r1, #7
> > -       /* see what cache we have at this level */
> > -       cmp     r1, #2
> > -       /* skip if no cache, or just i-cache */
> > -       blt     skip
> > -       /* select current cache level in cssr */
> > -       mcr     p15, 2, r10, c0, c0, 0
> > -       /* isb to sych the new cssr&csidr */
> > -       isb
> > -       /* read the new csidr */
> > -       mrc     p15, 1, r1, c0, c0, 0
> > -       /* extract the length of the cache lines */
> > -       and     r2, r1, #7
> > -       /* add 4 (line length offset) */
> > -       add     r2, r2, #4
> > -       ldr     r4, assoc_mask
> > -       /* find maximum number on the way size */
> > -       ands    r4, r4, r1, lsr #3
> > -       /* find bit position of way size increment */
> > -       clz     r5, r4
> > -       ldr     r7, numset_mask
> > -       /* extract max number of the index size*/
> > -       ands    r7, r7, r1, lsr #13
> > -loop2:
> > -       mov     r9, r4
> > -       /* create working copy of max way size*/
> > -loop3:
> > -       /* factor way and cache number into r11 */
> > -       orr     r11, r10, r9, lsl r5
> > -       /* factor index number into r11 */
> > -       orr     r11, r11, r7, lsl r2
> > -       /*clean & invalidate by set/way */
> > -       mcr     p15, 0, r11, c7, c10, 2
> > -       /* decrement the way*/
> > -       subs    r9, r9, #1
> > -       bge     loop3
> > -       /*decrement the index */
> > -       subs    r7, r7, #1
> > -       bge     loop2
> > -skip:
> > -       add     r10, r10, #2
> > -       /* increment cache number */
> > -       cmp     r3, r10
> > -       bgt     loop1
> > -finished:
> > -       /*swith back to cache level 0 */
> > -       mov     r10, #0
> > -       /* select current cache level in cssr */
> > -       mcr     p15, 2, r10, c0, c0, 0
> > -       isb
> > +       /*
> > +        * jump out to kernel flush routine
> > +        *  - resue that code is better
> Typo: 'reuse'
> 
> > +        *  - 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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/13] OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all

[Jean Pihet]

Hi Peter,

On Fri, Nov 19, 2010 at 10:57 AM, Peter 'p2' De Schrijver
<peter.de-schrijver@nokia.com> wrote:
> On Fri, Nov 19, 2010 at 10:46:19AM +0100, ext Jean Pihet wrote:
>> On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
>> > From: Richard Woodruff <r-woodruff2@ti.com>
>> >
>> > Analysis in TI kernel with ETM showed that using cache mapped flush
>> > in kernel instead of SO mapped flush cost drops by 65% (3.39mS down
>> > to 1.17mS) for clean_l2 which is used during sleep sequences.
>> > Overall:
>> >        - speed up
>> >        - unfortunately there isn't a good alternative flush method today
>> >        - code reduction and less maintenance and potential bug in
>> >          unmaintained code
>> >
>> > This also fixes the bug with the clean_l2 function usage.
>> >
>> > Reported-by: Tony Lindgren <tony@atomide.com>
>> >
>> > [nm@ti.com: ported rkw's proposal to 2.6.37-rc2]
>> > Signed-off-by: Nishanth Menon <nm@ti.com>
>> > Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
>> > ---
>> >
>> > 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(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
>> > index 2fb205a..8f207b2 100644
>> > --- a/arch/arm/mach-omap2/sleep34xx.S
>> > +++ b/arch/arm/mach-omap2/sleep34xx.S
>> > @@ -520,72 +520,17 @@ clean_caches:
>> >        cmp     r9, #1 /* Check whether L2 inval is required or not*/
>> >        bne     skip_l2_inval
>> >  clean_l2:
>> > -       /* read clidr */
>> > -       mrc     p15, 1, r0, c0, c0, 1
>> > -       /* extract loc from clidr */
>> > -       ands    r3, r0, #0x7000000
>> > -       /* left align loc bit field */
>> > -       mov     r3, r3, lsr #23
>> > -       /* if loc is 0, then no need to clean */
>> > -       beq     finished
>> > -       /* start clean at cache level 0 */
>> > -       mov     r10, #0
>> > -loop1:
>> > -       /* work out 3x current cache level */
>> > -       add     r2, r10, r10, lsr #1
>> > -       /* extract cache type bits from clidr*/
>> > -       mov     r1, r0, lsr r2
>> > -       /* mask of the bits for current cache only */
>> > -       and     r1, r1, #7
>> > -       /* see what cache we have at this level */
>> > -       cmp     r1, #2
>> > -       /* skip if no cache, or just i-cache */
>> > -       blt     skip
>> > -       /* select current cache level in cssr */
>> > -       mcr     p15, 2, r10, c0, c0, 0
>> > -       /* isb to sych the new cssr&csidr */
>> > -       isb
>> > -       /* read the new csidr */
>> > -       mrc     p15, 1, r1, c0, c0, 0
>> > -       /* extract the length of the cache lines */
>> > -       and     r2, r1, #7
>> > -       /* add 4 (line length offset) */
>> > -       add     r2, r2, #4
>> > -       ldr     r4, assoc_mask
>> > -       /* find maximum number on the way size */
>> > -       ands    r4, r4, r1, lsr #3
>> > -       /* find bit position of way size increment */
>> > -       clz     r5, r4
>> > -       ldr     r7, numset_mask
>> > -       /* extract max number of the index size*/
>> > -       ands    r7, r7, r1, lsr #13
>> > -loop2:
>> > -       mov     r9, r4
>> > -       /* create working copy of max way size*/
>> > -loop3:
>> > -       /* factor way and cache number into r11 */
>> > -       orr     r11, r10, r9, lsl r5
>> > -       /* factor index number into r11 */
>> > -       orr     r11, r11, r7, lsl r2
>> > -       /*clean & invalidate by set/way */
>> > -       mcr     p15, 0, r11, c7, c10, 2
>> > -       /* decrement the way*/
>> > -       subs    r9, r9, #1
>> > -       bge     loop3
>> > -       /*decrement the index */
>> > -       subs    r7, r7, #1
>> > -       bge     loop2
>> > -skip:
>> > -       add     r10, r10, #2
>> > -       /* increment cache number */
>> > -       cmp     r3, r10
>> > -       bgt     loop1
>> > -finished:
>> > -       /*swith back to cache level 0 */
>> > -       mov     r10, #0
>> > -       /* select current cache level in cssr */
>> > -       mcr     p15, 2, r10, c0, c0, 0
>> > -       isb
>> > +       /*
>> > +        * jump out to kernel flush routine
>> > +        *  - resue that code is better
>> Typo: 'reuse'
>>
>> > +        *  - 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
>
> 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 :)

Correct! My version now runs from DDR, this explains that!

>
> Cheers,
>
> Peter.
>

Thanks,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/13] OMAP3: PM: Errata i581 suppport: dll kick strategy

[Nishanth Menon]

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

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 <peter.de-schrijver@nokia.com>
---
 arch/arm/mach-omap2/sleep34xx.S |   52 +++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 5 deletions(-)

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

Re: [PATCH 02/13] OMAP3: PM: Errata i581 suppport: dll kick strategy

[Sripathy, Vishwanath]

Nishant,

On Fri, Nov 19, 2010 at 7:24 AM, Nishanth Menon <nm@ti.com> wrote:
> From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
>
> 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.

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
>
> Signed-off-by: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
> ---
>  arch/arm/mach-omap2/sleep34xx.S |   52 +++++++++++++++++++++++++++++++++++---
>  1 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index 8f207b2..5a4468f 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -42,6 +42,7 @@
>                                OMAP3430_PM_PREPWSTST)
>  #define PM_PWSTCTRL_MPU_P      OMAP3430_PRM_BASE + MPU_MOD + OMAP2_PM_PWSTCTRL
>  #define CM_IDLEST1_CORE_V      OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
> +#define CM_IDLEST_CKGEN_V      OMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST)
>  #define SRAM_BASE_P            0x40200000
>  #define CONTROL_STAT           0x480022F0
>  #define SCRATCHPAD_MEM_OFFS    0x310 /* Move this as correct place is
> @@ -554,31 +555,67 @@ skip_l2_inval:
>
>  /* Make sure SDRC accesses are ok */
>  wait_sdrc_ok:
> +
> +/* DPLL3 must be locked before accessing the SDRC. Maybe the HW ensures this. */
> +       ldr     r4, cm_idlest_ckgen
> +wait_dpll3_lock:
> +       ldr     r5, [r4]
> +       tst     r5, #1
> +       beq     wait_dpll3_lock
> +
>         ldr     r4, cm_idlest1_core
> +wait_sdrc_ready:
>         ldr     r5, [r4]
> -        and     r5, r5, #0x2
> -        cmp     r5, #0
> -        bne     wait_sdrc_ok
> +        tst     r5, #0x2
> +        bne     wait_sdrc_ready
> +       /* allow DLL powerdown upon hw idle req */
>         ldr     r4, sdrc_power
>         ldr     r5, [r4]
>         bic     r5, r5, #0x40
>         str     r5, [r4]
> -wait_dll_lock:
> +is_dll_in_lock_mode:
> +
>         /* Is dll in lock mode? */
>         ldr     r4, sdrc_dlla_ctrl
>         ldr     r5, [r4]
>         tst     r5, #0x4
>         bxne    lr
>         /* wait till dll locks */
> -        ldr     r4, sdrc_dlla_status
> +wait_dll_lock_timed:
> +       ldr     r4, wait_dll_lock_counter
> +       add     r4, r4, #1
> +       str     r4, wait_dll_lock_counter
> +       ldr     r4, sdrc_dlla_status
> +        mov    r6, #8          /* Wait 20uS for lock */
> +wait_dll_lock:
> +       subs    r6, r6, #0x1
> +       beq     kick_dll
>         ldr     r5, [r4]
>         and     r5, r5, #0x4
>         cmp     r5, #0x4
>         bne     wait_dll_lock
>         bx      lr
>
> +       /* disable/reenable DLL if not locked */
> +kick_dll:
> +       ldr     r4, sdrc_dlla_ctrl
> +       ldr     r5, [r4]
> +       mov     r6, r5
> +       bic     r6, #(1<<3)     /* disable dll */
> +       str     r6, [r4]
> +       dsb
> +       orr     r6, r6, #(1<<3) /* enable dll */
> +       str     r6, [r4]
> +       dsb
> +       ldr     r4, kick_counter
> +       add     r4, r4, #1
> +       str     r4, kick_counter
> +       b       wait_dll_lock_timed
> +
>  cm_idlest1_core:
>        .word   CM_IDLEST1_CORE_V
> +cm_idlest_ckgen:
> +       .word   CM_IDLEST_CKGEN_V
>  sdrc_dlla_status:
>        .word   SDRC_DLLA_STATUS_V
>  sdrc_dlla_ctrl:
> @@ -615,5 +652,10 @@ control_stat:
>        .word   CONTROL_STAT
>  kernel_flush:
>        .word v7_flush_dcache_all
> +       /* these 2 words need to be at the end !!! */
> +kick_counter:
> +       .word   0
> +wait_dll_lock_counter:
> +       .word   0
>  ENTRY(omap34xx_cpu_suspend_sz)
>        .word   . - omap34xx_cpu_suspend
> --
> 1.6.3.3
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/13] OMAP3: PM: Errata i581 suppport: dll kick strategy

[Nishanth Menon]

Sripathy, Vishwanath had written, on 11/24/2010 10:51 AM, the following:
> On Fri, Nov 19, 2010 at 7:24 AM, Nishanth Menon <nm@ti.com> wrote:
>> From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
>>
>> 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.
> 
> 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.

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


-- 
Regards,
Nishanth Menon

Re: [PATCH 02/13] OMAP3: PM: Errata i581 suppport: dll kick strategy

[Sripathy, Vishwanath]

Nishant,

On Wed, Nov 24, 2010 at 10:54 PM, Nishanth Menon <nm@ti.com> wrote:
> Sripathy, Vishwanath had written, on 11/24/2010 10:51 AM, the following:
>>
>> On Fri, Nov 19, 2010 at 7:24 AM, Nishanth Menon <nm@ti.com> wrote:
>>>
>>> From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
>>>
>>> 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.
>>
>> 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.
Then I do not find any need to keep this code in SRAM as we anyway
know that it does not help when Core enters off mode.
As you know already, in Jean's ASM clean up code, we are moving all
these ASM code to DDR.

Vishwa
>
> Maybe we can refactor this as part of Jean's cleanups.
>
>>
>> Vishwa
>>>
>>> Signed-off-by: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
>>> ---
>>>  arch/arm/mach-omap2/sleep34xx.S |   52
>>> +++++++++++++++++++++++++++++++++++---
>>>  1 files changed, 47 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/sleep34xx.S
>>> b/arch/arm/mach-omap2/sleep34xx.S
>>> index 8f207b2..5a4468f 100644
>>> --- a/arch/arm/mach-omap2/sleep34xx.S
>>> +++ b/arch/arm/mach-omap2/sleep34xx.S
>>> @@ -42,6 +42,7 @@
>>>                               OMAP3430_PM_PREPWSTST)
>>>  #define PM_PWSTCTRL_MPU_P      OMAP3430_PRM_BASE + MPU_MOD +
>>> OMAP2_PM_PWSTCTRL
>>>  #define CM_IDLEST1_CORE_V      OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
>>> +#define CM_IDLEST_CKGEN_V      OMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST)
>>>  #define SRAM_BASE_P            0x40200000
>>>  #define CONTROL_STAT           0x480022F0
>>>  #define SCRATCHPAD_MEM_OFFS    0x310 /* Move this as correct place is
>>> @@ -554,31 +555,67 @@ skip_l2_inval:
>>>
>>>  /* Make sure SDRC accesses are ok */
>>>  wait_sdrc_ok:
>>> +
>>> +/* DPLL3 must be locked before accessing the SDRC. Maybe the HW ensures
>>> this. */
>>> +       ldr     r4, cm_idlest_ckgen
>>> +wait_dpll3_lock:
>>> +       ldr     r5, [r4]
>>> +       tst     r5, #1
>>> +       beq     wait_dpll3_lock
>>> +
>>>        ldr     r4, cm_idlest1_core
>>> +wait_sdrc_ready:
>>>        ldr     r5, [r4]
>>> -        and     r5, r5, #0x2
>>> -        cmp     r5, #0
>>> -        bne     wait_sdrc_ok
>>> +        tst     r5, #0x2
>>> +        bne     wait_sdrc_ready
>>> +       /* allow DLL powerdown upon hw idle req */
>>>        ldr     r4, sdrc_power
>>>        ldr     r5, [r4]
>>>        bic     r5, r5, #0x40
>>>        str     r5, [r4]
>>> -wait_dll_lock:
>>> +is_dll_in_lock_mode:
>>> +
>>>        /* Is dll in lock mode? */
>>>        ldr     r4, sdrc_dlla_ctrl
>>>        ldr     r5, [r4]
>>>        tst     r5, #0x4
>>>        bxne    lr
>>>        /* wait till dll locks */
>>> -        ldr     r4, sdrc_dlla_status
>>> +wait_dll_lock_timed:
>>> +       ldr     r4, wait_dll_lock_counter
>>> +       add     r4, r4, #1
>>> +       str     r4, wait_dll_lock_counter
>>> +       ldr     r4, sdrc_dlla_status
>>> +        mov    r6, #8          /* Wait 20uS for lock */
>>> +wait_dll_lock:
>>> +       subs    r6, r6, #0x1
>>> +       beq     kick_dll
>>>        ldr     r5, [r4]
>>>        and     r5, r5, #0x4
>>>        cmp     r5, #0x4
>>>        bne     wait_dll_lock
>>>        bx      lr
>>>
>>> +       /* disable/reenable DLL if not locked */
>>> +kick_dll:
>>> +       ldr     r4, sdrc_dlla_ctrl
>>> +       ldr     r5, [r4]
>>> +       mov     r6, r5
>>> +       bic     r6, #(1<<3)     /* disable dll */
>>> +       str     r6, [r4]
>>> +       dsb
>>> +       orr     r6, r6, #(1<<3) /* enable dll */
>>> +       str     r6, [r4]
>>> +       dsb
>>> +       ldr     r4, kick_counter
>>> +       add     r4, r4, #1
>>> +       str     r4, kick_counter
>>> +       b       wait_dll_lock_timed
>>> +
>>>  cm_idlest1_core:
>>>       .word   CM_IDLEST1_CORE_V
>>> +cm_idlest_ckgen:
>>> +       .word   CM_IDLEST_CKGEN_V
>>>  sdrc_dlla_status:
>>>       .word   SDRC_DLLA_STATUS_V
>>>  sdrc_dlla_ctrl:
>>> @@ -615,5 +652,10 @@ control_stat:
>>>       .word   CONTROL_STAT
>>>  kernel_flush:
>>>       .word v7_flush_dcache_all
>>> +       /* these 2 words need to be at the end !!! */
>>> +kick_counter:
>>> +       .word   0
>>> +wait_dll_lock_counter:
>>> +       .word   0
>>>  ENTRY(omap34xx_cpu_suspend_sz)
>>>       .word   . - omap34xx_cpu_suspend
>>> --
>>> 1.6.3.3
>>>
>>>
>
>
> --
> Regards,
> Nishanth Menon
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/13] OMAP3: PM: Errata i581 suppport: dll kick strategy

[Peter 'p2' De Schrijver]

On Wed, Nov 24, 2010 at 05:51:50PM +0100, ext Sripathy, Vishwanath wrote:
> Nishant,
> 
> On Fri, Nov 19, 2010 at 7:24 AM, Nishanth Menon <nm@ti.com> wrote:
> > From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
> >
> > 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.
> 
> 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?
> 

This code is mostly important for inactive and retention. The ROM code
waits for the maximum dll lock time when resuming from off mode. So for
off mode this code isn't really needed.

Cheers,

Peter.

[PATCH 03/13] OMAP3: PM: make secure ram save size configurable

[Nishanth Menon]

OMAP3 users of HS/EMU devices at times choose to use their
own PPA which could be configured to use different sized storage
area based on their security needs. Convert the hardcoded size
define to a more configurable form to map to these users. we introduce
the structure omap3_secure_copy_data to describe PPA specific behavior
we will further populate in follow on patches. secure_copy_data_set
is introduced to allow for board files to populate custom parameters
as desired.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/pm.h     |   23 +++++++++++++++++++++++
 arch/arm/mach-omap2/pm34xx.c |   27 ++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 0d75bfd..c0af788 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -85,4 +85,27 @@ extern unsigned int save_secure_ram_context_sz;
 extern unsigned int omap24xx_cpu_suspend_sz;
 extern unsigned int omap34xx_cpu_suspend_sz;
 
+/**
+ * struct omap3_secure_copy_data - describe behavior for the secure ram copy
+ * @size:	size of copy to be saved - this is based on the PPA used
+ *		secure ram size could be configured to various sizes, this is
+ *		the size used + 64 byte header required.
+ *
+ * Different platforms use different security PPAs based on their unique needs.
+ * This structure describes the delta behavior expected for these custom
+ * platforms. The defaults are configured for official TI OMAP3 PPA behavior.
+ */
+struct omap3_secure_copy_data {
+	u32 size;
+};
+
+#if defined(CONFIG_PM)
+extern int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *d);
+#else
+static inline int omap3_secure_copy_data_set(struct omap3_secure_copy_data *d)
+{
+	return -EINVAL;
+}
+#endif
+
 #endif
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 75c0cd1..633b696 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -54,6 +54,11 @@
 #define OMAP343X_TABLE_VALUE_OFFSET	   0xc0
 #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
 
+/* Secure ram save size - store the defaults */
+static struct omap3_secure_copy_data secure_copy_data = {
+	.size = 0x803F,
+};
+
 struct power_state {
 	struct powerdomain *pwrdm;
 	u32 next_state;
@@ -154,6 +159,26 @@ static void omap3_core_restore_context(void)
 	omap_dma_global_context_restore();
 }
 
+/**
+ * omap3_secure_copy_data_set() - set up the secure ram copy size
+ * @data - platform specific customization
+ *
+ * This function should be invoked by the board's init_irq function to update
+ * data prior to pm_init call is invoked. This call be done to update based on
+ * ppa used on that platform.
+ *
+ * Returns -EINVAL for bad values, and 0 if all good.
+ */
+int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *data)
+{
+	if (!data || !data->size)
+		return -EINVAL;
+
+	memcpy(&secure_copy_data, data, sizeof(secure_copy_data));
+
+	return 0;
+}
+
 /*
  * FIXME: This function should be called before entering off-mode after
  * OMAP3 secure services have been accessed. Currently it is only called
@@ -1038,7 +1063,7 @@ static int __init omap3_pm_init(void)
 	clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
 	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
 		omap3_secure_ram_storage =
-			kmalloc(0x803F, GFP_KERNEL);
+			kmalloc(secure_copy_data.size, GFP_KERNEL);
 		if (!omap3_secure_ram_storage)
 			printk(KERN_ERR "Memory allocation failed when"
 					"allocating for secure sram context\n");
-- 
1.6.3.3

[PATCH 04/13] OMAP3: PM: Save secure RAM context before entering WFI

[Nishanth Menon]

From: Tero Kristo <tero.kristo@nokia.com>

Currently this is done during initialization. move this to just before
wfi call in omap_sram_idle.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   35 ++++++++++++++---------------------
 1 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 633b696..ba85e9c 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -77,6 +77,8 @@ static int (*_omap_save_secure_sram)(u32 *addr);
 static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
 static struct powerdomain *core_pwrdm, *per_pwrdm;
 static struct powerdomain *cam_pwrdm;
+static int secure_ram_save_status;
+static int secure_ram_saved;
 
 static inline void omap3_per_save_context(void)
 {
@@ -182,30 +184,22 @@ int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *data)
 /*
  * FIXME: This function should be called before entering off-mode after
  * OMAP3 secure services have been accessed. Currently it is only called
- * once during boot sequence, but this works as we are not using secure
+ * once during first OFF transition, but this works as we are not using secure
  * services.
  */
 static void omap3_save_secure_ram_context(u32 target_mpu_state)
 {
-	u32 ret;
-
-	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
+	if (!secure_ram_saved && omap_type() != OMAP2_DEVICE_TYPE_GP) {
 		/*
 		 * 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);
-		ret = _omap_save_secure_sram((u32 *)
+		secure_ram_save_status = _omap_save_secure_sram((u32 *)
 				__pa(omap3_secure_ram_storage));
 		pwrdm_set_next_pwrst(mpu_pwrdm, target_mpu_state);
-		/* Following is for error tracking, it should not happen */
-		if (ret) {
-			printk(KERN_ERR "save_secure_sram() returns %08x\n",
-				ret);
-			while (1)
-				;
-		}
+		secure_ram_saved = 1;
 	}
 }
 
@@ -426,6 +420,7 @@ void omap_sram_idle(void)
 		if (core_next_state == PWRDM_POWER_OFF) {
 			omap3_core_save_context();
 			omap3_prcm_save_context();
+			omap3_save_secure_ram_context(mpu_next_state);
 		}
 	}
 
@@ -498,6 +493,13 @@ void omap_sram_idle(void)
 
 	pwrdm_post_transition();
 
+	/* Check if secure RAM save failed for some reason */
+	if (secure_ram_save_status > 0) {
+		WARN(1, "secure ram save failed (%d)\n",
+			secure_ram_save_status);
+		secure_ram_save_status = 0;
+	}
+
 	omap2_clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
 }
 
@@ -1068,15 +1070,6 @@ static int __init omap3_pm_init(void)
 			printk(KERN_ERR "Memory allocation failed when"
 					"allocating for secure sram context\n");
 
-		local_irq_disable();
-		local_fiq_disable();
-
-		omap_dma_global_context_save();
-		omap3_save_secure_ram_context(PWRDM_POWER_ON);
-		omap_dma_global_context_restore();
-
-		local_irq_enable();
-		local_fiq_enable();
 	}
 
 	omap3_save_scratchpad_contents();
-- 
1.6.3.3

[PATCH 05/13] OMAP3: PM: optional save secure RAM context every core off cycle

[Nishanth Menon]

From: Tero Kristo <tero.kristo@nokia.com>

Some PPAs now supports saving secure RAM context several times.
Now we will save it before every core off cycle.
board files can register with params to allow configuration based
on the PPA used.

[nm@ti.com: converted to struct option for various PPAs in the wild]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/pm.h     |   13 +++++++++++++
 arch/arm/mach-omap2/pm34xx.c |    4 +++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index c0af788..39934ec 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -90,6 +90,18 @@ extern unsigned int omap34xx_cpu_suspend_sz;
  * @size:	size of copy to be saved - this is based on the PPA used
  *		secure ram size could be configured to various sizes, this is
  *		the size used + 64 byte header required.
+ * @save_every_cycle: While going to OFF mode and coming out of it on HS/EMU
+ *		devices, secure service is provided to enable saving the
+ *		contexts of secure ram and secure status of various drivers
+ *		using secure devices. However, there are many kinds of secure
+ *		conditions in the wild:
+ *		1. Implements its own save and restore using pm hooks
+ *		2. Generic drivers which depend on OMAP PM code to handle the
+ *		   same correspondingly, PPA may or may not allow capability
+ *		   to save in every transition to OFF mode.
+ *		3. PPA may be buggy and does'nt allow multiple saves
+ *		Support of this depends heavily on the PPA used and the security
+ *		driver capability. If in doubt, contact the security team.
  *
  * Different platforms use different security PPAs based on their unique needs.
  * This structure describes the delta behavior expected for these custom
@@ -97,6 +109,7 @@ extern unsigned int omap34xx_cpu_suspend_sz;
  */
 struct omap3_secure_copy_data {
 	u32 size;
+	bool save_every_cycle;
 };
 
 #if defined(CONFIG_PM)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index ba85e9c..b20ecf5 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -57,6 +57,7 @@
 /* Secure ram save size - store the defaults */
 static struct omap3_secure_copy_data secure_copy_data = {
 	.size = 0x803F,
+	.save_every_cycle = false,	/* explicit for readability */
 };
 
 struct power_state {
@@ -199,7 +200,8 @@ static void omap3_save_secure_ram_context(u32 target_mpu_state)
 		secure_ram_save_status = _omap_save_secure_sram((u32 *)
 				__pa(omap3_secure_ram_storage));
 		pwrdm_set_next_pwrst(mpu_pwrdm, target_mpu_state);
-		secure_ram_saved = 1;
+		if (!secure_copy_data.save_every_cycle)
+			secure_ram_saved = 1;
 	}
 }
 
-- 
1.6.3.3

[PATCH 06/13] OMAP3: PM: Fix secure save size for OMAP3

[Nishanth Menon]

Use TI recommended save_secure_ram size for OMAP3 using TI official
OMAP3 PPA. Note: custom platforms may have different save sizes.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/pm34xx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index b20ecf5..bbb1a40 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -56,7 +56,7 @@
 
 /* Secure ram save size - store the defaults */
 static struct omap3_secure_copy_data secure_copy_data = {
-	.size = 0x803F,
+	.size = 0xF040,			/* 60k + 64 byte header */
 	.save_every_cycle = false,	/* explicit for readability */
 };
 
-- 
1.6.3.3

[PATCH 07/13] OMAP3: PM: allocate secure RAM context memory from low-mem

[Nishanth Menon]

From: Tero Kristo <tero.kristo@nokia.com>

Currently memory is allocated from kernel heap, which is located at high-mem.
Low-mem allocation is needed due to limitation in ROMCode which mirrors
memory every 256MB of memory blocks back to the first block

Allocation needs to be done later in the process compared to the traditional
reserve as the size required to be allocated depends on the cpu type we are
booting on, and is done as part of the map_io path instead of the reserve
path

[nm@ti.com: modified to use memblock and rebased to 2.6.37-rc2]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/io.c     |   11 +++++++++++
 arch/arm/mach-omap2/pm.h     |    2 ++
 arch/arm/mach-omap2/pm34xx.c |   39 ++++++++++++++++++++++++++++++---------
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 40562dd..6098f81 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -41,6 +41,7 @@
 #include <plat/omap-pm.h>
 #include <plat/powerdomain.h>
 #include "powerdomains.h"
+#include "pm.h"
 
 #include <plat/clockdomain.h>
 #include "clockdomains.h"
@@ -230,6 +231,13 @@ static struct map_desc omap44xx_io_desc[] __initdata = {
 };
 #endif
 
+static void __init _omap_pm_reserve_sdram_memblock(void)
+{
+	if (cpu_is_omap34xx())
+		omap3_pm_reserve_sdram_memblock();
+
+}
+
 static void __init _omap2_map_common_io(void)
 {
 	/* Normally devicemaps_init() would flush caches and tlb after
@@ -241,6 +249,9 @@ static void __init _omap2_map_common_io(void)
 
 	omap2_check_revision();
 	omap_sram_init();
+
+	/* Allocate the secure SRAM storage after sram init and cpu detect */
+	_omap_pm_reserve_sdram_memblock();
 }
 
 #ifdef CONFIG_ARCH_OMAP2420
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 39934ec..fcca056 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -114,11 +114,13 @@ struct omap3_secure_copy_data {
 
 #if defined(CONFIG_PM)
 extern int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *d);
+extern void __init omap3_pm_reserve_sdram_memblock(void);
 #else
 static inline int omap3_secure_copy_data_set(struct omap3_secure_copy_data *d)
 {
 	return -EINVAL;
 }
+static inline void omap3_pm_reserve_sdram_memblock(void) { }
 #endif
 
 #endif
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index bbb1a40..7877f74 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -28,6 +28,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/memblock.h>
 
 #include <plat/sram.h>
 #include <plat/clockdomain.h>
@@ -60,6 +61,8 @@ static struct omap3_secure_copy_data secure_copy_data = {
 	.save_every_cycle = false,	/* explicit for readability */
 };
 
+#define OMAP3_SECURE_MAX_ALLOCATE_ADDRESS 0x8fffffff
+
 struct power_state {
 	struct powerdomain *pwrdm;
 	u32 next_state;
@@ -198,7 +201,7 @@ static void omap3_save_secure_ram_context(u32 target_mpu_state)
 		 */
 		pwrdm_set_next_pwrst(mpu_pwrdm, PWRDM_POWER_ON);
 		secure_ram_save_status = _omap_save_secure_sram((u32 *)
-				__pa(omap3_secure_ram_storage));
+				(omap3_secure_ram_storage));
 		pwrdm_set_next_pwrst(mpu_pwrdm, target_mpu_state);
 		if (!secure_copy_data.save_every_cycle)
 			secure_ram_saved = 1;
@@ -1065,14 +1068,6 @@ static int __init omap3_pm_init(void)
 	omap3_idle_init();
 
 	clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
-	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
-		omap3_secure_ram_storage =
-			kmalloc(secure_copy_data.size, GFP_KERNEL);
-		if (!omap3_secure_ram_storage)
-			printk(KERN_ERR "Memory allocation failed when"
-					"allocating for secure sram context\n");
-
-	}
 
 	omap3_save_scratchpad_contents();
 err1:
@@ -1087,3 +1082,29 @@ err2:
 }
 
 late_initcall(omap3_pm_init);
+
+void __init omap3_pm_reserve_sdram_memblock(void)
+{
+	phys_addr_t size = secure_copy_data.size;
+	phys_addr_t paddr;
+	phys_addr_t max_addr = OMAP3_SECURE_MAX_ALLOCATE_ADDRESS;
+
+	if (!size || !cpu_is_omap34xx() || omap_type() == OMAP2_DEVICE_TYPE_GP)
+		return;
+
+	/*
+	 * On OMAP3 Silicon, due to ROM Code limitation, we should
+	 * not have the allocation beyond the first 256MB area.
+	 * This limitation is true for both OMAP3430 and 3630 and
+	 * all versions of the same.
+	 */
+	paddr = memblock_alloc_base(size, SZ_1M, max_addr);
+
+	if (!paddr) {
+		pr_err("%s: failed to reserve %x bytes\n",
+				__func__, size);
+		return;
+	}
+
+	omap3_secure_ram_storage = (void *)paddr;
+}
-- 
1.6.3.3

[PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

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

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.

[nm@ti.com: rebased to 2.6.37-rc2, used omap2_clkdm_deny_idle for
clock prevention]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

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

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>
> 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

> [nm@ti.com: rebased to 2.6.37-rc2, used omap2_clkdm_deny_idle for
> clock prevention]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 7877f74..f520b38 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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;
>  	}

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2010 11:08 AM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>>
>> 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. 
yep, ROM code does use WFI in this path - I unfortunately cannot go into 
the details on why it does it :( - will modify the commit to be explicit 
and state that it does use WFI in this path.

> 
> 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.
After a long review, the impacted section is this logic alone.

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

not for this case.

> 
> 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.
We need to do save secureram for hitting OFF - depending on the PPA - we 
may need to do save secureram in every iteration, the criteria needed 
here to workaround the "limitation" we have in ROM Code is to deny 
idling the clocks as well for this path as done here.


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

As mentioned above, This is an isolated case based on our analysis.

> 
> Kevin
> 
>> [nm@ti.com: rebased to 2.6.37-rc2, used omap2_clkdm_deny_idle for
>> clock prevention]
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
>> ---
>>  arch/arm/mach-omap2/pm34xx.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index 7877f74..f520b38 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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;
>>  	}


-- 
Regards,
Nishanth Menon

RE: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] 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
>
> Nishanth Menon <nm@ti.com> writes:
>
> > From: Eduardo Valentin <eduardo.valentin@nokia.com>
> >
> > 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.
>
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.

Regards,
Santosh

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

Santosh Shilimkar had written, on 11/19/2010 11:18 AM, the following:
[..]
>> 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.
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.

-- 
Regards,
Nishanth Menon

RE: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Santosh Shilimkar]

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

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

Santosh Shilimkar had written, on 11/19/2010 11:28 AM, the following:
>> -----Original Message-----
>> From: Nishanth Menon [mailto:nm@ti.com]
>> 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:
>> [..]
>>>> 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.
>> 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.

[1]http://www.spinics.net/lists/linux-omap/msg37907.html

-- 
Regards,
Nishanth Menon

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Santosh Shilimkar had written, on 11/19/2010 11:28 AM, the following:
>>> -----Original Message-----
>>> From: Nishanth Menon [mailto:nm@ti.com]
>>> 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:
>>> [..]
>>>>> 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.
>>> 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

Ah, you're right.  I hadn't noticed that the current code is already
setting he MPU powerdomain to on.

> - so [1] should also have removed that - 

Agreed.

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.

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

Tero had a series a while back[1] that addressed this in a more general
way.  IIRC, with his series, he generalized the powerdomain states so
you could set a powerdomain to on, denying clkdm idle, or you could set
the powerdomain to inactive, allowing clkdm idle.

ISTR that I quite liked the inactive support from Tero in that series,
but Paul and myself had some issues with how the IDLEST bits were
managed in that series, which could probably be addressed with hwmod
today.

I think it's time to revisit this series from Tero.

Kevin

[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg25268.html

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
[...]
> 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 :(

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

[...]

-- 
Regards,
Nishanth Menon

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
> [...]
>> 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 :(

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

/me no likey

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

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.

Kevin

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2010 03:06 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
>> [...]
>>> 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 :(
> 
> Hmm, so the complexity and mess is pushed into the OMAP PM core...
> 
> /me no likey
/me neither :(

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

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

[1] 
http://www.google.com/search?q=Dmitry+Kasatkin+site%3Apatchwork.kernel.org&hl=en&num=10&lr=&ft=i&cr=&safe=images


-- 
Regards,
Nishanth Menon

RE: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Santosh Shilimkar]

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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:
> > Nishanth Menon <nm@ti.com> writes:
> >
> >> Kevin Hilman had written, on 11/19/2010 02:39 PM, the following:
> >> [...]
> >>> 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 :(
> >
> > Hmm, so the complexity and mess is pushed into the OMAP PM core...
> >
> > /me no likey
> /me neither :(
>
> >
> >>>> 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.
> >> 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?
> >
> > 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.
>
> > 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?
>
I read the thread again. Indeed the change log confused me as well.
The original patch as such is ok then. I thought you want to prevent
MPU PD not to transition and that you are achieving by not allowing
MPU clock domain idle.

Then your patch seems to be right fix.

Regards
Santosh

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Kevin Hilman]

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

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

Yes.

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

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

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

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

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

-- 
Regards,
Nishanth Menon

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 11/19/2010 01:41 PM, the following:
>
>>>> 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.
>
>>
>> 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,

Sorry if that sounded like a beating. 

Maybe this as a gentler way of saying the same thing: if information
about which SMIs are using WFI cannot be made public, we have no choice
but to protect them all.

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

> 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

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2010 02:55 PM, the following:
[...]
> 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.


[..]

Ref:
[1]http://groups.google.com/group/pandaboard/browse_thread/thread/b9a28cab32e10fd2/067acd6b61c1e9fa
-- 
Regards,
Nishanth Menon

Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 11/19/2010 02:55 PM, the following:
> [...]
>> 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] - 

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

Kevin

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

RE: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure RAM

[Santosh Shilimkar]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> 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
>
> Nishanth Menon <nm@ti.com> writes:
>
> > Kevin Hilman had written, on 11/19/2010 02:55 PM, the following:
> > [...]
> >> 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] -
>
> All the more reason that this secure mode code should be moved out of
> the core idle path into its own driver.
>
I also think this is better idea.

[PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Nishanth Menon]

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

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 <eduardo.valentin@nokia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index f520b38..c7e2db0 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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
-- 
1.6.3.3

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Jean Pihet]

On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>
> 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 <eduardo.valentin@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index f520b38..c7e2db0 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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.
> +        */
The comment should mention the errata ID i540, for easier readability
and maintenance.

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

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Nishanth Menon]

Jean Pihet wrote, on 11/19/2010 04:09 AM:
[...]
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index f520b38..c7e2db0 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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.
>> +        */
> 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

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Jean Pihet]

On Fri, Nov 19, 2010 at 1:12 PM, Nishanth Menon <nm@ti.com> wrote:
> Jean Pihet wrote, on 11/19/2010 04:09 AM:
> [...]
>>>
>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>>> index f520b38..c7e2db0 100644
>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>> @@ -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.
>>> +        */
>>
>> 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
Argh I did the same mistake. My latin lessons must be too for away now ;-)

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

Thanks,
Jean

>
> --
> Regards,
> Nishanth Menon
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>
> 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.

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

> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index f520b38..c7e2db0 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2010 11:15 AM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>>
>> 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.
> 
> 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..

> 
> Kevin
> 
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
>> ---
>>  arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index f520b38..c7e2db0 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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


-- 
Regards,
Nishanth Menon

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 11/19/2010 11:15 AM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>>>
>>> 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.
>>
>> 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..

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.

Kevin




>
>>
>> Kevin
>>
>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
>>> ---
>>>  arch/arm/mach-omap2/pm34xx.c |   10 ++++++++--
>>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>>> index f520b38..c7e2db0 100644
>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>> @@ -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

Re: [PATCH 09/13] OMAP3: PM: Apply errata i540 before save secure ram

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2010 01:47 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Kevin Hilman had written, on 11/19/2010 11:15 AM, the following:
>>> Nishanth Menon <nm@ti.com> writes:
>>>
>>>> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>>>>
>>>> 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.
>>> 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..
> 
> 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.
> 

yep, Thanks, this is indeed a better description - will take it in as 
part of my v2.

-- 
Regards,
Nishanth Menon

[PATCH 10/13] OMAP3: PM: Errata i582: per domain reset issue: uart

[Nishanth Menon]

From: Archana Sriram <archana.sriram@ti.com>

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 <tero.kristo@nokia.com>
Tested-by: Eduardo Valentin <eduardo.valentin@nokia.com>
Tested-by: Westerberg Mika <ext-mika.1.westerberg@nokia.com>

[vishwanath.bs@ti.com: initial code and suggestions]
Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
[nm@ti.com: forward ported to 2.6.37-rc2 and suggestions]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Archana Sriram <archana.sriram@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index c7e2db0..218d0b0 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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)
@@ -496,6 +501,23 @@ void omap_sram_idle(void)
 			omap3_per_restore_context();
 		omap_uart_resume_idle(2);
 		omap_uart_resume_idle(3);
+		if (IS_PM34XX_ERRATA(PER_WAKEUP_ERRATA_i582) &&
+				omap_uart_check_per_uarts_used() &&
+				(core_prev_state == PWRDM_POWER_ON) &&
+				(per_prev_state == PWRDM_POWER_OFF)) {
+			/*
+			 * 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
@@ -1067,6 +1101,7 @@ static int __init omap3_pm_init(void)
 	neon_clkdm = clkdm_lookup("neon_clkdm");
 	mpu_clkdm = clkdm_lookup("mpu_clkdm");
 	per_clkdm = clkdm_lookup("per_clkdm");
+	wkup_clkdm = clkdm_lookup("wkup_clkdm");
 	core_clkdm = clkdm_lookup("core_clkdm");
 
 	omap_push_sram_idle();
@@ -1077,6 +1112,10 @@ static int __init omap3_pm_init(void)
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();
 
+	/* Allow per to wakeup the system if errata is applicable */
+	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();
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index becf0e3..43c2ec4 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -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) {}
diff --git a/arch/arm/plat-omap/include/plat/serial.h b/arch/arm/plat-omap/include/plat/serial.h
index 19145f5..81169b2 100644
--- a/arch/arm/plat-omap/include/plat/serial.h
+++ b/arch/arm/plat-omap/include/plat/serial.h
@@ -102,6 +102,10 @@ extern void omap_uart_prepare_suspend(void);
 extern void omap_uart_prepare_idle(int num);
 extern void omap_uart_resume_idle(int num);
 extern void omap_uart_enable_irqs(int enable);
+#ifdef CONFIG_PM
+extern int omap_uart_per_errata(void);
+extern int omap_uart_check_per_uarts_used(void);
+#endif
 #endif
 
 #endif
-- 
1.6.3.3

Re: [PATCH 10/13] OMAP3: PM: Errata i582: per domain reset issue: uart

[Kevin Hilman]

[ +Govindraj for omap-serial ]

Nishanth Menon <nm@ti.com> writes:

> From: Archana Sriram <archana.sriram@ti.com>
>
> 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.

We currently prevent this from happening in CPUidle: omap3_enter_idle_bm.  
Its not clear if this patch is trying to do something additional

> * 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 <tero.kristo@nokia.com>
> Tested-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> Tested-by: Westerberg Mika <ext-mika.1.westerberg@nokia.com>
>
> [vishwanath.bs@ti.com: initial code and suggestions]
> Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
> [nm@ti.com: forward ported to 2.6.37-rc2 and suggestions]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Archana Sriram <archana.sriram@ti.com>

I don't think the workaround for this erratum belongs in the PM core.

Rather, it seems to me that it belongs in the UART core, or even better,
the omap-serial driver (once it grows runtime PM support, which should
happen real soon now.)

We would like to to keep device specific idle/errata management in
device specific code wherever possible.  This seems like an example
where this will be straight forward.

> ---
>  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(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index c7e2db0..218d0b0 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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)
> @@ -496,6 +501,23 @@ void omap_sram_idle(void)
>  			omap3_per_restore_context();
>  		omap_uart_resume_idle(2);
>  		omap_uart_resume_idle(3);
> +		if (IS_PM34XX_ERRATA(PER_WAKEUP_ERRATA_i582) &&
> +				omap_uart_check_per_uarts_used() &&
> +				(core_prev_state == PWRDM_POWER_ON) &&
> +				(per_prev_state == PWRDM_POWER_OFF)) {

If the condition above is prevented, how is this happening?

As stated above, why can't this checking be done inside the UART core
(in this case omap_uart_resume_idle() ?)

> +			/*
> +			 * 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
> @@ -1067,6 +1101,7 @@ static int __init omap3_pm_init(void)
>  	neon_clkdm = clkdm_lookup("neon_clkdm");
>  	mpu_clkdm = clkdm_lookup("mpu_clkdm");
>  	per_clkdm = clkdm_lookup("per_clkdm");
> +	wkup_clkdm = clkdm_lookup("wkup_clkdm");

this isn't used in this patch

>  	core_clkdm = clkdm_lookup("core_clkdm");
>  
>  	omap_push_sram_idle();
> @@ -1077,6 +1112,10 @@ static int __init omap3_pm_init(void)
>  	pm_idle = omap3_pm_idle;
>  	omap3_idle_init();
>  
> +	/* Allow per to wakeup the system if errata is applicable */

This comment isn't accurate with what the code is doing.  Adding a
wakedep does not necessarily allow per to wakeup the system.

> +	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();
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index becf0e3..43c2ec4 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -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) {}
> diff --git a/arch/arm/plat-omap/include/plat/serial.h b/arch/arm/plat-omap/include/plat/serial.h
> index 19145f5..81169b2 100644
> --- a/arch/arm/plat-omap/include/plat/serial.h
> +++ b/arch/arm/plat-omap/include/plat/serial.h
> @@ -102,6 +102,10 @@ extern void omap_uart_prepare_suspend(void);
>  extern void omap_uart_prepare_idle(int num);
>  extern void omap_uart_resume_idle(int num);
>  extern void omap_uart_enable_irqs(int enable);
> +#ifdef CONFIG_PM
> +extern int omap_uart_per_errata(void);
> +extern int omap_uart_check_per_uarts_used(void);
> +#endif
>  #endif
>  
>  #endif

Kevin

[PATCH 11/13] OMAP3630: PM: Errata i608: disable RTA

[Nishanth Menon]

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 <ambresh@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 1fa3294..728f268 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -241,7 +241,10 @@ void omap3_save_scratchpad_contents(void)
 
 	/* Populate the Scratchpad contents */
 	scratchpad_contents.boot_config_ptr = 0x0;
-	if (omap_rev() != OMAP3430_REV_ES3_0 &&
+	if (cpu_is_omap3630())
+		scratchpad_contents.public_restore_ptr =
+			virt_to_phys(get_omap3630_restore_pointer());
+	else if (omap_rev() != OMAP3430_REV_ES3_0 &&
 					omap_rev() != OMAP3430_REV_ES3_1)
 		scratchpad_contents.public_restore_ptr =
 			virt_to_phys(get_restore_pointer());
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index b6c6b7c..d7911c5 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -204,6 +204,10 @@
 #define OMAP343X_CONTROL_WKUP_DEBOBS3 (OMAP343X_CONTROL_GENERAL_WKUP + 0x014)
 #define OMAP343X_CONTROL_WKUP_DEBOBS4 (OMAP343X_CONTROL_GENERAL_WKUP + 0x018)
 
+/* 36xx-only RTA - Retention till Accesss control registers and bits */
+#define OMAP36XX_CONTROL_MEM_RTA_CTRL	0x40C
+#define OMAP36XX_RTA_DISABLE		0x0
+
 /* 34xx D2D idle-related pins, handled by PM core */
 #define OMAP3_PADCONF_SAD2D_MSTANDBY   0x250
 #define OMAP3_PADCONF_SAD2D_IDLEACK    0x254
@@ -347,6 +351,7 @@ extern void omap3_save_scratchpad_contents(void);
 extern void omap3_clear_scratchpad_contents(void);
 extern u32 *get_restore_pointer(void);
 extern u32 *get_es3_restore_pointer(void);
+extern u32 *get_omap3630_restore_pointer(void);
 extern u32 omap3_arm_context[128];
 extern void omap3_control_save_context(void);
 extern void omap3_control_restore_context(void);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 218d0b0..1ced230 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -85,6 +85,7 @@ static int secure_ram_save_status;
 static int secure_ram_saved;
 
 #define PER_WAKEUP_ERRATA_i582	(1 << 0)
+#define RTA_ERRATA_i608		(1 << 1)
 static u16 pm34xx_errata;
 #define IS_PM34XX_ERRATA(id)	(pm34xx_errata & (id))
 
@@ -1049,6 +1050,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())
+			pm34xx_errata |= RTA_ERRATA_i608;
 	}
 }
 
@@ -1115,6 +1118,15 @@ static int __init omap3_pm_init(void)
 	/* Allow per to wakeup the system if errata is applicable */
 	if (IS_PM34XX_ERRATA(PER_WAKEUP_ERRATA_i582) && cpu_is_omap34xx())
 		clkdm_add_wkdep(per_clkdm, wkup_clkdm);
+	/*
+	 * RTA is disabled during initialization as per errata i608
+	 * it is safer to disable rta by the bootloader, but we would like
+	 * to be doubly sure here and prevent any mishaps.
+	 */
+	if (IS_PM34XX_ERRATA(RTA_ERRATA_i608))
+		omap_ctrl_writel(OMAP36XX_RTA_DISABLE,
+			OMAP36XX_CONTROL_MEM_RTA_CTRL);
+
 
 	clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
 
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 5a4468f..7259541 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -45,6 +45,8 @@
 #define CM_IDLEST_CKGEN_V	OMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST)
 #define SRAM_BASE_P		0x40200000
 #define CONTROL_STAT		0x480022F0
+#define CONTROL_MEM_RTA_CTRL	(OMAP343X_CTRL_BASE\
+					+ OMAP36XX_CONTROL_MEM_RTA_CTRL)
 #define SCRATCHPAD_MEM_OFFS	0x310 /* Move this as correct place is
 				       * available */
 #define SCRATCHPAD_BASE_P	(OMAP343X_CTRL_BASE + OMAP343X_CONTROL_MEM_WKUP\
@@ -99,6 +101,14 @@ ENTRY(get_restore_pointer)
         ldmfd   sp!, {pc}     @ restore regs and return
 ENTRY(get_restore_pointer_sz)
         .word   . - get_restore_pointer
+	.text
+/* Function call to get the restore pointer for 3630 resume from OFF */
+ENTRY(get_omap3630_restore_pointer)
+        stmfd   sp!, {lr}     @ save registers on stack
+	adr	r0, restore_3630
+        ldmfd   sp!, {pc}     @ restore regs and return
+ENTRY(get_omap3630_restore_pointer_sz)
+        .word   . - get_omap3630_restore_pointer
 
 	.text
 /* Function call to get the restore pointer for for ES3 to resume from OFF */
@@ -246,6 +256,19 @@ copy_to_sram:
 	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*/
@@ -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

Re: [PATCH 11/13] OMAP3630: PM: Errata i608: disable RTA

[Jean Pihet]

On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
> 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 <ambresh@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  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(-)
>
...

> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index 5a4468f..7259541 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
...
>  /* Function call to get the restore pointer for for ES3 to resume from OFF */
> @@ -246,6 +256,19 @@ copy_to_sram:
This code is from 'restore_es3' which ends with a pass-through to 'restore:'...

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

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

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/13] OMAP3630: PM: Errata i608: disable RTA

[Nishanth Menon]

Jean Pihet wrote, on 11/19/2010 03:57 AM:
[...]
>> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
>> index 5a4468f..7259541 100644
>> --- a/arch/arm/mach-omap2/sleep34xx.S
>> +++ b/arch/arm/mach-omap2/sleep34xx.S
> ...
>>   /* Function call to get the restore pointer for for ES3 to resume from OFF */
>> @@ -246,6 +256,19 @@ copy_to_sram:
> This code is from 'restore_es3' which ends with a pass-through to 'restore:'...
>
>>         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?
arrgh.. thanks for catching it
I did run it on a SDP3430 ES3.1 platform.. hmmm... I did not intend that 
behavior though - there should be a b restore here!

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

During the initial days of 3630 code introduction we found that almost 
all of the code with 34xx could be reused for 3630 as well.. what ended 
up happening is 34xx now means 3630&3430, while 343x holds to the 3430 
family :(.. cpu_is_omap34xx() as an example - ideally should have been 
cpu_is_omap3xxx() ;) ..I guess it is becoming more of a convention to do 
3xxx for new code and leaving the old code as is :(

-- 
Regards,
Nishanth Menon

[PATCH 12/13] OMAP3630: PM: Disable L2 cache while invalidating L2 cache

[Nishanth Menon]

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

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

[nm@ti.com: ported to 2.6.37-rc2, added hooks to enable the logic only on 3630]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
Signed-off-by: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
---
 arch/arm/mach-omap2/pm.h        |    2 ++
 arch/arm/mach-omap2/pm34xx.c    |    4 ++++
 arch/arm/mach-omap2/sleep34xx.S |   31 +++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index fcca056..d50a1fc 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -115,12 +115,14 @@ struct omap3_secure_copy_data {
 #if defined(CONFIG_PM)
 extern int __init omap3_secure_copy_data_set(struct omap3_secure_copy_data *d);
 extern void __init omap3_pm_reserve_sdram_memblock(void);
+extern void enable_omap3630_toggle_l2_on_restore(void);
 #else
 static inline int omap3_secure_copy_data_set(struct omap3_secure_copy_data *d)
 {
 	return -EINVAL;
 }
 static inline void omap3_pm_reserve_sdram_memblock(void) { }
+static inline void enable_omap3630_toggle_l2_on_restore(void) { }
 #endif
 
 #endif
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 1ced230..0102d60 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -1052,6 +1052,10 @@ static void pm_errata_configure(void)
 			pm34xx_errata &= ~PER_WAKEUP_ERRATA_i582;
 		if (cpu_is_omap3630())
 			pm34xx_errata |= RTA_ERRATA_i608;
+		/* Enable the l2 cache toggling in sleep logic */
+		if (cpu_is_omap3630())
+			enable_omap3630_toggle_l2_on_restore();
+
 	}
 }
 
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 7259541..a5c63a6 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -111,6 +111,19 @@ ENTRY(get_omap3630_restore_pointer_sz)
         .word   . - get_omap3630_restore_pointer
 
 	.text
+/*
+ * L2 cache needs to be toggled for stable OFF mode functionality on 3630.
+ * This function sets up a fflag that will allow for this toggling to take
+ * place on 3630. Hopefully some version in the future maynot need this
+ */
+ENTRY(enable_omap3630_toggle_l2_on_restore)
+        stmfd   sp!, {lr}     @ save registers on stack
+	/* Setup so that we will disable and enable l2 */
+	mov	r1, #0x1
+	str	r1, l2dis_3630
+        ldmfd   sp!, {pc}     @ restore regs and return
+
+	.text
 /* Function call to get the restore pointer for for ES3 to resume from OFF */
 ENTRY(get_es3_restore_pointer)
 	stmfd	sp!, {lr}	@ save registers on stack
@@ -119,6 +132,7 @@ ENTRY(get_es3_restore_pointer)
 ENTRY(get_es3_restore_pointer_sz)
 	.word	. - get_es3_restore_pointer
 
+
 ENTRY(es3_sdrc_fix)
 	ldr	r4, sdrc_syscfg		@ get config addr
 	ldr	r5, [r4]		@ get value
@@ -282,6 +296,14 @@ restore:
         moveq   r9, #0x3        @ MPU OFF => L1 and L2 lost
 	movne	r9, #0x1	@ Only L1 and L2 lost => avoid L2 invalidation
 	bne	logic_l1_restore
+
+	ldr	r0, l2dis_3630
+	cmp	r0, #0x1	@ should we disable L2 on 3630?
+	bne	skipl2dis
+	mrc	p15, 0, r0, c1, c0, 1
+	bic	r0, r0, #2	@ disable L2 cache
+	mcr	p15, 0, r0, c1, c0, 1
+skipl2dis:
 	ldr	r0, control_stat
 	ldr	r1, [r0]
 	and	r1, #0x700
@@ -342,6 +364,13 @@ smi:    .word 0xE1600070		@ Call SMI monitor (smieq)
 	mov	r12, #0x2
 	.word 0xE1600070	@ Call SMI monitor (smieq)
 logic_l1_restore:
+	ldr	r1, l2dis_3630
+	cmp	r1, #0x1	@ Do we need to re-enable L2 on 3630?
+	bne	skipl2reen
+	mrc	p15, 0, r1, c1, c0, 1
+	orr	r1, r1, #2	@ re-enable L2 cache
+	mcr	p15, 0, r1, c1, c0, 1
+skipl2reen:
 	mov	r1, #0
 	/* Invalidate all instruction caches to PoU
 	 * and flush branch target cache */
@@ -677,6 +706,8 @@ control_mem_rta:
 	.word	CONTROL_MEM_RTA_CTRL
 kernel_flush:
 	.word v7_flush_dcache_all
+l2dis_3630:
+	.word 0
 	/* these 2 words need to be at the end !!! */
 kick_counter:
 	.word	0
-- 
1.6.3.3

[PATCH 13/13] OMAP3630: PM: Errata i583: disable coreoff if < ES1.2

[Nishanth Menon]

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

Limitation i583: Self_Refresh Exit issue after OFF mode

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

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

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

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

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0102d60..1890e49 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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)
 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

Re: [PATCH 13/13] OMAP3630: PM: Errata i583: disable coreoff if < ES1.2

[Jean Pihet]

On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>
> Limitation i583: Self_Refresh Exit issue after OFF mode
>
> Issue:
> When device is waking up from OFF mode, then SDRC state machine sends
> inappropriate sequence violating JEDEC standards.
>
> Impact:
> OMAP3630 < ES1.2 is impacted as follows depending on the platform:
> CS0: for 38.4MHz as internal sysclk, DDR content seen to be stable, while
>        for all other sysclk frequencies, varied levels of instability
>        seen based on varied parameters.
> CS1: impacted
>
> This patch takes option #3 as recommended by the Silicon 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.
>
> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code changed a bit]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 0102d60..1890e49 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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).

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

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] OMAP3630: PM: Errata i583: disable coreoff if < ES1.2

[Nishanth Menon]

Jean Pihet wrote, on 11/19/2010 04:07 AM:
[..]
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index 0102d60..1890e49 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -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).

Thanks. you are absolutely right.. Oh boy, I need to check what the 
caffeine I have been drinking..

-- 
Regards,
Nishanth Menon

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Jean Pihet]

Hi,

On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon <nm@ti.com> wrote:
> 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
>

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?

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Nishanth Menon]

Jean Pihet wrote, on 11/19/2010 04:18 AM:
> Hi,
>
> On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon<nm@ti.com>  wrote:
>> 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
>>
>
> 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?

I have the same feel as well.. thanks for your review. I suggest we take 
some more test feedback as well to ensure that there are no more mess 
ups I might have done as the paths are critical.

-- 
Regards,
Nishanth Menon

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

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

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.

Kevin

> 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

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Nishanth Menon]

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

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

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

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

-- 
Regards,
Nishanth Menon

RE: [PATCH 00/13] OMAP3: OFF mode fixes

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] 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:
>
> >> 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.
>
> >
> > 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.

We already have omap44xx-smc.S. I could make this generic. One problem
with
The secure APIs, they are consistent in all OMAP version. For example
Secure
SAR save has a different API and signature on OMAP3 and OMAP4.
But this is something doable.

Regards,
Santosh

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 11/19/2010 03:20 PM, the following:
>
>>> 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.
>
>>
>> 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.

No, I'd like to see the secure code in your patch all in a separate
file.  Jean's cleanups are independent of better organization and
structure of your code.

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

The board-level configuration is only one (minor) reason to have a
separate driver for the secure stuff.   Discussions on the other patches
suggested other reasons for this as well, including some reasons from
you as well. :)

Kevin

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

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

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.

Stated otherwise, the PM core is doing the job that should be done by
secure driver(s).  Rather than continuing to extend that hack, it's time
for that to be fixed correctly in a way that can be maintainted.

Kevin

> 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

RE: [PATCH 00/13] OMAP3: OFF mode fixes

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] 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
>
> Nishanth Menon <nm@ti.com> writes:
>
> > 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.
>
> 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.
>
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.

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Kevin Hilman]

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] 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
>>
>> Nishanth Menon <nm@ti.com> writes:
>>
>> > 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.
>>
>> 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.
>>
> 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.

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.  

Kevin

RE: [PATCH 00/13] OMAP3: OFF mode fixes

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] 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
>
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] 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
> >>
> >> Nishanth Menon <nm@ti.com> writes:
> >>
> >> > 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.
> >>
> >> 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.
> >>
> > 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.
>
> 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.
>
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
RAM where the secure HW registers, firewalls, cache controllers, interrupt
controllers are managed using secure APIs. All of this is must for correct
CPU OFF functioning.

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

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


Regards,
Santosh

RE: [PATCH 00/13] OMAP3: OFF mode fixes

[Santosh Shilimkar]

(Sorry. You will see two replies from me on your email)
> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar@ti.com]
> 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
>
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] 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
> >
[...]
> > > 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.
> >
> 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
> RAM where the secure HW registers, firewalls, cache controllers,
interrupt
> controllers are managed using secure APIs. All of this is must for
correct
> CPU OFF functioning.
>
> > 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.
> >
> 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.
> 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.
>
> > 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.
>
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

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Jean Pihet]

Nishant,

On Wed, Nov 24, 2010 at 10:22 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> (Sorry. You will see two replies from me on your email)
>> -----Original Message-----
>> From: Santosh Shilimkar [mailto:santosh.shilimkar@ti.com]
>> 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
>>
>> > -----Original Message-----
>> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> > owner@vger.kernel.org] 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
>> >
> [...]
>> > > 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.
>> >
>> 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
>> RAM where the secure HW registers, firewalls, cache controllers,
> interrupt
>> controllers are managed using secure APIs. All of this is must for
> correct
>> CPU OFF functioning.
>>
>> > 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.
>> >
>> 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.
>> 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.
>>
>> > 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.
>>
> 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.

When the code is split into the pure sleep code and the secure code
parts, I would like to have the sleep code part cleaned up.
BTW I rebased my clean-up patch on top of the 13 patches you sent and
performed some testing. It is working OK AFAICT (also stands for "As
far as I can test" TM).

Can this happen before the .38 merge window?

>
> Regards
> Santosh
>

Thanks,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/13] OMAP3: OFF mode fixes

[Nishanth Menon]

Jean Pihet had written, on 11/24/2010 11:11 AM, the following:
[...]
> When the code is split into the pure sleep code and the secure code
> parts, I would like to have the sleep code part cleaned up.
> BTW I rebased my clean-up patch on top of the 13 patches you sent and
> performed some testing. It is working OK AFAICT (also stands for "As
> far as I can test" TM).
I could help with testing on EMU zoom3 and SDP3430 If you could host a 
tree somewhere. Maybe some folks could pitch in for testing with N900 
which has HS device as well.

> 
> Can this happen before the .38 merge window?
I plan to work on this set over the next week - hopefully we can do the 
testing and posting by then. will probably post offline my initial 
versions for sync up.

-- 
Regards,
Nishanth Menon