* [PATCH] OMAP3: clean up ASM idle code
@ 2010-12-07 11:11 jean.pihet
2010-12-13 14:51 ` Vishwanath Sripathy
2010-12-14 4:12 ` Kevin Hilman
0 siblings, 2 replies; 6+ messages in thread
From: jean.pihet @ 2010-12-07 11:11 UTC (permalink / raw)
To: linux-omap; +Cc: Jean Pihet, Vishwanath BS
From: Jean Pihet <j-pihet@ti.com>
Clean up of the ASM code:
- reorganized the code in logical sections: defines, API
functions, internal functions and variables,
- reworked and simplified the execution paths, for better
readability and to avoid duplication of code,
- added comments on the entry and exit points,
- reworked the existing comments for better readability,
- reworked the code formating, alignment and white spaces,
- added comments for the i443 erratum workarounds,
- changed the hardcoded values in favor of existing macros
from include files,
- clean up of non used symbols.
Tested on OMAP3EVM and Beagleboard with full RET and OFF modes.
Heavily reworked from Vishwa's original patch.
Signed-off-by: Jean Pihet <j-pihet@ti.com>
Cc: Vishwanath BS <vishwanath.bs@ti.com>
---
Applies on top of Nishant's latest idle path errata fixes step 2,
cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2
arch/arm/mach-omap2/control.h | 2 +
arch/arm/mach-omap2/pm.h | 1 -
arch/arm/mach-omap2/pm34xx.c | 4 +-
arch/arm/mach-omap2/sleep34xx.S | 522 +++++++++++++++++++++------------------
4 files changed, 288 insertions(+), 241 deletions(-)
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index d7911c5..72efefb 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -274,6 +274,8 @@
#define OMAP343X_SCRATCHPAD_ROM (OMAP343X_CTRL_BASE + 0x860)
#define OMAP343X_SCRATCHPAD (OMAP343X_CTRL_BASE + 0x910)
#define OMAP343X_SCRATCHPAD_ROM_OFFSET 0x19C
+#define OMAP343X_SCRATCHPAD_REGADDR(reg) OMAP2_L4_IO_ADDRESS(\
+ OMAP343X_SCRATCHPAD + reg)
/* AM35XX_CONTROL_IPSS_CLK_CTRL bits */
#define AM35XX_USBOTG_VBUSP_CLK_SHIFT 0
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index aff39d0..e458b2a 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -80,7 +80,6 @@ extern void save_secure_ram_context(u32 *addr);
extern void omap3_save_scratchpad_contents(void);
extern unsigned int omap24xx_idle_loop_suspend_sz;
-extern unsigned int omap34xx_suspend_sz;
extern unsigned int save_secure_ram_context_sz;
extern unsigned int omap24xx_cpu_suspend_sz;
extern unsigned int omap34xx_cpu_suspend_sz;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index a37d53d..c93b872 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -135,7 +135,7 @@ static void omap3_core_save_context(void)
/*
* Force write last pad into memory, as this can fail in some
- * cases according to erratas 1.157, 1.185
+ * cases according to errata 1.157 & 1.185
*/
omap_ctrl_writel(omap_ctrl_readl(OMAP343X_PADCONF_ETK_D14),
OMAP343X_CONTROL_MEM_WKUP + 0x2a0);
@@ -432,7 +432,7 @@ void omap_sram_idle(void)
/*
* On EMU/HS devices ROM code restores a SRDC value
* from scratchpad which has automatic self refresh on timeout
- * of AUTO_CNT = 1 enabled. This takes care of errata 1.142.
+ * of AUTO_CNT = 1 enabled. This takes care of erratum ID i443.
* Hence store/restore the SDRC_POWER register here.
*/
if (omap_rev() >= OMAP3430_REV_ES3_0 &&
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index d2eda01..96a4864 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -33,24 +33,27 @@
#include "sdrc.h"
#include "control.h"
-#define SDRC_SCRATCHPAD_SEM_V 0xfa00291c
-
-#define PM_PREPWSTST_CORE_V OMAP34XX_PRM_REGADDR(CORE_MOD, \
- OMAP3430_PM_PREPWSTST)
-#define PM_PREPWSTST_CORE_P 0x48306AE8
-#define PM_PREPWSTST_MPU_V OMAP34XX_PRM_REGADDR(MPU_MOD, \
- OMAP3430_PM_PREPWSTST)
+/*
+ * Registers access definitions
+ */
+#define SDRC_SCRATCHPAD_SEM_OFFS 0xc
+#define SDRC_SCRATCHPAD_SEM_V OMAP343X_SCRATCHPAD_REGADDR\
+ (SDRC_SCRATCHPAD_SEM_OFFS)
+#define PM_PREPWSTST_CORE_P OMAP3430_PRM_BASE + CORE_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 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\
- + SCRATCHPAD_MEM_OFFS)
+#define CONTROL_STAT OMAP343X_CTRL_BASE + OMAP343X_CONTROL_STATUS
+#define CONTROL_MEM_RTA_CTRL (OMAP343X_CTRL_BASE +\
+ OMAP36XX_CONTROL_MEM_RTA_CTRL)
+
+/* Move this as correct place is available */
+#define SCRATCHPAD_MEM_OFFS 0x310
+#define SCRATCHPAD_BASE_P (OMAP343X_CTRL_BASE +\
+ OMAP343X_CONTROL_MEM_WKUP +\
+ SCRATCHPAD_MEM_OFFS)
#define SDRC_POWER_V OMAP34XX_SDRC_REGADDR(SDRC_POWER)
#define SDRC_SYSCONFIG_P (OMAP343X_SDRC_BASE + SDRC_SYSCONFIG)
#define SDRC_MR_0_P (OMAP343X_SDRC_BASE + SDRC_MR_0)
@@ -62,6 +65,10 @@
#define SDRC_DLLA_STATUS_V OMAP34XX_SDRC_REGADDR(SDRC_DLLA_STATUS)
#define SDRC_DLLA_CTRL_V OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
+
+/*
+ * API functions
+ */
.text
/* Function to acquire the semaphore in scratchpad */
ENTRY(lock_scratchpad_sem)
@@ -113,7 +120,7 @@ ENTRY(get_omap3630_restore_pointer_sz)
.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
+ * This function sets up a flag 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)
@@ -132,49 +139,6 @@ 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
- tst r5, #0x100 @ is part access blocked
- it eq
- biceq r5, r5, #0x100 @ clear bit if set
- str r5, [r4] @ write back change
- ldr r4, sdrc_mr_0 @ get config addr
- ldr r5, [r4] @ get value
- str r5, [r4] @ write back change
- ldr r4, sdrc_emr2_0 @ get config addr
- ldr r5, [r4] @ get value
- str r5, [r4] @ write back change
- ldr r4, sdrc_manual_0 @ get config addr
- mov r5, #0x2 @ autorefresh command
- str r5, [r4] @ kick off refreshes
- ldr r4, sdrc_mr_1 @ get config addr
- ldr r5, [r4] @ get value
- str r5, [r4] @ write back change
- ldr r4, sdrc_emr2_1 @ get config addr
- ldr r5, [r4] @ get value
- str r5, [r4] @ write back change
- ldr r4, sdrc_manual_1 @ get config addr
- mov r5, #0x2 @ autorefresh command
- str r5, [r4] @ kick off refreshes
- bx lr
-sdrc_syscfg:
- .word SDRC_SYSCONFIG_P
-sdrc_mr_0:
- .word SDRC_MR_0_P
-sdrc_emr2_0:
- .word SDRC_EMR2_0_P
-sdrc_manual_0:
- .word SDRC_MANUAL_0_P
-sdrc_mr_1:
- .word SDRC_MR_1_P
-sdrc_emr2_1:
- .word SDRC_EMR2_1_P
-sdrc_manual_1:
- .word SDRC_MANUAL_1_P
-ENTRY(es3_sdrc_fix_sz)
- .word . - es3_sdrc_fix
-
/* Function to call rom code to save secure ram context */
ENTRY(save_secure_ram_context)
stmfd sp!, {r1-r12, lr} @ save registers on stack
@@ -208,36 +172,153 @@ api_params:
ENTRY(save_secure_ram_context_sz)
.word . - save_secure_ram_context
+
+/* ======================
+ * == Idle entry point ==
+ * ======================
+ */
+
/*
* Forces OMAP into idle state
*
- * omap34xx_suspend() - This bit of code just executes the WFI
- * for normal idles.
+ * omap34xx_suspend() - This bit of code saves the CPU context if needed
+ * and executes the WFI instruction
+ *
+ * Note: This code get's copied to internal SRAM at boot.
*
- * 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.
+ * Note: When the OMAP wakes up it continues at different execution points,
+ * depending on the low power mode (non-OFF vs OFF modes),
+ * cf. 'Resume path for xxx mode' comments.
*/
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 */
- 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
+ /* r1 contains information about saving context: */
+ /* 1 - Only L1 and logic lost */
+ /* 2 - Only L2 lost */
+ /* 3 - Both L1 and L2 lost */
+ /* Save context only if required */
cmp r1, #0x0
- /* If context save is required, do that and execute wfi */
- bne save_context_wfi
+ beq omap3_do_wfi
+
+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, 1 @ Read Auxiliary Control Register
+ mov r4, #0x1 @ Number of parameters for restore call
+ stmia r8!, {r4-r5} @ Push parameters for restore call
+ mrc p15, 1, r5, c9, c0, 2 @ Read L2 AUX ctrl register
+ stmia r8!, {r4-r5} @ Push parameters for restore call
+
+ /* Check what that target sleep state is:stored in r1*/
+ /* 1 - Only L1 and logic lost */
+ /* 2 - Only L2 lost */
+ /* 3 - Both L1 and L2 lost */
+ cmp r1, #0x2 @ Only L2 lost, no need to save context
+ beq clean_caches
+
+l1_logic_lost:
+ /* Store sp and spsr to SDRAM */
+ mov r4, sp
+ mrs r5, spsr
+ mov r6, lr
+ stmia r8!, {r4-r6}
+ /* Save all ARM registers */
+ /* Coprocessor access control register */
+ mrc p15, 0, r6, c1, c0, 2
+ stmia r8!, {r6}
+ /* TTBR0, TTBR1 and Translation table base control */
+ mrc p15, 0, r4, c2, c0, 0
+ mrc p15, 0, r5, c2, c0, 1
+ mrc p15, 0, r6, c2, c0, 2
+ stmia r8!, {r4-r6}
+ /* Domain access control register, data fault status register,
+ and instruction fault status register */
+ mrc p15, 0, r4, c3, c0, 0
+ mrc p15, 0, r5, c5, c0, 0
+ mrc p15, 0, r6, c5, c0, 1
+ stmia r8!, {r4-r6}
+ /* Data aux fault status register, instruction aux fault status,
+ datat fault address register and instruction fault address register*/
+ mrc p15, 0, r4, c5, c1, 0
+ mrc p15, 0, r5, c5, c1, 1
+ mrc p15, 0, r6, c6, c0, 0
+ mrc p15, 0, r7, c6, c0, 2
+ stmia r8!, {r4-r7}
+ /* user r/w thread and process ID, user r/o thread and process ID,
+ priv only thread and process ID, cache size selection */
+ mrc p15, 0, r4, c13, c0, 2
+ mrc p15, 0, r5, c13, c0, 3
+ mrc p15, 0, r6, c13, c0, 4
+ mrc p15, 2, r7, c0, c0, 0
+ stmia r8!, {r4-r7}
+ /* Data TLB lockdown, instruction TLB lockdown registers */
+ mrc p15, 0, r5, c10, c0, 0
+ mrc p15, 0, r6, c10, c0, 1
+ stmia r8!, {r5-r6}
+ /* Secure or non secure vector base address, FCSE PID, Context PID*/
+ mrc p15, 0, r4, c12, c0, 0
+ mrc p15, 0, r5, c13, c0, 0
+ mrc p15, 0, r6, c13, c0, 1
+ stmia r8!, {r4-r6}
+ /* Primary remap, normal remap registers */
+ mrc p15, 0, r4, c10, c2, 0
+ mrc p15, 0, r5, c10, c2, 1
+ stmia r8!,{r4-r5}
+
+ /* Store current cpsr*/
+ mrs r2, cpsr
+ stmia r8!, {r2}
+
+ mrc p15, 0, r4, c1, c0, 0
+ /* save control register */
+ stmia r8!, {r4}
+
+clean_caches:
+ /* Clean Data or unified cache to POU*/
+ /* How to invalidate only L1 cache???? - #FIX_ME# */
+ /* mcr p15, 0, r11, c7, c11, 1 */
+ cmp r1, #0x1 @ Check whether L2 inval is required
+ beq omap3_do_wfi
+
+clean_l2:
+ /*
+ * jump out to kernel flush routine
+ * - reuse that code is better
+ * - it executes in a cached space so is faster than refetch per-block
+ * - should be faster and will change with kernel
+ * - 'might' have to copy address, load and jump to it
+ */
+ ldr r1, kernel_flush
+ mov lr, pc
+ bx r1
+
+omap3_do_wfi:
+ 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
+
/* Data memory barrier and Data sync barrier */
mov r1, #0
mcr p15, 0, r1, c7, c10, 4
mcr p15, 0, r1, c7, c10, 5
+/* ===================================
+ * == WFI instruction => Enter idle ==
+ * ===================================
+ */
wfi @ wait for interrupt
+/* ===================================
+ * == Resume path for non-OFF modes ==
+ * ===================================
+ */
nop
nop
nop
@@ -250,9 +331,28 @@ loop:
nop
bl wait_sdrc_ok
- ldmfd sp!, {r0-r12, pc} @ restore regs and return
+/* ===================================
+ * == Exit point from non-OFF modes ==
+ * ===================================
+ */
+ ldmfd sp!, {r0-r12, pc} @ restore regs and return
+
+
+/* ==============================
+ * == Resume path for OFF mode ==
+ * ==============================
+ */
+
+/*
+ * The restore_* functions are executed when back from WFI in OFF mode
+ *
+ * restore_es3: applies to 34xx >= ES3.0
+ * restore_3630: applies to 36xx
+ * restore: common code for 3xxx
+ */
restore_es3:
/*b restore_es3*/ @ Enable to debug restore code
+
ldr r5, pm_prepwstst_core_p
ldr r4, [r5]
and r4, r4, #0x3
@@ -282,18 +382,20 @@ restore_3630:
ldr r1, control_mem_rta
mov r2, #OMAP36XX_RTA_DISABLE
str r2, [r1]
- /* Fall thru for the remaining logic */
+
+ /* Fall thru to common code 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*/
- /* 1 - Only L1 and logic lost */
- /* 2 - Only L2 lost - In this case, we wont be here */
- /* 3 - Both L1 and L2 lost */
+ /* Check what was the reason for mpu reset and store the reason in r9*/
+ /* 1 - Only L1 and logic lost */
+ /* 2 - Only L2 lost - In this case, we wont be here */
+ /* 3 - Both L1 and L2 lost */
ldr r1, pm_pwstctrl_mpu
ldr r2, [r1]
and r2, r2, #0x3
cmp r2, #0x0 @ Check if target power state was OFF or RET
- moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
+ moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
movne r9, #0x1 @ Only L1 and L2 lost => avoid L2 invalidation
bne logic_l1_restore
@@ -309,23 +411,23 @@ skipl2dis:
and r1, #0x700
cmp r1, #0x300
beq l2_inv_gp
- mov r0, #40 @ set service ID for PPA
- mov r12, r0 @ copy secure Service ID in r12
- mov r1, #0 @ set task id for ROM code in r1
- mov r2, #4 @ set some flags in r2, r6
+ mov r0, #40 @ set service ID for PPA
+ mov r12, r0 @ copy secure Service ID in r12
+ mov r1, #0 @ set task id for ROM code in r1
+ mov r2, #4 @ set some flags in r2, r6
mov r6, #0xff
adr r3, l2_inv_api_params @ r3 points to dummy parameters
mcr p15, 0, r0, c7, c10, 4 @ data write barrier
mcr p15, 0, r0, c7, c10, 5 @ data memory barrier
.word 0xE1600071 @ call SMI monitor (smi #1)
/* Write to Aux control register to set some bits */
- mov r0, #42 @ set service ID for PPA
- mov r12, r0 @ copy secure Service ID in r12
- mov r1, #0 @ set task id for ROM code in r1
- mov r2, #4 @ set some flags in r2, r6
+ mov r0, #42 @ set service ID for PPA
+ mov r12, r0 @ copy secure Service ID in r12
+ mov r1, #0 @ set task id for ROM code in r1
+ mov r2, #4 @ set some flags in r2, r6
mov r6, #0xff
ldr r4, scratchpad_base
- ldr r3, [r4, #0xBC] @ r3 points to parameters
+ ldr r3, [r4, #0xBC] @ r3 points to parameters
mcr p15, 0, r0, c7, c10, 4 @ data write barrier
mcr p15, 0, r0, c7, c10, 5 @ data memory barrier
.word 0xE1600071 @ call SMI monitor (smi #1)
@@ -334,41 +436,42 @@ skipl2dis:
/* Restore L2 aux control register */
@ set service ID for PPA
mov r0, #CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID
- mov r12, r0 @ copy service ID in r12
- mov r1, #0 @ set task ID for ROM code in r1
- mov r2, #4 @ set some flags in r2, r6
+ mov r12, r0 @ copy service ID in r12
+ mov r1, #0 @ set task ID for ROM code in r1
+ mov r2, #4 @ set some flags in r2, r6
mov r6, #0xff
ldr r4, scratchpad_base
ldr r3, [r4, #0xBC]
- adds r3, r3, #8 @ r3 points to parameters
+ adds r3, r3, #8 @ r3 points to parameters
mcr p15, 0, r0, c7, c10, 4 @ data write barrier
mcr p15, 0, r0, c7, c10, 5 @ data memory barrier
.word 0xE1600071 @ call SMI monitor (smi #1)
#endif
b logic_l1_restore
+
l2_inv_api_params:
.word 0x1, 0x00
l2_inv_gp:
/* Execute smi to invalidate L2 cache */
- mov r12, #0x1 @ set up to invalide L2
+ mov r12, #0x1 @ set up to invalide L2
smi: .word 0xE1600070 @ Call SMI monitor (smieq)
/* Write to Aux control register to set some bits */
ldr r4, scratchpad_base
ldr r3, [r4,#0xBC]
ldr r0, [r3,#4]
mov r12, #0x3
- .word 0xE1600070 @ Call SMI monitor (smieq)
+ .word 0xE1600070 @ Call SMI monitor (smieq)
ldr r4, scratchpad_base
ldr r3, [r4,#0xBC]
ldr r0, [r3,#12]
mov r12, #0x2
- .word 0xE1600070 @ Call SMI monitor (smieq)
+ .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?
+ 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
+ orr r1, r1, #2 @ re-enable L2 cache
mcr p15, 0, r1, c1, c0, 1
skipl2reen:
mov r1, #0
@@ -439,11 +542,11 @@ skipl2reen:
MCR p15, 0, r5, c10, c2, 1
/* Restore cpsr */
- ldmia r3!,{r4} /*load CPSR from SDRAM*/
- msr cpsr, r4 /*store cpsr */
+ ldmia r3!,{r4} @ load CPSR from SDRAM
+ msr cpsr, r4 @ store cpsr
/* Enabling MMU here */
- mrc p15, 0, r7, c2, c0, 2 /* Read TTBRControl */
+ mrc p15, 0, r7, c2, c0, 2 @ Read TTBRControl
/* Extract N (0:2) bits and decide whether to use TTBR0 or TTBR1*/
and r7, #0x7
cmp r7, #0x0
@@ -491,119 +594,69 @@ usettbr0:
and r4, r2
mcr p15, 0, r4, c1, c0, 0
+/* ==============================
+ * == Exit point from OFF mode ==
+ * ==============================
+ */
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, 1 @ Read Auxiliary Control Register
- mov r4, #0x1 @ Number of parameters for restore call
- stmia r8!, {r4-r5} @ Push parameters for restore call
- mrc p15, 1, r5, c9, c0, 2 @ Read L2 AUX ctrl register
- stmia r8!, {r4-r5} @ Push parameters for restore call
- /* Check what that target sleep state is:stored in r1*/
- /* 1 - Only L1 and logic lost */
- /* 2 - Only L2 lost */
- /* 3 - Both L1 and L2 lost */
- cmp r1, #0x2 /* Only L2 lost */
- beq clean_l2
- cmp r1, #0x1 /* L2 retained */
- /* r9 stores whether to clean L2 or not*/
- moveq r9, #0x0 /* Dont Clean L2 */
- movne r9, #0x1 /* Clean L2 */
-l1_logic_lost:
- /* Store sp and spsr to SDRAM */
- mov r4, sp
- mrs r5, spsr
- mov r6, lr
- stmia r8!, {r4-r6}
- /* Save all ARM registers */
- /* Coprocessor access control register */
- mrc p15, 0, r6, c1, c0, 2
- stmia r8!, {r6}
- /* TTBR0, TTBR1 and Translation table base control */
- mrc p15, 0, r4, c2, c0, 0
- mrc p15, 0, r5, c2, c0, 1
- mrc p15, 0, r6, c2, c0, 2
- stmia r8!, {r4-r6}
- /* Domain access control register, data fault status register,
- and instruction fault status register */
- mrc p15, 0, r4, c3, c0, 0
- mrc p15, 0, r5, c5, c0, 0
- mrc p15, 0, r6, c5, c0, 1
- stmia r8!, {r4-r6}
- /* Data aux fault status register, instruction aux fault status,
- datat fault address register and instruction fault address register*/
- mrc p15, 0, r4, c5, c1, 0
- mrc p15, 0, r5, c5, c1, 1
- mrc p15, 0, r6, c6, c0, 0
- mrc p15, 0, r7, c6, c0, 2
- stmia r8!, {r4-r7}
- /* user r/w thread and process ID, user r/o thread and process ID,
- priv only thread and process ID, cache size selection */
- mrc p15, 0, r4, c13, c0, 2
- mrc p15, 0, r5, c13, c0, 3
- mrc p15, 0, r6, c13, c0, 4
- mrc p15, 2, r7, c0, c0, 0
- stmia r8!, {r4-r7}
- /* Data TLB lockdown, instruction TLB lockdown registers */
- mrc p15, 0, r5, c10, c0, 0
- mrc p15, 0, r6, c10, c0, 1
- stmia r8!, {r5-r6}
- /* Secure or non secure vector base address, FCSE PID, Context PID*/
- mrc p15, 0, r4, c12, c0, 0
- mrc p15, 0, r5, c13, c0, 0
- mrc p15, 0, r6, c13, c0, 1
- stmia r8!, {r4-r6}
- /* Primary remap, normal remap registers */
- mrc p15, 0, r4, c10, c2, 0
- mrc p15, 0, r5, c10, c2, 1
- stmia r8!,{r4-r5}
- /* Store current cpsr*/
- mrs r2, cpsr
- stmia r8!, {r2}
-
- mrc p15, 0, r4, c1, c0, 0
- /* save control register */
- stmia r8!, {r4}
-clean_caches:
- /* Clean Data or unified cache to POU*/
- /* How to invalidate only L1 cache???? - #FIX_ME# */
- /* mcr p15, 0, r11, c7, c11, 1 */
- cmp r9, #1 /* Check whether L2 inval is required or not*/
- bne skip_l2_inval
-clean_l2:
- /*
- * Jump out to kernel flush routine
- * - reuse that code is better
- * - it executes in a cached space so is faster than refetch per-block
- * - should be faster and will change with kernel
- * - 'might' have to copy address, load and jump to it
- */
- ldr r1, kernel_flush
- mov lr, pc
- bx r1
+/*
+ * Internal functions, for erratum i443 WA
+ */
-skip_l2_inval:
- /* Data memory barrier and Data sync barrier */
- mov r1, #0
- mcr p15, 0, r1, c7, c10, 4
- mcr p15, 0, r1, c7, c10, 5
+/* This function implements the Erratum ID i443 WA, applies to 34xx >= ES3.0 */
+ENTRY(es3_sdrc_fix)
+ ldr r4, sdrc_syscfg @ get config addr
+ ldr r5, [r4] @ get value
+ tst r5, #0x100 @ is part access blocked
+ it eq
+ biceq r5, r5, #0x100 @ clear bit if set
+ str r5, [r4] @ write back change
+ ldr r4, sdrc_mr_0 @ get config addr
+ ldr r5, [r4] @ get value
+ str r5, [r4] @ write back change
+ ldr r4, sdrc_emr2_0 @ get config addr
+ ldr r5, [r4] @ get value
+ str r5, [r4] @ write back change
+ ldr r4, sdrc_manual_0 @ get config addr
+ mov r5, #0x2 @ autorefresh command
+ str r5, [r4] @ kick off refreshes
+ ldr r4, sdrc_mr_1 @ get config addr
+ ldr r5, [r4] @ get value
+ str r5, [r4] @ write back change
+ ldr r4, sdrc_emr2_1 @ get config addr
+ ldr r5, [r4] @ get value
+ str r5, [r4] @ write back change
+ ldr r4, sdrc_manual_1 @ get config addr
+ mov r5, #0x2 @ autorefresh command
+ str r5, [r4] @ kick off refreshes
+ bx lr
+sdrc_syscfg:
+ .word SDRC_SYSCONFIG_P
+sdrc_mr_0:
+ .word SDRC_MR_0_P
+sdrc_emr2_0:
+ .word SDRC_EMR2_0_P
+sdrc_manual_0:
+ .word SDRC_MANUAL_0_P
+sdrc_mr_1:
+ .word SDRC_MR_1_P
+sdrc_emr2_1:
+ .word SDRC_EMR2_1_P
+sdrc_manual_1:
+ .word SDRC_MANUAL_1_P
+ENTRY(es3_sdrc_fix_sz)
+ .word . - es3_sdrc_fix
- wfi @ wait for interrupt
- nop
- nop
- nop
- nop
- nop
- nop
- nop
- nop
- nop
- nop
- bl wait_sdrc_ok
- /* restore regs and return */
- ldmfd sp!, {r0-r12, pc}
+/*
+ * Internal function, for SDRC state restore
+ * before accessing the SDRAM.
+ *
+ * Only used at return from non-OFF mode. For OFF
+ * mode the ROM code configures the SDRC and
+ * the DPLL before calling the restore code directly
+ * from DDR.
+ */
/* Make sure SDRC accesses are ok */
wait_sdrc_ok:
@@ -615,48 +668,50 @@ wait_dpll3_lock:
tst r5, #1
beq wait_dpll3_lock
- ldr r4, cm_idlest1_core
+ ldr r4, cm_idlest1_core
wait_sdrc_ready:
- ldr r5, [r4]
- tst r5, #0x2
- bne wait_sdrc_ready
+ ldr r5, [r4]
+ 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]
+ ldr r4, sdrc_power
+ ldr r5, [r4]
+ bic r5, r5, #0x40
+ str r5, [r4]
+
is_dll_in_lock_mode:
+ /* Is dll in lock mode? */
+ ldr r4, sdrc_dlla_ctrl
+ ldr r5, [r4]
+ tst r5, #0x4
+ bxne lr @ Return if locked
- /* Is dll in lock mode? */
- ldr r4, sdrc_dlla_ctrl
- ldr r5, [r4]
- tst r5, #0x4
- bxne lr
- /* wait till dll locks */
+ /* wait till dll locks */
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 */
+ 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
+ ldr r5, [r4]
+ and r5, r5, #0x4
+ cmp r5, #0x4
+ bne wait_dll_lock
+ bx lr @ Return when locked
/* 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 */
+ bic r6, #(1<<3) @ disable dll
str r6, [r4]
dsb
- orr r6, r6, #(1<<3) /* enable dll */
+ orr r6, r6, #(1<<3) @ enable dll
str r6, [r4]
dsb
ldr r4, kick_counter
@@ -672,12 +727,8 @@ sdrc_dlla_status:
.word SDRC_DLLA_STATUS_V
sdrc_dlla_ctrl:
.word SDRC_DLLA_CTRL_V
-pm_prepwstst_core:
- .word PM_PREPWSTST_CORE_V
pm_prepwstst_core_p:
.word PM_PREPWSTST_CORE_P
-pm_prepwstst_mpu:
- .word PM_PREPWSTST_MPU_V
pm_pwstctrl_mpu:
.word PM_PWSTCTRL_MPU_P
scratchpad_base:
@@ -686,12 +737,6 @@ sram_base:
.word SRAM_BASE_P + 0x8000
sdrc_power:
.word SDRC_POWER_V
-clk_stabilize_delay:
- .word 0x000001FF
-assoc_mask:
- .word 0x3ff
-numset_mask:
- .word 0x7fff
ttbrbit_mask:
.word 0xFFFFC000
table_index_mask:
@@ -713,5 +758,6 @@ kick_counter:
.word 0
wait_dll_lock_counter:
.word 0
+
ENTRY(omap34xx_cpu_suspend_sz)
.word . - omap34xx_cpu_suspend
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] OMAP3: clean up ASM idle code
2010-12-07 11:11 [PATCH] OMAP3: clean up ASM idle code jean.pihet
@ 2010-12-13 14:51 ` Vishwanath Sripathy
2010-12-14 4:12 ` Kevin Hilman
1 sibling, 0 replies; 6+ messages in thread
From: Vishwanath Sripathy @ 2010-12-13 14:51 UTC (permalink / raw)
To: jean.pihet, linux-omap; +Cc: Jean Pihet-XID
> -----Original Message-----
> From: jean.pihet@newoldbits.com [mailto:jean.pihet@newoldbits.com]
> Sent: Tuesday, December 07, 2010 4:42 PM
> To: linux-omap@vger.kernel.org
> Cc: Jean Pihet; Vishwanath BS
> Subject: [PATCH] OMAP3: clean up ASM idle code
>
> From: Jean Pihet <j-pihet@ti.com>
>
> Clean up of the ASM code:
> - reorganized the code in logical sections: defines, API
> functions, internal functions and variables,
> - reworked and simplified the execution paths, for better
> readability and to avoid duplication of code,
> - added comments on the entry and exit points,
> - reworked the existing comments for better readability,
> - reworked the code formating, alignment and white spaces,
> - added comments for the i443 erratum workarounds,
> - changed the hardcoded values in favor of existing macros
> from include files,
> - clean up of non used symbols.
>
> Tested on OMAP3EVM and Beagleboard with full RET and OFF modes.
>
> Heavily reworked from Vishwa's original patch.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> Cc: Vishwanath BS <vishwanath.bs@ti.com>
Tested on ZOOM3 for Retention and OFF in suspend and Idle path.
Acked-by: Vishwanath BS <vishwanath.bs@ti.com>
> ---
> Applies on top of Nishant's latest idle path errata fixes step 2,
> cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2
>
> arch/arm/mach-omap2/control.h | 2 +
> arch/arm/mach-omap2/pm.h | 1 -
> arch/arm/mach-omap2/pm34xx.c | 4 +-
> arch/arm/mach-omap2/sleep34xx.S | 522
> +++++++++++++++++++++------------------
> 4 files changed, 288 insertions(+), 241 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-
> omap2/control.h
> index d7911c5..72efefb 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -274,6 +274,8 @@
> #define OMAP343X_SCRATCHPAD_ROM
> (OMAP343X_CTRL_BASE + 0x860)
> #define OMAP343X_SCRATCHPAD (OMAP343X_CTRL_BASE +
> 0x910)
> #define OMAP343X_SCRATCHPAD_ROM_OFFSET 0x19C
> +#define OMAP343X_SCRATCHPAD_REGADDR(reg)
> OMAP2_L4_IO_ADDRESS(\
> + OMAP343X_SCRATCHPAD +
> reg)
>
> /* AM35XX_CONTROL_IPSS_CLK_CTRL bits */
> #define AM35XX_USBOTG_VBUSP_CLK_SHIFT 0
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index aff39d0..e458b2a 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -80,7 +80,6 @@ extern void save_secure_ram_context(u32 *addr);
> extern void omap3_save_scratchpad_contents(void);
>
> extern unsigned int omap24xx_idle_loop_suspend_sz;
> -extern unsigned int omap34xx_suspend_sz;
> extern unsigned int save_secure_ram_context_sz;
> extern unsigned int omap24xx_cpu_suspend_sz;
> extern unsigned int omap34xx_cpu_suspend_sz;
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> omap2/pm34xx.c
> index a37d53d..c93b872 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -135,7 +135,7 @@ static void omap3_core_save_context(void)
>
> /*
> * Force write last pad into memory, as this can fail in some
> - * cases according to erratas 1.157, 1.185
> + * cases according to errata 1.157 & 1.185
> */
>
> omap_ctrl_writel(omap_ctrl_readl(OMAP343X_PADCONF_ETK_D1
> 4),
> OMAP343X_CONTROL_MEM_WKUP + 0x2a0);
> @@ -432,7 +432,7 @@ void omap_sram_idle(void)
> /*
> * On EMU/HS devices ROM code restores a SRDC value
> * from scratchpad which has automatic self refresh on timeout
> - * of AUTO_CNT = 1 enabled. This takes care of errata 1.142.
> + * of AUTO_CNT = 1 enabled. This takes care of erratum ID i443.
> * Hence store/restore the SDRC_POWER register here.
> */
> if (omap_rev() >= OMAP3430_REV_ES3_0 &&
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-
> omap2/sleep34xx.S
> index d2eda01..96a4864 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -33,24 +33,27 @@
> #include "sdrc.h"
> #include "control.h"
>
> -#define SDRC_SCRATCHPAD_SEM_V 0xfa00291c
> -
> -#define PM_PREPWSTST_CORE_V
> OMAP34XX_PRM_REGADDR(CORE_MOD, \
> - OMAP3430_PM_PREPWSTST)
> -#define PM_PREPWSTST_CORE_P 0x48306AE8
> -#define PM_PREPWSTST_MPU_V
> OMAP34XX_PRM_REGADDR(MPU_MOD, \
> - OMAP3430_PM_PREPWSTST)
> +/*
> + * Registers access definitions
> + */
> +#define SDRC_SCRATCHPAD_SEM_OFFS 0xc
> +#define SDRC_SCRATCHPAD_SEM_V
> OMAP343X_SCRATCHPAD_REGADDR\
> + (SDRC_SCRATCHPAD_SEM_OFFS)
> +#define PM_PREPWSTST_CORE_P OMAP3430_PRM_BASE + CORE_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 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\
> - + SCRATCHPAD_MEM_OFFS)
> +#define CONTROL_STAT OMAP343X_CTRL_BASE +
> OMAP343X_CONTROL_STATUS
> +#define CONTROL_MEM_RTA_CTRL (OMAP343X_CTRL_BASE +\
> +
> OMAP36XX_CONTROL_MEM_RTA_CTRL)
> +
> +/* Move this as correct place is available */
> +#define SCRATCHPAD_MEM_OFFS 0x310
> +#define SCRATCHPAD_BASE_P (OMAP343X_CTRL_BASE +\
> + OMAP343X_CONTROL_MEM_WKUP +\
> + SCRATCHPAD_MEM_OFFS)
> #define SDRC_POWER_V
> OMAP34XX_SDRC_REGADDR(SDRC_POWER)
> #define SDRC_SYSCONFIG_P (OMAP343X_SDRC_BASE +
> SDRC_SYSCONFIG)
> #define SDRC_MR_0_P (OMAP343X_SDRC_BASE +
> SDRC_MR_0)
> @@ -62,6 +65,10 @@
> #define SDRC_DLLA_STATUS_V
> OMAP34XX_SDRC_REGADDR(SDRC_DLLA_STATUS)
> #define SDRC_DLLA_CTRL_V
> OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
>
> +
> +/*
> + * API functions
> + */
> .text
> /* Function to acquire the semaphore in scratchpad */
> ENTRY(lock_scratchpad_sem)
> @@ -113,7 +120,7 @@ ENTRY(get_omap3630_restore_pointer_sz)
> .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
> + * This function sets up a flag 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)
> @@ -132,49 +139,6 @@ 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
> - tst r5, #0x100 @ is part access blocked
> - it eq
> - biceq r5, r5, #0x100 @ clear bit if set
> - str r5, [r4] @ write back change
> - ldr r4, sdrc_mr_0 @ get config addr
> - ldr r5, [r4] @ get value
> - str r5, [r4] @ write back change
> - ldr r4, sdrc_emr2_0 @ get config addr
> - ldr r5, [r4] @ get value
> - str r5, [r4] @ write back change
> - ldr r4, sdrc_manual_0 @ get config addr
> - mov r5, #0x2 @ autorefresh command
> - str r5, [r4] @ kick off refreshes
> - ldr r4, sdrc_mr_1 @ get config addr
> - ldr r5, [r4] @ get value
> - str r5, [r4] @ write back change
> - ldr r4, sdrc_emr2_1 @ get config addr
> - ldr r5, [r4] @ get value
> - str r5, [r4] @ write back change
> - ldr r4, sdrc_manual_1 @ get config addr
> - mov r5, #0x2 @ autorefresh command
> - str r5, [r4] @ kick off refreshes
> - bx lr
> -sdrc_syscfg:
> - .word SDRC_SYSCONFIG_P
> -sdrc_mr_0:
> - .word SDRC_MR_0_P
> -sdrc_emr2_0:
> - .word SDRC_EMR2_0_P
> -sdrc_manual_0:
> - .word SDRC_MANUAL_0_P
> -sdrc_mr_1:
> - .word SDRC_MR_1_P
> -sdrc_emr2_1:
> - .word SDRC_EMR2_1_P
> -sdrc_manual_1:
> - .word SDRC_MANUAL_1_P
> -ENTRY(es3_sdrc_fix_sz)
> - .word . - es3_sdrc_fix
> -
> /* Function to call rom code to save secure ram context */
> ENTRY(save_secure_ram_context)
> stmfd sp!, {r1-r12, lr} @ save registers on stack
> @@ -208,36 +172,153 @@ api_params:
> ENTRY(save_secure_ram_context_sz)
> .word . - save_secure_ram_context
>
> +
> +/* ======================
> + * == Idle entry point ==
> + * ======================
> + */
> +
> /*
> * Forces OMAP into idle state
> *
> - * omap34xx_suspend() - This bit of code just executes the WFI
> - * for normal idles.
> + * omap34xx_suspend() - This bit of code saves the CPU context if
> needed
> + * and executes the WFI instruction
> + *
> + * Note: This code get's copied to internal SRAM at boot.
> *
> - * 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.
> + * Note: When the OMAP wakes up it continues at different execution
> points,
> + * depending on the low power mode (non-OFF vs OFF modes),
> + * cf. 'Resume path for xxx mode' comments.
> */
> 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 */
> - 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
> + /* r1 contains information about saving context: */
> + /* 1 - Only L1 and logic lost */
> + /* 2 - Only L2 lost */
> + /* 3 - Both L1 and L2 lost */
>
> + /* Save context only if required */
> cmp r1, #0x0
> - /* If context save is required, do that and execute wfi */
> - bne save_context_wfi
> + beq omap3_do_wfi
> +
> +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, 1 @ Read Auxiliary Control Register
> + mov r4, #0x1 @ Number of parameters for restore
> call
> + stmia r8!, {r4-r5} @ Push parameters for restore call
> + mrc p15, 1, r5, c9, c0, 2 @ Read L2 AUX ctrl register
> + stmia r8!, {r4-r5} @ Push parameters for restore call
> +
> + /* Check what that target sleep state is:stored in r1*/
> + /* 1 - Only L1 and logic lost */
> + /* 2 - Only L2 lost */
> + /* 3 - Both L1 and L2 lost */
> + cmp r1, #0x2 @ Only L2 lost, no need to save
> context
> + beq clean_caches
> +
> +l1_logic_lost:
> + /* Store sp and spsr to SDRAM */
> + mov r4, sp
> + mrs r5, spsr
> + mov r6, lr
> + stmia r8!, {r4-r6}
> + /* Save all ARM registers */
> + /* Coprocessor access control register */
> + mrc p15, 0, r6, c1, c0, 2
> + stmia r8!, {r6}
> + /* TTBR0, TTBR1 and Translation table base control */
> + mrc p15, 0, r4, c2, c0, 0
> + mrc p15, 0, r5, c2, c0, 1
> + mrc p15, 0, r6, c2, c0, 2
> + stmia r8!, {r4-r6}
> + /* Domain access control register, data fault status register,
> + and instruction fault status register */
> + mrc p15, 0, r4, c3, c0, 0
> + mrc p15, 0, r5, c5, c0, 0
> + mrc p15, 0, r6, c5, c0, 1
> + stmia r8!, {r4-r6}
> + /* Data aux fault status register, instruction aux fault status,
> + datat fault address register and instruction fault address
> register*/
> + mrc p15, 0, r4, c5, c1, 0
> + mrc p15, 0, r5, c5, c1, 1
> + mrc p15, 0, r6, c6, c0, 0
> + mrc p15, 0, r7, c6, c0, 2
> + stmia r8!, {r4-r7}
> + /* user r/w thread and process ID, user r/o thread and process
> ID,
> + priv only thread and process ID, cache size selection */
> + mrc p15, 0, r4, c13, c0, 2
> + mrc p15, 0, r5, c13, c0, 3
> + mrc p15, 0, r6, c13, c0, 4
> + mrc p15, 2, r7, c0, c0, 0
> + stmia r8!, {r4-r7}
> + /* Data TLB lockdown, instruction TLB lockdown registers */
> + mrc p15, 0, r5, c10, c0, 0
> + mrc p15, 0, r6, c10, c0, 1
> + stmia r8!, {r5-r6}
> + /* Secure or non secure vector base address, FCSE PID, Context
> PID*/
> + mrc p15, 0, r4, c12, c0, 0
> + mrc p15, 0, r5, c13, c0, 0
> + mrc p15, 0, r6, c13, c0, 1
> + stmia r8!, {r4-r6}
> + /* Primary remap, normal remap registers */
> + mrc p15, 0, r4, c10, c2, 0
> + mrc p15, 0, r5, c10, c2, 1
> + stmia r8!,{r4-r5}
> +
> + /* Store current cpsr*/
> + mrs r2, cpsr
> + stmia r8!, {r2}
> +
> + mrc p15, 0, r4, c1, c0, 0
> + /* save control register */
> + stmia r8!, {r4}
> +
> +clean_caches:
> + /* Clean Data or unified cache to POU*/
> + /* How to invalidate only L1 cache???? - #FIX_ME# */
> + /* mcr p15, 0, r11, c7, c11, 1 */
> + cmp r1, #0x1 @ Check whether L2 inval is
required
> + beq omap3_do_wfi
> +
> +clean_l2:
> + /*
> + * jump out to kernel flush routine
> + * - reuse that code is better
> + * - it executes in a cached space so is faster than refetch per-
> block
> + * - should be faster and will change with kernel
> + * - 'might' have to copy address, load and jump to it
> + */
> + ldr r1, kernel_flush
> + mov lr, pc
> + bx r1
> +
> +omap3_do_wfi:
> + 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
> +
> /* Data memory barrier and Data sync barrier */
> mov r1, #0
> mcr p15, 0, r1, c7, c10, 4
> mcr p15, 0, r1, c7, c10, 5
>
> +/* ===================================
> + * == WFI instruction => Enter idle ==
> + * ===================================
> + */
> wfi @ wait for interrupt
>
> +/* ===================================
> + * == Resume path for non-OFF modes ==
> + * ===================================
> + */
> nop
> nop
> nop
> @@ -250,9 +331,28 @@ loop:
> nop
> bl wait_sdrc_ok
>
> - ldmfd sp!, {r0-r12, pc} @ restore regs and return
> +/* ===================================
> + * == Exit point from non-OFF modes ==
> + * ===================================
> + */
> + ldmfd sp!, {r0-r12, pc} @ restore regs and return
> +
> +
> +/* ==============================
> + * == Resume path for OFF mode ==
> + * ==============================
> + */
> +
> +/*
> + * The restore_* functions are executed when back from WFI in OFF
> mode
> + *
> + * restore_es3: applies to 34xx >= ES3.0
> + * restore_3630: applies to 36xx
> + * restore: common code for 3xxx
> + */
> restore_es3:
> /*b restore_es3*/ @ Enable to debug restore code
> +
> ldr r5, pm_prepwstst_core_p
> ldr r4, [r5]
> and r4, r4, #0x3
> @@ -282,18 +382,20 @@ restore_3630:
> ldr r1, control_mem_rta
> mov r2, #OMAP36XX_RTA_DISABLE
> str r2, [r1]
> - /* Fall thru for the remaining logic */
> +
> + /* Fall thru to common code 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*/
> - /* 1 - Only L1 and logic lost */
> - /* 2 - Only L2 lost - In this case, we wont be here */
> - /* 3 - Both L1 and L2 lost */
> + /* Check what was the reason for mpu reset and store the reason
> in r9*/
> + /* 1 - Only L1 and logic lost */
> + /* 2 - Only L2 lost - In this case, we wont be here */
> + /* 3 - Both L1 and L2 lost */
> ldr r1, pm_pwstctrl_mpu
> ldr r2, [r1]
> and r2, r2, #0x3
> cmp r2, #0x0 @ Check if target power state was OFF or
> RET
> - moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
> + moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
> movne r9, #0x1 @ Only L1 and L2 lost => avoid L2
> invalidation
> bne logic_l1_restore
>
> @@ -309,23 +411,23 @@ skipl2dis:
> and r1, #0x700
> cmp r1, #0x300
> beq l2_inv_gp
> - mov r0, #40 @ set service ID for PPA
> - mov r12, r0 @ copy secure Service ID in r12
> - mov r1, #0 @ set task id for ROM code in r1
> - mov r2, #4 @ set some flags in r2, r6
> + mov r0, #40 @ set service ID for PPA
> + mov r12, r0 @ copy secure Service ID in r12
> + mov r1, #0 @ set task id for ROM code in r1
> + mov r2, #4 @ set some flags in r2, r6
> mov r6, #0xff
> adr r3, l2_inv_api_params @ r3 points to dummy
> parameters
> mcr p15, 0, r0, c7, c10, 4 @ data write barrier
> mcr p15, 0, r0, c7, c10, 5 @ data memory barrier
> .word 0xE1600071 @ call SMI monitor (smi #1)
> /* Write to Aux control register to set some bits */
> - mov r0, #42 @ set service ID for PPA
> - mov r12, r0 @ copy secure Service ID in r12
> - mov r1, #0 @ set task id for ROM code in r1
> - mov r2, #4 @ set some flags in r2, r6
> + mov r0, #42 @ set service ID for PPA
> + mov r12, r0 @ copy secure Service ID in r12
> + mov r1, #0 @ set task id for ROM code in r1
> + mov r2, #4 @ set some flags in r2, r6
> mov r6, #0xff
> ldr r4, scratchpad_base
> - ldr r3, [r4, #0xBC] @ r3 points to parameters
> + ldr r3, [r4, #0xBC] @ r3 points to parameters
> mcr p15, 0, r0, c7, c10, 4 @ data write barrier
> mcr p15, 0, r0, c7, c10, 5 @ data memory barrier
> .word 0xE1600071 @ call SMI monitor (smi #1)
> @@ -334,41 +436,42 @@ skipl2dis:
> /* Restore L2 aux control register */
> @ set service ID for PPA
> mov r0, #CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID
> - mov r12, r0 @ copy service ID in r12
> - mov r1, #0 @ set task ID for ROM code in r1
> - mov r2, #4 @ set some flags in r2, r6
> + mov r12, r0 @ copy service ID in r12
> + mov r1, #0 @ set task ID for ROM code in r1
> + mov r2, #4 @ set some flags in r2, r6
> mov r6, #0xff
> ldr r4, scratchpad_base
> ldr r3, [r4, #0xBC]
> - adds r3, r3, #8 @ r3 points to parameters
> + adds r3, r3, #8 @ r3 points to parameters
> mcr p15, 0, r0, c7, c10, 4 @ data write barrier
> mcr p15, 0, r0, c7, c10, 5 @ data memory barrier
> .word 0xE1600071 @ call SMI monitor (smi #1)
> #endif
> b logic_l1_restore
> +
> l2_inv_api_params:
> .word 0x1, 0x00
> l2_inv_gp:
> /* Execute smi to invalidate L2 cache */
> - mov r12, #0x1 @ set up to invalide L2
> + mov r12, #0x1 @ set up to invalide L2
> smi: .word 0xE1600070 @ Call SMI monitor (smieq)
> /* Write to Aux control register to set some bits */
> ldr r4, scratchpad_base
> ldr r3, [r4,#0xBC]
> ldr r0, [r3,#4]
> mov r12, #0x3
> - .word 0xE1600070 @ Call SMI monitor (smieq)
> + .word 0xE1600070 @ Call SMI monitor (smieq)
> ldr r4, scratchpad_base
> ldr r3, [r4,#0xBC]
> ldr r0, [r3,#12]
> mov r12, #0x2
> - .word 0xE1600070 @ Call SMI monitor (smieq)
> + .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?
> + 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
> + orr r1, r1, #2 @ re-enable L2 cache
> mcr p15, 0, r1, c1, c0, 1
> skipl2reen:
> mov r1, #0
> @@ -439,11 +542,11 @@ skipl2reen:
> MCR p15, 0, r5, c10, c2, 1
>
> /* Restore cpsr */
> - ldmia r3!,{r4} /*load CPSR from SDRAM*/
> - msr cpsr, r4 /*store cpsr */
> + ldmia r3!,{r4} @ load CPSR from SDRAM
> + msr cpsr, r4 @ store cpsr
>
> /* Enabling MMU here */
> - mrc p15, 0, r7, c2, c0, 2 /* Read TTBRControl */
> + mrc p15, 0, r7, c2, c0, 2 @ Read TTBRControl
> /* Extract N (0:2) bits and decide whether to use TTBR0 or
> TTBR1*/
> and r7, #0x7
> cmp r7, #0x0
> @@ -491,119 +594,69 @@ usettbr0:
> and r4, r2
> mcr p15, 0, r4, c1, c0, 0
>
> +/* ==============================
> + * == Exit point from OFF mode ==
> + * ==============================
> + */
> 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, 1 @ Read Auxiliary Control Register
> - mov r4, #0x1 @ Number of parameters for restore
> call
> - stmia r8!, {r4-r5} @ Push parameters for restore call
> - mrc p15, 1, r5, c9, c0, 2 @ Read L2 AUX ctrl register
> - stmia r8!, {r4-r5} @ Push parameters for restore call
> - /* Check what that target sleep state is:stored in r1*/
> - /* 1 - Only L1 and logic lost */
> - /* 2 - Only L2 lost */
> - /* 3 - Both L1 and L2 lost */
> - cmp r1, #0x2 /* Only L2 lost */
> - beq clean_l2
> - cmp r1, #0x1 /* L2 retained */
> - /* r9 stores whether to clean L2 or not*/
> - moveq r9, #0x0 /* Dont Clean L2 */
> - movne r9, #0x1 /* Clean L2 */
> -l1_logic_lost:
> - /* Store sp and spsr to SDRAM */
> - mov r4, sp
> - mrs r5, spsr
> - mov r6, lr
> - stmia r8!, {r4-r6}
> - /* Save all ARM registers */
> - /* Coprocessor access control register */
> - mrc p15, 0, r6, c1, c0, 2
> - stmia r8!, {r6}
> - /* TTBR0, TTBR1 and Translation table base control */
> - mrc p15, 0, r4, c2, c0, 0
> - mrc p15, 0, r5, c2, c0, 1
> - mrc p15, 0, r6, c2, c0, 2
> - stmia r8!, {r4-r6}
> - /* Domain access control register, data fault status register,
> - and instruction fault status register */
> - mrc p15, 0, r4, c3, c0, 0
> - mrc p15, 0, r5, c5, c0, 0
> - mrc p15, 0, r6, c5, c0, 1
> - stmia r8!, {r4-r6}
> - /* Data aux fault status register, instruction aux fault status,
> - datat fault address register and instruction fault address
> register*/
> - mrc p15, 0, r4, c5, c1, 0
> - mrc p15, 0, r5, c5, c1, 1
> - mrc p15, 0, r6, c6, c0, 0
> - mrc p15, 0, r7, c6, c0, 2
> - stmia r8!, {r4-r7}
> - /* user r/w thread and process ID, user r/o thread and process
> ID,
> - priv only thread and process ID, cache size selection */
> - mrc p15, 0, r4, c13, c0, 2
> - mrc p15, 0, r5, c13, c0, 3
> - mrc p15, 0, r6, c13, c0, 4
> - mrc p15, 2, r7, c0, c0, 0
> - stmia r8!, {r4-r7}
> - /* Data TLB lockdown, instruction TLB lockdown registers */
> - mrc p15, 0, r5, c10, c0, 0
> - mrc p15, 0, r6, c10, c0, 1
> - stmia r8!, {r5-r6}
> - /* Secure or non secure vector base address, FCSE PID, Context
> PID*/
> - mrc p15, 0, r4, c12, c0, 0
> - mrc p15, 0, r5, c13, c0, 0
> - mrc p15, 0, r6, c13, c0, 1
> - stmia r8!, {r4-r6}
> - /* Primary remap, normal remap registers */
> - mrc p15, 0, r4, c10, c2, 0
> - mrc p15, 0, r5, c10, c2, 1
> - stmia r8!,{r4-r5}
>
> - /* Store current cpsr*/
> - mrs r2, cpsr
> - stmia r8!, {r2}
> -
> - mrc p15, 0, r4, c1, c0, 0
> - /* save control register */
> - stmia r8!, {r4}
> -clean_caches:
> - /* Clean Data or unified cache to POU*/
> - /* How to invalidate only L1 cache???? - #FIX_ME# */
> - /* mcr p15, 0, r11, c7, c11, 1 */
> - cmp r9, #1 /* Check whether L2 inval is required or not*/
> - bne skip_l2_inval
> -clean_l2:
> - /*
> - * Jump out to kernel flush routine
> - * - reuse that code is better
> - * - it executes in a cached space so is faster than refetch per-
> block
> - * - should be faster and will change with kernel
> - * - 'might' have to copy address, load and jump to it
> - */
> - ldr r1, kernel_flush
> - mov lr, pc
> - bx r1
> +/*
> + * Internal functions, for erratum i443 WA
> + */
>
> -skip_l2_inval:
> - /* Data memory barrier and Data sync barrier */
> - mov r1, #0
> - mcr p15, 0, r1, c7, c10, 4
> - mcr p15, 0, r1, c7, c10, 5
> +/* This function implements the Erratum ID i443 WA, applies to 34xx
> >= ES3.0 */
> +ENTRY(es3_sdrc_fix)
> + ldr r4, sdrc_syscfg @ get config addr
> + ldr r5, [r4] @ get value
> + tst r5, #0x100 @ is part access blocked
> + it eq
> + biceq r5, r5, #0x100 @ clear bit if set
> + str r5, [r4] @ write back change
> + ldr r4, sdrc_mr_0 @ get config addr
> + ldr r5, [r4] @ get value
> + str r5, [r4] @ write back change
> + ldr r4, sdrc_emr2_0 @ get config addr
> + ldr r5, [r4] @ get value
> + str r5, [r4] @ write back change
> + ldr r4, sdrc_manual_0 @ get config addr
> + mov r5, #0x2 @ autorefresh command
> + str r5, [r4] @ kick off refreshes
> + ldr r4, sdrc_mr_1 @ get config addr
> + ldr r5, [r4] @ get value
> + str r5, [r4] @ write back change
> + ldr r4, sdrc_emr2_1 @ get config addr
> + ldr r5, [r4] @ get value
> + str r5, [r4] @ write back change
> + ldr r4, sdrc_manual_1 @ get config addr
> + mov r5, #0x2 @ autorefresh command
> + str r5, [r4] @ kick off refreshes
> + bx lr
> +sdrc_syscfg:
> + .word SDRC_SYSCONFIG_P
> +sdrc_mr_0:
> + .word SDRC_MR_0_P
> +sdrc_emr2_0:
> + .word SDRC_EMR2_0_P
> +sdrc_manual_0:
> + .word SDRC_MANUAL_0_P
> +sdrc_mr_1:
> + .word SDRC_MR_1_P
> +sdrc_emr2_1:
> + .word SDRC_EMR2_1_P
> +sdrc_manual_1:
> + .word SDRC_MANUAL_1_P
> +ENTRY(es3_sdrc_fix_sz)
> + .word . - es3_sdrc_fix
>
> - wfi @ wait for interrupt
> - nop
> - nop
> - nop
> - nop
> - nop
> - nop
> - nop
> - nop
> - nop
> - nop
> - bl wait_sdrc_ok
> - /* restore regs and return */
> - ldmfd sp!, {r0-r12, pc}
> +/*
> + * Internal function, for SDRC state restore
> + * before accessing the SDRAM.
> + *
> + * Only used at return from non-OFF mode. For OFF
> + * mode the ROM code configures the SDRC and
> + * the DPLL before calling the restore code directly
> + * from DDR.
> + */
>
> /* Make sure SDRC accesses are ok */
> wait_sdrc_ok:
> @@ -615,48 +668,50 @@ wait_dpll3_lock:
> tst r5, #1
> beq wait_dpll3_lock
>
> - ldr r4, cm_idlest1_core
> + ldr r4, cm_idlest1_core
> wait_sdrc_ready:
> - ldr r5, [r4]
> - tst r5, #0x2
> - bne wait_sdrc_ready
> + ldr r5, [r4]
> + 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]
> + ldr r4, sdrc_power
> + ldr r5, [r4]
> + bic r5, r5, #0x40
> + str r5, [r4]
> +
> is_dll_in_lock_mode:
> + /* Is dll in lock mode? */
> + ldr r4, sdrc_dlla_ctrl
> + ldr r5, [r4]
> + tst r5, #0x4
> + bxne lr @ Return if locked
>
> - /* Is dll in lock mode? */
> - ldr r4, sdrc_dlla_ctrl
> - ldr r5, [r4]
> - tst r5, #0x4
> - bxne lr
> - /* wait till dll locks */
> + /* wait till dll locks */
> 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 */
> + 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
> + ldr r5, [r4]
> + and r5, r5, #0x4
> + cmp r5, #0x4
> + bne wait_dll_lock
> + bx lr @ Return when locked
>
> /* 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 */
> + bic r6, #(1<<3) @ disable dll
> str r6, [r4]
> dsb
> - orr r6, r6, #(1<<3) /* enable dll */
> + orr r6, r6, #(1<<3) @ enable dll
> str r6, [r4]
> dsb
> ldr r4, kick_counter
> @@ -672,12 +727,8 @@ sdrc_dlla_status:
> .word SDRC_DLLA_STATUS_V
> sdrc_dlla_ctrl:
> .word SDRC_DLLA_CTRL_V
> -pm_prepwstst_core:
> - .word PM_PREPWSTST_CORE_V
> pm_prepwstst_core_p:
> .word PM_PREPWSTST_CORE_P
> -pm_prepwstst_mpu:
> - .word PM_PREPWSTST_MPU_V
> pm_pwstctrl_mpu:
> .word PM_PWSTCTRL_MPU_P
> scratchpad_base:
> @@ -686,12 +737,6 @@ sram_base:
> .word SRAM_BASE_P + 0x8000
> sdrc_power:
> .word SDRC_POWER_V
> -clk_stabilize_delay:
> - .word 0x000001FF
> -assoc_mask:
> - .word 0x3ff
> -numset_mask:
> - .word 0x7fff
> ttbrbit_mask:
> .word 0xFFFFC000
> table_index_mask:
> @@ -713,5 +758,6 @@ kick_counter:
> .word 0
> wait_dll_lock_counter:
> .word 0
> +
> ENTRY(omap34xx_cpu_suspend_sz)
> .word . - omap34xx_cpu_suspend
> --
> 1.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] OMAP3: clean up ASM idle code
2010-12-07 11:11 [PATCH] OMAP3: clean up ASM idle code jean.pihet
2010-12-13 14:51 ` Vishwanath Sripathy
@ 2010-12-14 4:12 ` Kevin Hilman
2010-12-14 9:22 ` Jean Pihet
1 sibling, 1 reply; 6+ messages in thread
From: Kevin Hilman @ 2010-12-14 4:12 UTC (permalink / raw)
To: jean.pihet; +Cc: linux-omap, Jean Pihet, Vishwanath BS
jean.pihet@newoldbits.com writes:
> From: Jean Pihet <j-pihet@ti.com>
>
> Clean up of the ASM code:
> - reorganized the code in logical sections: defines, API
> functions, internal functions and variables,
> - reworked and simplified the execution paths, for better
> readability and to avoid duplication of code,
> - added comments on the entry and exit points,
> - reworked the existing comments for better readability,
> - reworked the code formating, alignment and white spaces,
> - added comments for the i443 erratum workarounds,
> - changed the hardcoded values in favor of existing macros
> from include files,
> - clean up of non used symbols.
The 'lock_scratchpad_sem' code is also unused. IIRC, you removed that
in an earlier version of the series. Are you still planning to remove
that? maybe in a subsequent patch?
That being said, pure whitespace changes and unused code removal should
probably be a separate patch. It's a great help to reviewers if
functional changes are separated from whitespace changes. It's a bit
tricky in this series as there's lots of code moving as well, so I'll
leave it up to your judgement on how/if to separate these out.
> Tested on OMAP3EVM and Beagleboard with full RET and OFF modes.
In idle? in suspend? both? was CPUidle enabled?
FWIW, I tested on 3430-ES3.1/n900 with retention idle & suspend and off
idle & suspend with CPUidle enabled.
> Heavily reworked from Vishwa's original patch.
Here, it's more customary to say "Based on original patch from Vishwa"
and ensure the original author is CC'd (which you've done.)
Other than that, this is a great cleanup, and makes this much more
readable. Thanks.
Some other minor comments below.
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> Cc: Vishwanath BS <vishwanath.bs@ti.com>
> ---
> Applies on top of Nishant's latest idle path errata fixes step 2,
> cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2
>
[...]
> @@ -208,36 +172,153 @@ api_params:
> ENTRY(save_secure_ram_context_sz)
> .word . - save_secure_ram_context
>
> +
> +/* ======================
> + * == Idle entry point ==
> + * ======================
> + */
Let's keep C multi-line coding style even for assembly. Same goes for
several other places below.
> /*
> * Forces OMAP into idle state
> *
> - * omap34xx_suspend() - This bit of code just executes the WFI
> - * for normal idles.
> + * omap34xx_suspend() - This bit of code saves the CPU context if needed
> + * and executes the WFI instruction
> + *
> + * Note: This code get's copied to internal SRAM at boot.
> *
> - * 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.
> + * Note: When the OMAP wakes up it continues at different execution points,
> + * depending on the low power mode (non-OFF vs OFF modes),
> + * cf. 'Resume path for xxx mode' comments.
> */
> ENTRY(omap34xx_cpu_suspend)
> stmfd sp!, {r0-r12, lr} @ save registers on stack
> loop:
> /*b loop*/ @Enable to debug by stepping through code
While here, let's get rid of these infinite loop hacks for debugging as
anyone debugging this code can add these back as needed. Otherwise,
they clutter the code. There are a few of theses throughout the
original code.
[...]
> @@ -250,9 +331,28 @@ loop:
> nop
> bl wait_sdrc_ok
>
> - ldmfd sp!, {r0-r12, pc} @ restore regs and return
> +/* ===================================
> + * == Exit point from non-OFF modes ==
> + * ===================================
> + */
> + ldmfd sp!, {r0-r12, pc} @ restore regs and return
> +
> +
> +/* ==============================
> + * == Resume path for OFF mode ==
> + * ==============================
> + */
I don't think this is quite correct. I don't believe it starts
immediately here. Doesn't it resume from DDR first, and then jump
here. A brief description of that process would help clarify that process.
> +/*
> + * The restore_* functions are executed when back from WFI in OFF mode
> + *
> + * restore_es3: applies to 34xx >= ES3.0
> + * restore_3630: applies to 36xx
> + * restore: common code for 3xxx
> + */
> restore_es3:
> /*b restore_es3*/ @ Enable to debug restore code
> +
> ldr r5, pm_prepwstst_core_p
> ldr r4, [r5]
> and r4, r4, #0x3
> @@ -282,18 +382,20 @@ restore_3630:
> ldr r1, control_mem_rta
> mov r2, #OMAP36XX_RTA_DISABLE
> str r2, [r1]
> - /* Fall thru for the remaining logic */
> +
> + /* Fall thru to common code 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*/
> - /* 1 - Only L1 and logic lost */
> - /* 2 - Only L2 lost - In this case, we wont be here */
> - /* 3 - Both L1 and L2 lost */
> + /* Check what was the reason for mpu reset and store the reason in r9*/
> + /* 1 - Only L1 and logic lost */
> + /* 2 - Only L2 lost - In this case, we wont be here */
> + /* 3 - Both L1 and L2 lost */
> ldr r1, pm_pwstctrl_mpu
> ldr r2, [r1]
> and r2, r2, #0x3
> cmp r2, #0x0 @ Check if target power state was OFF or RET
> - moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
> + moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
> movne r9, #0x1 @ Only L1 and L2 lost => avoid L2 invalidation
> bne logic_l1_restore
>
> @@ -309,23 +411,23 @@ skipl2dis:
> and r1, #0x700
> cmp r1, #0x300
> beq l2_inv_gp
> - mov r0, #40 @ set service ID for PPA
> - mov r12, r0 @ copy secure Service ID in r12
> - mov r1, #0 @ set task id for ROM code in r1
> - mov r2, #4 @ set some flags in r2, r6
> + mov r0, #40 @ set service ID for PPA
> + mov r12, r0 @ copy secure Service ID in r12
> + mov r1, #0 @ set task id for ROM code in r1
> + mov r2, #4 @ set some flags in r2, r6
This is the type of change that should normally be in a separate patch,
as it seems to be pure whitespace change.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] OMAP3: clean up ASM idle code
2010-12-14 4:12 ` Kevin Hilman
@ 2010-12-14 9:22 ` Jean Pihet
2010-12-14 11:34 ` Vishwanath Sripathy
0 siblings, 1 reply; 6+ messages in thread
From: Jean Pihet @ 2010-12-14 9:22 UTC (permalink / raw)
To: Kevin Hilman, Vishwanath BS; +Cc: linux-omap, Jean Pihet
Kevin,
On Tue, Dec 14, 2010 at 5:12 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> jean.pihet@newoldbits.com writes:
>
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> Clean up of the ASM code:
>> - reorganized the code in logical sections: defines, API
>> functions, internal functions and variables,
>> - reworked and simplified the execution paths, for better
>> readability and to avoid duplication of code,
>> - added comments on the entry and exit points,
>> - reworked the existing comments for better readability,
>> - reworked the code formating, alignment and white spaces,
>> - added comments for the i443 erratum workarounds,
>> - changed the hardcoded values in favor of existing macros
>> from include files,
>> - clean up of non used symbols.
>
> The 'lock_scratchpad_sem' code is also unused. IIRC, you removed that
> in an earlier version of the series. Are you still planning to remove
> that? maybe in a subsequent patch?
I asked previously about it and the reply was that this code is
needed: (quoting Vishwa's reply) "This is indeed used during DVFS when
Core DPLL configuration is changed". Note: the DVFS code is not
upstreamed yet.
Vishwa, can you confirm?
> That being said, pure whitespace changes and unused code removal should
> probably be a separate patch. It's a great help to reviewers if
> functional changes are separated from whitespace changes. It's a bit
> tricky in this series as there's lots of code moving as well, so I'll
> leave it up to your judgement on how/if to separate these out.
There is no functional change at all. The code has been reorganized
for better readability.
I agree the patch is not easy to read but that is the way diff reports
the changes.
I do not see the point to provide separate patches for code move,
white space clean-up, alignment fixes etc.
>> Tested on OMAP3EVM and Beagleboard with full RET and OFF modes.
>
> In idle? in suspend? both? was CPUidle enabled?
>
> FWIW, I tested on 3430-ES3.1/n900 with retention idle & suspend and off
> idle & suspend with CPUidle enabled.
Tested with cpuidle and suspend. I will update the description.
>
>> Heavily reworked from Vishwa's original patch.
>
> Here, it's more customary to say "Based on original patch from Vishwa"
> and ensure the original author is CC'd (which you've done.)
>
> Other than that, this is a great cleanup, and makes this much more
> readable. Thanks.
>
> Some other minor comments below.
OK thanks for the review. I will post a new version asap.
>
>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> Cc: Vishwanath BS <vishwanath.bs@ti.com>
>> ---
>> Applies on top of Nishant's latest idle path errata fixes step 2,
>> cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2
>>
>
> [...]
>
>> @@ -208,36 +172,153 @@ api_params:
>> ENTRY(save_secure_ram_context_sz)
>> .word . - save_secure_ram_context
>>
>> +
>> +/* ======================
>> + * == Idle entry point ==
>> + * ======================
>> + */
>
> Let's keep C multi-line coding style even for assembly. Same goes for
> several other places below.
Ok
>
>> /*
>> * Forces OMAP into idle state
>> *
>> - * omap34xx_suspend() - This bit of code just executes the WFI
>> - * for normal idles.
>> + * omap34xx_suspend() - This bit of code saves the CPU context if needed
>> + * and executes the WFI instruction
>> + *
>> + * Note: This code get's copied to internal SRAM at boot.
>> *
>> - * 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.
>> + * Note: When the OMAP wakes up it continues at different execution points,
>> + * depending on the low power mode (non-OFF vs OFF modes),
>> + * cf. 'Resume path for xxx mode' comments.
>> */
>> ENTRY(omap34xx_cpu_suspend)
>> stmfd sp!, {r0-r12, lr} @ save registers on stack
>> loop:
>> /*b loop*/ @Enable to debug by stepping through code
>
> While here, let's get rid of these infinite loop hacks for debugging as
> anyone debugging this code can add these back as needed. Otherwise,
> they clutter the code. There are a few of theses throughout the
> original code.
Ok. Agree those are not used at all (even when doing heavy debugging).
> [...]
>
>> @@ -250,9 +331,28 @@ loop:
>> nop
>> bl wait_sdrc_ok
>>
>> - ldmfd sp!, {r0-r12, pc} @ restore regs and return
>> +/* ===================================
>> + * == Exit point from non-OFF modes ==
>> + * ===================================
>> + */
>> + ldmfd sp!, {r0-r12, pc} @ restore regs and return
>> +
>> +
>> +/* ==============================
>> + * == Resume path for OFF mode ==
>> + * ==============================
>> + */
>
> I don't think this is quite correct. I don't believe it starts
> immediately here. Doesn't it resume from DDR first, and then jump
> here. A brief description of that process would help clarify that process.
This is the restore point from OFF mode. The ROM code calls this
directly, cf. the get_pointer* functions that are used to get the
resume pointer.
I will update the comment.
>> +/*
>> + * The restore_* functions are executed when back from WFI in OFF mode
>> + *
>> + * restore_es3: applies to 34xx >= ES3.0
>> + * restore_3630: applies to 36xx
>> + * restore: common code for 3xxx
>> + */
>> restore_es3:
>> /*b restore_es3*/ @ Enable to debug restore code
>> +
>> ldr r5, pm_prepwstst_core_p
>> ldr r4, [r5]
>> and r4, r4, #0x3
>> @@ -282,18 +382,20 @@ restore_3630:
>> ldr r1, control_mem_rta
>> mov r2, #OMAP36XX_RTA_DISABLE
>> str r2, [r1]
>> - /* Fall thru for the remaining logic */
>> +
>> + /* Fall thru to common code 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*/
>> - /* 1 - Only L1 and logic lost */
>> - /* 2 - Only L2 lost - In this case, we wont be here */
>> - /* 3 - Both L1 and L2 lost */
>> + /* Check what was the reason for mpu reset and store the reason in r9*/
>> + /* 1 - Only L1 and logic lost */
>> + /* 2 - Only L2 lost - In this case, we wont be here */
>> + /* 3 - Both L1 and L2 lost */
>> ldr r1, pm_pwstctrl_mpu
>> ldr r2, [r1]
>> and r2, r2, #0x3
>> cmp r2, #0x0 @ Check if target power state was OFF or RET
>> - moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
>> + moveq r9, #0x3 @ MPU OFF => L1 and L2 lost
>> movne r9, #0x1 @ Only L1 and L2 lost => avoid L2 invalidation
>> bne logic_l1_restore
>>
>> @@ -309,23 +411,23 @@ skipl2dis:
>> and r1, #0x700
>> cmp r1, #0x300
>> beq l2_inv_gp
>> - mov r0, #40 @ set service ID for PPA
>> - mov r12, r0 @ copy secure Service ID in r12
>> - mov r1, #0 @ set task id for ROM code in r1
>> - mov r2, #4 @ set some flags in r2, r6
>> + mov r0, #40 @ set service ID for PPA
>> + mov r12, r0 @ copy secure Service ID in r12
>> + mov r1, #0 @ set task id for ROM code in r1
>> + mov r2, #4 @ set some flags in r2, r6
>
> This is the type of change that should normally be in a separate patch,
> as it seems to be pure whitespace change.
>
>
> Kevin
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] OMAP3: clean up ASM idle code
2010-12-14 9:22 ` Jean Pihet
@ 2010-12-14 11:34 ` Vishwanath Sripathy
2010-12-14 17:30 ` Jean Pihet
0 siblings, 1 reply; 6+ messages in thread
From: Vishwanath Sripathy @ 2010-12-14 11:34 UTC (permalink / raw)
To: Jean Pihet, Kevin Hilman; +Cc: linux-omap, Jean Pihet-XID
Jean,
> -----Original Message-----
> From: Jean Pihet [mailto:jean.pihet@newoldbits.com]
> Sent: Tuesday, December 14, 2010 2:53 PM
> To: Kevin Hilman; Vishwanath BS
> Cc: linux-omap@vger.kernel.org; Jean Pihet
> Subject: Re: [PATCH] OMAP3: clean up ASM idle code
>
> Kevin,
>
> On Tue, Dec 14, 2010 at 5:12 AM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
> > jean.pihet@newoldbits.com writes:
> >
> >> From: Jean Pihet <j-pihet@ti.com>
> >>
> >> Clean up of the ASM code:
> >> - reorganized the code in logical sections: defines, API
> >> á áfunctions, internal functions and variables,
> >> - reworked and simplified the execution paths, for better
> >> á áreadability and to avoid duplication of code,
> >> - added comments on the entry and exit points,
> >> - reworked the existing comments for better readability,
> >> - reworked the code formating, alignment and white spaces,
> >> - added comments for the i443 erratum workarounds,
> >> - changed the hardcoded values in favor of existing macros
> >> á áfrom include files,
> >> - clean up of non used symbols.
> >
> > The 'lock_scratchpad_sem' code is also unused. áIIRC, you removed
> that
> > in an earlier version of the series. áAre you still planning to remove
> > that? maybe in a subsequent patch?
>
> I asked previously about it and the reply was that this code is
> needed: (quoting Vishwa's reply) "This is indeed used during DVFS when
> Core DPLL configuration is changed". Note: the DVFS code is not
> upstreamed yet.
>
> Vishwa, can you confirm?
lock_scratchpad_sem is needed when DVFS feature is supported. If you want
to add this implementation as part of DVFS feature, then also it's fine.
Vishwa
>
> > That being said, pure whitespace changes and unused code removal
> should
> > probably be a separate patch. áIt's a great help to reviewers if
> > functional changes are separated from whitespace changes. áIt's a bit
> > tricky in this series as there's lots of code moving as well, so I'll
> > leave it up to your judgement on how/if to separate these out.
>
> There is no functional change at all. The code has been reorganized
> for better readability.
> I agree the patch is not easy to read but that is the way diff reports
> the changes.
>
> I do not see the point to provide separate patches for code move,
> white space clean-up, alignment fixes etc.
>
> >> Tested on OMAP3EVM and Beagleboard with full RET and OFF modes.
> >
> > In idle? áin suspend? áboth? áwas CPUidle enabled?
> >
> > FWIW, I tested on 3430-ES3.1/n900 with retention idle & suspend and
> off
> > idle & suspend with CPUidle enabled.
> Tested with cpuidle and suspend. I will update the description.
>
> >
> >> Heavily reworked from Vishwa's original patch.
> >
> > Here, it's more customary to ásay "Based on original patch from
> Vishwa"
> > and ensure the original author is CC'd (which you've done.)
> >
> > Other than that, this is a great cleanup, and makes this much more
> > readable. áThanks.
> >
> > Some other minor comments below.
>
> OK thanks for the review. I will post a new version asap.
>
> >
> >> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> >> Cc: Vishwanath BS <vishwanath.bs@ti.com>
> >> ---
> >> Applies on top of Nishant's latest idle path errata fixes step 2,
> >> cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2
> >>
> >
> > [...]
> >
> >> @@ -208,36 +172,153 @@ api_params:
> >> áENTRY(save_secure_ram_context_sz)
> >> á á á .word á . - save_secure_ram_context
> >>
> >> +
> >> +/* ======================
> >> + * == Idle entry point ==
> >> + * ======================
> >> + */
> >
> > Let's keep C multi-line coding style even for assembly. á Same goes
for
> > several other places below.
> Ok
>
> >
> >> á/*
> >> á * Forces OMAP into idle state
> >> á *
> >> - * omap34xx_suspend() - This bit of code just executes the WFI
> >> - * for normal idles.
> >> + * omap34xx_suspend() - This bit of code saves the CPU context if
> needed
> >> + * and executes the WFI instruction
> >> + *
> >> + * Note: This code get's copied to internal SRAM at boot.
> >> á *
> >> - * 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.
> >> + * Note: When the OMAP wakes up it continues at different
> execution points,
> >> + * ádepending on the low power mode (non-OFF vs OFF modes),
> >> + * ácf. 'Resume path for xxx mode' comments.
> >> á */
> >> áENTRY(omap34xx_cpu_suspend)
> >> á á á stmfd á sp!, {r0-r12, lr} á á á á á á á @ save registers on
stack
> >> áloop:
> >> á á á /*b á á loop*/ á@Enable to debug by stepping through code
> >
> > While here, let's get rid of these infinite loop hacks for debugging
as
> > anyone debugging this code can add these back as needed.
> áOtherwise,
> > they clutter the code. á There are a few of theses throughout the
> > original code.
> Ok. Agree those are not used at all (even when doing heavy debugging).
>
> > [...]
> >
> >> @@ -250,9 +331,28 @@ loop:
> >> á á á nop
> >> á á á bl wait_sdrc_ok
> >>
> >> - á á ldmfd á sp!, {r0-r12, pc} á á á á á á á @ restore regs and
return
> >> +/* ===================================
> >> + * == Exit point from non-OFF modes ==
> >> + * ===================================
> >> + */
> >> + á á ldmfd á sp!, {r0-r12, pc} á á á @ restore regs and return
> >> +
> >> +
> >> +/* ==============================
> >> + * == Resume path for OFF mode ==
> >> + * ==============================
> >> + */
> >
> > I don't think this is quite correct. áI don't believe it starts
> > immediately here. áDoesn't it resume from DDR first, and then ájump
> > here. áA brief description of that process would help clarify that
> process.
> This is the restore point from OFF mode. The ROM code calls this
> directly, cf. the get_pointer* functions that are used to get the
> resume pointer.
> I will update the comment.
>
> >> +/*
> >> + * The restore_* functions are executed when back from WFI in
> OFF mode
> >> + *
> >> + * árestore_es3: applies to 34xx >= ES3.0
> >> + * árestore_3630: applies to 36xx
> >> + * árestore: common code for 3xxx
> >> + */
> >> árestore_es3:
> >> á á á /*b restore_es3*/ á á á á á á á @ Enable to debug restore code
> >> +
> >> á á á ldr á á r5, pm_prepwstst_core_p
> >> á á á ldr á á r4, [r5]
> >> á á á and á á r4, r4, #0x3
> >> @@ -282,18 +382,20 @@ restore_3630:
> >> á á á ldr á á r1, control_mem_rta
> >> á á á mov á á r2, #OMAP36XX_RTA_DISABLE
> >> á á á str á á r2, [r1]
> >> - á á /* Fall thru for the remaining logic */
> >> +
> >> + á á /* Fall thru to common code 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*/
> >> - á á á á/* 1 - Only L1 and logic lost */
> >> - á á á á/* 2 - Only L2 lost - In this case, we wont be here */
> >> - á á á á/* 3 - Both L1 and L2 lost */
> >> + á á /* Check what was the reason for mpu reset and store the
> reason in r9*/
> >> + á á /* 1 - Only L1 and logic lost */
> >> + á á /* 2 - Only L2 lost - In this case, we wont be here */
> >> + á á /* 3 - Both L1 and L2 lost */
> >> á á á ldr á á r1, pm_pwstctrl_mpu
> >> á á á ldr á á r2, [r1]
> >> á á á and á á r2, r2, #0x3
> >> á á á cmp á á r2, #0x0 á á á á@ Check if target power state was OFF
or
> RET
> >> - á á á ámoveq á r9, #0x3 á á á á@ MPU OFF => L1 and L2 lost
> >> + á á moveq á r9, #0x3 á á á á@ MPU OFF => L1 and L2 lost
> >> á á á movne á r9, #0x1 á á á á@ Only L1 and L2 lost => avoid L2
> invalidation
> >> á á á bne á á logic_l1_restore
> >>
> >> @@ -309,23 +411,23 @@ skipl2dis:
> >> á á á and á á r1, #0x700
> >> á á á cmp á á r1, #0x300
> >> á á á beq á á l2_inv_gp
> >> - á á mov á á r0, #40 á á á á @ set service ID for PPA
> >> - á á mov á á r12, r0 á á á á @ copy secure Service ID in r12
> >> - á á mov á á r1, #0 á á á á á@ set task id for ROM code in r1
> >> - á á mov á á r2, #4 á á á á á@ set some flags in r2, r6
> >> + á á mov á á r0, #40 á á á á á á á á @ set service ID for PPA
> >> + á á mov á á r12, r0 á á á á á á á á @ copy secure Service ID in r12
> >> + á á mov á á r1, #0 á á á á á á á á á@ set task id for ROM code in
r1
> >> + á á mov á á r2, #4 á á á á á á á á á@ set some flags in r2, r6
> >
> > This is the type of change that should normally be in a separate
patch,
> > as it seems to be pure whitespace change.
> >
> >
> > Kevin
> >
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] OMAP3: clean up ASM idle code
2010-12-14 11:34 ` Vishwanath Sripathy
@ 2010-12-14 17:30 ` Jean Pihet
0 siblings, 0 replies; 6+ messages in thread
From: Jean Pihet @ 2010-12-14 17:30 UTC (permalink / raw)
To: Kevin Hilman; +Cc: Vishwanath Sripathy, linux-omap, Jean Pihet-XID
Kevin,
The reworked version has been resent. Please use '[PATCH v3] OMAP3:
clean up ASM idle code' which has the correct e-mail addresses for the
sign-offs.
Thanks,
Jean
On Tue, Dec 14, 2010 at 12:34 PM, Vishwanath Sripathy
<vishwanath.bs@ti.com> wrote:
> Jean,
>
>> -----Original Message-----
>> From: Jean Pihet [mailto:jean.pihet@newoldbits.com]
>> Sent: Tuesday, December 14, 2010 2:53 PM
>> To: Kevin Hilman; Vishwanath BS
>> Cc: linux-omap@vger.kernel.org; Jean Pihet
>> Subject: Re: [PATCH] OMAP3: clean up ASM idle code
>>
>> Kevin,
>>
>> On Tue, Dec 14, 2010 at 5:12 AM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>> > jean.pihet@newoldbits.com writes:
>> >
>> >> From: Jean Pihet <j-pihet@ti.com>
>> >>
>> >> Clean up of the ASM code:
>> >> - reorganized the code in logical sections: defines, API
>> >> á áfunctions, internal functions and variables,
>> >> - reworked and simplified the execution paths, for better
>> >> á áreadability and to avoid duplication of code,
>> >> - added comments on the entry and exit points,
>> >> - reworked the existing comments for better readability,
>> >> - reworked the code formating, alignment and white spaces,
>> >> - added comments for the i443 erratum workarounds,
>> >> - changed the hardcoded values in favor of existing macros
>> >> á áfrom include files,
>> >> - clean up of non used symbols.
>> >
>> > The 'lock_scratchpad_sem' code is also unused. áIIRC, you removed
>> that
>> > in an earlier version of the series. áAre you still planning to remove
>> > that? maybe in a subsequent patch?
>>
>> I asked previously about it and the reply was that this code is
>> needed: (quoting Vishwa's reply) "This is indeed used during DVFS when
>> Core DPLL configuration is changed". Note: the DVFS code is not
>> upstreamed yet.
>>
>> Vishwa, can you confirm?
> lock_scratchpad_sem is needed when DVFS feature is supported. If you want
> to add this implementation as part of DVFS feature, then also it's fine.
>
> Vishwa
>>
>> > That being said, pure whitespace changes and unused code removal
>> should
>> > probably be a separate patch. áIt's a great help to reviewers if
>> > functional changes are separated from whitespace changes. áIt's a bit
>> > tricky in this series as there's lots of code moving as well, so I'll
>> > leave it up to your judgement on how/if to separate these out.
>>
>> There is no functional change at all. The code has been reorganized
>> for better readability.
>> I agree the patch is not easy to read but that is the way diff reports
>> the changes.
>>
>> I do not see the point to provide separate patches for code move,
>> white space clean-up, alignment fixes etc.
>>
>> >> Tested on OMAP3EVM and Beagleboard with full RET and OFF modes.
>> >
>> > In idle? áin suspend? áboth? áwas CPUidle enabled?
>> >
>> > FWIW, I tested on 3430-ES3.1/n900 with retention idle & suspend and
>> off
>> > idle & suspend with CPUidle enabled.
>> Tested with cpuidle and suspend. I will update the description.
>>
>> >
>> >> Heavily reworked from Vishwa's original patch.
>> >
>> > Here, it's more customary to ásay "Based on original patch from
>> Vishwa"
>> > and ensure the original author is CC'd (which you've done.)
>> >
>> > Other than that, this is a great cleanup, and makes this much more
>> > readable. áThanks.
>> >
>> > Some other minor comments below.
>>
>> OK thanks for the review. I will post a new version asap.
>>
>> >
>> >> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> >> Cc: Vishwanath BS <vishwanath.bs@ti.com>
>> >> ---
>> >> Applies on top of Nishant's latest idle path errata fixes step 2,
>> >> cf. http://marc.info/?l=linux-omap&m=129139584919242&w=2
>> >>
>> >
>> > [...]
>> >
>> >> @@ -208,36 +172,153 @@ api_params:
>> >> áENTRY(save_secure_ram_context_sz)
>> >> á á á .word á . - save_secure_ram_context
>> >>
>> >> +
>> >> +/* ======================
>> >> + * == Idle entry point ==
>> >> + * ======================
>> >> + */
>> >
>> > Let's keep C multi-line coding style even for assembly. á Same goes
> for
>> > several other places below.
>> Ok
>>
>> >
>> >> á/*
>> >> á * Forces OMAP into idle state
>> >> á *
>> >> - * omap34xx_suspend() - This bit of code just executes the WFI
>> >> - * for normal idles.
>> >> + * omap34xx_suspend() - This bit of code saves the CPU context if
>> needed
>> >> + * and executes the WFI instruction
>> >> + *
>> >> + * Note: This code get's copied to internal SRAM at boot.
>> >> á *
>> >> - * 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.
>> >> + * Note: When the OMAP wakes up it continues at different
>> execution points,
>> >> + * ádepending on the low power mode (non-OFF vs OFF modes),
>> >> + * ácf. 'Resume path for xxx mode' comments.
>> >> á */
>> >> áENTRY(omap34xx_cpu_suspend)
>> >> á á á stmfd á sp!, {r0-r12, lr} á á á á á á á @ save registers on
> stack
>> >> áloop:
>> >> á á á /*b á á loop*/ á@Enable to debug by stepping through code
>> >
>> > While here, let's get rid of these infinite loop hacks for debugging
> as
>> > anyone debugging this code can add these back as needed.
>> áOtherwise,
>> > they clutter the code. á There are a few of theses throughout the
>> > original code.
>> Ok. Agree those are not used at all (even when doing heavy debugging).
>>
>> > [...]
>> >
>> >> @@ -250,9 +331,28 @@ loop:
>> >> á á á nop
>> >> á á á bl wait_sdrc_ok
>> >>
>> >> - á á ldmfd á sp!, {r0-r12, pc} á á á á á á á @ restore regs and
> return
>> >> +/* ===================================
>> >> + * == Exit point from non-OFF modes ==
>> >> + * ===================================
>> >> + */
>> >> + á á ldmfd á sp!, {r0-r12, pc} á á á @ restore regs and return
>> >> +
>> >> +
>> >> +/* ==============================
>> >> + * == Resume path for OFF mode ==
>> >> + * ==============================
>> >> + */
>> >
>> > I don't think this is quite correct. áI don't believe it starts
>> > immediately here. áDoesn't it resume from DDR first, and then ájump
>> > here. áA brief description of that process would help clarify that
>> process.
>> This is the restore point from OFF mode. The ROM code calls this
>> directly, cf. the get_pointer* functions that are used to get the
>> resume pointer.
>> I will update the comment.
>>
>> >> +/*
>> >> + * The restore_* functions are executed when back from WFI in
>> OFF mode
>> >> + *
>> >> + * árestore_es3: applies to 34xx >= ES3.0
>> >> + * árestore_3630: applies to 36xx
>> >> + * árestore: common code for 3xxx
>> >> + */
>> >> árestore_es3:
>> >> á á á /*b restore_es3*/ á á á á á á á @ Enable to debug restore code
>> >> +
>> >> á á á ldr á á r5, pm_prepwstst_core_p
>> >> á á á ldr á á r4, [r5]
>> >> á á á and á á r4, r4, #0x3
>> >> @@ -282,18 +382,20 @@ restore_3630:
>> >> á á á ldr á á r1, control_mem_rta
>> >> á á á mov á á r2, #OMAP36XX_RTA_DISABLE
>> >> á á á str á á r2, [r1]
>> >> - á á /* Fall thru for the remaining logic */
>> >> +
>> >> + á á /* Fall thru to common code 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*/
>> >> - á á á á/* 1 - Only L1 and logic lost */
>> >> - á á á á/* 2 - Only L2 lost - In this case, we wont be here */
>> >> - á á á á/* 3 - Both L1 and L2 lost */
>> >> + á á /* Check what was the reason for mpu reset and store the
>> reason in r9*/
>> >> + á á /* 1 - Only L1 and logic lost */
>> >> + á á /* 2 - Only L2 lost - In this case, we wont be here */
>> >> + á á /* 3 - Both L1 and L2 lost */
>> >> á á á ldr á á r1, pm_pwstctrl_mpu
>> >> á á á ldr á á r2, [r1]
>> >> á á á and á á r2, r2, #0x3
>> >> á á á cmp á á r2, #0x0 á á á á@ Check if target power state was OFF
> or
>> RET
>> >> - á á á ámoveq á r9, #0x3 á á á á@ MPU OFF => L1 and L2 lost
>> >> + á á moveq á r9, #0x3 á á á á@ MPU OFF => L1 and L2 lost
>> >> á á á movne á r9, #0x1 á á á á@ Only L1 and L2 lost => avoid L2
>> invalidation
>> >> á á á bne á á logic_l1_restore
>> >>
>> >> @@ -309,23 +411,23 @@ skipl2dis:
>> >> á á á and á á r1, #0x700
>> >> á á á cmp á á r1, #0x300
>> >> á á á beq á á l2_inv_gp
>> >> - á á mov á á r0, #40 á á á á @ set service ID for PPA
>> >> - á á mov á á r12, r0 á á á á @ copy secure Service ID in r12
>> >> - á á mov á á r1, #0 á á á á á@ set task id for ROM code in r1
>> >> - á á mov á á r2, #4 á á á á á@ set some flags in r2, r6
>> >> + á á mov á á r0, #40 á á á á á á á á @ set service ID for PPA
>> >> + á á mov á á r12, r0 á á á á á á á á @ copy secure Service ID in r12
>> >> + á á mov á á r1, #0 á á á á á á á á á@ set task id for ROM code in
> r1
>> >> + á á mov á á r2, #4 á á á á á á á á á@ set some flags in r2, r6
>> >
>> > This is the type of change that should normally be in a separate
> patch,
>> > as it seems to be pure whitespace change.
>> >
>> >
>> > Kevin
>> >
>>
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-14 17:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-07 11:11 [PATCH] OMAP3: clean up ASM idle code jean.pihet
2010-12-13 14:51 ` Vishwanath Sripathy
2010-12-14 4:12 ` Kevin Hilman
2010-12-14 9:22 ` Jean Pihet
2010-12-14 11:34 ` Vishwanath Sripathy
2010-12-14 17:30 ` Jean Pihet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.