Kevin Hilman had written, on 12/17/2010 04:54 PM, the following:
Nishanth Menon <n...@ti.com> writes:
Kevin Hilman had written, on 12/16/2010 12:57 PM, the following:
Nishanth Menon <n...@ti.com> writes:
Nishanth Menon had written, on 12/15/2010 06:05 PM, the following:
Kevin Hilman had written, on 12/15/2010 05:47 PM, the following:
I agree that this additional check in sram_idle should be removed, but
as long as I handle it in omap3_pm_off_mode_enable where the next
states are configured, is'nt that enough or am I missing something?
Setting the next states only sets the default states, but CPUidle
changes them.
Looking closer at omap3_pm_off_mode_enable() though, it already calls
into CPUidle and disables the valid bit for any states that have
*either* MPU or core off. You'll probably just need to extend this
approach to disable only CORE off state(s).
Thx. it is clear now. let me see how to clean this up.
k. Does the attached look any better now :)?
Yes, but, I still don't quite like it. Basically, I'm not crazy about
the errata knowledge being centralized in pm34xx.c. How about this:
Move the Errata handling core code (pm_errata_* PM_ERRATTA_*) to pm.[ch]
as a single patch. Then both pm34xx.c and cpuidle34xx.c would be free
to use it.
This would allow CPUidle handle the errata itself in the 'update_states'
function. Or even better, if CPUidle core can check this errata, it
should probably just never register C7 in the first place, because it is
*never* a valid C-state.
Make sense?
hmm.. IMHO at the problems themselves:
a) cpuidle_params_table -> this needs to become dynamically generated
if you'd like cpuidle to not know about the existance of the invalid
states at all(C7 has to disappear from it's radar - but extending it
to a generic solution where an inbetween C state might be disabled as
well)
b) extending the problem further - cpu idle state latencies by
themselves might vary between boards(PMIC variances causing delta in
voltage setup times as an example).. I suppose we have some way to
plug that data in as well? but irrelevant to this discussion. or maybe
some board would like to have a customized additional c state which is
not really practical for other platforms for what ever reasons..
c) if cpuidle where to get pm errata info, it is nice thing to do,
but, dont you think it is an overkill atm for just one errata?
d) omap_init_power_states may need some cleanups as well.. too many =
assignments IMHO..
e) On the topic of argument that the states controlled by
enable_off_mode is dynamic, though even though enable_off_mode is a
variable, it is in debugfs - not really a dynamic variant in a
product.. with that in mind, we may want to have off OR not have off
mode in a product board file and folks would probably call
omap3_pm_off_mode_enable or set the variable and set it to 1. That
makes it even more crazy IMHO.
f) Finally, where do we detect the erratas? it is more handy to have
them in one place - pm34xx.c was chosen to make it independent of
silicon type - pm44xx.c might endup having different set with it's own
requirements - so not all together convinced it should be in
pm.[ch]. I have tried to restrict the detection and usage purely to
the file that needs it - pm34xx.c
I think I agree to your overall thought that C state by itself
should'nt have been registered, but would'nt it be better to do the
cpuidle cleanups in a different context?
If you want to do all those cleanups, feel free. They all are valid.
However, your patch targets an isolated problem, and I'm OK with an
isolated fix.
All I was suggesting is moving the PM errata detection/macros etc. into
pm.h, and doing somthing simple (below) in CPUidle.
Kevin
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 81b0a90..92873b4 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -452,6 +452,15 @@ void omap_init_power_states(void)
omap3_power_states[OMAP3_STATE_C7].core_state = PWRDM_POWER_OFF;
omap3_power_states[OMAP3_STATE_C7].flags = CPUIDLE_FLAG_TIME_VALID |
CPUIDLE_FLAG_CHECK_BM;
+
+ /*
+ * Erratum i583: implementation for ES rev < Es1.2 on 3630
+ * We cannot enable OFF mode in a stable form for previous
+ * revisions, transition instead to RET
+ */
+ if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)
+ omap3_power_states[OMAP3_STATE_C7].valid = 0;
+
}
Look at the following sequence:
a) omap_init_power_states is called at init
omap3_power_states[OMAP3_STATE_C7].valid = 0
lets say we make cpuidle_params_table[OMAP3_STATE_C7].valid = 0 as well
here.
b) user enables enable_off_mode
omap3_pm_off_mode_enable(pm34xx.c) -> calls omap3_cpuidle_update_states
for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
struct omap3_processor_cx *cx = &omap3_power_states[i];
if (enable_off_mode) {
cx->valid = 1;
---> valid bit is back to 1.
} else {
if ((cx->mpu_state == PWRDM_POWER_OFF) ||
(cx->core_state == PWRDM_POWER_OFF))
cx->valid = 0;
}
}
c) idle will not hit the low state coz
cpuidle_params_table[OMAP3_STATE_C7].valid will not be set
d) you try suspend path, well, we dont check the cpuidle_params_table..
we will attempt to hit off.
This approach of selective disable of omap3_power_states will not work
without modifying omap3_cpuidle_update_states. am I wrong?
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
