Kevin,
On Wed, Mar 9, 2011 at 06:54, Varadarajan, Charulatha <charu@xxxxxx> wrote:
> On Tue, Mar 8, 2011 at 13:23, Kevin Hilman <khilman@xxxxxx> wrote:
>>> On Tue, Mar 8, 2011 at 00:25, Kevin Hilman <khilman@xxxxxx> wrote:
>>>> "Varadarajan, Charulatha" <charu@xxxxxx> writes:
>>>>
>>>> [...]
>>>>
>>>>>>> GPIO driver is modified to use dev_pm_ops instead of sysdev_class.
>>>>>>> With this approach, gpio_bank_suspend() & gpio_bank_resume()
>>>>>>> are not part of sys_dev_class.
>>>>>>>
>>>>>>> Usage of PM runtime get/put APIs in GPIO driver is as given below:
>>>>>>> pm_runtime_get_sync():
>>>>>>> * In the probe before accessing the GPIO registers
>>>>>>> * at the beginning of omap_gpio_request()
>>>>>>> -only when one of the gpios is requested on a bank, in which,
>>>>>>> no other gpio is being used (when mod_usage becomes non-zero).
>>>>>>
>>>>>> When using runtime PM, bank->mod_usage acutally becomes redundant with
>>>>>> usage counting already done at the runtime PM level. IOW, checking
>>>>>> bank->mod_usage is he equivalent of checking pm_runtime_suspended(), so
>>>>>> I think you can totally get rid of bank->mod_usage.
>>>>>
>>>>> I wish to differ here. bank->mod_usage is filled for each GPIO pin in a bank.
>>>>> Hence during request/free if pm_runtime_get_sync() is called for each GPIO
>>>>> pin, then the count gets increased for each GPIO pin in a bank. But
>>>>> gpio_prepare_for_idle/suspend calls pm_runtime_put() only once for
>>>>> each bank. Hence there will be a count mismatch and hence this will lead
>>>>> to problems and never a bank will get suspended.
>>>>>
>>>>> IMO it is required to have bank->mod_usage checks. Please correct
>>>>> me if I am wrong.
>>>>
>>>> You're right, I see what you're saying now. Thanks for clarifying.
>>>
>>> Okay.
>>>
>>>>
>>>> In that case, keeping bank->mod_usage should be OK for now.
>>>
>>> Okay.
>>>
>>>>
>>>> That being said, as I'm looking at the idle prepare/resume hooks
>>>> something else came to mind.
>>>>
>>>> Most of what the idle prepare/resume mehods do should actually be done
>>>> in the ->runtime_suspend() and ->runtime_resume() hooks (e.g. debounce
>>>> clock disable, edge-detect stuff, context save/restore). IOW, that
>>>> stuff does not need to be done until the bank is actually
>>>> disabled/enabled. For example, prepare_for_idle itself could be a
>>>> relatively simple check for bank->mod_usage and a call to
>>>> pm_runtime_put_sync().
>>>>
>>>> What do you think?
>>>
>>> I also thought about this and my very old implementation with hwmod
>>> series was like this. But,
>>> a. prepare_for_idle has an Erratum 1.101 handling, debounce clock disable,
>>> context save based on offmode flag
>>> b. omap_gpio_suspend has wkup related code handling in it along
>>> with context save w/o any flag
>>
>> Don't forget that the suspend path also calls prepare_for_idle (and
>> resume path calls resume_from_idle) so that (b) actually includes (a).
>> In fact, looking closer at the code, it appears we save context twice on
>> a static suspend.
>
> Yes. You are right. Will modify this.
>
>>
>>> c. gpio_free need not do any of the above mentioned stuff.
>>
>> But it would be harmless if the ->runtime_suspend/resume methods were
>> called. If bank->mod_usage were zero, these hooks would just return.
>>
>>> Similar for resume_after_idle, gpio_request, omap_resume.
>>>
>>> But the runtime_suspend/resume hooks would be called for all the above.
>>>
>>> Hence I thought that it might not be correct to move all the code from
>>> prepare_for_idle() to runtime_suspend hook of GPIO. Similar for
>>> resume_after_idle()
>>> and runtime_resume hook.
>>
>> You're right, there are currently different paths for the 3 different
>> users of the runtime PM API (your a, b & c above), but to me that leads
>> to serious readability problems. (NOTE: this isn't your fault, the
>> current code suffers from this already, I'm just hoping we can clean it
>> up with the runtime PM conversion.)
>>
>> I think this could be much cleaner if everything was moved to the
>> ->runtime_suspend/resume hooks and a couple checks were added. For
>> example, the runtime_suspend would look like:
>>
>> for each bank:
>>
>> /* this handles the gpio_free case */
>> if (!bank->mod_usage)
>> continue;
>>
>> /* debounce clock disable */
>>
>> /* off-mode: remove triggering */
>>
>> /* save context */
>>
>> /* Extra stuff for static suspend */
>> if (bank->is_suspending)
>> /* set wakeup bits */
>
> Okay. But I felt that adding more flags to manage this might look
> ugly. But yes, this is better in terms of readability. Will do the
> needful.
>
>>
>>> Also, from implementation point of view it needs to be taken care to
>>> pass off_mode flag to runtime_suspend hook and use it only for
>>> prepare_for idle and not for other cases
>>> (omap_gpio_suspend/gpio_free).
>>
>> Actually, I'm not a big fan of the off_mode flag passed from the PM
>> core.
>>
>> Here's what would be much nicer. Each bank can get it's pwrdm from its
>> hwmod. So the ->runtime_suspend method should just read the next_state
>> of its powerdomain to see if it's going off, and save context (and do
>> errata workarounds) accordingly.
To read the next_state, the driver needs to be aware of the power
domain ptr and the power domain states. With this change, the
omap_gpio_runtime_suspend() would look like this:
{
...
...
pdev = to_platform_device(bank->dev);
od = to_omap_device(pdev);
pwrdm = omap_device_get_pwrdm (od);
/* or get the pwrdm via pdata during probe */
nxt_state = pwrdm_read_next_pwrst(pwrdm);
if (next_state == PWRDM_POWER_OFF) {
....
...
save_context();
}
....
....
}
IMO, the above doesn't look nice in a driver, as driver should not be
aware of power states. Also, a similar approach was taken in the
previous GPIO hwmod & PM runtime series and it was rejected.
Pls provide me some pointers on this.
Thanks,
V Charulatha
>
> Ok. I will modify the GPIO driver to access these APIs directly
> from runtime_suspend hook of GPIO to read the next_state
> of its powerdomain.
>
>> In addition, it will
>> _get_context_loss_count() and store the counter. The resume method then
>> does _get_context_loss_count() again and compare to see if context needs
>> to be restored.
>
> Okay.
>
>>
>>> Do you still think that it is appropriate to do this code movement from
>>> prepare_for_idle() to GPIO's runtime_suspend?
>>
>> Based on the above suggestions, yes.
>
> Thanks,
> V Charulatha
>
> <<snip>>
>
--
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
