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 writes:
>
>> From: Tero Kristo
>>
>> 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
>
>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 st
Re: [PATCHv2 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level
Tero Kristo writes:
> From: Tero Kristo
>
> 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
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),
>
[PATCHv2 1/6] OMAP: Powerdomains: Add support for INACTIVE state on pwrdm level
From: Tero Kristo
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
---
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 = mpu_34xx_wkdeps,
- .pwrsts = PWRSTS_OFF_RET_ON,
+ .pwrsts = PWRSTS_OFF_RET_INA_ON,
.pwrsts_logic_ret = PWRSTS_OFF_RET,
.banks= 1,
.pwrsts_mem_ret = {
@@ -206,7 +206,7 @@ static struct powerdomain core_34xx_pre_es3_1_pwrdm = {
.omap_chip= OMAP_CHIP_INIT(CHIP_IS_OMAP3430ES1 |
CHIP_IS_OMAP3430ES2 |
CHIP_IS_OMAP3430ES3_0),
- .pwrsts = PWRSTS_OFF_RET_ON,
+ .pwrsts = PWRSTS_OFF_RET_INA_ON,
.dep_bit = OMAP3430_EN_CORE_SHIFT,
.banks= 2,
.pwrsts_mem_ret = {
@@ -214,8 +214,8 @@ static struct powerdomain core_34xx_pre_es3_1_pwrdm = {
[1] = PWRSTS_OFF_RET,/* MEM2RETSTATE */
},
.pwrsts_mem_on= {
- [0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
- [1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
+ [0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */
+ [1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */
},
};
@@ -224,7 +224,7 @
