Problems in cpuidle

Commit Message

Sanjeev Premi March 10, 2009, 1:42 a.m. UTC

> -----Original Message-----
> From: Högander Jouni [mailto:[e-mail]] 
> Sent: Monday, March 09, 2009 3:36 PM
> To: Premi, Sanjeev
> Cc: [e-mail]
> Subject: Re: Problems in cpuidle
> 
> "ext Premi, Sanjeev" <[e-mail]> 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:

> 
> --
> Jouni Högander
> 
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kevin Hilman March 10, 2009, 3:14 p.m. UTC | #1

"Premi, Sanjeev" <[e-mail]> writes:

>> -----Original Message-----
>> From: Högander Jouni [mailto:[e-mail]] 
>> Sent: Monday, March 09, 2009 3:36 PM
>> To: Premi, Sanjeev
>> Cc: [e-mail]
>> Subject: Re: Problems in cpuidle
>> 
>> "ext Premi, Sanjeev" <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sanjeev Premi March 11, 2009, 1:23 p.m. UTC | #2

> -----Original Message-----
> From: Kevin Hilman [mailto:[e-mail]] 
> Sent: Tuesday, March 10, 2009 8:45 PM
> To: Premi, Sanjeev
> Cc: Högander Jouni; [e-mail]
> Subject: Re: Problems in cpuidle
> 
> "Premi, Sanjeev" <[e-mail]> writes:
> 
> >> -----Original Message-----
> >> From: Högander Jouni [mailto:[e-mail]]
> >> Sent: Monday, March 09, 2009 3:36 PM
> >> To: Premi, Sanjeev
> >> Cc: [e-mail]
> >> Subject: Re: Problems in cpuidle
> >> 
> >> "ext Premi, Sanjeev" <[e-mail]> 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.

[sp] I was trying to avoid duplicate code.
     When all states go thru bm_check, omap3_enter_idle_bm() and
     omap3_enter_idle() can be collapsed into single function.
     Your thoughts?

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

Jouni Hogander March 13, 2009, 6:53 a.m. UTC | #3

"ext Premi, Sanjeev" <[e-mail]> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:[e-mail]] 
>> Sent: Tuesday, March 10, 2009 8:45 PM
>> To: Premi, Sanjeev
>> Cc: Högander Jouni; [e-mail]
>> Subject: Re: Problems in cpuidle
>> 
>> "Premi, Sanjeev" <[e-mail]> writes:
>> 
>> >> -----Original Message-----
>> >> From: Högander Jouni [mailto:[e-mail]]
>> >> Sent: Monday, March 09, 2009 3:36 PM
>> >> To: Premi, Sanjeev
>> >> Cc: [e-mail]
>> >> Subject: Re: Problems in cpuidle
>> >> 
>> >> "ext Premi, Sanjeev" <[e-mail]> 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.
>
> [sp] I was trying to avoid duplicate code.
>      When all states go thru bm_check, omap3_enter_idle_bm() and
>      omap3_enter_idle() can be collapsed into single function.
>      Your thoughts?

I'm not sure if you noticed patch for Peter De Schrivjer. In that
patch he is adding one more C state, which is basically only WFI. MPU
and CORE are forced to stay active. He is also removing fclk check. So
that would mean no sleep if (sleep_block == 1 || !enable_dyn_sleep ||
!uart_can_sleep).

If sleep is still wanted in case of uart_can_sleep, maybe we could
just remove the check from omap3_can_sleep?

>
>> Kevin
>> 
>>

Patch

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();