"Varadarajan, Charulatha" <ch...@ti.com> 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.
In that case, keeping bank->mod_usage should be OK for now.
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?
[...]
>>> @@ -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.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
