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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Kevin,

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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux