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

[Kevin Hilman <khilman@ti.com>]

"Varadarajan, Charulatha" <charu@ti.com> writes:

> Kevin,
>
> On Tue, Mar 8, 2011 at 00:25, Kevin Hilman <khilman@ti.com> wrote:
>> "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.
>
> 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

Don't forget that the suspend path also calls prepare_for_idle (and
resume path calls resume_from_idle) so that (b) actually includes (a).
In fact, looking closer at the code, it appears we save context twice on
a static suspend.

> c. gpio_free need not do any of the above mentioned stuff.

But it would be harmless if the ->runtime_suspend/resume methods were
called.  If bank->mod_usage were zero, these hooks would just return.

> 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.

You're right, there are currently different paths for the 3 different
users of the runtime PM API (your a, b & c above), but to me that leads
to serious readability problems.   (NOTE: this isn't your fault, the
current code suffers from this already, I'm just hoping we can clean it
up with the runtime PM conversion.)

I think this could be much cleaner if everything was moved to the
->runtime_suspend/resume hooks and a couple checks were added.  For
example, the runtime_suspend would look like:

 for each bank:

    /* this handles the gpio_free case */
    if (!bank->mod_usage)
       continue;    

    /* debounce clock disable */
 
    /* off-mode: remove triggering */

    /* save context */

    /* Extra stuff for static suspend */
    if (bank->is_suspending)
      /* set wakeup bits */       

> 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).

Actually, I'm not a big fan of the off_mode flag passed from the PM
core.

Here's what would be much nicer.  Each bank can get it's pwrdm from its
hwmod.  So the ->runtime_suspend method should just read the next_state
of its powerdomain to see if it's going off, and save context (and do
errata workarounds) accordingly.  In addition, it will
_get_context_loss_count() and store the counter.  The resume method then
does _get_context_loss_count() again and compare to see if context needs
to be restored.

> Do you still think that it is appropriate to do this code movement  from
> prepare_for_idle() to GPIO's runtime_suspend?

Based on the above suggestions, yes.

>>
>> [...]
>>
>>>>> @@ -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().

OK, I see it now.  Guess I grep'd for the wrong things.

Thanks for pointing it out.

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