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

Paul Walmsley [off-list ref] writes:

Hi Kevin

On Tue, 21 Sep 2010, Kevin Hilman wrote:

quoted

Paul Walmsley [off-list ref] writes:

quoted

On Wed, 15 Sep 2010, Kevin Hilman wrote:

quoted

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:

quoted

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.

quoted

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?

Absolutely...  will do.

Kevin