Re: [PATCH 05/12] ARM: OMAP3xxx: PM: convert to use the functional power states API

2013-01-04 Thread Tero Kristo 
Hi Paul,

On Sun, 2012-12-09 at 10:53 -0700, Paul Walmsley wrote:

clip

 @@ -112,24 +112,26 @@ static void omap3_core_restore_context(void)
  static void omap3_save_secure_ram_context(void)
  {
   u32 ret;
 - int mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
 + int mpu_next_fpwrst;
  
 - if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
 - /*
 -  * MPU next state must be set to POWER_ON temporarily,
 -  * otherwise the WFI executed inside the ROM code
 -  * will hang the system.
 -  */
 - pwrdm_set_next_pwrst(mpu_pwrdm, PWRDM_POWER_ON);
 - ret = _omap_save_secure_sram((u32 *)
 - __pa(omap3_secure_ram_storage));
 - pwrdm_set_next_pwrst(mpu_pwrdm, mpu_next_state);
 - /* Following is for error tracking, it should not happen */
 - if (ret) {
 - pr_err(save_secure_sram() returns %08x\n, ret);
 - while (1)
 - ;
 - }
 + if (omap_type() == OMAP2_DEVICE_TYPE_GP)
 + return;
 +
 + /*
 +  * MPU next state must be set to POWER_ON temporarily,
 +  * otherwise the WFI executed inside the ROM code will hang
 +  * the system.
 +  */
 + mpu_next_fpwrst = pwrdm_read_next_fpwrst(mpu_pwrdm);
 + pwrdm_set_next_fpwrst(mpu_pwrdm, PWRDM_FUNC_PWRST_ON);
 + ret = _omap_save_secure_sram((u32 *)__pa(omap3_secure_ram_storage));
 + pwrdm_set_next_fpwrst(mpu_pwrdm, mpu_next_fpwrst);
 + /* Following is for error tracking, it should not happen */
 + /* XXX This needs to be converted to a BUG() or removed */
 + if (ret) {
 + pr_err(save_secure_sram() returns %08x\n, ret);
 + while (1)
 + ;

Just a minor comment here, this can be converted to BUG_ON right now, as
it is only used from init code.

Another thing to consider with this code is, that there might be someone
who actually wants to use secure services from the kernel, and in this
case you need to do the save every time you are entering off-mode if
secure services have been used. If this save fails, the core powerdomain
should be re-programmed to RET, or prevent idle completely. Maybe this
needs to be moved into secure driver or something, and use cpu_pm
notifiers they don't support failures currently though.

-Tero


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

Re: [PATCH 05/12] ARM: OMAP3xxx: PM: convert to use the functional power states API

2012-12-12 Thread Jean Pihet 
Paul,

-resending in plain text only, sorry about that-

On Sun, Dec 9, 2012 at 6:53 PM, Paul Walmsley p...@pwsan.com wrote:

 From: Jean Pihet jean.pi...@newoldbits.com

 Use the functional power states as the API to control power domains:

 - use the PWRDM_FUNC_PWRST_* macros for the power states and logic
   settings,

 - the function pwrdm_set_next_fpwrst(), which controls the power domains
   next power and logic settings, shall be used instead of
   pwrdm_set_next_pwrst() to program the power domains next states,

 - the function pwrdm_set_fpwrst(), which programs the power domains
   power and logic settings, shall be used instead of
   omap_set_pwrdm_state().

 Signed-off-by: Jean Pihet j-pi...@ti.com
 [p...@pwsan.com: split the original patch into OMAP2/3/4 variants;
  clean up omap3_save_secure_ram_context(); fix commit message;
  warn if sets fail; various other changes]


There are a lot of 'XXX' comments, can this be fixed by a proper comment?

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

[PATCH 05/12] ARM: OMAP3xxx: PM: convert to use the functional power states API

2012-12-09 Thread Paul Walmsley 
From: Jean Pihet jean.pi...@newoldbits.com

Use the functional power states as the API to control power domains:

- use the PWRDM_FUNC_PWRST_* macros for the power states and logic
  settings,

- the function pwrdm_set_next_fpwrst(), which controls the power domains
  next power and logic settings, shall be used instead of
  pwrdm_set_next_pwrst() to program the power domains next states,

- the function pwrdm_set_fpwrst(), which programs the power domains
  power and logic settings, shall be used instead of
  omap_set_pwrdm_state().

Signed-off-by: Jean Pihet j-pi...@ti.com
[p...@pwsan.com: split the original patch into OMAP2/3/4 variants;
 clean up omap3_save_secure_ram_context(); fix commit message;
 warn if sets fail; various other changes]
Signed-off-by: Paul Walmsley p...@pwsan.com
---
 arch/arm/mach-omap2/cpuidle34xx.c |   95 ---
 arch/arm/mach-omap2/pm.h  |2 
 arch/arm/mach-omap2/pm34xx.c  |  156 +++--
 3 files changed, 130 insertions(+), 123 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
b/arch/arm/mach-omap2/cpuidle34xx.c
index e6215b5..a8e6263 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -36,9 +36,9 @@
 
 /* Mach specific information to be recorded in the C-state driver_data */
 struct omap3_idle_statedata {
-   u8 mpu_state;
-   u8 core_state;
-   u8 per_min_state;
+   u8 mpu_fpwrst;
+   u8 core_fpwrst;
+   u8 per_min_fpwrst;
u8 flags;
 };
 
@@ -61,41 +61,41 @@ static struct powerdomain *mpu_pd, *core_pd, *per_pd, 
*cam_pd;
  */
 static struct omap3_idle_statedata omap3_idle_data[] = {
{
-   .mpu_state = PWRDM_POWER_ON,
-   .core_state = PWRDM_POWER_ON,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_ON,
+   .core_fpwrst = PWRDM_FUNC_PWRST_ON,
/* In C1 do not allow PER state lower than CORE state */
-   .per_min_state = PWRDM_POWER_ON,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_ON,
.flags = OMAP_CPUIDLE_CX_NO_CLKDM_IDLE,
},
{
-   .mpu_state = PWRDM_POWER_ON,
-   .core_state = PWRDM_POWER_ON,
-   .per_min_state = PWRDM_POWER_RET,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_ON,
+   .core_fpwrst = PWRDM_FUNC_PWRST_ON,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_CSWR,
},
{
-   .mpu_state = PWRDM_POWER_RET,
-   .core_state = PWRDM_POWER_ON,
-   .per_min_state = PWRDM_POWER_RET,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_CSWR,
+   .core_fpwrst = PWRDM_FUNC_PWRST_ON,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_CSWR,
},
{
-   .mpu_state = PWRDM_POWER_OFF,
-   .core_state = PWRDM_POWER_ON,
-   .per_min_state = PWRDM_POWER_RET,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_OFF,
+   .core_fpwrst = PWRDM_FUNC_PWRST_ON,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_CSWR,
},
{
-   .mpu_state = PWRDM_POWER_RET,
-   .core_state = PWRDM_POWER_RET,
-   .per_min_state = PWRDM_POWER_OFF,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_CSWR,
+   .core_fpwrst = PWRDM_FUNC_PWRST_CSWR,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_OFF,
},
{
-   .mpu_state = PWRDM_POWER_OFF,
-   .core_state = PWRDM_POWER_RET,
-   .per_min_state = PWRDM_POWER_OFF,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_OFF,
+   .core_fpwrst = PWRDM_FUNC_PWRST_CSWR,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_OFF,
},
{
-   .mpu_state = PWRDM_POWER_OFF,
-   .core_state = PWRDM_POWER_OFF,
-   .per_min_state = PWRDM_POWER_OFF,
+   .mpu_fpwrst = PWRDM_FUNC_PWRST_OFF,
+   .core_fpwrst = PWRDM_FUNC_PWRST_OFF,
+   .per_min_fpwrst = PWRDM_FUNC_PWRST_OFF,
},
 };
 
@@ -116,15 +116,15 @@ static int __omap3_enter_idle(struct cpuidle_device *dev,
if (cx-flags  OMAP_CPUIDLE_CX_NO_CLKDM_IDLE) {
clkdm_deny_idle(mpu_pd-pwrdm_clkdms[0]);
} else {
-   pwrdm_set_next_pwrst(mpu_pd, cx-mpu_state);
-   pwrdm_set_next_pwrst(core_pd, cx-core_state);
+   pwrdm_set_next_fpwrst(mpu_pd, cx-mpu_fpwrst);
+   pwrdm_set_next_fpwrst(core_pd, cx-core_fpwrst);
}
 
/*
 * Call idle CPU PM enter notifier chain so that
 * VFP context is saved.
 */
-   if (cx-mpu_state == PWRDM_POWER_OFF)
+   if (cx-mpu_fpwrst == PWRDM_FUNC_PWRST_OFF)
cpu_pm_enter();
 
/* Execute ARM wfi */
@@ -134,8 +134,8 @@ static int __omap3_enter_idle(struct cpuidle_device *dev,
 * Call idle CPU PM enter notifier chain to restore
 * VFP context.