Problems in cpuidle

Problems in cpuidle

[Premi, Sanjeev]

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.

2) When 'enable_off_mode' is 0, and (mpu_state < PWRDM_POWER_RET)
   the local variables mpu_state and core_state are modified; but
   the usage count for the original state selected by the governor
   are updated.

   Solution:
     Recalculate the idle state and update the dev->last_state
     (for cpuidle driver) to ensure that usage statistics for the
     actual state are entered.

     Only problem is that latency corresponding to original state
     would have been used by the dynamic tick functions.

If things go well, I will be able to submit patches on top of pm branch
by tomorrow...

Best regards,
Sanjeev

Re: Problems in cpuidle

[Högander Jouni]

"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.

-- 
Jouni Högander
--
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

RE: Problems in cpuidle

[Premi, Sanjeev]

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

> 
> --
> Jouni Högander
> 
> --
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

Re: Problems in cpuidle

[Kevin Hilman]

"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

RE: Problems in cpuidle

[Premi, Sanjeev]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Tuesday, March 10, 2009 8:45 PM
> To: Premi, Sanjeev
> Cc: Högander Jouni; linux-omap@vger.kernel.org
> Subject: Re: Problems in cpuidle
> 
> "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.

[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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems in cpuidle

[Högander Jouni]

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

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Tuesday, March 10, 2009 8:45 PM
>> To: Premi, Sanjeev
>> Cc: Högander Jouni; linux-omap@vger.kernel.org
>> Subject: Re: Problems in cpuidle
>> 
>> "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.
>
> [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
>> 
>> 

-- 
Jouni Högander
--
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