Hi Kevin,
On Wed, 15 Sep 2010, Kevin Hilman wrote:
> In an effort to simplify the core idle path, move any device-specific
> special case handling from the core PM idle path into the CPUidle
> pre-idle checking path.
>
> This keeps the core, interrupts-disabled idle path streamlined and
> independent of any device-specific handling, and also allows CPUidle
> to do the checking only for certain C-states as needed. This patch
> has the device checks in place for all states with the CHECK_BM flag,
> namely all states >= C2.
>
> This patch was inspired by a similar patch written by Tero Kristo as
> part of a larger series to add INACTIVE state support.
As with the original patch, I don't quite understand the improvement
here. In particular, this part:
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 3d3d035..0923b82 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -233,14 +234,54 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
> struct cpuidle_state *state)
> {
> struct cpuidle_state *new_state = next_valid_state(dev, state);
> + u32 core_next_state, per_next_state = 0, per_saved_state = 0;
> + u32 cam_state;
> + struct omap3_processor_cx *cx;
> + int ret;
>
> if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
> BUG_ON(!dev->safe_state);
> new_state = dev->safe_state;
> + goto select_state;
> + }
> +
> + cx = cpuidle_get_statedata(state);
> + core_next_state = cx->core_state;
> +
> + /*
> + * Prevent idle completely if CAM is active.
> + * CAM does not have wakeup capability in OMAP3.
> + */
> + cam_state = pwrdm_read_pwrst(cam_pd);
> + if (cam_state == PWRDM_POWER_ON) {
> + new_state = dev->safe_state;
> + goto select_state;
> }
>
> + /*
> + * Prevent PER off if CORE is not in retention or off as this
> + * would disable PER wakeups completely.
> + */
> + per_next_state = per_saved_state = pwrdm_read_next_pwrst(per_pd);
> + if ((per_next_state == PWRDM_POWER_OFF) &&
> + (core_next_state > PWRDM_POWER_RET)) {
> + per_next_state = PWRDM_POWER_RET;
> + pwrdm_set_next_pwrst(per_pd, per_next_state);
> + }
> +
> + /* Are we changing PER target state? */
> + if (per_next_state != per_saved_state)
> + pwrdm_set_next_pwrst(per_pd, per_next_state);
In this case, the PER / CORE constraints don't have anything to do with
the MPU or CPUIdle, so they don't seem to belong in the CPUIdle code. The
extra comments are certainly nice -- they make it more clear as to what is
going on here -- but maybe those can just be added to pm34xx.c ?
> +
> +select_state:
> dev->last_state = new_state;
> - return omap3_enter_idle(dev, new_state);
> + ret = omap3_enter_idle(dev, new_state);
> +
> + /* Restore original PER state if it was modified */
> + if (per_next_state != per_saved_state)
> + pwrdm_set_next_pwrst(per_pd, per_saved_state);
> +
> + return ret;
> }
>
> DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> @@ -328,7 +369,8 @@ void omap_init_power_states(void)
> cpuidle_params_table[OMAP3_STATE_C2].threshold;
> omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_ON;
> omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_ON;
> - omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID;
> + omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
> + CPUIDLE_FLAG_CHECK_BM;
>
> /* C3 . MPU CSWR + Core inactive */
> omap3_power_states[OMAP3_STATE_C3].valid =
> @@ -426,6 +468,8 @@ int __init omap3_idle_init(void)
>
> mpu_pd = pwrdm_lookup("mpu_pwrdm");
> core_pd = pwrdm_lookup("core_pwrdm");
> + per_pd = pwrdm_lookup("per_pwrdm");
> + cam_pd = pwrdm_lookup("cam_pwrdm");
>
> omap_init_power_states();
> cpuidle_register_driver(&omap3_idle_driver);
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 429268e..bb2ba1e 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -346,7 +346,6 @@ void omap_sram_idle(void)
> int core_next_state = PWRDM_POWER_ON;
> int core_prev_state, per_prev_state;
> u32 sdrc_pwr = 0;
> - int per_state_modified = 0;
>
> if (!_omap_sram_idle)
> return;
> @@ -391,19 +390,10 @@ void omap_sram_idle(void)
> if (per_next_state < PWRDM_POWER_ON) {
> omap_uart_prepare_idle(2);
> omap2_gpio_prepare_for_idle(per_next_state);
> - if (per_next_state == PWRDM_POWER_OFF) {
> - if (core_next_state == PWRDM_POWER_ON) {
> - per_next_state = PWRDM_POWER_RET;
> - pwrdm_set_next_pwrst(per_pwrdm, per_next_state);
> - per_state_modified = 1;
> - } else
> + if (per_next_state == PWRDM_POWER_OFF)
> omap3_per_save_context();
> - }
> }
>
> - if (pwrdm_read_pwrst(cam_pwrdm) == PWRDM_POWER_ON)
> - omap2_clkdm_deny_idle(mpu_pwrdm->pwrdm_clkdms[0]);
> -
> /* CORE */
> if (core_next_state < PWRDM_POWER_ON) {
> omap_uart_prepare_idle(0);
> @@ -470,8 +460,6 @@ void omap_sram_idle(void)
> if (per_prev_state == PWRDM_POWER_OFF)
> omap3_per_restore_context();
> omap_uart_resume_idle(2);
> - if (per_state_modified)
> - pwrdm_set_next_pwrst(per_pwrdm, PWRDM_POWER_OFF);
> }
>
> /* Disable IO-PAD and IO-CHAIN wakeup */
> --
> 1.7.2.1
>
> --
> 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
>
- Paul
--
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
