Re: [PATCH] OMAP3: clean up ASM idle code
Kevin,
On Tue, Dec 14, 2010 at 5:12 AM, Kevin Hilman
khil...@deeprootsystems.com wrote:
jean.pi...@newoldbits.com writes:
From: Jean Pihet j-pi...@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-pi...@ti.com
Cc: Vishwanath BS vishwanath...@ti.com
---
Applies on top of Nishant's latest idle path errata fixes step 2,
cf. http://marc.info/?l=linux-omapm=129139584919242w=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
RE: [PATCH] OMAP3: clean up ASM idle code
Jean,
-Original Message-
From: Jean Pihet [mailto:jean.pi...@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
khil...@deeprootsystems.com wrote:
jean.pi...@newoldbits.com writes:
From: Jean Pihet j-pi...@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-pi...@ti.com
Cc: Vishwanath BS vishwanath...@ti.com
---
Applies on top of Nishant's latest idle path errata fixes step 2,
cf. http://marc.info/?l=linux-omapm=129139584919242w=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
Re: [PATCH] OMAP3: clean up ASM idle code
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...@ti.com wrote:
Jean,
-Original Message-
From: Jean Pihet [mailto:jean.pi...@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
khil...@deeprootsystems.com wrote:
jean.pi...@newoldbits.com writes:
From: Jean Pihet j-pi...@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-pi...@ti.com
Cc: Vishwanath BS vishwanath...@ti.com
---
Applies on top of Nishant's latest idle path errata fixes step 2,
cf. http://marc.info/?l=linux-omapm=129139584919242w=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
- á á
RE: [PATCH] OMAP3: clean up ASM idle code
-Original Message- From: jean.pi...@newoldbits.com [mailto:jean.pi...@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-pi...@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-pi...@ti.com Cc: Vishwanath BS vishwanath...@ti.com Tested on ZOOM3 for Retention and OFF in suspend and Idle path. Acked-by: Vishwanath BS vishwanath...@ti.com --- Applies on top of Nishant's latest idle path errata fixes step 2, cf. http://marc.info/?l=linux-omapm=129139584919242w=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_V0xfa00291c - -#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_POMAP3430_PRM_BASE + MPU_MOD + OMAP2_PM_PWSTCTRL #define CM_IDLEST1_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1) #define CM_IDLEST_CKGEN_VOMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST) #define SRAM_BASE_P 0x4020 -#define CONTROL_STAT 0x480022F0 -#define CONTROL_MEM_RTA_CTRL
Re: [PATCH] OMAP3: clean up ASM idle code
jean.pi...@newoldbits.com writes:
From: Jean Pihet j-pi...@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-pi...@ti.com
Cc: Vishwanath BS vishwanath...@ti.com
---
Applies on top of Nishant's latest idle path errata fixes step 2,
cf. http://marc.info/?l=linux-omapm=129139584919242w=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
