Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework

[Kevin Hilman <khilman@ti.com>]

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