"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.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
