"Basak, Partha" <p-basak2@xxxxxx> writes: > > >> -----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? Right, but we *should* be able to. ;) I'm still trying to craft a good description of this problem so I can argue better for it on linux-pm. Until then... A bit of a hack, but you could do a _get_noresume() (which is safe from interrupt context) and directly call the drivers ->runtime_resume() method, which would be the equivalent of a _get_sync(). Followed of course by a _put() (async version, also interrupt safe) at the end of the ISR to keep the usecount correct. > 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. The problem though is that without a main clock, autodeps are not tracked which means that PER might transition independently of the initiator (in this case, MPU) which we do not want either. I discussed this a little more with Paul, and the root of the problem is that the clock framework really should not disable iclks if iclk autoidle is enabled. This is a change that is needed in the clock framework that might have other side effects so is to late to attempt for this merge window. So for now, I suggest we try the above hack (well commented in the code, of course.) > 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. This would certainly be the ideal solution. It would be great to get rid of the need for calling GPIO core in the idle path, but I'm not yet convinced we can do that. For example, there is some other errata handling in my pm-gpio branch (that I've been waiting to rework until after this hwmod conversion.) I would want to be sure that those things can still be correctly handled. Also How would you propose to handle 2430 CORE GPIOs? 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
