Hi Paul,<br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Dec 9, 2012 at 2:23 AM, Paul Walmsley <span dir="ltr"><<a href="mailto:paul@pwsan.com" target="_blank">paul@pwsan.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Move omap_set_pwrdm_state() from the PM code to the powerdomain code,<br>
and refactor it to split it up into several functions.  A subsequent patch<br>
will rename it to conform with the existing powerdomain function names.<br></blockquote><div>Glad to see this rather cryptic function getting a rewrite. It had never been clear what the function is doing so I think it owes some more comments.<br>
<br>More comments below. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Signed-off-by: Paul Walmsley <<a href="mailto:paul@pwsan.com">paul@pwsan.com</a>><br>
Cc: Jean Pihet <<a href="mailto:jean.pihet@newoldbits.com">jean.pihet@newoldbits.com</a>><br>
Cc: Kevin Hilman <<a href="mailto:khilman@deeprootsystems.com">khilman@deeprootsystems.com</a>><br>
---<br>
 arch/arm/mach-omap2/pm.c          |   61 --------------------<br>
 arch/arm/mach-omap2/pm.h          |    1<br>
 arch/arm/mach-omap2/powerdomain.c |  112 +++++++++++++++++++++++++++----------<br>
 arch/arm/mach-omap2/powerdomain.h |    3 +<br>
 4 files changed, 85 insertions(+), 92 deletions(-)<br>
<br>
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c<br>
index cc8ed0f..2e2a897 100644<br>
--- a/arch/arm/mach-omap2/pm.c<br>
+++ b/arch/arm/mach-omap2/pm.c<br>
@@ -76,10 +76,6 @@ static void __init omap2_init_processor_devices(void)<br>
        }<br>
 }<br>
<br>
-/* Types of sleep_switch used in omap_set_pwrdm_state */<br>
-#define FORCEWAKEUP_SWITCH     0<br>
-#define LOWPOWERSTATE_SWITCH   1<br>
-<br>
 int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)<br>
 {<br>
        if ((clkdm->flags & CLKDM_CAN_ENABLE_AUTO) &&<br>
@@ -92,63 +88,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)<br>
 }<br>
<br>
 /*<br>
- * This sets pwrdm state (other than mpu & core. Currently only ON &<br>
- * RET are supported.<br>
- */<br>
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)<br>
-{<br>
-       u8 curr_pwrst, next_pwrst;<br>
-       int sleep_switch = -1, ret = 0, hwsup = 0;<br>
-<br>
-       if (!pwrdm || IS_ERR(pwrdm))<br>
-               return -EINVAL;<br>
-<br>
-       while (!(pwrdm->pwrsts & (1 << pwrst))) {<br>
-               if (pwrst == PWRDM_POWER_OFF)<br>
-                       return ret;<br>
-               pwrst--;<br>
-       }<br>
-<br>
-       next_pwrst = pwrdm_read_next_pwrst(pwrdm);<br>
-       if (next_pwrst == pwrst)<br>
-               return ret;<br>
-<br>
-       curr_pwrst = pwrdm_read_pwrst(pwrdm);<br>
-       if (curr_pwrst < PWRDM_POWER_ON) {<br>
-               if ((curr_pwrst > pwrst) &&<br>
-                       (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE)) {<br>
-                       sleep_switch = LOWPOWERSTATE_SWITCH;<br>
-               } else {<br>
-                       hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]);<br>
-                       clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);<br>
-                       sleep_switch = FORCEWAKEUP_SWITCH;<br>
-               }<br>
-       }<br>
-<br>
-       ret = pwrdm_set_next_pwrst(pwrdm, pwrst);<br>
-       if (ret)<br>
-               pr_err("%s: unable to set power state of powerdomain: %s\n",<br>
-                      __func__, pwrdm->name);<br>
-<br>
-       switch (sleep_switch) {<br>
-       case FORCEWAKEUP_SWITCH:<br>
-               if (hwsup)<br>
-                       clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);<br>
-               else<br>
-                       clkdm_sleep(pwrdm->pwrdm_clkdms[0]);<br>
-               break;<br>
-       case LOWPOWERSTATE_SWITCH:<br>
-               pwrdm_set_lowpwrstchange(pwrdm);<br>
-               pwrdm_state_switch(pwrdm);<br>
-               break;<br>
-       }<br>
-<br>
-       return ret;<br>
-}<br>
-<br>
-<br>
-<br>
-/*<br>
  * This API is to be called during init to set the various voltage<br>
  * domains to the voltage as per the opp table. Typically we boot up<br>
  * at the nominal voltage. So this function finds out the rate of<br>
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h<br>
index 686137d..707e9cb 100644<br>
--- a/arch/arm/mach-omap2/pm.h<br>
+++ b/arch/arm/mach-omap2/pm.h<br>
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)<br>
 extern void *omap3_secure_ram_storage;<br>
 extern void omap3_pm_off_mode_enable(int);<br>
 extern void omap_sram_idle(void);<br>
-extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);<br>
 extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);<br>
 extern int (*omap_pm_suspend)(void);<br>
<br>
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c<br>
index 97b3881..05f00660 100644<br>
--- a/arch/arm/mach-omap2/powerdomain.c<br>
+++ b/arch/arm/mach-omap2/powerdomain.c<br>
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)<br>
        return (pwrdm && pwrdm->flags & PWRDM_HAS_HDWR_SAR) ? 1 : 0;<br>
 }<br>
<br>
-/**<br>
- * pwrdm_set_lowpwrstchange - Request a low power state change<br>
- * @pwrdm: struct powerdomain *<br>
- *<br>
- * Allows a powerdomain to transtion to a lower power sleep state<br>
- * from an existing sleep state without waking up the powerdomain.<br>
- * Returns -EINVAL if the powerdomain pointer is null or if the<br>
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0<br>
- * upon success.<br>
- */<br></blockquote><div><br>Can this kerneldoc be reused in the new code?<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm)<br>
-{<br>
-       int ret = -EINVAL;<br>
-<br>
-       if (!pwrdm)<br>
-               return -EINVAL;<br>
-<br>
-       if (!(pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE))<br>
-               return -EINVAL;<br>
-<br>
-       pr_debug("powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n",<br>
-                pwrdm->name);<br>
-<br>
-       if (arch_pwrdm && arch_pwrdm->pwrdm_set_lowpwrstchange)<br>
-               ret = arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);<br>
-<br>
-       return ret;<br>
-}<br>
-<br>
 int pwrdm_state_switch(struct powerdomain *pwrdm)<br>
 {<br>
        int ret;<br>
@@ -984,6 +955,89 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)<br>
        return 0;<br>
 }<br>
<br>
+/* Types of sleep_switch used in omap_set_pwrdm_state */<br>
+#define ALREADYACTIVE_SWITCH   0<br>
+#define FORCEWAKEUP_SWITCH     1<br>
+#define LOWPOWERSTATE_SWITCH   2 <br></blockquote><div><br>Could you add some more documentation here?<br>What is the goal of the function, what does it return etc. ?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,<br>
+                                              u8 pwrst, bool *hwsup) </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+{<br>
+       u8 curr_pwrst, sleep_switch;<br>
+<br>
+       curr_pwrst = pwrdm_read_pwrst(pwrdm);<br>
+       if (curr_pwrst < PWRDM_POWER_ON) {<br>
+               if (curr_pwrst > pwrst &&<br>
+                   pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&<br>
+                   arch_pwrdm->pwrdm_set_lowpwrstchange) {<br>
+                       sleep_switch = LOWPOWERSTATE_SWITCH;<br>
+               } else {<br>
+                       *hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]);<br>
+                       clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);<br>
+                       sleep_switch = FORCEWAKEUP_SWITCH;<br>
+               }<br>
+       } else {<br>
+               sleep_switch = ALREADYACTIVE_SWITCH;<br>
+       }<br>
+<br>
+       return sleep_switch;<br>
+}<br>
+<br></blockquote><div><br>Same here<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+static void _pwrdm_restore_clkdm_state(struct powerdomain *pwrdm,<br>
+                                      u8 sleep_switch, bool hwsup)<br>
+{<br>
+       switch (sleep_switch) {<br>
+       case FORCEWAKEUP_SWITCH:<br>
+               if (hwsup)<br>
+                       clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);<br>
+               else<br>
+                       clkdm_sleep(pwrdm->pwrdm_clkdms[0]);<br>
+               break;<br>
+       case LOWPOWERSTATE_SWITCH:<br>
+               if (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&<br>
+                   arch_pwrdm->pwrdm_set_lowpwrstchange)<br>
+                       arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);<br>
+               pwrdm_state_switch(pwrdm);<br>
+               break;<br>
+       }<br>
+}<br>
+<br>
+/*<br>
+ * This sets pwrdm state (other than mpu & core. Currently only ON &<br>
+ * RET are supported.<br>
+ */<br></blockquote><div>Same here.<br>Is this one correct?<br><br>Regards,<br>Jean<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

+int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)<br>
+{<br>
+       u8 next_pwrst, sleep_switch;<br>
+       int ret = 0;<br>
+       bool hwsup = false;<br>
+<br>
+       if (!pwrdm || IS_ERR(pwrdm))<br>
+               return -EINVAL;<br>
+<br>
+       while (!(pwrdm->pwrsts & (1 << pwrst))) {<br>
+               if (pwrst == PWRDM_POWER_OFF)<br>
+                       return ret;<br>
+               pwrst--;<br>
+       }<br>
+<br>
+       next_pwrst = pwrdm_read_next_pwrst(pwrdm);<br>
+       if (next_pwrst == pwrst)<br>
+               return ret;<br>
+<br>
+       sleep_switch = _pwrdm_save_clkdm_state_and_activate(pwrdm, pwrst,<br>
+                                                           &hwsup);<br>
+<br>
+       ret = pwrdm_set_next_pwrst(pwrdm, pwrst);<br>
+       if (ret)<br>
+               pr_err("%s: unable to set power state of powerdomain: %s\n",<br>
+                      __func__, pwrdm->name);<br>
+<br>
+       _pwrdm_restore_clkdm_state(pwrdm, sleep_switch, hwsup);<br>
+<br>
+       return ret;<br>
+}<br>
+<br>
 /**<br>
  * pwrdm_get_context_loss_count - get powerdomain's context loss count<br>
  * @pwrdm: struct powerdomain * to wait for<br>
diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h<br>
index 7c1534b..1edb3b7 100644<br>
--- a/arch/arm/mach-omap2/powerdomain.h<br>
+++ b/arch/arm/mach-omap2/powerdomain.h<br>
@@ -228,10 +228,11 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);<br>
 int pwrdm_state_switch(struct powerdomain *pwrdm);<br>
 int pwrdm_pre_transition(struct powerdomain *pwrdm);<br>
 int pwrdm_post_transition(struct powerdomain *pwrdm);<br>
-int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm);<br>
 int pwrdm_get_context_loss_count(struct powerdomain *pwrdm);<br>
 bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm);<br>
<br>
+extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 state);<br>
+<br>
 extern void omap242x_powerdomains_init(void);<br>
 extern void omap243x_powerdomains_init(void);<br>
 extern void omap3xxx_powerdomains_init(void);<br>
<br>
<br>
</blockquote></div><br></div>