Nishanth Menon <nm@xxxxxx> writes: > Kevin Hilman had written, on 12/16/2010 12:57 PM, the following: >> Nishanth Menon <nm@xxxxxx> 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; + } -- 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
