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?
--
Regards,
Nishanth Menon
--
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
