"ext Premi, Sanjeev" <premi@xxxxxx> writes: >> -----Original Message----- >> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >> Sent: Tuesday, March 10, 2009 8:45 PM >> To: Premi, Sanjeev >> Cc: Högander Jouni; linux-omap@xxxxxxxxxxxxxxx >> Subject: Re: Problems in cpuidle >> >> "Premi, Sanjeev" <premi@xxxxxx> writes: >> >> >> -----Original Message----- >> >> From: Högander Jouni [mailto:jouni.hogander@xxxxxxxxx] >> >> Sent: Monday, March 09, 2009 3:36 PM >> >> To: Premi, Sanjeev >> >> Cc: linux-omap@xxxxxxxxxxxxxxx >> >> Subject: Re: Problems in cpuidle >> >> >> >> "ext Premi, Sanjeev" <premi@xxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
