Nishanth Menon <nm@xxxxxx> writes:
> Kevin Hilman had written, on 12/17/2010 04:54 PM, the following:
>> 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;
>> + }
>>
>
> 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?
You're right.
I see you've done both in the updated series. Will have a closer look
shortly, but looks ok for now.
Kevin
--
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
