* Problems in cpuidle
@ 2009-03-06 17:32 Premi, Sanjeev
2009-03-09 10:05 ` Högander Jouni
0 siblings, 1 reply; 6+ messages in thread
From: Premi, Sanjeev @ 2009-03-06 17:32 UTC (permalink / raw)
To: linux-omap
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Problems in cpuidle 2009-03-06 17:32 Problems in cpuidle Premi, Sanjeev @ 2009-03-09 10:05 ` Högander Jouni 2009-03-10 1:42 ` Premi, Sanjeev 0 siblings, 1 reply; 6+ messages in thread From: Högander Jouni @ 2009-03-09 10:05 UTC (permalink / raw) To: ext Premi, Sanjeev; +Cc: linux-omap "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: Problems in cpuidle 2009-03-09 10:05 ` Högander Jouni @ 2009-03-10 1:42 ` Premi, Sanjeev 2009-03-10 15:14 ` Kevin Hilman 0 siblings, 1 reply; 6+ messages in thread From: Premi, Sanjeev @ 2009-03-10 1:42 UTC (permalink / raw) To: Högander Jouni; +Cc: linux-omap > -----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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Problems in cpuidle 2009-03-10 1:42 ` Premi, Sanjeev @ 2009-03-10 15:14 ` Kevin Hilman 2009-03-11 13:23 ` Premi, Sanjeev 0 siblings, 1 reply; 6+ messages in thread From: Kevin Hilman @ 2009-03-10 15:14 UTC (permalink / raw) To: Premi, Sanjeev; +Cc: Högander Jouni, linux-omap "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: Problems in cpuidle 2009-03-10 15:14 ` Kevin Hilman @ 2009-03-11 13:23 ` Premi, Sanjeev 2009-03-13 6:53 ` Högander Jouni 0 siblings, 1 reply; 6+ messages in thread From: Premi, Sanjeev @ 2009-03-11 13:23 UTC (permalink / raw) To: Kevin Hilman; +Cc: Högander Jouni, linux-omap > -----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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Problems in cpuidle 2009-03-11 13:23 ` Premi, Sanjeev @ 2009-03-13 6:53 ` Högander Jouni 0 siblings, 0 replies; 6+ messages in thread From: Högander Jouni @ 2009-03-13 6:53 UTC (permalink / raw) To: ext Premi, Sanjeev; +Cc: Kevin Hilman, linux-omap "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-13 6:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-03-06 17:32 Problems in cpuidle Premi, Sanjeev 2009-03-09 10:05 ` Högander Jouni 2009-03-10 1:42 ` Premi, Sanjeev 2009-03-10 15:14 ` Kevin Hilman 2009-03-11 13:23 ` Premi, Sanjeev 2009-03-13 6:53 ` Högander Jouni
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.