[PATCH] arm: omap: Use only valid power domain states

Jean Pihet jean.pihet at newoldbits.com
Tue May 1 04:47:35 EDT 2012


Hi Mark,

On Mon, Apr 30, 2012 at 11:25 PM, Mark A. Greer <mgreer at animalcreek.com> wrote:
> From: "Mark A. Greer" <mgreer at animalcreek.com>
>
> 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 at animalcreek.com>
> ---
>
> 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(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 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.

So to summarize I think the code duplication should be avoided.

Regards,
Jean

> 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);
> --
> 1.7.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



More information about the linux-arm-kernel mailing list