> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Thursday, September 23, 2010 9:08 PM > To: Basak, Partha > Cc: linux-omap@xxxxxxxxxxxxxxx; Varadarajan, Charulatha; Tero Kristo > Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of > interrupts-disabled idle path > > "Basak, Partha" <p-basak2@xxxxxx> writes: > > > > > > >> -----Original Message----- > >> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > >> Sent: Tuesday, September 14, 2010 10:28 PM > >> To: Basak, Partha > >> Cc: linux-omap@xxxxxxxxxxxxxxx; Varadarajan, Charulatha; > Tero Kristo > >> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of > >> interrupts-disabled idle path > >> > >> "Basak, Partha" <p-basak2@xxxxxx> writes: > >> > >> >> From: Kevin Hilman <khilman@xxxxxx> > >> >> > >> >> Currently, we wait until late in the idle path where > interrupts are > >> >> disabled to do runtime-PM-like management for certain > special-case > >> >> devices like GPIO. > >> >> > >> >> As a prerequiste to moving GPIO to the new runtime PM > >> framework, move > >> >> this runtime-PM-like code out of the late idle path > into new device > >> >> idle and resume functions that can be called before > interrupts are > >> >> disabled by CPUidle and/or suspend. > >> >> > >> >> In addition, move all the GPIO-specific logic into the GPIO core > >> >> instead of keeping GPIO-specific knowledge of > power-states, context > >> >> saving etc. in the PM core. > >> >> > >> >> Also, call the new device-idle and -resume methods from > CPUidle and > >> >> static suspend path. > >> >> > >> >> Signed-off-by: Kevin Hilman <khilman@xxxxxx> > >> >> --- > >> >> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++ > >> >> arch/arm/mach-omap2/pm.h | 2 + > >> >> arch/arm/mach-omap2/pm24xx.c | 2 +- > >> >> arch/arm/mach-omap2/pm34xx.c | 38 > >> +++++++++------------ > >> >> arch/arm/plat-omap/gpio.c | 57 > >> >> ++++++++++++++++++++++++-------- > >> >> arch/arm/plat-omap/include/plat/gpio.h | 4 +-- > >> >> 6 files changed, 67 insertions(+), 40 deletions(-) > >> >> > >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c > >> >> b/arch/arm/mach-omap2/cpuidle34xx.c > >> >> index 0923b82..681d823 100644 > >> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c > >> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c > >> >> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct > >> >> cpuidle_device *dev, > >> >> pwrdm_set_next_pwrst(per_pd, per_next_state); > >> >> > >> >> select_state: > >> >> + omap3_device_idle(); > >> >> + > >> >> dev->last_state = new_state; > >> >> ret = omap3_enter_idle(dev, new_state); > >> >> > >> >> + omap3_device_resume(); > >> >> + > >> > In the generic cpu_idle() in process.c, interrupts are > >> already disabled > >> > before control comes to cpuidle_idle_call() via pm_idle() > >> > local_irq_disable(); > >> > if (hlt_counter) { > >> > local_irq_enable(); > >> > cpu_relax(); > >> > } else { > >> > stop_critical_timings(); > >> > pm_idle(); > >> > start_critical_timings(); > >> > /* > >> > * This will eventually be > >> removed - pm_idle > >> > * functions should always > >> return with IRQs > >> > * enabled. > >> > */ > >> > WARN_ON(irqs_disabled()); > >> > local_irq_enable(); > >> > } > >> > > >> > omap3_enter_idle_bm() will be called from inside > >> cpuidle_idle_call() > >> > via target_state->enter(dev, target_state). > >> > So, interrupts are already disabled here. > >> > > >> > Am I missing something? > >> > >> You're right. > >> > >> I knew this was the case for !CPUIDLE setup, but had > thought (without > >> testing) that the CPUidle core had re-enabled interrupts during the > >> governor selection process etc. > >> > >> While I investigate ways to manage this in CPUidle, the > >> following should > >> be fine for now to include with $SUBJECT patch. > >> > >> Kevin > >> > >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c > >> b/arch/arm/mach-omap2/cpuidle34xx.c > >> index 681d823..c5cb9d0 100644 > >> --- a/arch/arm/mach-omap2/cpuidle34xx.c > >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c > >> @@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct > >> cpuidle_device *dev, > >> goto select_state; > >> } > >> > >> + /* > >> + * Enable IRQs during the device activity checking and > >> idle management. > >> + * IRQs are later (re)disabled when entering the actual > >> idle function. > >> + * Device idle management that is using runtime PM needs to have > >> + * interrupts enabled when calling into the runtime PM core. > >> + */ > >> + local_irq_enable(); > > > > After put_sync() retuns, there will be a time window where > interrupts > > are enabled but clocks are disabled before the interrupts > are disabled again. > > Accessing any register to service a device interrupt coming > during this window > > will lead to a crash for cases where iclk and fclks are > same and we have the > > iclk defined as the main_clk as well. > > > > Same argument holds while returning from Idle. We are > facing this issue for OMAP3 > > GPIO while trying to define the main_clk = interface clock > based on your other commment. > > This is the same problem as has existed in static/suspend resume. > > IOW, if it's possible for a device interrupt to arrive between device > suspend and actual suspend, then the device interrupt should be > disabled in the suspend hook (or runtime_suspend hook in your case.) > > The catch is how to handle these interrupts if they are > wakeup sources. Precisely. For example, the ethernet interrupts over GPIO are causing problem. > > If these interrupt are wakeup sources, then they should not > be disabled > in the [runtime_]suspend path, but rather the ISR for that > device should > just do a get_sync() and continue. We cannot do a get_sync() from ISR context, right? For GPIOs in particular, this problem can be resolved if we do not tie up the interface clock as the main_clk. So long the interface/slave clock is not also a main_clk & OCPIF_SWSUP_IDLE flag is not set for a hwmod there is no issue, as slave clocks are NOT turned on/off in _enable_clocks() /_disable_clocks() inside hwmod fw. Alternatively, we could have thought of removing get_sync/put_sync altogether from the Idle path for GPIO. But though this would work fine for OMAP3/OMAP4 & OMAP2420/2430 Wakeup domain GPIOs, but for OMAP2430 CORE domain GPIOs, since it has a separate fclk & iclk, we would still need to cut the fclk in the Idle-path to enable CORE transition, thereby needing a get_sync/put_sync . If this is OK, we can consider this approach. > > Kevin > -- 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
