Re: Problems in cpuidle

[Kevin Hilman <khilman@deeprootsystems.com>]

"Premi, Sanjeev" <premi@ti.com> writes:

>> -----Original Message-----
>> From: Högander Jouni [mailto:jouni.hogander@nokia.com] 
>> Sent: Monday, March 09, 2009 3:36 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: Problems in cpuidle
>> 
>> "ext Premi, Sanjeev" <premi@ti.com> writes:
>> 
>> > While working with cpuidle, I have come across these problems.
>> > I am also working on the solutions, but would be good to hear more 
>> > thoughts.
>> >
>> > 1) The flag 'enable_dyn_sleep' is honoured only in 
>> omap3_idle_bm_check()
>> >    but in the C1 state, omap3_enter_idle() is invoked directly.
>> >    So, the system can transition to deeper idle state(s)
>> >
>> >    Same is the case with 'sleep_block'.
>> >
>> >    Possible Solutions:
>> >      a)  Call omap3_can_sleep() in omap3_enter_idle().
>> >          This makes omap3_idle_bm_check() redundant; and 
>> can be removed.
>> >
>> >      b)  Make single entry point for all idle states
>> >          But would be an overkill for C1 state.
>> >
>> >      c)  Change omap3_can_sleep() to check for omap_uart_can_sleep()
>> >          and omap3_fclks_active() only.
>> >          Move check for 'enable_dyn_sleep' and 'sleep_block' into
>> >          omap3_enter_idle()
>> >
>> >     I believe (c) would be the most optimal.
>> 
>> Selecting (c) will break traditional pm_idle. Current plan is 
>> to add one more C state (C1) which would prevent mpu/core 
>> sleep transitions. Then remove fclk_active check completely.
>
> [sp] I meant doing the same for pm_idle as well.
>      Also, the new cstate will not help with 'sleep' block.
>
>      The proposed change look like:
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx
> index 7fc3cb3..c25158c 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -82,6 +82,11 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>         /* Used to keep track of the total time in idle */
>         getnstimeofday(&ts_preidle);
>
> +       if (!enable_dyn_sleep)
> +               goto return_sleep_time;
> +       if (atomic_read(&sleep_block) > 0)
> +               goto return_sleep_time;
> +
>         /*
>          * Adjust the idle state (if required).
>          * Also, ensure that usage statistics of correct state are updated.
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 0716d60..5c7819a 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -476,14 +476,10 @@ static int omap3_fclks_active(void)
>
>  int omap3_can_sleep(void)
>  {
> -       if (!enable_dyn_sleep)
> -               return 0;
>         if (!omap_uart_can_sleep())
>                 return 0;
>         if (omap3_fclks_active())
>                 return 0;
> -       if (atomic_read(&sleep_block) > 0)
> -               return 0;
>         return 1;
>  }
>
> @@ -534,6 +530,11 @@ err:
>
>  static void omap3_pm_idle(void)
>  {
> +       if (!enable_dyn_sleep)
> +               return;
> +       if (atomic_read(&sleep_block) > 0)
> +               return;
> +
>         local_irq_disable();
>         local_fiq_disable();
>

Sanjeev,

I think it's cleaner to just make all the states go through the 'bm_check',
so the standard PM idle and CPUidle use the same paths.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html