Re: [PATCH 2/2] OMAP3 PM: sleep code clean up

2010-10-04 Thread Amit Kucheria 
On 10 Oct 04, Sripathy, Vishwanath wrote:
 Kevin,
 
[snip]

 
  But this also makes me wonder, if we're going to clean this up, the bulk
  of it could be re-written in C, with some inline asm helpers as needed.
 Probably yes. But as this code is specific to OMAP3, do you think it's
 worth spending time on rewriting the entire code in C? It might be a
 significant effort and it will not be reusable for other ARM SOCs.

The C code will be more maintainable and offer more opportunities to
refactor across OMAP flavours - perhaps OMAP4 as well.

I'm also interested in code reuse across other SoCs. We've got to be able to
shut down the ARM core using common code instead of each SoC copying buggy
code from each other. Could you list reasons why you don't think the code is
re-usable?

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

RE: [PATCH 2/2] OMAP3 PM: sleep code clean up

2010-10-04 Thread Shilimkar, Santosh 
 -Original Message-
 From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
 ow...@vger.kernel.org] On Behalf Of Amit Kucheria
 Sent: Monday, October 04, 2010 2:27 PM
 To: Sripathy, Vishwanath
 Cc: Kevin Hilman; linux-omap@vger.kernel.org; linaro-...@lists.linaro.org
 Subject: Re: [PATCH 2/2] OMAP3 PM: sleep code clean up
 
 On 10 Oct 04, Sripathy, Vishwanath wrote:
  Kevin,
 
 [snip]
 
  
   But this also makes me wonder, if we're going to clean this up, the
 bulk
   of it could be re-written in C, with some inline asm helpers as needed.
  Probably yes. But as this code is specific to OMAP3, do you think it's
  worth spending time on rewriting the entire code in C? It might be a
  significant effort and it will not be reusable for other ARM SOCs.
 
 The C code will be more maintainable and offer more opportunities to
 refactor across OMAP flavours - perhaps OMAP4 as well.
 
 I'm also interested in code reuse across other SoCs. We've got to be able
 to
 shut down the ARM core using common code instead of each SoC copying buggy
 code from each other. Could you list reasons why you don't think the code
 is
 re-usable?
Trust zone implementation and it's varients
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] OMAP3 PM: sleep code clean up

2010-10-04 Thread Sripathy, Vishwanath 
Kevin,

 -Original Message-
 From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
 Sent: Saturday, September 25, 2010 1:35 AM
 To: Sripathy, Vishwanath
 Cc: linux-omap@vger.kernel.org; linaro-...@lists.linaro.org
 Subject: Re: [PATCH 2/2] OMAP3 PM: sleep code clean up

 Vishwanath BS vishwanath...@ti.com writes:

  This patch has done some clean up of omap3 sleep code.
  Basically all possible hardcodings are removed and code is Reorganized
  into more logical buckets for better readability and instrumentation.
 
  Tested on ZOOM3.

 Again, please describe more about how it was tested.  idle?  suspend?
 retention? off?
This has been tested for both RET and OFF mode in Idle and suspend path. Will 
update the change log for the same.

 Also please fix long-line checkpatch warnings.
OK.

 While breaking this up in to subroutines, why not just call them all
 from the C-code instead of assembly?
I thought about that. But unfortunately, while saving CPU registers (in 
save_context_wfi), LR also gets saved. So if I call wfi routine 
(omap34xx_do_wfi ) from C code itself, then LR would have pointed to 
omap34xx_do_wfi while saving
the registers which is not correct.

 But this also makes me wonder, if we're going to clean this up, the bulk
 of it could be re-written in C, with some inline asm helpers as needed.
Probably yes. But as this code is specific to OMAP3, do you think it's worth 
spending time on rewriting the entire code in C? It might be a significant 
effort and it will not be reusable for other ARM SOCs.

Vishwa

 Kevin

  Signed-off-by: Vishwanath BS vishwanath...@ti.com
  Cc: Kevin Hillman khil...@deeprootsystems.com
  Cc: Linaro linaro-...@lists.linaro.org
  ---
   arch/arm/mach-omap2/sleep34xx.S   |  377 ++---
 
   arch/arm/plat-omap/include/plat/control.h |2 +
   2 files changed, 189 insertions(+), 190 deletions(-)
 
  diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-
 omap2/sleep34xx.S
  index ba53191..734f82a
  --- a/arch/arm/mach-omap2/sleep34xx.S
  +++ b/arch/arm/mach-omap2/sleep34xx.S
  @@ -33,17 +33,20 @@
   #include prm.h
   #include sdrc.h
 
  -#define SDRC_SCRATCHPAD_SEM_V  0xfa00291c
  +#define SDRC_SCRATCHPAD_SEM_OFFS   0xc
  +#define SDRC_SCRATCHPAD_SEM_V  OMAP343X_SCRATCHPAD_REGADDR \
  +   
  (SDRC_SCRATCHPAD_SEM_OFFS)
 
   #define PM_PREPWSTST_CORE_VOMAP34XX_PRM_REGADDR(CORE_MOD, \
  -   OMAP3430_PM_PREPWSTST)
  -#define PM_PREPWSTST_CORE_P0x48306AE8
  +   OMAP3430_PM_PREPWSTST)
  +#define PM_PREPWSTST_CORE_POMAP3430_PRM_BASE + CORE_MOD + \
  +   
  OMAP3430_PM_PREPWSTST
   #define PM_PREPWSTST_MPU_V OMAP34XX_PRM_REGADDR(MPU_MOD, \
  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 SRAM_BASE_P0x4020
  -#define CONTROL_STAT   0x480022F0
  +#define SRAM_BASE_P0x4020
  +#define CONTROL_STAT   OMAP343X_CTRL_BASE +
 OMAP343X_CONTROL_STATUS
   #define SCRATCHPAD_MEM_OFFS0x310 /* Move this as correct place is
 * available */
   #define SCRATCHPAD_BASE_P  (OMAP343X_CTRL_BASE +
 OMAP343X_CONTROL_MEM_WKUP\
  @@ -184,29 +187,16 @@ api_params:
   ENTRY(save_secure_ram_context_sz)
  .word   . - save_secure_ram_context
 
  -/*
  - * Forces OMAP into idle state
  - *
  - * omap34xx_suspend() - This bit of code just executes the WFI
  - * for normal idles.
  - *
  - * Note: This code get's copied to internal SRAM at boot. When the OMAP
  - *  wakes up it continues execution at the point it went to sleep.
  - */
  -ENTRY(omap34xx_cpu_suspend)
  +/* Function to execute WFI. When the MPU wakes up from retention
  + * or inactive mode, it continues execution just after wfi */

 fix multi-line comment style

  +ENTRY(omap34xx_do_wfi)
  stmfd   sp!, {r0-r12, lr}   @ save registers on stack
  -loop:
  -   /*b loop*/  @Enable to debug by stepping through code
  -   /* r0 contains restore pointer in sdram */
  -   /* r1 contains information about saving context */
  +
  ldr r4, sdrc_power  @ read the SDRC_POWER register
  ldr r5, [r4]@ read the contents of SDRC_POWER
  orr r5, r5, #0x40   @ enable self refresh on idle req
  str r5, [r4]@ write back to SDRC_POWER register
 
  -   cmp r1, #0x0
  -   /* If context save is required, do that and execute wfi */
  -   bne save_context_wfi
  /* Data memory barrier and Data sync barrier */
  mov r1, #0
  mcr p15, 0, r1, c7, c10, 4
  @@ -225,8 +215,182 @@ loop:
  nop
  nop
  bl

Re: [PATCH 2/2] OMAP3 PM: sleep code clean up

2010-09-24 Thread Kevin Hilman 
Vishwanath BS vishwanath...@ti.com writes:

 This patch has done some clean up of omap3 sleep code.
 Basically all possible hardcodings are removed and code is Reorganized
 into more logical buckets for better readability and instrumentation.

 Tested on ZOOM3.

Again, please describe more about how it was tested.  idle?  suspend?
retention? off?

Also please fix long-line checkpatch warnings.

While breaking this up in to subroutines, why not just call them all
from the C-code instead of assembly?

But this also makes me wonder, if we're going to clean this up, the bulk
of it could be re-written in C, with some inline asm helpers as needed.

Kevin

 Signed-off-by: Vishwanath BS vishwanath...@ti.com
 Cc: Kevin Hillman khil...@deeprootsystems.com
 Cc: Linaro linaro-...@lists.linaro.org
 ---
  arch/arm/mach-omap2/sleep34xx.S   |  377 
 ++---
  arch/arm/plat-omap/include/plat/control.h |2 +
  2 files changed, 189 insertions(+), 190 deletions(-)

 diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
 index ba53191..734f82a
 --- a/arch/arm/mach-omap2/sleep34xx.S
 +++ b/arch/arm/mach-omap2/sleep34xx.S
 @@ -33,17 +33,20 @@
  #include prm.h
  #include sdrc.h
  
 -#define SDRC_SCRATCHPAD_SEM_V0xfa00291c
 +#define SDRC_SCRATCHPAD_SEM_OFFS 0xc
 +#define SDRC_SCRATCHPAD_SEM_VOMAP343X_SCRATCHPAD_REGADDR \
 + 
 (SDRC_SCRATCHPAD_SEM_OFFS)
  
  #define PM_PREPWSTST_CORE_V  OMAP34XX_PRM_REGADDR(CORE_MOD, \
 - OMAP3430_PM_PREPWSTST)
 -#define PM_PREPWSTST_CORE_P  0x48306AE8
 + OMAP3430_PM_PREPWSTST)
 +#define PM_PREPWSTST_CORE_P  OMAP3430_PRM_BASE + CORE_MOD + \
 + 
 OMAP3430_PM_PREPWSTST
  #define PM_PREPWSTST_MPU_V   OMAP34XX_PRM_REGADDR(MPU_MOD, \
   OMAP3430_PM_PREPWSTST)
  #define PM_PWSTCTRL_MPU_POMAP3430_PRM_BASE + MPU_MOD + OMAP2_PM_PWSTCTRL
  #define CM_IDLEST1_CORE_VOMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
 -#define SRAM_BASE_P  0x4020
 -#define CONTROL_STAT 0x480022F0
 +#define SRAM_BASE_P  0x4020
 +#define CONTROL_STAT OMAP343X_CTRL_BASE + OMAP343X_CONTROL_STATUS
  #define SCRATCHPAD_MEM_OFFS  0x310 /* Move this as correct place is
  * available */
  #define SCRATCHPAD_BASE_P(OMAP343X_CTRL_BASE + OMAP343X_CONTROL_MEM_WKUP\
 @@ -184,29 +187,16 @@ api_params:
  ENTRY(save_secure_ram_context_sz)
   .word   . - save_secure_ram_context
  
 -/*
 - * Forces OMAP into idle state
 - *
 - * omap34xx_suspend() - This bit of code just executes the WFI
 - * for normal idles.
 - *
 - * Note: This code get's copied to internal SRAM at boot. When the OMAP
 - *wakes up it continues execution at the point it went to sleep.
 - */
 -ENTRY(omap34xx_cpu_suspend)
 +/* Function to execute WFI. When the MPU wakes up from retention
 + * or inactive mode, it continues execution just after wfi */

fix multi-line comment style

 +ENTRY(omap34xx_do_wfi)
   stmfd   sp!, {r0-r12, lr}   @ save registers on stack
 -loop:
 - /*b loop*/  @Enable to debug by stepping through code
 - /* r0 contains restore pointer in sdram */
 - /* r1 contains information about saving context */
 +
   ldr r4, sdrc_power  @ read the SDRC_POWER register
   ldr r5, [r4]@ read the contents of SDRC_POWER
   orr r5, r5, #0x40   @ enable self refresh on idle req
   str r5, [r4]@ write back to SDRC_POWER register
  
 - cmp r1, #0x0
 - /* If context save is required, do that and execute wfi */
 - bne save_context_wfi
   /* Data memory barrier and Data sync barrier */
   mov r1, #0
   mcr p15, 0, r1, c7, c10, 4
 @@ -225,8 +215,182 @@ loop:
   nop
   nop
   bl wait_sdrc_ok
 + ldmfd   sp!, {r0-r12, pc}   @ restore regs and return
 +
 +/*
 + * Forces OMAP into idle state
 + *
 + * omap34xx_cpu_suspend() - This bit of code just executes the WFI
 + * for normal idles and saves the context before WFI on off modes.
 + *
 + */
 +
 +ENTRY(omap34xx_cpu_suspend)
 + stmfd   sp!, {r0-r12, lr}   @ save registers on stack
 +loop:
 + /*b loop*/  @Enable to debug by stepping through code
 + /* r0 contains restore pointer in sdram */
 + /* r1 contains information about saving context */
 +
 + cmp r1, #0x0
 + /* If context save is required, do that and execute wfi */
 + bne save_context_wfi
 + bl omap34xx_do_wfi
  
   ldmfd   sp!, {r0-r12, pc}   @ restore regs and return
 +
 +save_context_wfi:
 + /*b save_context_wfi*/  @ enable to debug save code
 + mov r8, r0 /* Store SDRAM address in r8 */
 + mrc p15, 0, r5, c1, c0,