Re: [PATCH 05/10] ARM: OMAP2+: PM/powerdomain: move omap_set_pwrdm_state() to powerdomain code
Hi,
(redacted irrelevant code sections)
On Wed, 12 Dec 2012, Jean Pihet wrote:
On Sun, Dec 9, 2012 at 2:23 AM, Paul Walmsley p...@pwsan.com wrote:
-/**
- * pwrdm_set_lowpwrstchange - Request a low power state change
- * @pwrdm: struct powerdomain *
- *
- * Allows a powerdomain to transtion to a lower power sleep state
- * from an existing sleep state without waking up the powerdomain.
- * Returns -EINVAL if the powerdomain pointer is null or if the
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
- * upon success.
- */
Can this kerneldoc be reused in the new code?
Not directly, since the function is being removed. But I've added some
LOWPOWERSTATECHANGE-related documentation in powerdomain.h in the updated
patch (below).
@@ -984,6 +955,89 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)
return 0;
}
+/* Types of sleep_switch used in omap_set_pwrdm_state */
+#define ALREADYACTIVE_SWITCH 0
+#define FORCEWAKEUP_SWITCH 1
+#define LOWPOWERSTATE_SWITCH 2
Could you add some more documentation here?
What is the goal of the function, what does it return etc. ?
I've added some related kerneldoc to omap_set_pwrdm_state().
+
+static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
+ u8 pwrst, bool *hwsup)
Same here
+static void _pwrdm_restore_clkdm_state(struct powerdomain *pwrdm,
+ u8 sleep_switch, bool hwsup)
Same here.
I've added kerneldoc for both of these functions.
+/*
+ * This sets pwrdm state (other than mpu core. Currently only ON
+ * RET are supported.
+ */
Is this one correct?
This is just a cut and paste from the previous comments in the file. I
agree that it is not helpful, so I've added kerneldoc for
omap_set_pwrdm_state().
The updated patch follows.
- Paul
From: Paul Walmsley p...@pwsan.com
Date: Tue, 29 Jan 2013 13:45:09 -0700
Subject: [PATCH] ARM: OMAP2+: PM/powerdomain: move omap_set_pwrdm_state() to
powerdomain code
Move omap_set_pwrdm_state() from the PM code to the powerdomain code,
and refactor it to split it up into several functions. A subsequent patch
will rename it to conform with the existing powerdomain function names.
This version includes some additional documentation, based on a
suggestion from Jean Pihet. It also modifies omap_set_pwrdm_state()
to not bail out early unless both the powerdomain current power state
and the next power state are equal. (Previously it would terminate
early if the next power state was equal to the target power state,
which was insufficiently rigorous.)
Signed-off-by: Paul Walmsley p...@pwsan.com
Cc: Jean Pihet jean.pi...@newoldbits.com
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Tero Kristo t-kri...@ti.com
---
arch/arm/mach-omap2/pm.c | 61 --
arch/arm/mach-omap2/pm.h |1 -
arch/arm/mach-omap2/powerdomain.c | 161 ++---
arch/arm/mach-omap2/powerdomain.h | 13 ++-
4 files changed, 144 insertions(+), 92 deletions(-)
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index f18afc9..48d6d5d 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -108,10 +108,6 @@ static void __init omap2_init_processor_devices(void)
}
}
-/* Types of sleep_switch used in omap_set_pwrdm_state */
-#define FORCEWAKEUP_SWITCH 0
-#define LOWPOWERSTATE_SWITCH 1
-
int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
{
if ((clkdm-flags CLKDM_CAN_ENABLE_AUTO)
@@ -124,63 +120,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm,
void *unused)
}
/*
- * This sets pwrdm state (other than mpu core. Currently only ON
- * RET are supported.
- */
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
-{
- u8 curr_pwrst, next_pwrst;
- int sleep_switch = -1, ret = 0, hwsup = 0;
-
- if (!pwrdm || IS_ERR(pwrdm))
- return -EINVAL;
-
- while (!(pwrdm-pwrsts (1 pwrst))) {
- if (pwrst == PWRDM_POWER_OFF)
- return ret;
- pwrst--;
- }
-
- next_pwrst = pwrdm_read_next_pwrst(pwrdm);
- if (next_pwrst == pwrst)
- return ret;
-
- curr_pwrst = pwrdm_read_pwrst(pwrdm);
- if (curr_pwrst PWRDM_POWER_ON) {
- if ((curr_pwrst pwrst)
- (pwrdm-flags PWRDM_HAS_LOWPOWERSTATECHANGE)) {
- sleep_switch = LOWPOWERSTATE_SWITCH;
- } else {
- hwsup = clkdm_in_hwsup(pwrdm-pwrdm_clkdms[0]);
- clkdm_wakeup(pwrdm-pwrdm_clkdms[0]);
- sleep_switch = FORCEWAKEUP_SWITCH;
- }
- }
-
- ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
- if (ret)
- pr_err(%s: unable to set power state of powerdomain: %s\n,
-
Re: [PATCH 05/10] ARM: OMAP2+: PM/powerdomain: move omap_set_pwrdm_state() to powerdomain code
On Wed, Dec 12, 2012 at 03:51:49PM +0530, Vaibhav Hiremath wrote:
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
-{
- u8 curr_pwrst, next_pwrst;
- int sleep_switch = -1, ret = 0, hwsup = 0;
-
- if (!pwrdm || IS_ERR(pwrdm))
You can use IS_ERR_OR_NULL here.
As this is removing code...
However, if you pay attention to linux-kernel or linux-arm-kernel, you will
notice that I've sent a patch deprecating IS_ERR_OR_NULL() and I'm starting
to remove its use. IS_ERR_OR_NULL() creates more problems than it solves
because people don't think about what the hell they're doing with it, or
even whether it's appropriate to use it.
--
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/10] ARM: OMAP2+: PM/powerdomain: move omap_set_pwrdm_state() to powerdomain code
Minor comment below,
On 12/9/2012 6:53 AM, Paul Walmsley wrote:
Move omap_set_pwrdm_state() from the PM code to the powerdomain code,
and refactor it to split it up into several functions. A subsequent patch
will rename it to conform with the existing powerdomain function names.
Signed-off-by: Paul Walmsley p...@pwsan.com
Cc: Jean Pihet jean.pi...@newoldbits.com
Cc: Kevin Hilman khil...@deeprootsystems.com
---
arch/arm/mach-omap2/pm.c | 61
arch/arm/mach-omap2/pm.h |1
arch/arm/mach-omap2/powerdomain.c | 112
+++--
arch/arm/mach-omap2/powerdomain.h |3 +
4 files changed, 85 insertions(+), 92 deletions(-)
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index cc8ed0f..2e2a897 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -76,10 +76,6 @@ static void __init omap2_init_processor_devices(void)
}
}
-/* Types of sleep_switch used in omap_set_pwrdm_state */
-#define FORCEWAKEUP_SWITCH 0
-#define LOWPOWERSTATE_SWITCH 1
-
int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
{
if ((clkdm-flags CLKDM_CAN_ENABLE_AUTO)
@@ -92,63 +88,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm,
void *unused)
}
/*
- * This sets pwrdm state (other than mpu core. Currently only ON
- * RET are supported.
- */
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
-{
- u8 curr_pwrst, next_pwrst;
- int sleep_switch = -1, ret = 0, hwsup = 0;
-
- if (!pwrdm || IS_ERR(pwrdm))
You can use IS_ERR_OR_NULL here.
Thanks,
Vaibhav
- return -EINVAL;
-
- while (!(pwrdm-pwrsts (1 pwrst))) {
- if (pwrst == PWRDM_POWER_OFF)
- return ret;
- pwrst--;
- }
-
- next_pwrst = pwrdm_read_next_pwrst(pwrdm);
- if (next_pwrst == pwrst)
- return ret;
-
- curr_pwrst = pwrdm_read_pwrst(pwrdm);
- if (curr_pwrst PWRDM_POWER_ON) {
- if ((curr_pwrst pwrst)
- (pwrdm-flags PWRDM_HAS_LOWPOWERSTATECHANGE)) {
- sleep_switch = LOWPOWERSTATE_SWITCH;
- } else {
- hwsup = clkdm_in_hwsup(pwrdm-pwrdm_clkdms[0]);
- clkdm_wakeup(pwrdm-pwrdm_clkdms[0]);
- sleep_switch = FORCEWAKEUP_SWITCH;
- }
- }
-
- ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
- if (ret)
- pr_err(%s: unable to set power state of powerdomain: %s\n,
-__func__, pwrdm-name);
-
- switch (sleep_switch) {
- case FORCEWAKEUP_SWITCH:
- if (hwsup)
- clkdm_allow_idle(pwrdm-pwrdm_clkdms[0]);
- else
- clkdm_sleep(pwrdm-pwrdm_clkdms[0]);
- break;
- case LOWPOWERSTATE_SWITCH:
- pwrdm_set_lowpwrstchange(pwrdm);
- pwrdm_state_switch(pwrdm);
- break;
- }
-
- return ret;
-}
-
-
-
-/*
* This API is to be called during init to set the various voltage
* domains to the voltage as per the opp table. Typically we boot up
* at the nominal voltage. So this function finds out the rate of
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 686137d..707e9cb 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)
extern void *omap3_secure_ram_storage;
extern void omap3_pm_off_mode_enable(int);
extern void omap_sram_idle(void);
-extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
extern int (*omap_pm_suspend)(void);
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 97b3881..05f00660 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
return (pwrdm pwrdm-flags PWRDM_HAS_HDWR_SAR) ? 1 : 0;
}
-/**
- * pwrdm_set_lowpwrstchange - Request a low power state change
- * @pwrdm: struct powerdomain *
- *
- * Allows a powerdomain to transtion to a lower power sleep state
- * from an existing sleep state without waking up the powerdomain.
- * Returns -EINVAL if the powerdomain pointer is null or if the
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
- * upon success.
- */
-int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm)
-{
- int ret = -EINVAL;
-
- if (!pwrdm)
- return -EINVAL;
-
- if (!(pwrdm-flags PWRDM_HAS_LOWPOWERSTATECHANGE))
- return -EINVAL;
-
- pr_debug(powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n,
- pwrdm-name);
-
- if (arch_pwrdm
Re: [PATCH 05/10] ARM: OMAP2+: PM/powerdomain: move omap_set_pwrdm_state() to powerdomain code
Hi Paul,
-resending in plain text only, sorry about that-
On Sun, Dec 9, 2012 at 2:23 AM, Paul Walmsley p...@pwsan.com wrote:
Move omap_set_pwrdm_state() from the PM code to the powerdomain code,
and refactor it to split it up into several functions. A subsequent patch
will rename it to conform with the existing powerdomain function names.
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.
More comments below.
Signed-off-by: Paul Walmsley p...@pwsan.com
Cc: Jean Pihet jean.pi...@newoldbits.com
Cc: Kevin Hilman khil...@deeprootsystems.com
---
arch/arm/mach-omap2/pm.c | 61
arch/arm/mach-omap2/pm.h |1
arch/arm/mach-omap2/powerdomain.c | 112
+++--
arch/arm/mach-omap2/powerdomain.h |3 +
4 files changed, 85 insertions(+), 92 deletions(-)
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index cc8ed0f..2e2a897 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -76,10 +76,6 @@ static void __init omap2_init_processor_devices(void)
}
}
-/* Types of sleep_switch used in omap_set_pwrdm_state */
-#define FORCEWAKEUP_SWITCH 0
-#define LOWPOWERSTATE_SWITCH 1
-
int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
{
if ((clkdm-flags CLKDM_CAN_ENABLE_AUTO)
@@ -92,63 +88,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm,
void *unused)
}
/*
- * This sets pwrdm state (other than mpu core. Currently only ON
- * RET are supported.
- */
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
-{
- u8 curr_pwrst, next_pwrst;
- int sleep_switch = -1, ret = 0, hwsup = 0;
-
- if (!pwrdm || IS_ERR(pwrdm))
- return -EINVAL;
-
- while (!(pwrdm-pwrsts (1 pwrst))) {
- if (pwrst == PWRDM_POWER_OFF)
- return ret;
- pwrst--;
- }
-
- next_pwrst = pwrdm_read_next_pwrst(pwrdm);
- if (next_pwrst == pwrst)
- return ret;
-
- curr_pwrst = pwrdm_read_pwrst(pwrdm);
- if (curr_pwrst PWRDM_POWER_ON) {
- if ((curr_pwrst pwrst)
- (pwrdm-flags PWRDM_HAS_LOWPOWERSTATECHANGE)) {
- sleep_switch = LOWPOWERSTATE_SWITCH;
- } else {
- hwsup = clkdm_in_hwsup(pwrdm-pwrdm_clkdms[0]);
- clkdm_wakeup(pwrdm-pwrdm_clkdms[0]);
- sleep_switch = FORCEWAKEUP_SWITCH;
- }
- }
-
- ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
- if (ret)
- pr_err(%s: unable to set power state of powerdomain: %s\n,
- __func__, pwrdm-name);
-
- switch (sleep_switch) {
- case FORCEWAKEUP_SWITCH:
- if (hwsup)
- clkdm_allow_idle(pwrdm-pwrdm_clkdms[0]);
- else
- clkdm_sleep(pwrdm-pwrdm_clkdms[0]);
- break;
- case LOWPOWERSTATE_SWITCH:
- pwrdm_set_lowpwrstchange(pwrdm);
- pwrdm_state_switch(pwrdm);
- break;
- }
-
- return ret;
-}
-
-
-
-/*
* This API is to be called during init to set the various voltage
* domains to the voltage as per the opp table. Typically we boot up
* at the nominal voltage. So this function finds out the rate of
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 686137d..707e9cb 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)
extern void *omap3_secure_ram_storage;
extern void omap3_pm_off_mode_enable(int);
extern void omap_sram_idle(void);
-extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
extern int (*omap_pm_suspend)(void);
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 97b3881..05f00660 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
return (pwrdm pwrdm-flags PWRDM_HAS_HDWR_SAR) ? 1 : 0;
}
-/**
- * pwrdm_set_lowpwrstchange - Request a low power state change
- * @pwrdm: struct powerdomain *
- *
- * Allows a powerdomain to transtion to a lower power sleep state
- * from an existing sleep state without waking up the powerdomain.
- * Returns -EINVAL if the powerdomain pointer is null or if the
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
- * upon success.
- */
Can this kerneldoc be reused in the new code?
Could you add some more documentation here?
What is the goal of the function,
[PATCH 05/10] ARM: OMAP2+: PM/powerdomain: move omap_set_pwrdm_state() to powerdomain code
Move omap_set_pwrdm_state() from the PM code to the powerdomain code,
and refactor it to split it up into several functions. A subsequent patch
will rename it to conform with the existing powerdomain function names.
Signed-off-by: Paul Walmsley p...@pwsan.com
Cc: Jean Pihet jean.pi...@newoldbits.com
Cc: Kevin Hilman khil...@deeprootsystems.com
---
arch/arm/mach-omap2/pm.c | 61
arch/arm/mach-omap2/pm.h |1
arch/arm/mach-omap2/powerdomain.c | 112 +++--
arch/arm/mach-omap2/powerdomain.h |3 +
4 files changed, 85 insertions(+), 92 deletions(-)
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index cc8ed0f..2e2a897 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -76,10 +76,6 @@ static void __init omap2_init_processor_devices(void)
}
}
-/* Types of sleep_switch used in omap_set_pwrdm_state */
-#define FORCEWAKEUP_SWITCH 0
-#define LOWPOWERSTATE_SWITCH 1
-
int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
{
if ((clkdm-flags CLKDM_CAN_ENABLE_AUTO)
@@ -92,63 +88,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm,
void *unused)
}
/*
- * This sets pwrdm state (other than mpu core. Currently only ON
- * RET are supported.
- */
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
-{
- u8 curr_pwrst, next_pwrst;
- int sleep_switch = -1, ret = 0, hwsup = 0;
-
- if (!pwrdm || IS_ERR(pwrdm))
- return -EINVAL;
-
- while (!(pwrdm-pwrsts (1 pwrst))) {
- if (pwrst == PWRDM_POWER_OFF)
- return ret;
- pwrst--;
- }
-
- next_pwrst = pwrdm_read_next_pwrst(pwrdm);
- if (next_pwrst == pwrst)
- return ret;
-
- curr_pwrst = pwrdm_read_pwrst(pwrdm);
- if (curr_pwrst PWRDM_POWER_ON) {
- if ((curr_pwrst pwrst)
- (pwrdm-flags PWRDM_HAS_LOWPOWERSTATECHANGE)) {
- sleep_switch = LOWPOWERSTATE_SWITCH;
- } else {
- hwsup = clkdm_in_hwsup(pwrdm-pwrdm_clkdms[0]);
- clkdm_wakeup(pwrdm-pwrdm_clkdms[0]);
- sleep_switch = FORCEWAKEUP_SWITCH;
- }
- }
-
- ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
- if (ret)
- pr_err(%s: unable to set power state of powerdomain: %s\n,
- __func__, pwrdm-name);
-
- switch (sleep_switch) {
- case FORCEWAKEUP_SWITCH:
- if (hwsup)
- clkdm_allow_idle(pwrdm-pwrdm_clkdms[0]);
- else
- clkdm_sleep(pwrdm-pwrdm_clkdms[0]);
- break;
- case LOWPOWERSTATE_SWITCH:
- pwrdm_set_lowpwrstchange(pwrdm);
- pwrdm_state_switch(pwrdm);
- break;
- }
-
- return ret;
-}
-
-
-
-/*
* This API is to be called during init to set the various voltage
* domains to the voltage as per the opp table. Typically we boot up
* at the nominal voltage. So this function finds out the rate of
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 686137d..707e9cb 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)
extern void *omap3_secure_ram_storage;
extern void omap3_pm_off_mode_enable(int);
extern void omap_sram_idle(void);
-extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
extern int (*omap_pm_suspend)(void);
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 97b3881..05f00660 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
return (pwrdm pwrdm-flags PWRDM_HAS_HDWR_SAR) ? 1 : 0;
}
-/**
- * pwrdm_set_lowpwrstchange - Request a low power state change
- * @pwrdm: struct powerdomain *
- *
- * Allows a powerdomain to transtion to a lower power sleep state
- * from an existing sleep state without waking up the powerdomain.
- * Returns -EINVAL if the powerdomain pointer is null or if the
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
- * upon success.
- */
-int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm)
-{
- int ret = -EINVAL;
-
- if (!pwrdm)
- return -EINVAL;
-
- if (!(pwrdm-flags PWRDM_HAS_LOWPOWERSTATECHANGE))
- return -EINVAL;
-
- pr_debug(powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n,
-pwrdm-name);
-
- if (arch_pwrdm arch_pwrdm-pwrdm_set_lowpwrstchange)
- ret = arch_pwrdm-pwrdm_set_lowpwrstchange(pwrdm);
-
- return ret;
-}
-
int pwrdm_state_switch(struct powerdomain *pwrdm)
{
