Re: [PATCH] arm: omap: Use only valid power domain states
Hi Mark,
On Thu, May 3, 2012 at 12:04 AM, Mark A. Greer wrote:
> On Tue, May 01, 2012 at 10:47:35AM +0200, Jean Pihet wrote:
>> Hi Mark,
>
> Hi Jean. Thanks for the review.
>
>> On Mon, Apr 30, 2012 at 11:25 PM, Mark A. Greer
>> wrote:
>> > From: "Mark A. Greer"
>
>> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> > b/arch/arm/mach-omap2/cpuidle34xx.c
>> > index 5358664..2c91711 100644
>> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> > @@ -175,19 +175,25 @@ static int next_valid_state(struct cpuidle_device
>> > *dev,
>> > struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
>> > struct cpuidle_state *curr = &drv->states[index];
>> > struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
>> > - u32 mpu_deepest_state = PWRDM_POWER_RET;
>> > - u32 core_deepest_state = PWRDM_POWER_RET;
>> > + u32 mpu_deepest_state, mpu_deepest_possible;
>> > + u32 core_deepest_state, core_deepest_possible;
>> > int next_index = -1;
>> >
>> > + mpu_deepest_possible = pwrdm_get_deepest_state(mpu_pd);
>> > + mpu_deepest_state = max_t(u32, mpu_deepest_possible,
>> > PWRDM_POWER_RET);
>> I do not think you need to change the pwrdm API and the cpuidle code for
>> that.
>> Instead you should use the pwrst* fields in the power domains data
>> files, which allow to specify the allowed states.
>> After reading the rest of your patches it seems you are addressing the
>> issue in a subsequent patch 'arm: omap3: am35x: Set proper powerdomain
>> states'.
>
> That patch sets the allowable power domain states for the am35x; the intent
> of this patch was to make the code respect what is set in the pwrst* fields.
>
>> > diff --git a/arch/arm/mach-omap2/powerdomain.c
>> > b/arch/arm/mach-omap2/powerdomain.c
>> > index 96ad3dbe..9c80c19 100644
>> > --- a/arch/arm/mach-omap2/powerdomain.c
>> > +++ b/arch/arm/mach-omap2/powerdomain.c
>> > @@ -1078,3 +1078,28 @@ bool pwrdm_can_ever_lose_context(struct powerdomain
>> > *pwrdm)
>> >
>> > return 0;
>> > }
>> > +
>> > +/**
>> > + * pwrdm_get_deepest_state - Get deepest valid state domain can enter
>> > + * @pwrdm: struct powerdomain *
>> > + *
>> > + * Find and return the deepest valid state a power domain can be in.
>> > + * Returns the deepest state that @pwrdm can enter.
>> > + */
>> > +u32 pwrdm_get_deepest_state(struct powerdomain *pwrdm)
>> > +{
>> > + u32 valid_states, deepest_state;
>> > +
>> > + valid_states = pwrdm->pwrsts;
>> > +
>> > + if (valid_states & PWRSTS_OFF)
>> > + deepest_state = PWRDM_POWER_OFF;
>> > + else if (valid_states & PWRSTS_RET)
>> > + deepest_state = PWRDM_POWER_RET;
>> > + else if (valid_states & PWRSTS_INACTIVE)
>> > + deepest_state = PWRDM_POWER_INACTIVE;
>> > + else
>> > + deepest_state = PWRDM_POWER_ON;
>> > +
>> > + return deepest_state;
>> > +}
>>
>> The pwrdm API already performs those checks on pwrst*, cf.
>> pwrdm_set_next_pwrst for example.
>
> Well, sort of. pwrdm_set_next_pwrst() fails if its an invalid state
> returning -EINVAL. Very few of the callers actually check to see if
> it failed so they don't try the "next-one-up" power state. The net
> result is that the next state doesn't change even though it could
> move to a lower state (just not the one that was asked for).
>
> Is that acceptable?
No that is not acceptable. In fact the next power state should only be
set using omap_set_pwrdm_state, which falls back to the next valid
power state. Note: my latest patches about the functional power states
is using omap_set_pwrdm_state as the only function to set the power,
logic states.
> Another sort of strange one is omap3_pm_off_mode_enable(). If its
> called with 'enable' set and PWRDM_POWER_OFF isn't a valid state,
> omap_set_pwrdm_state() will return 0 and leave the next state alone.
> When omap3_pm_off_mode_enable() is called again with 'enable' set,
> omap_set_pwrdm_state() will find the lowest valid state >= RET.
> So, if you had a device where OFF wasn't valid but RET was and
> its currently ON, when the user enables OFF mode, it stays ON
> and when they disable OFF mode, it moves to RET.
>
> Is that acceptable?
No. omap_set_pwrdm_state should be fixed in that case.
In fact the enable_off_mode flag is scheduled for removal in favor of
the PM QoS constraints.
> If the things above are acceptable, then I think you're right, we can
> forget this patch. FYI, the am35x appears to work okay and no invalid
> states are entered (according to /sys/kernel/debug/pm_debug/count)
> without this patch.
I think we should fix the existing code if it has problems, not add
more specific code.
Kevin, what is your take on this?
> Mark
Regards,
Jean
--
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://vge
Re: [PATCH] arm: omap: Use only valid power domain states
On Tue, May 01, 2012 at 10:47:35AM +0200, Jean Pihet wrote:
> Hi Mark,
Hi Jean. Thanks for the review.
> On Mon, Apr 30, 2012 at 11:25 PM, Mark A. Greer
> wrote:
> > From: "Mark A. Greer"
> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> > b/arch/arm/mach-omap2/cpuidle34xx.c
> > index 5358664..2c91711 100644
> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> > @@ -175,19 +175,25 @@ static int next_valid_state(struct cpuidle_device
> > *dev,
> > struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
> > struct cpuidle_state *curr = &drv->states[index];
> > struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
> > - u32 mpu_deepest_state = PWRDM_POWER_RET;
> > - u32 core_deepest_state = PWRDM_POWER_RET;
> > + u32 mpu_deepest_state, mpu_deepest_possible;
> > + u32 core_deepest_state, core_deepest_possible;
> > int next_index = -1;
> >
> > + mpu_deepest_possible = pwrdm_get_deepest_state(mpu_pd);
> > + mpu_deepest_state = max_t(u32, mpu_deepest_possible,
> > PWRDM_POWER_RET);
> I do not think you need to change the pwrdm API and the cpuidle code for that.
> Instead you should use the pwrst* fields in the power domains data
> files, which allow to specify the allowed states.
> After reading the rest of your patches it seems you are addressing the
> issue in a subsequent patch 'arm: omap3: am35x: Set proper powerdomain
> states'.
That patch sets the allowable power domain states for the am35x; the intent
of this patch was to make the code respect what is set in the pwrst* fields.
> > diff --git a/arch/arm/mach-omap2/powerdomain.c
> > b/arch/arm/mach-omap2/powerdomain.c
> > index 96ad3dbe..9c80c19 100644
> > --- a/arch/arm/mach-omap2/powerdomain.c
> > +++ b/arch/arm/mach-omap2/powerdomain.c
> > @@ -1078,3 +1078,28 @@ bool pwrdm_can_ever_lose_context(struct powerdomain
> > *pwrdm)
> >
> > return 0;
> > }
> > +
> > +/**
> > + * pwrdm_get_deepest_state - Get deepest valid state domain can enter
> > + * @pwrdm: struct powerdomain *
> > + *
> > + * Find and return the deepest valid state a power domain can be in.
> > + * Returns the deepest state that @pwrdm can enter.
> > + */
> > +u32 pwrdm_get_deepest_state(struct powerdomain *pwrdm)
> > +{
> > + u32 valid_states, deepest_state;
> > +
> > + valid_states = pwrdm->pwrsts;
> > +
> > + if (valid_states & PWRSTS_OFF)
> > + deepest_state = PWRDM_POWER_OFF;
> > + else if (valid_states & PWRSTS_RET)
> > + deepest_state = PWRDM_POWER_RET;
> > + else if (valid_states & PWRSTS_INACTIVE)
> > + deepest_state = PWRDM_POWER_INACTIVE;
> > + else
> > + deepest_state = PWRDM_POWER_ON;
> > +
> > + return deepest_state;
> > +}
>
> The pwrdm API already performs those checks on pwrst*, cf.
> pwrdm_set_next_pwrst for example.
Well, sort of. pwrdm_set_next_pwrst() fails if its an invalid state
returning -EINVAL. Very few of the callers actually check to see if
it failed so they don't try the "next-one-up" power state. The net
result is that the next state doesn't change even though it could
move to a lower state (just not the one that was asked for).
Is that acceptable?
Another sort of strange one is omap3_pm_off_mode_enable(). If its
called with 'enable' set and PWRDM_POWER_OFF isn't a valid state,
omap_set_pwrdm_state() will return 0 and leave the next state alone.
When omap3_pm_off_mode_enable() is called again with 'enable' set,
omap_set_pwrdm_state() will find the lowest valid state >= RET.
So, if you had a device where OFF wasn't valid but RET was and
its currently ON, when the user enables OFF mode, it stays ON
and when they disable OFF mode, it moves to RET.
Is that acceptable?
If the things above are acceptable, then I think you're right, we can
forget this patch. FYI, the am35x appears to work okay and no invalid
states are entered (according to /sys/kernel/debug/pm_debug/count)
without this patch.
Mark
--
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] arm: omap: Use only valid power domain states
Hi Mark,
On Mon, Apr 30, 2012 at 11:25 PM, Mark A. Greer wrote:
> From: "Mark A. Greer"
>
> Some parts of the OMAP code assume that all power
> domains support certain states (e.g., RET & OFF).
> This isn't always true (e.g., the am35x family of
> SoC's) so those assumptions need to be removed.
>
> Remove those assumptions by looking up the deepest
> state that a power domain can be in whereever its
> being blindly set. The 'max()' of the deepest
> state and what the code formerly wanted to set it
> to, is the correct state. If the code formerly
> wanted to set it to PWRDM_POWER_OFF, then the
> deepest possible state will be used (i.e., no
> need to perform the 'max()').
>
> The code still assumes that ON is a valid state.
>
> Signed-off-by: Mark A. Greer
> ---
>
> These patches apply on top of Kevin's patch series,
> "[PATCH/RFT 0/8] ARM: OMAP: remove IP checks from SoC family detection"
>
> Tested on an am3517 evm and am37x evm.
>
> arch/arm/mach-omap2/cpuidle34xx.c | 19 +--
> arch/arm/mach-omap2/pm34xx.c | 15 +--
> arch/arm/mach-omap2/powerdomain.c | 25 +
> arch/arm/mach-omap2/powerdomain.h | 2 ++
> 4 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 5358664..2c91711 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -175,19 +175,25 @@ static int next_valid_state(struct cpuidle_device *dev,
> struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
> struct cpuidle_state *curr = &drv->states[index];
> struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
> - u32 mpu_deepest_state = PWRDM_POWER_RET;
> - u32 core_deepest_state = PWRDM_POWER_RET;
> + u32 mpu_deepest_state, mpu_deepest_possible;
> + u32 core_deepest_state, core_deepest_possible;
> int next_index = -1;
>
> + mpu_deepest_possible = pwrdm_get_deepest_state(mpu_pd);
> + mpu_deepest_state = max_t(u32, mpu_deepest_possible, PWRDM_POWER_RET);
I do not think you need to change the pwrdm API and the cpuidle code for that.
Instead you should use the pwrst* fields in the power domains data
files, which allow to specify the allowed states.
After reading the rest of your patches it seems you are addressing the
issue in a subsequent patch 'arm: omap3: am35x: Set proper powerdomain
states'.
> +
> + core_deepest_possible = pwrdm_get_deepest_state(core_pd);
> + core_deepest_state = max_t(u32, core_deepest_possible,
> PWRDM_POWER_RET);
> +
> if (enable_off_mode) {
> - mpu_deepest_state = PWRDM_POWER_OFF;
> + mpu_deepest_state = mpu_deepest_possible;
> /*
> * Erratum i583: valable for ES rev < Es1.2 on 3630.
> * CORE OFF mode is not supported in a stable form, restrict
> * instead the CORE state to RET.
> */
> if (!IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583))
> - core_deepest_state = PWRDM_POWER_OFF;
> + core_deepest_state = core_deepest_possible;
> }
>
> /* Check if current state is valid */
> @@ -422,8 +428,9 @@ int __init omap3_idle_init(void)
> pr_warn("%s: core off state C7 disabled due to i583\n",
> __func__);
> }
> - cx->mpu_state = PWRDM_POWER_OFF;
> - cx->core_state = PWRDM_POWER_OFF;
> +
> + cx->mpu_state = pwrdm_get_deepest_state(mpu_pd);
> + cx->core_state = pwrdm_get_deepest_state(core_pd);
>
> drv->state_count = OMAP3_NUM_STATES;
> cpuidle_register_driver(&omap3_idle_driver);
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index ec92676..40d8d07 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -614,12 +614,11 @@ void omap3_pm_off_mode_enable(int enable)
> struct power_state *pwrst;
> u32 state;
>
> - if (enable)
> - state = PWRDM_POWER_OFF;
> - else
> - state = PWRDM_POWER_RET;
> -
> list_for_each_entry(pwrst, &pwrst_list, node) {
> + state = pwrdm_get_deepest_state(pwrst->pwrdm);
> + if (!enable)
> + state = max_t(u32, state, PWRDM_POWER_RET);
> +
> if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583) &&
> pwrst->pwrdm == core_pwrdm &&
> state == PWRDM_POWER_OFF) {
> @@ -660,6 +659,7 @@ int omap3_pm_set_suspend_state(struct powerdomain *pwrdm,
> int state)
> static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> {
> struct power_state *pwrst;
> + u32 state;
>
> if (!pwrdm->pwrsts)
> return 0;
> @@ -668,7 +668,10 @@ static int __init pwrdms_setup(struc
[PATCH] arm: omap: Use only valid power domain states
From: "Mark A. Greer"
Some parts of the OMAP code assume that all power
domains support certain states (e.g., RET & OFF).
This isn't always true (e.g., the am35x family of
SoC's) so those assumptions need to be removed.
Remove those assumptions by looking up the deepest
state that a power domain can be in whereever its
being blindly set. The 'max()' of the deepest
state and what the code formerly wanted to set it
to, is the correct state. If the code formerly
wanted to set it to PWRDM_POWER_OFF, then the
deepest possible state will be used (i.e., no
need to perform the 'max()').
The code still assumes that ON is a valid state.
Signed-off-by: Mark A. Greer
---
These patches apply on top of Kevin's patch series,
"[PATCH/RFT 0/8] ARM: OMAP: remove IP checks from SoC family detection"
Tested on an am3517 evm and am37x evm.
arch/arm/mach-omap2/cpuidle34xx.c | 19 +--
arch/arm/mach-omap2/pm34xx.c | 15 +--
arch/arm/mach-omap2/powerdomain.c | 25 +
arch/arm/mach-omap2/powerdomain.h |2 ++
4 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 5358664..2c91711 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -175,19 +175,25 @@ static int next_valid_state(struct cpuidle_device *dev,
struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
struct cpuidle_state *curr = &drv->states[index];
struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
- u32 mpu_deepest_state = PWRDM_POWER_RET;
- u32 core_deepest_state = PWRDM_POWER_RET;
+ u32 mpu_deepest_state, mpu_deepest_possible;
+ u32 core_deepest_state, core_deepest_possible;
int next_index = -1;
+ mpu_deepest_possible = pwrdm_get_deepest_state(mpu_pd);
+ mpu_deepest_state = max_t(u32, mpu_deepest_possible, PWRDM_POWER_RET);
+
+ core_deepest_possible = pwrdm_get_deepest_state(core_pd);
+ core_deepest_state = max_t(u32, core_deepest_possible, PWRDM_POWER_RET);
+
if (enable_off_mode) {
- mpu_deepest_state = PWRDM_POWER_OFF;
+ mpu_deepest_state = mpu_deepest_possible;
/*
* Erratum i583: valable for ES rev < Es1.2 on 3630.
* CORE OFF mode is not supported in a stable form, restrict
* instead the CORE state to RET.
*/
if (!IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583))
- core_deepest_state = PWRDM_POWER_OFF;
+ core_deepest_state = core_deepest_possible;
}
/* Check if current state is valid */
@@ -422,8 +428,9 @@ int __init omap3_idle_init(void)
pr_warn("%s: core off state C7 disabled due to i583\n",
__func__);
}
- cx->mpu_state = PWRDM_POWER_OFF;
- cx->core_state = PWRDM_POWER_OFF;
+
+ cx->mpu_state = pwrdm_get_deepest_state(mpu_pd);
+ cx->core_state = pwrdm_get_deepest_state(core_pd);
drv->state_count = OMAP3_NUM_STATES;
cpuidle_register_driver(&omap3_idle_driver);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index ec92676..40d8d07 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -614,12 +614,11 @@ void omap3_pm_off_mode_enable(int enable)
struct power_state *pwrst;
u32 state;
- if (enable)
- state = PWRDM_POWER_OFF;
- else
- state = PWRDM_POWER_RET;
-
list_for_each_entry(pwrst, &pwrst_list, node) {
+ state = pwrdm_get_deepest_state(pwrst->pwrdm);
+ if (!enable)
+ state = max_t(u32, state, PWRDM_POWER_RET);
+
if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583) &&
pwrst->pwrdm == core_pwrdm &&
state == PWRDM_POWER_OFF) {
@@ -660,6 +659,7 @@ int omap3_pm_set_suspend_state(struct powerdomain *pwrdm,
int state)
static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
{
struct power_state *pwrst;
+ u32 state;
if (!pwrdm->pwrsts)
return 0;
@@ -668,7 +668,10 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm,
void *unused)
if (!pwrst)
return -ENOMEM;
pwrst->pwrdm = pwrdm;
- pwrst->next_state = PWRDM_POWER_RET;
+
+ state = pwrdm_get_deepest_state(pwrdm);
+ pwrst->next_state = max_t(u32, state, PWRDM_POWER_RET);
+
list_add(&pwrst->node, &pwrst_list);
if (pwrdm_has_hdwr_sar(pwrdm))
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 96ad3dbe..9c80c19 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1078,3 +1078,28 @@ bool pwr
