RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path

["Basak, Partha" <p-basak2@ti.com>]

 

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Tuesday, September 14, 2010 10:28 PM
> To: Basak, Partha
> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of 
> interrupts-disabled idle path
> 
> "Basak, Partha" <p-basak2@ti.com> writes:
> 
> >> From: Kevin Hilman <khilman@ti.com>
> >> 
> >> 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@ti.com>
> >> ---
> >>  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();
> +
>  	cx = cpuidle_get_statedata(state);
>  	core_next_state = cx->core_state;
>  
OK. We will base on top of this patch.

> k
>