[PATCH 03/10] OMAP3: PM: move device-specific special cases from PM core into CPUidle

[Paul Walmsley]

Hi Kevin

On Tue, 21 Sep 2010, Kevin Hilman wrote:

> Paul Walmsley <paul at pwsan.com> writes:
> 
> > On Wed, 15 Sep 2010, Kevin Hilman wrote:
> >
> >> In an effort to simplify the core idle path, move any device-specific
> >> special case handling from the core PM idle path into the CPUidle
> >> pre-idle checking path.
> >
> > As with the original patch, I don't quite understand the improvement 
> > here.  In particular, this part:
> >
> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> >> index 3d3d035..0923b82 100644
> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> @@ -233,14 +234,54 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
> >>  			       struct cpuidle_state *state)
> >>  {
> >>  	struct cpuidle_state *new_state = next_valid_state(dev, state);
> >> +	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
> >> +	u32 cam_state;
> >> +	struct omap3_processor_cx *cx;
> >> +	int ret;
> >>  
> >>  	if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
> >>  		BUG_ON(!dev->safe_state);
> >>  		new_state = dev->safe_state;
> >> +		goto select_state;
> >> +	}
> >> +
> >> +	cx = cpuidle_get_statedata(state);
> >> +	core_next_state = cx->core_state;
> >> +
> >> +	/*
> >> +	 * Prevent idle completely if CAM is active.
> >> +	 * CAM does not have wakeup capability in OMAP3.
> >> +	 */
> >> +	cam_state = pwrdm_read_pwrst(cam_pd);
> >> +	if (cam_state == PWRDM_POWER_ON) {
> >> +		new_state = dev->safe_state;
> >> +		goto select_state;
> >>  	}
> >>  
> >> +	/*
> >> +	 * Prevent PER off if CORE is not in retention or off as this
> >> +	 * would disable PER wakeups completely.
> >> +	 */
> >> +	per_next_state = per_saved_state = pwrdm_read_next_pwrst(per_pd);
> >> +	if ((per_next_state == PWRDM_POWER_OFF) &&
> >> +	    (core_next_state > PWRDM_POWER_RET)) {
> >> +		per_next_state = PWRDM_POWER_RET;
> >> +		pwrdm_set_next_pwrst(per_pd, per_next_state);
> >> +	}
> >> +
> >> +	/* Are we changing PER target state? */
> >> +	if (per_next_state != per_saved_state)
> >> +		pwrdm_set_next_pwrst(per_pd, per_next_state);
> >
> > In this case, the PER / CORE constraints don't have anything to do with 
> > the MPU or CPUIdle, so they don't seem to belong in the CPUIdle code.  The 
> > extra comments are certainly nice -- they make it more clear as to what is 
> > going on here -- but maybe those can just be added to pm34xx.c ?
> 
> CPUidle currently manages MPU and CORE powerdomains, so the CORE
> constraints seem to make perfect sense here (at least to me.)

I do mean CORE also -- basically, anything that is not the CPU.  IMHO 
CPUIdle shouldn't manage CORE directly since it's not part of the CPU.  
Also since OMAPs have other processors (e.g., DSP, DMA, etc) that can use 
the CORE independently of the CPU's power state, it doesn't make sense for 
that code to live inside CPUIdle -- probably it should live in some type 
of SystemIdle, CORE powerdomain idle or L3 idle.  Again IMHO, the C states 
should only represent the MPU's idle state.

> The question is probably more about the PER constraints.
> 
> The basic goal of this is to streamline the core idle (omap_sram_idle())
> to be the minimum streamline idle, and to move all the constraint
> checking and activity checking to higher layers (like CPUidle.)
> Specifically, I'm working towards moving the device-specific idle
> constraints out of the core idle path (omap_sram_idle()) and move them
> into higher layers where we're checking for activity etc.
> 
> This is just a baby step towards moving the device-idle out of CPUidle
> completely to a place where it can be managed by the driver themeselves
> using runtime PM or by using constraints instead of these hard-coded
> hacks.

I agree completely with the goal; it's the implementation that I don't 
like ;-)  But if you agree with the basic idea, that CORE / PER / 
whatever-idle should ultimately go elsewhere, since I don't have time to 
come up with a constructive alternative at the moment, would you be 
willing to just drop a FIXME comment in that code, near the CAM and the 
PER / CORE stuff, that mentions that that code should ultimately be 
segmented out into its own idle code?

thanks

- Paul