RE: [PATCHv2 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level
-Original Message-
From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: 12 January, 2010 20:25
To: Kristo Tero (Nokia-D/Tampere)
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCHv2 1/6] OMAP: Powerdomains: Add support for
INACTIVE state on pwrdm level
Tero Kristo tero.kri...@nokia.com writes:
From: Tero Kristo tero.kri...@nokia.com
Currently only ON, RET and OFF are supported, and ON is
arguably broken as it
allows the powerdomain to enter INACTIVE state unless idle
is prevented.
Now, pwrdm code prevents idle if ON is selected, and also
adds support for
INACTIVE. This simplifies the control needed inside sleep code.
Signed-off-by: Tero Kristo tero.kri...@nokia.com
Hi Tero,
apologies for the long delay in reviewing this updated series.
No problem, I have actually been on a holiday myself so I did not really miss
review comments that much. Thanks for taking the time to look at this.
I really like this idea as it indeed simplifies the control code
inside the idle path. This also needs a review by Paul and should
merge via his powerdomain updates for 2.6.34.
The changelog should also describe that the powerdomain struct
now caches the next_state.
Yeah, I can add this comment.
One minor comment. I think the introduction of signed compares in
certain cases leads to possible confusion and readability problems.
I'm not sure I realy follow the need for the invalid state. Instead
of setting next_state to -1 in pwrdm_register, why not read the
current HW value and use that as the starting value?
I guess this invalid stuff comes from my old lazy initialization habits. :) But
yes, I can change this into a format where we just read the current value from
the register.
If the invalid state is really needed, instead of using -1 and having
to change to using signed comparisons in certain cases, it seems like
we could just define a new invalid state as zero, and move the others
up a notch.
This is probably not good, as it would break the direct SW to HW value mapping,
so I will go with the previous one.
Then, use something like this to check for a valid state:
static inline bool pwrdm_is_valid_state(struct powerdomain *pwrdm) {
return (pwrdm-state PWRDM_POWER_INVALID) ? true : false;
}
Kevin
---
arch/arm/mach-omap2/powerdomain.c | 32
+
arch/arm/mach-omap2/powerdomains34xx.h| 26
++--
arch/arm/plat-omap/include/plat/powerdomain.h |6 -
3 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index b6990e3..1237717 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -112,8 +112,8 @@ static struct powerdomain
*_pwrdm_deps_lookup(struct powerdomain *pwrdm,
static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
{
-int prev;
-int state;
+u8 prev;
+u8 state;
if (pwrdm == NULL)
return -EINVAL;
@@ -220,7 +220,7 @@ int pwrdm_register(struct powerdomain *pwrdm)
pr_debug(powerdomain: registered %s\n, pwrdm-name);
ret = 0;
-
+pwrdm-next_state = -1;
pr_unlock:
write_unlock_irqrestore(pwrdm_rwlock, flags);
@@ -701,19 +701,38 @@ int pwrdm_get_mem_bank_count(struct
powerdomain *pwrdm)
*/
int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
{
+u8 prg_pwrst;
+
if (!pwrdm)
return -EINVAL;
+if (pwrdm-next_state == pwrst)
+return 0;
+
if (!(pwrdm-pwrsts (1 pwrst)))
return -EINVAL;
pr_debug(powerdomain: setting next powerstate for %s to %0x\n,
pwrdm-name, pwrst);
+/* INACTIVE is reserved, so we program pwrdm as ON */
+if (pwrst == PWRDM_POWER_INACTIVE)
+prg_pwrst = PWRDM_POWER_ON;
+else
+prg_pwrst = pwrst;
+
prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
- (pwrst OMAP_POWERSTATE_SHIFT),
+ (prg_pwrst OMAP_POWERSTATE_SHIFT),
pwrdm-prcm_offs, PM_PWSTCTRL);
+/* If next state is ON, prevent idle */
+if (pwrst == PWRDM_POWER_ON)
+omap2_clkdm_deny_idle(pwrdm-pwrdm_clkdms[0]);
+else
+omap2_clkdm_allow_idle(pwrdm-pwrdm_clkdms[0]);
+
+pwrdm-next_state = pwrst;
+
return 0;
}
@@ -730,8 +749,11 @@ int pwrdm_read_next_pwrst(struct
powerdomain *pwrdm)
if (!pwrdm)
return -EINVAL;
+if (pwrdm-next_state -1)
+return pwrdm-next_state;
+
return prm_read_mod_bits_shift(pwrdm-prcm_offs, PM_PWSTCTRL,
-OMAP_POWERSTATE_MASK);
+ OMAP_POWERSTATE_MASK);
}
/**
diff --git a/arch/arm/mach-omap2/powerdomains34xx.h
b/arch/arm/mach-omap2/powerdomains34xx.h
index fd09b08..9eb2dc5 100644
Re: [PATCHv2 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level
Tero Kristo tero.kri...@nokia.com writes:
From: Tero Kristo tero.kri...@nokia.com
Currently only ON, RET and OFF are supported, and ON is arguably broken as it
allows the powerdomain to enter INACTIVE state unless idle is prevented.
Now, pwrdm code prevents idle if ON is selected, and also adds support for
INACTIVE. This simplifies the control needed inside sleep code.
Signed-off-by: Tero Kristo tero.kri...@nokia.com
Hi Tero,
apologies for the long delay in reviewing this updated series.
I really like this idea as it indeed simplifies the control code
inside the idle path. This also needs a review by Paul and should
merge via his powerdomain updates for 2.6.34.
The changelog should also describe that the powerdomain struct
now caches the next_state.
One minor comment. I think the introduction of signed compares in
certain cases leads to possible confusion and readability problems.
I'm not sure I realy follow the need for the invalid state. Instead
of setting next_state to -1 in pwrdm_register, why not read the
current HW value and use that as the starting value?
If the invalid state is really needed, instead of using -1 and having
to change to using signed comparisons in certain cases, it seems like
we could just define a new invalid state as zero, and move the others
up a notch.
Then, use something like this to check for a valid state:
static inline bool pwrdm_is_valid_state(struct powerdomain *pwrdm) {
return (pwrdm-state PWRDM_POWER_INVALID) ? true : false;
}
Kevin
---
arch/arm/mach-omap2/powerdomain.c | 32
+
arch/arm/mach-omap2/powerdomains34xx.h| 26 ++--
arch/arm/plat-omap/include/plat/powerdomain.h |6 -
3 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index b6990e3..1237717 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -112,8 +112,8 @@ static struct powerdomain *_pwrdm_deps_lookup(struct
powerdomain *pwrdm,
static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
{
- int prev;
- int state;
+ u8 prev;
+ u8 state;
if (pwrdm == NULL)
return -EINVAL;
@@ -220,7 +220,7 @@ int pwrdm_register(struct powerdomain *pwrdm)
pr_debug(powerdomain: registered %s\n, pwrdm-name);
ret = 0;
-
+ pwrdm-next_state = -1;
pr_unlock:
write_unlock_irqrestore(pwrdm_rwlock, flags);
@@ -701,19 +701,38 @@ int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm)
*/
int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
{
+ u8 prg_pwrst;
+
if (!pwrdm)
return -EINVAL;
+ if (pwrdm-next_state == pwrst)
+ return 0;
+
if (!(pwrdm-pwrsts (1 pwrst)))
return -EINVAL;
pr_debug(powerdomain: setting next powerstate for %s to %0x\n,
pwrdm-name, pwrst);
+ /* INACTIVE is reserved, so we program pwrdm as ON */
+ if (pwrst == PWRDM_POWER_INACTIVE)
+ prg_pwrst = PWRDM_POWER_ON;
+ else
+ prg_pwrst = pwrst;
+
prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
- (pwrst OMAP_POWERSTATE_SHIFT),
+ (prg_pwrst OMAP_POWERSTATE_SHIFT),
pwrdm-prcm_offs, PM_PWSTCTRL);
+ /* If next state is ON, prevent idle */
+ if (pwrst == PWRDM_POWER_ON)
+ omap2_clkdm_deny_idle(pwrdm-pwrdm_clkdms[0]);
+ else
+ omap2_clkdm_allow_idle(pwrdm-pwrdm_clkdms[0]);
+
+ pwrdm-next_state = pwrst;
+
return 0;
}
@@ -730,8 +749,11 @@ int pwrdm_read_next_pwrst(struct powerdomain *pwrdm)
if (!pwrdm)
return -EINVAL;
+ if (pwrdm-next_state -1)
+ return pwrdm-next_state;
+
return prm_read_mod_bits_shift(pwrdm-prcm_offs, PM_PWSTCTRL,
- OMAP_POWERSTATE_MASK);
+OMAP_POWERSTATE_MASK);
}
/**
diff --git a/arch/arm/mach-omap2/powerdomains34xx.h
b/arch/arm/mach-omap2/powerdomains34xx.h
index fd09b08..9eb2dc5 100644
--- a/arch/arm/mach-omap2/powerdomains34xx.h
+++ b/arch/arm/mach-omap2/powerdomains34xx.h
@@ -165,7 +165,7 @@ static struct powerdomain iva2_pwrdm = {
.omap_chip= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
.dep_bit = OMAP3430_PM_WKDEP_MPU_EN_IVA2_SHIFT,
.wkdep_srcs = iva2_wkdeps,
- .pwrsts = PWRSTS_OFF_RET_ON,
+ .pwrsts = PWRSTS_OFF_RET_INA_ON,
.pwrsts_logic_ret = PWRSTS_OFF_RET,
.banks= 4,
.pwrsts_mem_ret = {
@@ -188,7 +188,7 @@ static struct powerdomain mpu_34xx_pwrdm = {
.omap_chip= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
.dep_bit = OMAP3430_EN_MPU_SHIFT,
.wkdep_srcs =
