"Mark A. Greer" <mgreer@xxxxxxxxxxxxxxx> writes:
> On Wed, Apr 11, 2012 at 03:46:20PM -0700, Kevin Hilman wrote:
>> Hi Mark,
>
> Hi Kevin.
>
>> "Mark A. Greer" <mgreer@xxxxxxxxxxxxxxx> writes:
>>
>> > From: "Mark A. Greer" <mgreer@xxxxxxxxxxxxxxx>
>> >
>> > Typical OMAP3 SoCs have four power domain states: ON,
>> > INACTIVE, RETENTION, and OFF. The am35x family of SoCs
>> > has only two states: ON and INACTIVE. To distinguish which
>> > set of states the current device has, add the 'OMAP3_HAS_PWROFF'
>> > feature. When that feature/bit is set, the device supports the
>> > RETENTION and OFF states; otherwise, it doesn't.
>> >
>> > Signed-off-by: Mark A. Greer <mgreer@xxxxxxxxxxxxxxx>
>>
>> Paul has mentioned this already, but the same applies here: We shouldn't
>> be using SoC-global feature flag for this. We already have per-pwrdm
>> flags that indicate what states a given powerdomain suports (see .pwrsts
>> field.)
>>
>> Wherever we have blind writes to next powerstates that assume support
>> for RET/OFF is present, those should probably use a helper function from
>> the powerdomain code that checks if that state is even supported.
>
> How about something like the patch below?
> Note: its not well tested; just RFC.
Yes, your proposed patch looks right to me.
I guess it's up to Paul & Jean to see if they'd rather see this build on
top of the Jean's functional power state work, or take this as a
standalone fix.
Kevin
>> Jean's work on functional powerstates will probably help here if you
>> really need to support INACTIVE. However, Paul may be right in that you
>> might just start with supporing ON only, and validate that module-level
>> wakups acutally work.
>
> Yes, I think INACTIVE is a red herring so I'm going to stick with k.o
> master branch for now (IOW, not base on Jean's patches). If you still
> want me to base on his patches, just let me know.
>
> Mark
> --
>
> From 32c54adb15c76396aeec809d38de4dde936b1e66 Mon Sep 17 00:00:00 2001
> From: "Mark A. Greer" <mgreer@xxxxxxxxxxxxxxx>
> Date: Mon, 23 Apr 2012 17:48:06 -0700
> Subject: [PATCH] arm: omap: Use only valid power domain states
>
> 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 <mgreer@xxxxxxxxxxxxxxx>
> ---
> arch/arm/mach-omap2/cpuidle34xx.c | 14 ++++++++++----
> arch/arm/mach-omap2/pm34xx.c | 15 +++++++++------
> arch/arm/mach-omap2/powerdomain.c | 25 +++++++++++++++++++++++++
> arch/arm/mach-omap2/powerdomain.h | 2 ++
> 4 files changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 5358664..60aa0c3 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(mpu_deepest_possible, (u32)PWRDM_POWER_RET);
> +
> + core_deepest_possible = pwrdm_get_deepest_state(core_pd);
> + core_deepest_state = max(core_deepest_possible, (u32)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 */
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index ec92676..7737bfb 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(state, (u32)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(state, (u32)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 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;
> +}
> diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
> index 0d72a8a..9688ff1 100644
> --- a/arch/arm/mach-omap2/powerdomain.h
> +++ b/arch/arm/mach-omap2/powerdomain.h
> @@ -220,6 +220,8 @@ int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm);
> int pwrdm_get_context_loss_count(struct powerdomain *pwrdm);
> bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm);
>
> +u32 pwrdm_get_deepest_state(struct powerdomain *pwrdm);
> +
> extern void omap242x_powerdomains_init(void);
> extern void omap243x_powerdomains_init(void);
> extern void omap3xxx_powerdomains_init(void);
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
