Kevin,
On Tue, Mar 8, 2011 at 00:25, Kevin Hilman <[email protected]> wrote:
> "Varadarajan, Charulatha" <[email protected]> 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
c. gpio_free need not do any of the above mentioned stuff.
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.
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).
Do you still think that it is appropriate to do this code movement from
prepare_for_idle() to GPIO's runtime_suspend?
>
> [...]
>
>>>> @@ -1058,22 +1079,7 @@ static int omap_gpio_request(struct gpio_chip
>>>> *chip, unsigned offset)
>>>> __raw_writel(__raw_readl(reg) | (1 << offset), reg);
>>>> }
>>>> #endif
>>>> - if (!cpu_class_is_omap1()) {
>>>> - if (!bank->mod_usage) {
>>>> - void __iomem *reg = bank->base;
>>>> - u32 ctrl;
>>>> -
>>>> - if (cpu_is_omap24xx() || cpu_is_omap34xx())
>>>> - reg += OMAP24XX_GPIO_CTRL;
>>>> - else if (cpu_is_omap44xx())
>>>> - reg += OMAP4_GPIO_CTRL;
>>>> - ctrl = __raw_readl(reg);
>>>> - /* Module is enabled, clocks are not gated */
>>>> - ctrl &= 0xFFFFFFFE;
>>>> - __raw_writel(ctrl, reg);
>>>> - }
>>>> - bank->mod_usage |= 1 << offset;
>>>> - }
>>>
>>> Where did this code go? I expected it to be moved, but not removed
>>> completely.
>>
>> It is only moved and not removed.
>> bank->mod_usage |= 1 << offset; is done above and the rest is done below.
>
> I found the set of bank->mod_usage, but I do not see the clock un-gating
> in the resulting code. Can you please share the line number in the
> resulting code where this is done? I just grep'd for 'Module is
> enabled' and the 'ctrl &= 0xFFFFFFFE' line and could not find them.
This is done as part of omap_gpio_mod_init() (which writes zero into
ctrl register)
and it is called from omap_gpio_request().
-V Charulatha
>
> Kevin
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
