Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
Kevin, On Wed, Mar 9, 2011 at 06:54, Varadarajan, Charulatha wrote: > On Tue, Mar 8, 2011 at 13:23, Kevin Hilman wrote: >>> On Tue, Mar 8, 2011 at 00:25, Kevin Hilman wrote: "Varadarajan, Charulatha" 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. > > Yes. You are right. Will modify this. > >> >>> 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 */ > > Okay. But I felt that adding more flags to manage this might look > ugly. But yes, this is better in terms of readability. Will do the > needful. > >> >>> 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'
Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
Kevin, On Tue, Mar 8, 2011 at 13:23, Kevin Hilman wrote: > "Varadarajan, Charulatha" writes: > >> Kevin, >> >> On Tue, Mar 8, 2011 at 00:25, Kevin Hilman wrote: >>> "Varadarajan, Charulatha" 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. Yes. You are right. Will modify this. > >> 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 */ Okay. But I felt that adding more flags to manage this might look ugly. But yes, this is better in terms of readability. Will do the needful. > >> 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, an
Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
"Varadarajan, Charulatha" writes: > Kevin, > > On Tue, Mar 8, 2011 at 00:25, Kevin Hilman wrote: >> "Varadarajan, Charulatha" 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
Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
Kevin,
On Tue, Mar 8, 2011 at 00:25, Kevin Hilman wrote:
> "Varadarajan, Charulatha" 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 &= 0xFFFE;
- __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 &= 0xFFFE' 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
"Varadarajan, Charulatha" 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 &= 0xFFFE;
>>> - __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 &= 0xFFFE' 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
Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
Hi Kevin,
Thanks for the detailed review.
On Sat, Mar 5, 2011 at 03:29, Kevin Hilman wrote:
> Charulatha V writes:
>
>> Call runtime pm APIs pm_runtime_put_sync() and pm_runtime_get()
>
> Minor: I think you mean _get_sync() and _put()
Yes, thanks for catching it. It was a typo :-(
>
>> for enabling/disabling the clocks, sysconfig settings instead of using
>> clock FW APIs.
>> Note: OMAP16xx & OMAP2 has interface and functional clocks whereas
>> OMAP3&4 has interface and debounce clocks.
>>
>> 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.
>
> pm_runtime_get* on an already active device is harmless and will just
> increment the runtime PM internal use counting. It does however have
> the additional benefit of taking advantage of the runtime PM statistics
> so using tools like powertop, we will be able to see stats for *all*
> GPIO users, not just the first (and last) ones to use a given bank.
> IMO, This is a big win for PM debug.
>
> More on the implementation of this below...
>
>> * at the beginning of gpio_resume_after_idle()
>> - only if the GPIO bank is under use
>> (and)
>> - if the bank is in non-wkup power domain
>> * at the beginning of gpio_irq_handler()
>> - only if the specific GPIO bank is pm_runtime_suspended()
>> * at the beginning of omap_gpio_resume()
>> - only if the GPIO bank is under use
>>
>> pm_runtime_put():
>> * In the probe after completing GPIO register access
>> * at the end of omap_gpio_free()
>> - only when the last used gpio in the gpio bank is
>> freed (when mod_usage becomes 0).
>> * at the end of gpio_prepare_for_idle()
>> - only if the GPIO bank is under use
>> (and)
>> - if the bank is in non-wkup power domain
>> * at the end of gpio_irq_handler()
>> - only if a corresponding pm_runtime_get_sync() was done
>> in gpio_irq_handler()
>> * at the end of omap_gpio_suspend()
>> - only if the GPIO bank is under use
>>
>> OMAP GPIO Request/ Free:
>> *During a gpio_request when mod_usage becomes non-zero, the bank
>> registers are configured with init time configurations inorder to
>> make sure that the GPIO init time configurations are not lost if
>> the context was earlier lost when the GPIO bank was not in use.
>>
>> TODO:
>> Cleanup GPIO driver to avoid usage of gpio_bank_count &
>> cpu_is_* checks
>>
>> Signed-off-by: Charulatha V
>> Signed-off-by: Tarun Kanti DebBarma
>> ---
>> arch/arm/plat-omap/gpio.c | 305
>> +
>> 1 files changed, 167 insertions(+), 138 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index 10792b6..908bad2 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -177,6 +177,7 @@ struct gpio_bank {
>>
>> static void omap_gpio_save_context(struct gpio_bank *bank);
>> static void omap_gpio_restore_context(struct gpio_bank *bank);
>> +static void omap_gpio_mod_init(struct gpio_bank *bank, int id);
>>
>> /*
>> * TODO: Cleanup gpio_bank usage as it is having information
>> @@ -1042,8 +1043,28 @@ static int omap_gpio_request(struct gpio_chip *chip,
>> unsigned offset)
>> struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
>> unsigned long flags;
>>
>> + /*
>> + * If this is the first gpio_request for the bank,
>> + * enable the bank module
>> + */
>> + if (!bank->mod_usage) {
>> + struct platform_device *pdev = to_platform_device(bank->dev);
>> +
>> + if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
>> + dev_err(bank->dev, "%s: GPIO bank %d "
>> + "pm_runtime_get_sync failed\n",
>> +
Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
Charulatha V writes:
> Call runtime pm APIs pm_runtime_put_sync() and pm_runtime_get()
Minor: I think you mean _get_sync() and _put()
> for enabling/disabling the clocks, sysconfig settings instead of using
> clock FW APIs.
> Note: OMAP16xx & OMAP2 has interface and functional clocks whereas
> OMAP3&4 has interface and debounce clocks.
>
> 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.
pm_runtime_get* on an already active device is harmless and will just
increment the runtime PM internal use counting. It does however have
the additional benefit of taking advantage of the runtime PM statistics
so using tools like powertop, we will be able to see stats for *all*
GPIO users, not just the first (and last) ones to use a given bank.
IMO, This is a big win for PM debug.
More on the implementation of this below...
> * at the beginning of gpio_resume_after_idle()
> - only if the GPIO bank is under use
> (and)
> - if the bank is in non-wkup power domain
> * at the beginning of gpio_irq_handler()
> - only if the specific GPIO bank is pm_runtime_suspended()
> * at the beginning of omap_gpio_resume()
> - only if the GPIO bank is under use
>
> pm_runtime_put():
> * In the probe after completing GPIO register access
> * at the end of omap_gpio_free()
> - only when the last used gpio in the gpio bank is
> freed (when mod_usage becomes 0).
> * at the end of gpio_prepare_for_idle()
> - only if the GPIO bank is under use
> (and)
> - if the bank is in non-wkup power domain
> * at the end of gpio_irq_handler()
> - only if a corresponding pm_runtime_get_sync() was done
> in gpio_irq_handler()
> * at the end of omap_gpio_suspend()
> - only if the GPIO bank is under use
>
> OMAP GPIO Request/ Free:
> *During a gpio_request when mod_usage becomes non-zero, the bank
> registers are configured with init time configurations inorder to
> make sure that the GPIO init time configurations are not lost if
> the context was earlier lost when the GPIO bank was not in use.
>
> TODO:
> Cleanup GPIO driver to avoid usage of gpio_bank_count &
> cpu_is_* checks
>
> Signed-off-by: Charulatha V
> Signed-off-by: Tarun Kanti DebBarma
> ---
> arch/arm/plat-omap/gpio.c | 305
> +
> 1 files changed, 167 insertions(+), 138 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 10792b6..908bad2 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -177,6 +177,7 @@ struct gpio_bank {
>
> static void omap_gpio_save_context(struct gpio_bank *bank);
> static void omap_gpio_restore_context(struct gpio_bank *bank);
> +static void omap_gpio_mod_init(struct gpio_bank *bank, int id);
>
> /*
> * TODO: Cleanup gpio_bank usage as it is having information
> @@ -1042,8 +1043,28 @@ static int omap_gpio_request(struct gpio_chip *chip,
> unsigned offset)
> struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
> unsigned long flags;
>
> + /*
> + * If this is the first gpio_request for the bank,
> + * enable the bank module
> + */
> + if (!bank->mod_usage) {
> + struct platform_device *pdev = to_platform_device(bank->dev);
> +
> + if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
> + dev_err(bank->dev, "%s: GPIO bank %d "
> + "pm_runtime_get_sync failed\n",
> + __func__, pdev->id);
> + return -EINVAL;
> + }
> +
> + /* Initialize the gpio bank registers to init time value */
> + omap_gpio_mod_init(bank, pdev->id);
> + }
> +
This could all be replaced by:
if (!pm_runtime_get_sync(bank->dev))
omap_gpio_mod_init(bank, pdev->id);
since the first 'get' that actually resumes the device will return zero,
all the others will return 1.
Actually, even better (and my prefernce) would be to just do the
_get_sync() for every request as above, but put the omap_gpio_mod_init()
in the ->runtime_resume() hook so it gets called whenever the first GPIO
in the bank is activated.
> spin_lock_irqsave(&bank->lock, flags);
[PATCH 5/5] OMAP: GPIO: use PM runtime framework
Call runtime pm APIs pm_runtime_put_sync() and pm_runtime_get()
for enabling/disabling the clocks, sysconfig settings instead of using
clock FW APIs.
Note: OMAP16xx & OMAP2 has interface and functional clocks whereas
OMAP3&4 has interface and debounce clocks.
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).
* at the beginning of gpio_resume_after_idle()
- only if the GPIO bank is under use
(and)
- if the bank is in non-wkup power domain
* at the beginning of gpio_irq_handler()
- only if the specific GPIO bank is pm_runtime_suspended()
* at the beginning of omap_gpio_resume()
- only if the GPIO bank is under use
pm_runtime_put():
* In the probe after completing GPIO register access
* at the end of omap_gpio_free()
- only when the last used gpio in the gpio bank is
freed (when mod_usage becomes 0).
* at the end of gpio_prepare_for_idle()
- only if the GPIO bank is under use
(and)
- if the bank is in non-wkup power domain
* at the end of gpio_irq_handler()
- only if a corresponding pm_runtime_get_sync() was done
in gpio_irq_handler()
* at the end of omap_gpio_suspend()
- only if the GPIO bank is under use
OMAP GPIO Request/ Free:
*During a gpio_request when mod_usage becomes non-zero, the bank
registers are configured with init time configurations inorder to
make sure that the GPIO init time configurations are not lost if
the context was earlier lost when the GPIO bank was not in use.
TODO:
Cleanup GPIO driver to avoid usage of gpio_bank_count &
cpu_is_* checks
Signed-off-by: Charulatha V
Signed-off-by: Tarun Kanti DebBarma
---
arch/arm/plat-omap/gpio.c | 305 +
1 files changed, 167 insertions(+), 138 deletions(-)
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 10792b6..908bad2 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -177,6 +177,7 @@ struct gpio_bank {
static void omap_gpio_save_context(struct gpio_bank *bank);
static void omap_gpio_restore_context(struct gpio_bank *bank);
+static void omap_gpio_mod_init(struct gpio_bank *bank, int id);
/*
* TODO: Cleanup gpio_bank usage as it is having information
@@ -1042,8 +1043,28 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+ /*
+* If this is the first gpio_request for the bank,
+* enable the bank module
+*/
+ if (!bank->mod_usage) {
+ struct platform_device *pdev = to_platform_device(bank->dev);
+
+ if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
+ dev_err(bank->dev, "%s: GPIO bank %d "
+ "pm_runtime_get_sync failed\n",
+ __func__, pdev->id);
+ return -EINVAL;
+ }
+
+ /* Initialize the gpio bank registers to init time value */
+ omap_gpio_mod_init(bank, pdev->id);
+ }
+
spin_lock_irqsave(&bank->lock, flags);
+ bank->mod_usage |= 1 << offset;
+
/* Set trigger to none. You need to enable the desired trigger with
* request_irq() or set_irq_type().
*/
@@ -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 &= 0xFFFE;
- __raw_writel(ctrl, reg);
- }
- bank->mod_usage |= 1 << offset;
- }
+
spin_unlock_irqrestore(&bank->lock, flags);
return 0;
@@ -1106,24 +1112,39 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
__raw_writel(1 << offset, reg);
}
#endif
- if (!cpu_class_is_omap1()) {
- bank->mod_usage &= ~
