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>