* [PATCH 1/5] OMAP: GPIO: make gpio_context part of gpio_bank structure
2011-02-28 10:57 [PATCH 0/5] OMAP: GPIO: use PM runtime framework Charulatha V
@ 2011-02-28 10:57 ` Charulatha V
2011-02-28 10:57 ` [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO Charulatha V
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Charulatha V @ 2011-02-28 10:57 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, tony, paul, Charulatha V
gpio_context array, which is used to save gpio bank's context,
is used only for OMAP3 architecture.
Move gpio_context as part of gpio_bank structure so that
it can be specific to each gpio bank and can be used for
any OMAP architecture
Signed-off-by: Charulatha V <charu@ti.com>
Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
(2430-SDP testing)
---
TODO: extend the gpio save/restore context function for OMAP4
architecture. This is done in one of the next patches in this
series
arch/arm/plat-omap/gpio.c | 72 +++++++++++++++++++++-----------------------
1 files changed, 34 insertions(+), 38 deletions(-)
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 971d186..c6ab0ff 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -133,6 +133,19 @@
#define OMAP4_GPIO_CLEARDATAOUT 0x0190
#define OMAP4_GPIO_SETDATAOUT 0x0194
+struct gpio_regs {
+ u32 irqenable1;
+ u32 irqenable2;
+ u32 wake_en;
+ u32 ctrl;
+ u32 oe;
+ u32 leveldetect0;
+ u32 leveldetect1;
+ u32 risingdetect;
+ u32 fallingdetect;
+ u32 dataout;
+};
+
struct gpio_bank {
unsigned long pbase;
void __iomem *base;
@@ -145,7 +158,7 @@ struct gpio_bank {
#endif
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
-
+ struct gpio_regs context;
u32 saved_datain;
u32 saved_fallingdetect;
u32 saved_risingdetect;
@@ -161,23 +174,6 @@ struct gpio_bank {
int stride;
};
-#ifdef CONFIG_ARCH_OMAP3
-struct omap3_gpio_regs {
- u32 irqenable1;
- u32 irqenable2;
- u32 wake_en;
- u32 ctrl;
- u32 oe;
- u32 leveldetect0;
- u32 leveldetect1;
- u32 risingdetect;
- u32 fallingdetect;
- u32 dataout;
-};
-
-static struct omap3_gpio_regs gpio_context[OMAP34XX_NR_GPIOS];
-#endif
-
/*
* TODO: Cleanup gpio_bank usage as it is having information
* related to all instances of the device
@@ -2043,25 +2039,25 @@ void omap_gpio_save_context(void)
/* saving banks from 2-6 only since GPIO1 is in WKUP */
for (i = 1; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
- gpio_context[i].irqenable1 =
+ bank->context.irqenable1 =
__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
- gpio_context[i].irqenable2 =
+ bank->context.irqenable2 =
__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE2);
- gpio_context[i].wake_en =
+ bank->context.wake_en =
__raw_readl(bank->base + OMAP24XX_GPIO_WAKE_EN);
- gpio_context[i].ctrl =
+ bank->context.ctrl =
__raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
- gpio_context[i].oe =
+ bank->context.oe =
__raw_readl(bank->base + OMAP24XX_GPIO_OE);
- gpio_context[i].leveldetect0 =
+ bank->context.leveldetect0 =
__raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT0);
- gpio_context[i].leveldetect1 =
+ bank->context.leveldetect1 =
__raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT1);
- gpio_context[i].risingdetect =
+ bank->context.risingdetect =
__raw_readl(bank->base + OMAP24XX_GPIO_RISINGDETECT);
- gpio_context[i].fallingdetect =
+ bank->context.fallingdetect =
__raw_readl(bank->base + OMAP24XX_GPIO_FALLINGDETECT);
- gpio_context[i].dataout =
+ bank->context.dataout =
__raw_readl(bank->base + OMAP24XX_GPIO_DATAOUT);
}
}
@@ -2073,25 +2069,25 @@ void omap_gpio_restore_context(void)
for (i = 1; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
- __raw_writel(gpio_context[i].irqenable1,
+ __raw_writel(bank->context.irqenable1,
bank->base + OMAP24XX_GPIO_IRQENABLE1);
- __raw_writel(gpio_context[i].irqenable2,
+ __raw_writel(bank->context.irqenable2,
bank->base + OMAP24XX_GPIO_IRQENABLE2);
- __raw_writel(gpio_context[i].wake_en,
+ __raw_writel(bank->context.wake_en,
bank->base + OMAP24XX_GPIO_WAKE_EN);
- __raw_writel(gpio_context[i].ctrl,
+ __raw_writel(bank->context.ctrl,
bank->base + OMAP24XX_GPIO_CTRL);
- __raw_writel(gpio_context[i].oe,
+ __raw_writel(bank->context.oe,
bank->base + OMAP24XX_GPIO_OE);
- __raw_writel(gpio_context[i].leveldetect0,
+ __raw_writel(bank->context.leveldetect0,
bank->base + OMAP24XX_GPIO_LEVELDETECT0);
- __raw_writel(gpio_context[i].leveldetect1,
+ __raw_writel(bank->context.leveldetect1,
bank->base + OMAP24XX_GPIO_LEVELDETECT1);
- __raw_writel(gpio_context[i].risingdetect,
+ __raw_writel(bank->context.risingdetect,
bank->base + OMAP24XX_GPIO_RISINGDETECT);
- __raw_writel(gpio_context[i].fallingdetect,
+ __raw_writel(bank->context.fallingdetect,
bank->base + OMAP24XX_GPIO_FALLINGDETECT);
- __raw_writel(gpio_context[i].dataout,
+ __raw_writel(bank->context.dataout,
bank->base + OMAP24XX_GPIO_DATAOUT);
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO
2011-02-28 10:57 [PATCH 0/5] OMAP: GPIO: use PM runtime framework Charulatha V
2011-02-28 10:57 ` [PATCH 1/5] OMAP: GPIO: make gpio_context part of gpio_bank structure Charulatha V
@ 2011-02-28 10:57 ` Charulatha V
2011-03-04 21:51 ` Kevin Hilman
2011-02-28 10:57 ` [PATCH 3/5] OMAP4: GPIO: save/restore context Charulatha V
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Charulatha V @ 2011-02-28 10:57 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, tony, paul, Charulatha V
In omap3, save/restore context is implemented for GPIO
banks 2-6 as GPIO bank1 is in wakeup domain. Instead
of identifying bank's power domain by bank id, make use
of powerdomain name itself.
For this, omap_hwmod_get_pwrdm() is used. omap_device_get_pwrdm()
could not be used as the pwrdm information needs to be filled
in pdata. But omap_device_get_pwrdm() can be used only after
omap_device_build() call.
Signed-off-by: Charulatha V <charu@ti.com>
Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
(2430-SDP testing)
---
arch/arm/mach-omap2/gpio.c | 6 ++++++
arch/arm/plat-omap/gpio.c | 31 ++++++++++++++++++++-----------
arch/arm/plat-omap/include/plat/gpio.h | 1 +
3 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
index 413de18..39b0d96 100644
--- a/arch/arm/mach-omap2/gpio.c
+++ b/arch/arm/mach-omap2/gpio.c
@@ -24,6 +24,8 @@
#include <plat/omap_hwmod.h>
#include <plat/omap_device.h>
+#include "powerdomain.h"
+
static struct omap_device_pm_latency omap_gpio_latency[] = {
[0] = {
.deactivate_func = omap_device_idle_hwmods,
@@ -39,6 +41,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
struct omap_gpio_dev_attr *dev_attr;
char *name = "omap_gpio";
int id;
+ struct powerdomain *pwrdm;
/*
* extract the device id from name field available in the
@@ -75,6 +78,9 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
return -EINVAL;
}
+ pwrdm = omap_hwmod_get_pwrdm(oh);
+ strcpy(pdata->pwrdm_name, pwrdm->name);
+
od = omap_device_build(name, id - 1, oh, pdata,
sizeof(*pdata), omap_gpio_latency,
ARRAY_SIZE(omap_gpio_latency),
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index c6ab0ff..ff341ec 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -172,6 +172,7 @@ struct gpio_bank {
struct device *dev;
bool dbck_flag;
int stride;
+ const char *pwrdm_name;
};
/*
@@ -1720,6 +1721,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
bank->dbck_flag = pdata->dbck_flag;
bank->stride = pdata->bank_stride;
bank_width = pdata->bank_width;
+ bank->pwrdm_name = pdata->pwrdm_name;
spin_lock_init(&bank->lock);
@@ -1865,16 +1867,15 @@ static int workaround_enabled;
void omap2_gpio_prepare_for_idle(int off_mode)
{
int i, c = 0;
- int min = 0;
- if (cpu_is_omap34xx())
- min = 1;
-
- for (i = min; i < gpio_bank_count; i++) {
+ for (i = 0; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
u32 l1 = 0, l2 = 0;
int j;
+ if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
+ continue;
+
for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
clk_disable(bank->dbck);
@@ -1934,15 +1935,15 @@ void omap2_gpio_prepare_for_idle(int off_mode)
void omap2_gpio_resume_after_idle(void)
{
int i;
- int min = 0;
- if (cpu_is_omap34xx())
- min = 1;
- for (i = min; i < gpio_bank_count; i++) {
+ for (i = 0; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
u32 l = 0, gen, gen0, gen1;
int j;
+ if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
+ continue;
+
for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
clk_enable(bank->dbck);
@@ -2037,8 +2038,12 @@ void omap_gpio_save_context(void)
int i;
/* saving banks from 2-6 only since GPIO1 is in WKUP */
- for (i = 1; i < gpio_bank_count; i++) {
+ for (i = 0; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
+
+ if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
+ continue;
+
bank->context.irqenable1 =
__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
bank->context.irqenable2 =
@@ -2067,8 +2072,12 @@ void omap_gpio_restore_context(void)
{
int i;
- for (i = 1; i < gpio_bank_count; i++) {
+ for (i = 0; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
+
+ if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
+ continue;
+
__raw_writel(bank->context.irqenable1,
bank->base + OMAP24XX_GPIO_IRQENABLE1);
__raw_writel(bank->context.irqenable2,
diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h
index d6f9fa0..2c46caa 100644
--- a/arch/arm/plat-omap/include/plat/gpio.h
+++ b/arch/arm/plat-omap/include/plat/gpio.h
@@ -77,6 +77,7 @@ struct omap_gpio_platform_data {
int bank_width; /* GPIO bank width */
int bank_stride; /* Only needed for omap1 MPUIO */
bool dbck_flag; /* dbck required or not - True for OMAP3&4 */
+ char pwrdm_name[20]; /* bank powerdomain name */
};
/* TODO: Analyze removing gpio_bank_count usage from driver code */
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO
2011-02-28 10:57 ` [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO Charulatha V
@ 2011-03-04 21:51 ` Kevin Hilman
2011-03-05 5:21 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hilman @ 2011-03-04 21:51 UTC (permalink / raw)
To: Charulatha V; +Cc: linux-omap, tony, paul
Charulatha V <charu@ti.com> writes:
> In omap3, save/restore context is implemented for GPIO
> banks 2-6 as GPIO bank1 is in wakeup domain. Instead
> of identifying bank's power domain by bank id, make use
> of powerdomain name itself.
>
> For this, omap_hwmod_get_pwrdm() is used. omap_device_get_pwrdm()
> could not be used as the pwrdm information needs to be filled
> in pdata. But omap_device_get_pwrdm() can be used only after
> omap_device_build() call.
>
> Signed-off-by: Charulatha V <charu@ti.com>
>
> Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> (2430-SDP testing)
I like the idea of this change, but not the implementation...
[...]
> @@ -1865,16 +1867,15 @@ static int workaround_enabled;
> void omap2_gpio_prepare_for_idle(int off_mode)
> {
> int i, c = 0;
> - int min = 0;
>
> - if (cpu_is_omap34xx())
> - min = 1;
> -
> - for (i = min; i < gpio_bank_count; i++) {
> + for (i = 0; i < gpio_bank_count; i++) {
> struct gpio_bank *bank = &gpio_bank[i];
> u32 l1 = 0, l2 = 0;
> int j;
>
> + if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> + continue;
> +
This adds a string compare for every bank during every idle
transistion (and every resume.) That's a lot of unneeded overhead.
I'd rather see a per-bank flag 'looses_context' or something that can be
checked more efficiently in this fast path. This flag can be set based
on the powerdomain name in the device init code.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO
2011-03-04 21:51 ` Kevin Hilman
@ 2011-03-05 5:21 ` Varadarajan, Charulatha
2011-03-07 18:56 ` Kevin Hilman
0 siblings, 1 reply; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-05 5:21 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, tony, paul
On Sat, Mar 5, 2011 at 02:21, Kevin Hilman <khilman@ti.com> wrote:
> Charulatha V <charu@ti.com> writes:
>
>> In omap3, save/restore context is implemented for GPIO
>> banks 2-6 as GPIO bank1 is in wakeup domain. Instead
>> of identifying bank's power domain by bank id, make use
>> of powerdomain name itself.
>>
>> For this, omap_hwmod_get_pwrdm() is used. omap_device_get_pwrdm()
>> could not be used as the pwrdm information needs to be filled
>> in pdata. But omap_device_get_pwrdm() can be used only after
>> omap_device_build() call.
>>
>> Signed-off-by: Charulatha V <charu@ti.com>
>>
>> Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> (2430-SDP testing)
>
> I like the idea of this change, but not the implementation...
>
> [...]
>
>> @@ -1865,16 +1867,15 @@ static int workaround_enabled;
>> void omap2_gpio_prepare_for_idle(int off_mode)
>> {
>> int i, c = 0;
>> - int min = 0;
>>
>> - if (cpu_is_omap34xx())
>> - min = 1;
>> -
>> - for (i = min; i < gpio_bank_count; i++) {
>> + for (i = 0; i < gpio_bank_count; i++) {
>> struct gpio_bank *bank = &gpio_bank[i];
>> u32 l1 = 0, l2 = 0;
>> int j;
>>
>> + if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
>> + continue;
>> +
>
> This adds a string compare for every bank during every idle
> transistion (and every resume.) That's a lot of unneeded overhead.
>
> I'd rather see a per-bank flag 'looses_context' or something that can be
> checked more efficiently in this fast path. This flag can be set based
> on the powerdomain name in the device init code.
This looks better. Will do the needful.
One question, can "looses_context" be made as part of dev_attr?
>
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO
2011-03-05 5:21 ` Varadarajan, Charulatha
@ 2011-03-07 18:56 ` Kevin Hilman
2011-03-08 2:25 ` Paul Walmsley
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hilman @ 2011-03-07 18:56 UTC (permalink / raw)
To: Varadarajan, Charulatha, Benoit Cousson; +Cc: linux-omap, tony, paul
"Varadarajan, Charulatha" <charu@ti.com> writes:
> On Sat, Mar 5, 2011 at 02:21, Kevin Hilman <khilman@ti.com> wrote:
>> Charulatha V <charu@ti.com> writes:
>>
>>> In omap3, save/restore context is implemented for GPIO
>>> banks 2-6 as GPIO bank1 is in wakeup domain. Instead
>>> of identifying bank's power domain by bank id, make use
>>> of powerdomain name itself.
>>>
>>> For this, omap_hwmod_get_pwrdm() is used. omap_device_get_pwrdm()
>>> could not be used as the pwrdm information needs to be filled
>>> in pdata. But omap_device_get_pwrdm() can be used only after
>>> omap_device_build() call.
>>>
>>> Signed-off-by: Charulatha V <charu@ti.com>
>>>
>>> Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>>> (2430-SDP testing)
>>
>> I like the idea of this change, but not the implementation...
>>
>> [...]
>>
>>> @@ -1865,16 +1867,15 @@ static int workaround_enabled;
>>> void omap2_gpio_prepare_for_idle(int off_mode)
>>> {
>>> int i, c = 0;
>>> - int min = 0;
>>>
>>> - if (cpu_is_omap34xx())
>>> - min = 1;
>>> -
>>> - for (i = min; i < gpio_bank_count; i++) {
>>> + for (i = 0; i < gpio_bank_count; i++) {
>>> struct gpio_bank *bank = &gpio_bank[i];
>>> u32 l1 = 0, l2 = 0;
>>> int j;
>>>
>>> + if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
>>> + continue;
>>> +
>>
>> This adds a string compare for every bank during every idle
>> transistion (and every resume.) That's a lot of unneeded overhead.
>>
>> I'd rather see a per-bank flag 'looses_context' or something that can be
>> checked more efficiently in this fast path. This flag can be set based
>> on the powerdomain name in the device init code.
>
> This looks better. Will do the needful.
> One question, can "looses_context" be made as part of dev_attr?
I guess that's up to Benoît.
But, I don't think that's necessary. It should be easy to set at runtime
just doing a strcmp on the powerdomain during the device init,
omap_device_build phase.
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO
2011-03-07 18:56 ` Kevin Hilman
@ 2011-03-08 2:25 ` Paul Walmsley
2011-03-08 5:45 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Paul Walmsley @ 2011-03-08 2:25 UTC (permalink / raw)
To: Varadarajan\, Charulatha, Kevin Hilman; +Cc: Benoit Cousson, linux-omap, tony
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1756 bytes --]
Hi
On Mon, 7 Mar 2011, Kevin Hilman wrote:
> "Varadarajan, Charulatha" <charu@ti.com> writes:
>
> > On Sat, Mar 5, 2011 at 02:21, Kevin Hilman <khilman@ti.com> wrote:
> >> Charulatha V <charu@ti.com> writes:
> >>>
> >>> + if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> >>> + continue;
> >>> +
> >>
> >> This adds a string compare for every bank during every idle
> >> transistion (and every resume.) That's a lot of unneeded overhead.
> >>
> >> I'd rather see a per-bank flag 'looses_context' or something that can be
> >> checked more efficiently in this fast path. This flag can be set based
> >> on the powerdomain name in the device init code.
> >
> > This looks better. Will do the needful.
> > One question, can "looses_context" be made as part of dev_attr?
>
> I guess that's up to Benoît.
>
> But, I don't think that's necessary. It should be easy to set at runtime
> just doing a strcmp on the powerdomain during the device init,
> omap_device_build phase.
It shouldn't be part of .dev_attr, since it's not a IP block-specific
attribute, it's a powerdomain-specific attribute. The same hwmod
structure might be used on another OMAP chip that places the device in a
different powerdomain.
It would also be good to avoid doing strcmp()s here. The powerdomain name
string, like any name string, should basically be opaque to code.
In this case, the best approach is probably for the subarch integration
code to ask the powerdomain code whether the hwmod's powerdomain can ever
lose context. I just posted a patch series to do this[1], so I'd suggest
you use the function that it exports.
- Paul
1. http://marc.info/?l=linux-omap&m=129955064112024&w=2
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO
2011-03-08 2:25 ` Paul Walmsley
@ 2011-03-08 5:45 ` Varadarajan, Charulatha
0 siblings, 0 replies; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-08 5:45 UTC (permalink / raw)
To: Paul Walmsley; +Cc: Kevin Hilman, Benoit Cousson, linux-omap, tony
Paul, Kevin,
Thanks.
On Tue, Mar 8, 2011 at 07:55, Paul Walmsley <paul@pwsan.com> wrote:
> Hi
>
> On Mon, 7 Mar 2011, Kevin Hilman wrote:
>
>> "Varadarajan, Charulatha" <charu@ti.com> writes:
>>
>> > On Sat, Mar 5, 2011 at 02:21, Kevin Hilman <khilman@ti.com> wrote:
>> >> Charulatha V <charu@ti.com> writes:
>> >>>
>> >>> + if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
>> >>> + continue;
>> >>> +
>> >>
>> >> This adds a string compare for every bank during every idle
>> >> transistion (and every resume.) That's a lot of unneeded overhead.
>> >>
>> >> I'd rather see a per-bank flag 'looses_context' or something that can be
>> >> checked more efficiently in this fast path. This flag can be set based
>> >> on the powerdomain name in the device init code.
>> >
>> > This looks better. Will do the needful.
>> > One question, can "looses_context" be made as part of dev_attr?
>>
>> I guess that's up to Benoît.
>>
>> But, I don't think that's necessary. It should be easy to set at runtime
>> just doing a strcmp on the powerdomain during the device init,
>> omap_device_build phase.
>
> It shouldn't be part of .dev_attr, since it's not a IP block-specific
> attribute, it's a powerdomain-specific attribute. The same hwmod
> structure might be used on another OMAP chip that places the device in a
> different powerdomain.
>
> It would also be good to avoid doing strcmp()s here. The powerdomain name
> string, like any name string, should basically be opaque to code.
>
> In this case, the best approach is probably for the subarch integration
> code to ask the powerdomain code whether the hwmod's powerdomain can ever
> lose context. I just posted a patch series to do this[1], so I'd suggest
> you use the function that it exports.
Will do the needful.
- V Charulatha
>
>
> - Paul
>
> 1. http://marc.info/?l=linux-omap&m=129955064112024&w=2
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] OMAP4: GPIO: save/restore context
2011-02-28 10:57 [PATCH 0/5] OMAP: GPIO: use PM runtime framework Charulatha V
2011-02-28 10:57 ` [PATCH 1/5] OMAP: GPIO: make gpio_context part of gpio_bank structure Charulatha V
2011-02-28 10:57 ` [PATCH 2/5] OMAP: GPIO: use pwrdmn name to find wkup dmn GPIO Charulatha V
@ 2011-02-28 10:57 ` Charulatha V
2011-02-28 10:57 ` [PATCH 4/5] OMAP: GPIO: call save/restore ctxt from GPIO driver Charulatha V
2011-02-28 10:57 ` [PATCH 5/5] OMAP: GPIO: use PM runtime framework Charulatha V
4 siblings, 0 replies; 20+ messages in thread
From: Charulatha V @ 2011-02-28 10:57 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, tony, paul, Charulatha V
Modify the omap_gpio_save/restore_context to support OMAP4
architecture so that the GPIO driver need not be modified
when OMAP4 off mode support is available.
Signed-off-by: Charulatha V <charu@ti.com>
Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
(2430-SDP testing)
---
arch/arm/plat-omap/gpio.c | 131 +++++++++++++++++++++++++++++---------------
1 files changed, 86 insertions(+), 45 deletions(-)
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index ff341ec..1da3233 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -2031,43 +2031,62 @@ void omap2_gpio_resume_after_idle(void)
#endif
-#ifdef CONFIG_ARCH_OMAP3
-/* save the registers of bank 2-6 */
void omap_gpio_save_context(void)
{
int i;
- /* saving banks from 2-6 only since GPIO1 is in WKUP */
for (i = 0; i < gpio_bank_count; i++) {
struct gpio_bank *bank = &gpio_bank[i];
if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
continue;
- bank->context.irqenable1 =
- __raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
- bank->context.irqenable2 =
- __raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE2);
- bank->context.wake_en =
- __raw_readl(bank->base + OMAP24XX_GPIO_WAKE_EN);
- bank->context.ctrl =
- __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
- bank->context.oe =
- __raw_readl(bank->base + OMAP24XX_GPIO_OE);
- bank->context.leveldetect0 =
- __raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT0);
- bank->context.leveldetect1 =
- __raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT1);
- bank->context.risingdetect =
- __raw_readl(bank->base + OMAP24XX_GPIO_RISINGDETECT);
- bank->context.fallingdetect =
- __raw_readl(bank->base + OMAP24XX_GPIO_FALLINGDETECT);
- bank->context.dataout =
- __raw_readl(bank->base + OMAP24XX_GPIO_DATAOUT);
+ if (bank->method == METHOD_GPIO_24XX) {
+ bank->context.irqenable1 = __raw_readl(
+ bank->base + OMAP24XX_GPIO_IRQENABLE1);
+ bank->context.irqenable2 = __raw_readl(
+ bank->base + OMAP24XX_GPIO_IRQENABLE2);
+ bank->context.wake_en = __raw_readl(
+ bank->base + OMAP24XX_GPIO_WAKE_EN);
+ bank->context.ctrl = __raw_readl(
+ bank->base + OMAP24XX_GPIO_CTRL);
+ bank->context.oe = __raw_readl(
+ bank->base + OMAP24XX_GPIO_OE);
+ bank->context.leveldetect0 = __raw_readl(bank->base +
+ OMAP24XX_GPIO_LEVELDETECT0);
+ bank->context.leveldetect1 = __raw_readl(bank->base +
+ OMAP24XX_GPIO_LEVELDETECT1);
+ bank->context.risingdetect = __raw_readl(bank->base +
+ OMAP24XX_GPIO_RISINGDETECT);
+ bank->context.fallingdetect = __raw_readl(bank->base +
+ OMAP24XX_GPIO_FALLINGDETECT);
+ bank->context.dataout = __raw_readl(
+ bank->base + OMAP24XX_GPIO_DATAOUT);
+ } else if (bank->method == METHOD_GPIO_44XX) {
+ bank->context.irqenable1 = __raw_readl(
+ bank->base + OMAP4_GPIO_IRQSTATUSSET0);
+ bank->context.irqenable2 = __raw_readl(
+ bank->base + OMAP4_GPIO_IRQSTATUSSET1);
+ bank->context.wake_en = __raw_readl(
+ bank->base + OMAP4_GPIO_IRQWAKEN0);
+ bank->context.ctrl = __raw_readl(
+ bank->base + OMAP4_GPIO_CTRL);
+ bank->context.oe = __raw_readl(
+ bank->base + OMAP24XX_GPIO_OE);
+ bank->context.leveldetect0 = __raw_readl(bank->base +
+ OMAP4_GPIO_LEVELDETECT0);
+ bank->context.leveldetect1 = __raw_readl(bank->base +
+ OMAP4_GPIO_LEVELDETECT1);
+ bank->context.risingdetect = __raw_readl(bank->base +
+ OMAP4_GPIO_RISINGDETECT);
+ bank->context.fallingdetect = __raw_readl(bank->base +
+ OMAP4_GPIO_FALLINGDETECT);
+ bank->context.dataout = __raw_readl(
+ bank->base + OMAP4_GPIO_DATAOUT);
+ }
}
}
-/* restore the required registers of bank 2-6 */
void omap_gpio_restore_context(void)
{
int i;
@@ -2078,29 +2097,51 @@ void omap_gpio_restore_context(void)
if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
continue;
- __raw_writel(bank->context.irqenable1,
- bank->base + OMAP24XX_GPIO_IRQENABLE1);
- __raw_writel(bank->context.irqenable2,
- bank->base + OMAP24XX_GPIO_IRQENABLE2);
- __raw_writel(bank->context.wake_en,
- bank->base + OMAP24XX_GPIO_WAKE_EN);
- __raw_writel(bank->context.ctrl,
- bank->base + OMAP24XX_GPIO_CTRL);
- __raw_writel(bank->context.oe,
- bank->base + OMAP24XX_GPIO_OE);
- __raw_writel(bank->context.leveldetect0,
- bank->base + OMAP24XX_GPIO_LEVELDETECT0);
- __raw_writel(bank->context.leveldetect1,
- bank->base + OMAP24XX_GPIO_LEVELDETECT1);
- __raw_writel(bank->context.risingdetect,
- bank->base + OMAP24XX_GPIO_RISINGDETECT);
- __raw_writel(bank->context.fallingdetect,
- bank->base + OMAP24XX_GPIO_FALLINGDETECT);
- __raw_writel(bank->context.dataout,
- bank->base + OMAP24XX_GPIO_DATAOUT);
+ if (bank->method == METHOD_GPIO_24XX) {
+ __raw_writel(bank->context.irqenable1, bank->base +
+ OMAP24XX_GPIO_IRQENABLE1);
+ __raw_writel(bank->context.irqenable2, bank->base +
+ OMAP24XX_GPIO_IRQENABLE2);
+ __raw_writel(bank->context.wake_en, bank->base +
+ OMAP24XX_GPIO_WAKE_EN);
+ __raw_writel(bank->context.ctrl, bank->base +
+ OMAP24XX_GPIO_CTRL);
+ __raw_writel(bank->context.oe, bank->base +
+ OMAP24XX_GPIO_OE);
+ __raw_writel(bank->context.leveldetect0, bank->base +
+ OMAP24XX_GPIO_LEVELDETECT0);
+ __raw_writel(bank->context.leveldetect1, bank->base +
+ OMAP24XX_GPIO_LEVELDETECT1);
+ __raw_writel(bank->context.risingdetect, bank->base +
+ OMAP24XX_GPIO_RISINGDETECT);
+ __raw_writel(bank->context.fallingdetect, bank->base +
+ OMAP24XX_GPIO_FALLINGDETECT);
+ __raw_writel(bank->context.dataout, bank->base +
+ OMAP24XX_GPIO_DATAOUT);
+ } else if (bank->method == METHOD_GPIO_44XX) {
+ __raw_writel(bank->context.irqenable1, bank->base +
+ OMAP4_GPIO_IRQSTATUSSET0);
+ __raw_writel(bank->context.irqenable2, bank->base +
+ OMAP4_GPIO_IRQSTATUSSET1);
+ __raw_writel(bank->context.wake_en, bank->base +
+ OMAP4_GPIO_IRQWAKEN0);
+ __raw_writel(bank->context.ctrl, bank->base +
+ OMAP4_GPIO_CTRL);
+ __raw_writel(bank->context.oe, bank->base +
+ OMAP24XX_GPIO_OE);
+ __raw_writel(bank->context.leveldetect0, bank->base +
+ OMAP4_GPIO_LEVELDETECT0);
+ __raw_writel(bank->context.leveldetect1, bank->base +
+ OMAP4_GPIO_LEVELDETECT1);
+ __raw_writel(bank->context.risingdetect, bank->base +
+ OMAP4_GPIO_RISINGDETECT);
+ __raw_writel(bank->context.fallingdetect, bank->base +
+ OMAP4_GPIO_FALLINGDETECT);
+ __raw_writel(bank->context.dataout, bank->base +
+ OMAP4_GPIO_DATAOUT);
+ }
}
}
-#endif
static struct platform_driver omap_gpio_driver = {
.probe = omap_gpio_probe,
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 4/5] OMAP: GPIO: call save/restore ctxt from GPIO driver
2011-02-28 10:57 [PATCH 0/5] OMAP: GPIO: use PM runtime framework Charulatha V
` (2 preceding siblings ...)
2011-02-28 10:57 ` [PATCH 3/5] OMAP4: GPIO: save/restore context Charulatha V
@ 2011-02-28 10:57 ` Charulatha V
2011-03-04 22:20 ` Kevin Hilman
2011-02-28 10:57 ` [PATCH 5/5] OMAP: GPIO: use PM runtime framework Charulatha V
4 siblings, 1 reply; 20+ messages in thread
From: Charulatha V @ 2011-02-28 10:57 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, tony, paul, Charulatha V
Make gpio_prepare_for_idle() & gpio_resume_after_idle() functions
handle save context & restore context respectively in OMAP GPIO driver
instead of calling these functions directly from pm layer. This would
be useful while modifying the OMAP GPIO driver to use PM runtime framework.
Also modfiy gpio_prepare_for_idle() & gpio_resume_after_idle()
to do nothing if none of the GPIOs in a bank is being used.
Also remove usage of cpu_is_* checks from the above mentioned
functions
Signed-off-by: Charulatha V <charu@ti.com>
Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
(2430-SDP testing)
---
arch/arm/mach-omap2/pm24xx.c | 2 +-
arch/arm/mach-omap2/pm34xx.c | 20 +---
arch/arm/plat-omap/gpio.c | 218 ++++++++++++++++----------------
arch/arm/plat-omap/include/plat/gpio.h | 4 +-
4 files changed, 113 insertions(+), 131 deletions(-)
diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
index 97feb3a..b71072d 100644
--- a/arch/arm/mach-omap2/pm24xx.c
+++ b/arch/arm/mach-omap2/pm24xx.c
@@ -162,7 +162,7 @@ no_sleep:
tmp = timespec_to_ns(&ts_idle) * NSEC_PER_USEC;
omap2_pm_dump(0, 1, tmp);
}
- omap2_gpio_resume_after_idle();
+ omap2_gpio_resume_after_idle(0);
clk_enable(osc_ck);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 2f864e4..07af07e 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -90,16 +90,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
static struct powerdomain *core_pwrdm, *per_pwrdm;
static struct powerdomain *cam_pwrdm;
-static inline void omap3_per_save_context(void)
-{
- omap_gpio_save_context();
-}
-
-static inline void omap3_per_restore_context(void)
-{
- omap_gpio_restore_context();
-}
-
static void omap3_enable_io_chain(void)
{
int timeout = 0;
@@ -408,8 +398,6 @@ void omap_sram_idle(void)
omap_uart_prepare_idle(2);
omap_uart_prepare_idle(3);
omap2_gpio_prepare_for_idle(per_going_off);
- if (per_next_state == PWRDM_POWER_OFF)
- omap3_per_save_context();
}
/* CORE */
@@ -473,10 +461,12 @@ void omap_sram_idle(void)
/* PER */
if (per_next_state < PWRDM_POWER_ON) {
+ int is_per_prev_state_off;
+
per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
- omap2_gpio_resume_after_idle();
- if (per_prev_state == PWRDM_POWER_OFF)
- omap3_per_restore_context();
+ is_per_prev_state_off = (per_prev_state ==
+ PWRDM_POWER_OFF) ? 1 : 0;
+ omap2_gpio_resume_after_idle(is_per_prev_state_off);
omap_uart_resume_idle(2);
omap_uart_resume_idle(3);
}
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 1da3233..10792b6 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -175,6 +175,9 @@ struct gpio_bank {
const char *pwrdm_name;
};
+static void omap_gpio_save_context(struct gpio_bank *bank);
+static void omap_gpio_restore_context(struct gpio_bank *bank);
+
/*
* TODO: Cleanup gpio_bank usage as it is having information
* related to all instances of the device
@@ -1860,8 +1863,6 @@ static struct sys_device omap_gpio_device = {
#endif
-#ifdef CONFIG_ARCH_OMAP2PLUS
-
static int workaround_enabled;
void omap2_gpio_prepare_for_idle(int off_mode)
@@ -1873,6 +1874,10 @@ void omap2_gpio_prepare_for_idle(int off_mode)
u32 l1 = 0, l2 = 0;
int j;
+ /* If the gpio bank is not used, do nothing */
+ if (!bank->mod_usage)
+ continue;
+
if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
continue;
@@ -1882,22 +1887,22 @@ void omap2_gpio_prepare_for_idle(int off_mode)
if (!off_mode)
continue;
- /* If going to OFF, remove triggering for all
+ /*
+ * If going to OFF, remove triggering for all
* non-wakeup GPIOs. Otherwise spurious IRQs will be
- * generated. See OMAP2420 Errata item 1.101. */
+ * generated. See OMAP2420 Errata item 1.101.
+ */
if (!(bank->enabled_non_wakeup_gpios))
- continue;
+ goto save_gpio_ctx;
- if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+ if (bank->method == METHOD_GPIO_24XX) {
bank->saved_datain = __raw_readl(bank->base +
OMAP24XX_GPIO_DATAIN);
l1 = __raw_readl(bank->base +
OMAP24XX_GPIO_FALLINGDETECT);
l2 = __raw_readl(bank->base +
OMAP24XX_GPIO_RISINGDETECT);
- }
-
- if (cpu_is_omap44xx()) {
+ } else if (bank->method == METHOD_GPIO_44XX) {
bank->saved_datain = __raw_readl(bank->base +
OMAP4_GPIO_DATAIN);
l1 = __raw_readl(bank->base +
@@ -1911,19 +1916,20 @@ void omap2_gpio_prepare_for_idle(int off_mode)
l1 &= ~bank->enabled_non_wakeup_gpios;
l2 &= ~bank->enabled_non_wakeup_gpios;
- if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+ if (bank->method == METHOD_GPIO_24XX) {
__raw_writel(l1, bank->base +
OMAP24XX_GPIO_FALLINGDETECT);
__raw_writel(l2, bank->base +
OMAP24XX_GPIO_RISINGDETECT);
- }
-
- if (cpu_is_omap44xx()) {
+ } else if (bank->method == METHOD_GPIO_44XX) {
__raw_writel(l1, bank->base + OMAP4_GPIO_FALLINGDETECT);
__raw_writel(l2, bank->base + OMAP4_GPIO_RISINGDETECT);
}
c++;
+
+save_gpio_ctx:
+ omap_gpio_save_context(bank);
}
if (!c) {
workaround_enabled = 0;
@@ -1932,7 +1938,7 @@ void omap2_gpio_prepare_for_idle(int off_mode)
workaround_enabled = 1;
}
-void omap2_gpio_resume_after_idle(void)
+void omap2_gpio_resume_after_idle(int off_mode)
{
int i;
@@ -1941,27 +1947,32 @@ void omap2_gpio_resume_after_idle(void)
u32 l = 0, gen, gen0, gen1;
int j;
+ /* If the gpio bank is not used, do nothing */
+ if (!bank->mod_usage)
+ continue;
+
if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
continue;
for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
clk_enable(bank->dbck);
- if (!workaround_enabled)
+ if (!off_mode)
continue;
+ if (!workaround_enabled)
+ goto restore_gpio_ctx;
+
if (!(bank->enabled_non_wakeup_gpios))
- continue;
+ goto restore_gpio_ctx;
- if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+ if (bank->method == METHOD_GPIO_24XX) {
__raw_writel(bank->saved_fallingdetect,
bank->base + OMAP24XX_GPIO_FALLINGDETECT);
__raw_writel(bank->saved_risingdetect,
bank->base + OMAP24XX_GPIO_RISINGDETECT);
l = __raw_readl(bank->base + OMAP24XX_GPIO_DATAIN);
- }
-
- if (cpu_is_omap44xx()) {
+ } else if (bank->method == METHOD_GPIO_44XX) {
__raw_writel(bank->saved_fallingdetect,
bank->base + OMAP4_GPIO_FALLINGDETECT);
__raw_writel(bank->saved_risingdetect,
@@ -1969,10 +1980,12 @@ void omap2_gpio_resume_after_idle(void)
l = __raw_readl(bank->base + OMAP4_GPIO_DATAIN);
}
- /* Check if any of the non-wakeup interrupt GPIOs have changed
+ /*
+ * Check if any of the non-wakeup interrupt GPIOs have changed
* state. If so, generate an IRQ by software. This is
* horribly racy, but it's the best we can do to work around
- * this silicon bug. */
+ * this silicon bug.
+ */
l ^= bank->saved_datain;
l &= bank->enabled_non_wakeup_gpios;
@@ -1995,7 +2008,7 @@ void omap2_gpio_resume_after_idle(void)
if (gen) {
u32 old0, old1;
- if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+ if (bank->method == METHOD_GPIO_24XX) {
old0 = __raw_readl(bank->base +
OMAP24XX_GPIO_LEVELDETECT0);
old1 = __raw_readl(bank->base +
@@ -2008,9 +2021,7 @@ void omap2_gpio_resume_after_idle(void)
OMAP24XX_GPIO_LEVELDETECT0);
__raw_writel(old1, bank->base +
OMAP24XX_GPIO_LEVELDETECT1);
- }
-
- if (cpu_is_omap44xx()) {
+ } else if (bank->method == METHOD_GPIO_44XX) {
old0 = __raw_readl(bank->base +
OMAP4_GPIO_LEVELDETECT0);
old1 = __raw_readl(bank->base +
@@ -2025,121 +2036,104 @@ void omap2_gpio_resume_after_idle(void)
OMAP4_GPIO_LEVELDETECT1);
}
}
+
+restore_gpio_ctx:
+ omap_gpio_restore_context(bank);
}
}
-#endif
-
-void omap_gpio_save_context(void)
+void omap_gpio_save_context(struct gpio_bank *bank)
{
- int i;
-
- for (i = 0; i < gpio_bank_count; i++) {
- struct gpio_bank *bank = &gpio_bank[i];
-
- if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
- continue;
-
- if (bank->method == METHOD_GPIO_24XX) {
- bank->context.irqenable1 = __raw_readl(
+ if (bank->method == METHOD_GPIO_24XX) {
+ bank->context.irqenable1 = __raw_readl(
bank->base + OMAP24XX_GPIO_IRQENABLE1);
- bank->context.irqenable2 = __raw_readl(
+ bank->context.irqenable2 = __raw_readl(
bank->base + OMAP24XX_GPIO_IRQENABLE2);
- bank->context.wake_en = __raw_readl(
+ bank->context.wake_en = __raw_readl(
bank->base + OMAP24XX_GPIO_WAKE_EN);
- bank->context.ctrl = __raw_readl(
+ bank->context.ctrl = __raw_readl(
bank->base + OMAP24XX_GPIO_CTRL);
- bank->context.oe = __raw_readl(
+ bank->context.oe = __raw_readl(
bank->base + OMAP24XX_GPIO_OE);
- bank->context.leveldetect0 = __raw_readl(bank->base +
+ bank->context.leveldetect0 = __raw_readl(bank->base +
OMAP24XX_GPIO_LEVELDETECT0);
- bank->context.leveldetect1 = __raw_readl(bank->base +
+ bank->context.leveldetect1 = __raw_readl(bank->base +
OMAP24XX_GPIO_LEVELDETECT1);
- bank->context.risingdetect = __raw_readl(bank->base +
+ bank->context.risingdetect = __raw_readl(bank->base +
OMAP24XX_GPIO_RISINGDETECT);
- bank->context.fallingdetect = __raw_readl(bank->base +
+ bank->context.fallingdetect = __raw_readl(bank->base +
OMAP24XX_GPIO_FALLINGDETECT);
- bank->context.dataout = __raw_readl(
+ bank->context.dataout = __raw_readl(
bank->base + OMAP24XX_GPIO_DATAOUT);
- } else if (bank->method == METHOD_GPIO_44XX) {
- bank->context.irqenable1 = __raw_readl(
+ } else if (bank->method == METHOD_GPIO_44XX) {
+ bank->context.irqenable1 = __raw_readl(
bank->base + OMAP4_GPIO_IRQSTATUSSET0);
- bank->context.irqenable2 = __raw_readl(
+ bank->context.irqenable2 = __raw_readl(
bank->base + OMAP4_GPIO_IRQSTATUSSET1);
- bank->context.wake_en = __raw_readl(
+ bank->context.wake_en = __raw_readl(
bank->base + OMAP4_GPIO_IRQWAKEN0);
- bank->context.ctrl = __raw_readl(
+ bank->context.ctrl = __raw_readl(
bank->base + OMAP4_GPIO_CTRL);
- bank->context.oe = __raw_readl(
+ bank->context.oe = __raw_readl(
bank->base + OMAP24XX_GPIO_OE);
- bank->context.leveldetect0 = __raw_readl(bank->base +
+ bank->context.leveldetect0 = __raw_readl(bank->base +
OMAP4_GPIO_LEVELDETECT0);
- bank->context.leveldetect1 = __raw_readl(bank->base +
+ bank->context.leveldetect1 = __raw_readl(bank->base +
OMAP4_GPIO_LEVELDETECT1);
- bank->context.risingdetect = __raw_readl(bank->base +
+ bank->context.risingdetect = __raw_readl(bank->base +
OMAP4_GPIO_RISINGDETECT);
- bank->context.fallingdetect = __raw_readl(bank->base +
+ bank->context.fallingdetect = __raw_readl(bank->base +
OMAP4_GPIO_FALLINGDETECT);
- bank->context.dataout = __raw_readl(
+ bank->context.dataout = __raw_readl(
bank->base + OMAP4_GPIO_DATAOUT);
- }
}
}
-void omap_gpio_restore_context(void)
+void omap_gpio_restore_context(struct gpio_bank *bank)
{
- int i;
-
- for (i = 0; i < gpio_bank_count; i++) {
- struct gpio_bank *bank = &gpio_bank[i];
-
- if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
- continue;
-
- if (bank->method == METHOD_GPIO_24XX) {
- __raw_writel(bank->context.irqenable1, bank->base +
- OMAP24XX_GPIO_IRQENABLE1);
- __raw_writel(bank->context.irqenable2, bank->base +
- OMAP24XX_GPIO_IRQENABLE2);
- __raw_writel(bank->context.wake_en, bank->base +
- OMAP24XX_GPIO_WAKE_EN);
- __raw_writel(bank->context.ctrl, bank->base +
- OMAP24XX_GPIO_CTRL);
- __raw_writel(bank->context.oe, bank->base +
- OMAP24XX_GPIO_OE);
- __raw_writel(bank->context.leveldetect0, bank->base +
- OMAP24XX_GPIO_LEVELDETECT0);
- __raw_writel(bank->context.leveldetect1, bank->base +
- OMAP24XX_GPIO_LEVELDETECT1);
- __raw_writel(bank->context.risingdetect, bank->base +
- OMAP24XX_GPIO_RISINGDETECT);
- __raw_writel(bank->context.fallingdetect, bank->base +
- OMAP24XX_GPIO_FALLINGDETECT);
- __raw_writel(bank->context.dataout, bank->base +
- OMAP24XX_GPIO_DATAOUT);
- } else if (bank->method == METHOD_GPIO_44XX) {
- __raw_writel(bank->context.irqenable1, bank->base +
- OMAP4_GPIO_IRQSTATUSSET0);
- __raw_writel(bank->context.irqenable2, bank->base +
- OMAP4_GPIO_IRQSTATUSSET1);
- __raw_writel(bank->context.wake_en, bank->base +
- OMAP4_GPIO_IRQWAKEN0);
- __raw_writel(bank->context.ctrl, bank->base +
- OMAP4_GPIO_CTRL);
- __raw_writel(bank->context.oe, bank->base +
- OMAP24XX_GPIO_OE);
- __raw_writel(bank->context.leveldetect0, bank->base +
- OMAP4_GPIO_LEVELDETECT0);
- __raw_writel(bank->context.leveldetect1, bank->base +
- OMAP4_GPIO_LEVELDETECT1);
- __raw_writel(bank->context.risingdetect, bank->base +
- OMAP4_GPIO_RISINGDETECT);
- __raw_writel(bank->context.fallingdetect, bank->base +
- OMAP4_GPIO_FALLINGDETECT);
- __raw_writel(bank->context.dataout, bank->base +
- OMAP4_GPIO_DATAOUT);
- }
+ if (bank->method == METHOD_GPIO_24XX) {
+ __raw_writel(bank->context.irqenable1, bank->base +
+ OMAP24XX_GPIO_IRQENABLE1);
+ __raw_writel(bank->context.irqenable2, bank->base +
+ OMAP24XX_GPIO_IRQENABLE2);
+ __raw_writel(bank->context.wake_en, bank->base +
+ OMAP24XX_GPIO_WAKE_EN);
+ __raw_writel(bank->context.ctrl, bank->base +
+ OMAP24XX_GPIO_CTRL);
+ __raw_writel(bank->context.oe, bank->base +
+ OMAP24XX_GPIO_OE);
+ __raw_writel(bank->context.leveldetect0, bank->base +
+ OMAP24XX_GPIO_LEVELDETECT0);
+ __raw_writel(bank->context.leveldetect1, bank->base +
+ OMAP24XX_GPIO_LEVELDETECT1);
+ __raw_writel(bank->context.risingdetect, bank->base +
+ OMAP24XX_GPIO_RISINGDETECT);
+ __raw_writel(bank->context.fallingdetect, bank->base +
+ OMAP24XX_GPIO_FALLINGDETECT);
+ __raw_writel(bank->context.dataout, bank->base +
+ OMAP24XX_GPIO_DATAOUT);
+ } else if (bank->method == METHOD_GPIO_44XX) {
+ __raw_writel(bank->context.irqenable1, bank->base +
+ OMAP4_GPIO_IRQSTATUSSET0);
+ __raw_writel(bank->context.irqenable2, bank->base +
+ OMAP4_GPIO_IRQSTATUSSET1);
+ __raw_writel(bank->context.wake_en, bank->base +
+ OMAP4_GPIO_IRQWAKEN0);
+ __raw_writel(bank->context.ctrl, bank->base +
+ OMAP4_GPIO_CTRL);
+ __raw_writel(bank->context.oe, bank->base +
+ OMAP24XX_GPIO_OE);
+ __raw_writel(bank->context.leveldetect0, bank->base +
+ OMAP4_GPIO_LEVELDETECT0);
+ __raw_writel(bank->context.leveldetect1, bank->base +
+ OMAP4_GPIO_LEVELDETECT1);
+ __raw_writel(bank->context.risingdetect, bank->base +
+ OMAP4_GPIO_RISINGDETECT);
+ __raw_writel(bank->context.fallingdetect, bank->base +
+ OMAP4_GPIO_FALLINGDETECT);
+ __raw_writel(bank->context.dataout, bank->base +
+ OMAP4_GPIO_DATAOUT);
}
}
diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h
index 2c46caa..00a9d02 100644
--- a/arch/arm/plat-omap/include/plat/gpio.h
+++ b/arch/arm/plat-omap/include/plat/gpio.h
@@ -84,11 +84,9 @@ struct omap_gpio_platform_data {
extern int gpio_bank_count;
extern void omap2_gpio_prepare_for_idle(int off_mode);
-extern void omap2_gpio_resume_after_idle(void);
+extern void omap2_gpio_resume_after_idle(int off_mode);
extern void omap_set_gpio_debounce(int gpio, int enable);
extern void omap_set_gpio_debounce_time(int gpio, int enable);
-extern void omap_gpio_save_context(void);
-extern void omap_gpio_restore_context(void);
/*-------------------------------------------------------------------------*/
/* Wrappers for "new style" GPIO calls, using the new infrastructure
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] OMAP: GPIO: call save/restore ctxt from GPIO driver
2011-02-28 10:57 ` [PATCH 4/5] OMAP: GPIO: call save/restore ctxt from GPIO driver Charulatha V
@ 2011-03-04 22:20 ` Kevin Hilman
2011-03-05 5:15 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hilman @ 2011-03-04 22:20 UTC (permalink / raw)
To: Charulatha V; +Cc: linux-omap, tony, paul
Charulatha V <charu@ti.com> writes:
> Make gpio_prepare_for_idle() & gpio_resume_after_idle() functions
> handle save context & restore context respectively in OMAP GPIO driver
> instead of calling these functions directly from pm layer. This would
> be useful while modifying the OMAP GPIO driver to use PM runtime framework.
Excellent!
I really like this change, but have some minor issues with the
implementation.
Basically, you should no longer need to manually read PER prev state.
Instead, in prepare_for_idle(), call
omap_device_get_context_loss_count(), in resume_from_idle() call it
again. If the count is different, you need to restore context.
This way, you don't need to modify the resume_from_idle function, *and*
you can get rid of the read of PER prev state.
Otherwise, I like this change.
Kevin
> Also modfiy gpio_prepare_for_idle() & gpio_resume_after_idle()
> to do nothing if none of the GPIOs in a bank is being used.
>
> Also remove usage of cpu_is_* checks from the above mentioned
> functions
> Signed-off-by: Charulatha V <charu@ti.com>
>
> Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> (2430-SDP testing)
> ---
> arch/arm/mach-omap2/pm24xx.c | 2 +-
> arch/arm/mach-omap2/pm34xx.c | 20 +---
> arch/arm/plat-omap/gpio.c | 218 ++++++++++++++++----------------
> arch/arm/plat-omap/include/plat/gpio.h | 4 +-
> 4 files changed, 113 insertions(+), 131 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
> index 97feb3a..b71072d 100644
> --- a/arch/arm/mach-omap2/pm24xx.c
> +++ b/arch/arm/mach-omap2/pm24xx.c
> @@ -162,7 +162,7 @@ no_sleep:
> tmp = timespec_to_ns(&ts_idle) * NSEC_PER_USEC;
> omap2_pm_dump(0, 1, tmp);
> }
> - omap2_gpio_resume_after_idle();
> + omap2_gpio_resume_after_idle(0);
>
> clk_enable(osc_ck);
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 2f864e4..07af07e 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -90,16 +90,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
> static struct powerdomain *core_pwrdm, *per_pwrdm;
> static struct powerdomain *cam_pwrdm;
>
> -static inline void omap3_per_save_context(void)
> -{
> - omap_gpio_save_context();
> -}
> -
> -static inline void omap3_per_restore_context(void)
> -{
> - omap_gpio_restore_context();
> -}
> -
> static void omap3_enable_io_chain(void)
> {
> int timeout = 0;
> @@ -408,8 +398,6 @@ void omap_sram_idle(void)
> omap_uart_prepare_idle(2);
> omap_uart_prepare_idle(3);
> omap2_gpio_prepare_for_idle(per_going_off);
> - if (per_next_state == PWRDM_POWER_OFF)
> - omap3_per_save_context();
> }
>
> /* CORE */
> @@ -473,10 +461,12 @@ void omap_sram_idle(void)
>
> /* PER */
> if (per_next_state < PWRDM_POWER_ON) {
> + int is_per_prev_state_off;
> +
> per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
> - omap2_gpio_resume_after_idle();
> - if (per_prev_state == PWRDM_POWER_OFF)
> - omap3_per_restore_context();
> + is_per_prev_state_off = (per_prev_state ==
> + PWRDM_POWER_OFF) ? 1 : 0;
> + omap2_gpio_resume_after_idle(is_per_prev_state_off);
> omap_uart_resume_idle(2);
> omap_uart_resume_idle(3);
> }
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 1da3233..10792b6 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -175,6 +175,9 @@ struct gpio_bank {
> const char *pwrdm_name;
> };
>
> +static void omap_gpio_save_context(struct gpio_bank *bank);
> +static void omap_gpio_restore_context(struct gpio_bank *bank);
> +
> /*
> * TODO: Cleanup gpio_bank usage as it is having information
> * related to all instances of the device
> @@ -1860,8 +1863,6 @@ static struct sys_device omap_gpio_device = {
>
> #endif
>
> -#ifdef CONFIG_ARCH_OMAP2PLUS
> -
Why remove this? This will just be bloat for OMAP1-only builds.
> static int workaround_enabled;
>
> void omap2_gpio_prepare_for_idle(int off_mode)
> @@ -1873,6 +1874,10 @@ void omap2_gpio_prepare_for_idle(int off_mode)
> u32 l1 = 0, l2 = 0;
> int j;
>
> + /* If the gpio bank is not used, do nothing */
> + if (!bank->mod_usage)
> + continue;
> +
> if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> continue;
>
> @@ -1882,22 +1887,22 @@ void omap2_gpio_prepare_for_idle(int off_mode)
> if (!off_mode)
> continue;
>
> - /* If going to OFF, remove triggering for all
> + /*
> + * If going to OFF, remove triggering for all
> * non-wakeup GPIOs. Otherwise spurious IRQs will be
> - * generated. See OMAP2420 Errata item 1.101. */
> + * generated. See OMAP2420 Errata item 1.101.
> + */
> if (!(bank->enabled_non_wakeup_gpios))
> - continue;
> + goto save_gpio_ctx;
>
> - if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
> + if (bank->method == METHOD_GPIO_24XX) {
> bank->saved_datain = __raw_readl(bank->base +
> OMAP24XX_GPIO_DATAIN);
> l1 = __raw_readl(bank->base +
> OMAP24XX_GPIO_FALLINGDETECT);
> l2 = __raw_readl(bank->base +
> OMAP24XX_GPIO_RISINGDETECT);
> - }
> -
> - if (cpu_is_omap44xx()) {
> + } else if (bank->method == METHOD_GPIO_44XX) {
> bank->saved_datain = __raw_readl(bank->base +
> OMAP4_GPIO_DATAIN);
> l1 = __raw_readl(bank->base +
> @@ -1911,19 +1916,20 @@ void omap2_gpio_prepare_for_idle(int off_mode)
> l1 &= ~bank->enabled_non_wakeup_gpios;
> l2 &= ~bank->enabled_non_wakeup_gpios;
>
> - if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
> + if (bank->method == METHOD_GPIO_24XX) {
> __raw_writel(l1, bank->base +
> OMAP24XX_GPIO_FALLINGDETECT);
> __raw_writel(l2, bank->base +
> OMAP24XX_GPIO_RISINGDETECT);
> - }
> -
> - if (cpu_is_omap44xx()) {
> + } else if (bank->method == METHOD_GPIO_44XX) {
> __raw_writel(l1, bank->base + OMAP4_GPIO_FALLINGDETECT);
> __raw_writel(l2, bank->base + OMAP4_GPIO_RISINGDETECT);
> }
>
> c++;
> +
> +save_gpio_ctx:
> + omap_gpio_save_context(bank);
> }
> if (!c) {
> workaround_enabled = 0;
> @@ -1932,7 +1938,7 @@ void omap2_gpio_prepare_for_idle(int off_mode)
> workaround_enabled = 1;
> }
>
> -void omap2_gpio_resume_after_idle(void)
> +void omap2_gpio_resume_after_idle(int off_mode)
> {
> int i;
>
> @@ -1941,27 +1947,32 @@ void omap2_gpio_resume_after_idle(void)
> u32 l = 0, gen, gen0, gen1;
> int j;
>
> + /* If the gpio bank is not used, do nothing */
> + if (!bank->mod_usage)
> + continue;
> +
> if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> continue;
>
> for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
> clk_enable(bank->dbck);
>
> - if (!workaround_enabled)
> + if (!off_mode)
> continue;
>
> + if (!workaround_enabled)
> + goto restore_gpio_ctx;
> +
> if (!(bank->enabled_non_wakeup_gpios))
> - continue;
> + goto restore_gpio_ctx;
>
> - if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
> + if (bank->method == METHOD_GPIO_24XX) {
> __raw_writel(bank->saved_fallingdetect,
> bank->base + OMAP24XX_GPIO_FALLINGDETECT);
> __raw_writel(bank->saved_risingdetect,
> bank->base + OMAP24XX_GPIO_RISINGDETECT);
> l = __raw_readl(bank->base + OMAP24XX_GPIO_DATAIN);
> - }
> -
> - if (cpu_is_omap44xx()) {
> + } else if (bank->method == METHOD_GPIO_44XX) {
> __raw_writel(bank->saved_fallingdetect,
> bank->base + OMAP4_GPIO_FALLINGDETECT);
> __raw_writel(bank->saved_risingdetect,
> @@ -1969,10 +1980,12 @@ void omap2_gpio_resume_after_idle(void)
> l = __raw_readl(bank->base + OMAP4_GPIO_DATAIN);
> }
>
> - /* Check if any of the non-wakeup interrupt GPIOs have changed
> + /*
> + * Check if any of the non-wakeup interrupt GPIOs have changed
> * state. If so, generate an IRQ by software. This is
> * horribly racy, but it's the best we can do to work around
> - * this silicon bug. */
> + * this silicon bug.
> + */
> l ^= bank->saved_datain;
> l &= bank->enabled_non_wakeup_gpios;
>
> @@ -1995,7 +2008,7 @@ void omap2_gpio_resume_after_idle(void)
> if (gen) {
> u32 old0, old1;
>
> - if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
> + if (bank->method == METHOD_GPIO_24XX) {
> old0 = __raw_readl(bank->base +
> OMAP24XX_GPIO_LEVELDETECT0);
> old1 = __raw_readl(bank->base +
> @@ -2008,9 +2021,7 @@ void omap2_gpio_resume_after_idle(void)
> OMAP24XX_GPIO_LEVELDETECT0);
> __raw_writel(old1, bank->base +
> OMAP24XX_GPIO_LEVELDETECT1);
> - }
> -
> - if (cpu_is_omap44xx()) {
> + } else if (bank->method == METHOD_GPIO_44XX) {
> old0 = __raw_readl(bank->base +
> OMAP4_GPIO_LEVELDETECT0);
> old1 = __raw_readl(bank->base +
> @@ -2025,121 +2036,104 @@ void omap2_gpio_resume_after_idle(void)
> OMAP4_GPIO_LEVELDETECT1);
> }
> }
> +
> +restore_gpio_ctx:
> + omap_gpio_restore_context(bank);
> }
>
> }
>
> -#endif
> -
> -void omap_gpio_save_context(void)
> +void omap_gpio_save_context(struct gpio_bank *bank)
> {
> - int i;
> -
> - for (i = 0; i < gpio_bank_count; i++) {
> - struct gpio_bank *bank = &gpio_bank[i];
> -
> - if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> - continue;
> -
> - if (bank->method == METHOD_GPIO_24XX) {
> - bank->context.irqenable1 = __raw_readl(
> + if (bank->method == METHOD_GPIO_24XX) {
> + bank->context.irqenable1 = __raw_readl(
> bank->base + OMAP24XX_GPIO_IRQENABLE1);
> - bank->context.irqenable2 = __raw_readl(
> + bank->context.irqenable2 = __raw_readl(
> bank->base + OMAP24XX_GPIO_IRQENABLE2);
> - bank->context.wake_en = __raw_readl(
> + bank->context.wake_en = __raw_readl(
> bank->base + OMAP24XX_GPIO_WAKE_EN);
> - bank->context.ctrl = __raw_readl(
> + bank->context.ctrl = __raw_readl(
> bank->base + OMAP24XX_GPIO_CTRL);
> - bank->context.oe = __raw_readl(
> + bank->context.oe = __raw_readl(
> bank->base + OMAP24XX_GPIO_OE);
> - bank->context.leveldetect0 = __raw_readl(bank->base +
> + bank->context.leveldetect0 = __raw_readl(bank->base +
> OMAP24XX_GPIO_LEVELDETECT0);
> - bank->context.leveldetect1 = __raw_readl(bank->base +
> + bank->context.leveldetect1 = __raw_readl(bank->base +
> OMAP24XX_GPIO_LEVELDETECT1);
> - bank->context.risingdetect = __raw_readl(bank->base +
> + bank->context.risingdetect = __raw_readl(bank->base +
> OMAP24XX_GPIO_RISINGDETECT);
> - bank->context.fallingdetect = __raw_readl(bank->base +
> + bank->context.fallingdetect = __raw_readl(bank->base +
> OMAP24XX_GPIO_FALLINGDETECT);
> - bank->context.dataout = __raw_readl(
> + bank->context.dataout = __raw_readl(
> bank->base + OMAP24XX_GPIO_DATAOUT);
> - } else if (bank->method == METHOD_GPIO_44XX) {
> - bank->context.irqenable1 = __raw_readl(
> + } else if (bank->method == METHOD_GPIO_44XX) {
> + bank->context.irqenable1 = __raw_readl(
> bank->base + OMAP4_GPIO_IRQSTATUSSET0);
> - bank->context.irqenable2 = __raw_readl(
> + bank->context.irqenable2 = __raw_readl(
> bank->base + OMAP4_GPIO_IRQSTATUSSET1);
> - bank->context.wake_en = __raw_readl(
> + bank->context.wake_en = __raw_readl(
> bank->base + OMAP4_GPIO_IRQWAKEN0);
> - bank->context.ctrl = __raw_readl(
> + bank->context.ctrl = __raw_readl(
> bank->base + OMAP4_GPIO_CTRL);
> - bank->context.oe = __raw_readl(
> + bank->context.oe = __raw_readl(
> bank->base + OMAP24XX_GPIO_OE);
> - bank->context.leveldetect0 = __raw_readl(bank->base +
> + bank->context.leveldetect0 = __raw_readl(bank->base +
> OMAP4_GPIO_LEVELDETECT0);
> - bank->context.leveldetect1 = __raw_readl(bank->base +
> + bank->context.leveldetect1 = __raw_readl(bank->base +
> OMAP4_GPIO_LEVELDETECT1);
> - bank->context.risingdetect = __raw_readl(bank->base +
> + bank->context.risingdetect = __raw_readl(bank->base +
> OMAP4_GPIO_RISINGDETECT);
> - bank->context.fallingdetect = __raw_readl(bank->base +
> + bank->context.fallingdetect = __raw_readl(bank->base +
> OMAP4_GPIO_FALLINGDETECT);
> - bank->context.dataout = __raw_readl(
> + bank->context.dataout = __raw_readl(
> bank->base + OMAP4_GPIO_DATAOUT);
> - }
> }
> }
>
> -void omap_gpio_restore_context(void)
> +void omap_gpio_restore_context(struct gpio_bank *bank)
> {
> - int i;
> -
> - for (i = 0; i < gpio_bank_count; i++) {
> - struct gpio_bank *bank = &gpio_bank[i];
> -
> - if (!strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> - continue;
> -
> - if (bank->method == METHOD_GPIO_24XX) {
> - __raw_writel(bank->context.irqenable1, bank->base +
> - OMAP24XX_GPIO_IRQENABLE1);
> - __raw_writel(bank->context.irqenable2, bank->base +
> - OMAP24XX_GPIO_IRQENABLE2);
> - __raw_writel(bank->context.wake_en, bank->base +
> - OMAP24XX_GPIO_WAKE_EN);
> - __raw_writel(bank->context.ctrl, bank->base +
> - OMAP24XX_GPIO_CTRL);
> - __raw_writel(bank->context.oe, bank->base +
> - OMAP24XX_GPIO_OE);
> - __raw_writel(bank->context.leveldetect0, bank->base +
> - OMAP24XX_GPIO_LEVELDETECT0);
> - __raw_writel(bank->context.leveldetect1, bank->base +
> - OMAP24XX_GPIO_LEVELDETECT1);
> - __raw_writel(bank->context.risingdetect, bank->base +
> - OMAP24XX_GPIO_RISINGDETECT);
> - __raw_writel(bank->context.fallingdetect, bank->base +
> - OMAP24XX_GPIO_FALLINGDETECT);
> - __raw_writel(bank->context.dataout, bank->base +
> - OMAP24XX_GPIO_DATAOUT);
> - } else if (bank->method == METHOD_GPIO_44XX) {
> - __raw_writel(bank->context.irqenable1, bank->base +
> - OMAP4_GPIO_IRQSTATUSSET0);
> - __raw_writel(bank->context.irqenable2, bank->base +
> - OMAP4_GPIO_IRQSTATUSSET1);
> - __raw_writel(bank->context.wake_en, bank->base +
> - OMAP4_GPIO_IRQWAKEN0);
> - __raw_writel(bank->context.ctrl, bank->base +
> - OMAP4_GPIO_CTRL);
> - __raw_writel(bank->context.oe, bank->base +
> - OMAP24XX_GPIO_OE);
> - __raw_writel(bank->context.leveldetect0, bank->base +
> - OMAP4_GPIO_LEVELDETECT0);
> - __raw_writel(bank->context.leveldetect1, bank->base +
> - OMAP4_GPIO_LEVELDETECT1);
> - __raw_writel(bank->context.risingdetect, bank->base +
> - OMAP4_GPIO_RISINGDETECT);
> - __raw_writel(bank->context.fallingdetect, bank->base +
> - OMAP4_GPIO_FALLINGDETECT);
> - __raw_writel(bank->context.dataout, bank->base +
> - OMAP4_GPIO_DATAOUT);
> - }
> + if (bank->method == METHOD_GPIO_24XX) {
> + __raw_writel(bank->context.irqenable1, bank->base +
> + OMAP24XX_GPIO_IRQENABLE1);
> + __raw_writel(bank->context.irqenable2, bank->base +
> + OMAP24XX_GPIO_IRQENABLE2);
> + __raw_writel(bank->context.wake_en, bank->base +
> + OMAP24XX_GPIO_WAKE_EN);
> + __raw_writel(bank->context.ctrl, bank->base +
> + OMAP24XX_GPIO_CTRL);
> + __raw_writel(bank->context.oe, bank->base +
> + OMAP24XX_GPIO_OE);
> + __raw_writel(bank->context.leveldetect0, bank->base +
> + OMAP24XX_GPIO_LEVELDETECT0);
> + __raw_writel(bank->context.leveldetect1, bank->base +
> + OMAP24XX_GPIO_LEVELDETECT1);
> + __raw_writel(bank->context.risingdetect, bank->base +
> + OMAP24XX_GPIO_RISINGDETECT);
> + __raw_writel(bank->context.fallingdetect, bank->base +
> + OMAP24XX_GPIO_FALLINGDETECT);
> + __raw_writel(bank->context.dataout, bank->base +
> + OMAP24XX_GPIO_DATAOUT);
> + } else if (bank->method == METHOD_GPIO_44XX) {
> + __raw_writel(bank->context.irqenable1, bank->base +
> + OMAP4_GPIO_IRQSTATUSSET0);
> + __raw_writel(bank->context.irqenable2, bank->base +
> + OMAP4_GPIO_IRQSTATUSSET1);
> + __raw_writel(bank->context.wake_en, bank->base +
> + OMAP4_GPIO_IRQWAKEN0);
> + __raw_writel(bank->context.ctrl, bank->base +
> + OMAP4_GPIO_CTRL);
> + __raw_writel(bank->context.oe, bank->base +
> + OMAP24XX_GPIO_OE);
> + __raw_writel(bank->context.leveldetect0, bank->base +
> + OMAP4_GPIO_LEVELDETECT0);
> + __raw_writel(bank->context.leveldetect1, bank->base +
> + OMAP4_GPIO_LEVELDETECT1);
> + __raw_writel(bank->context.risingdetect, bank->base +
> + OMAP4_GPIO_RISINGDETECT);
> + __raw_writel(bank->context.fallingdetect, bank->base +
> + OMAP4_GPIO_FALLINGDETECT);
> + __raw_writel(bank->context.dataout, bank->base +
> + OMAP4_GPIO_DATAOUT);
> }
> }
>
> diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h
> index 2c46caa..00a9d02 100644
> --- a/arch/arm/plat-omap/include/plat/gpio.h
> +++ b/arch/arm/plat-omap/include/plat/gpio.h
> @@ -84,11 +84,9 @@ struct omap_gpio_platform_data {
> extern int gpio_bank_count;
>
> extern void omap2_gpio_prepare_for_idle(int off_mode);
> -extern void omap2_gpio_resume_after_idle(void);
> +extern void omap2_gpio_resume_after_idle(int off_mode);
> extern void omap_set_gpio_debounce(int gpio, int enable);
> extern void omap_set_gpio_debounce_time(int gpio, int enable);
> -extern void omap_gpio_save_context(void);
> -extern void omap_gpio_restore_context(void);
> /*-------------------------------------------------------------------------*/
>
> /* Wrappers for "new style" GPIO calls, using the new infrastructure
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] OMAP: GPIO: call save/restore ctxt from GPIO driver
2011-03-04 22:20 ` Kevin Hilman
@ 2011-03-05 5:15 ` Varadarajan, Charulatha
0 siblings, 0 replies; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-05 5:15 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, tony, paul
On Sat, Mar 5, 2011 at 02:50, Kevin Hilman <khilman@ti.com> wrote:
> Charulatha V <charu@ti.com> writes:
>
>> Make gpio_prepare_for_idle() & gpio_resume_after_idle() functions
>> handle save context & restore context respectively in OMAP GPIO driver
>> instead of calling these functions directly from pm layer. This would
>> be useful while modifying the OMAP GPIO driver to use PM runtime framework.
>
> Excellent!
Thanks.
>
> I really like this change, but have some minor issues with the
> implementation.
>
> Basically, you should no longer need to manually read PER prev state.
> Instead, in prepare_for_idle(), call
> omap_device_get_context_loss_count(), in resume_from_idle() call it
> again. If the count is different, you need to restore context.
Okay, will do this way.
>
> This way, you don't need to modify the resume_from_idle function, *and*
> you can get rid of the read of PER prev state.
>
> Otherwise, I like this change.
>
> Kevin
>
>> Also modfiy gpio_prepare_for_idle() & gpio_resume_after_idle()
>> to do nothing if none of the GPIOs in a bank is being used.
>>
>> Also remove usage of cpu_is_* checks from the above mentioned
>> functions
>
>> Signed-off-by: Charulatha V <charu@ti.com>
>>
>> Tested-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> (2430-SDP testing)
>> ---
>> arch/arm/mach-omap2/pm24xx.c | 2 +-
>> arch/arm/mach-omap2/pm34xx.c | 20 +---
>> arch/arm/plat-omap/gpio.c | 218 ++++++++++++++++----------------
>> arch/arm/plat-omap/include/plat/gpio.h | 4 +-
>> 4 files changed, 113 insertions(+), 131 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
>> index 97feb3a..b71072d 100644
>> --- a/arch/arm/mach-omap2/pm24xx.c
>> +++ b/arch/arm/mach-omap2/pm24xx.c
>> @@ -162,7 +162,7 @@ no_sleep:
>> tmp = timespec_to_ns(&ts_idle) * NSEC_PER_USEC;
>> omap2_pm_dump(0, 1, tmp);
>> }
>> - omap2_gpio_resume_after_idle();
>> + omap2_gpio_resume_after_idle(0);
>>
>> clk_enable(osc_ck);
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index 2f864e4..07af07e 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -90,16 +90,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
>> static struct powerdomain *core_pwrdm, *per_pwrdm;
>> static struct powerdomain *cam_pwrdm;
>>
>> -static inline void omap3_per_save_context(void)
>> -{
>> - omap_gpio_save_context();
>> -}
>> -
>> -static inline void omap3_per_restore_context(void)
>> -{
>> - omap_gpio_restore_context();
>> -}
>> -
>> static void omap3_enable_io_chain(void)
>> {
>> int timeout = 0;
>> @@ -408,8 +398,6 @@ void omap_sram_idle(void)
>> omap_uart_prepare_idle(2);
>> omap_uart_prepare_idle(3);
>> omap2_gpio_prepare_for_idle(per_going_off);
>> - if (per_next_state == PWRDM_POWER_OFF)
>> - omap3_per_save_context();
>> }
>>
>> /* CORE */
>> @@ -473,10 +461,12 @@ void omap_sram_idle(void)
>>
>> /* PER */
>> if (per_next_state < PWRDM_POWER_ON) {
>> + int is_per_prev_state_off;
>> +
>> per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>> - omap2_gpio_resume_after_idle();
>> - if (per_prev_state == PWRDM_POWER_OFF)
>> - omap3_per_restore_context();
>> + is_per_prev_state_off = (per_prev_state ==
>> + PWRDM_POWER_OFF) ? 1 : 0;
>> + omap2_gpio_resume_after_idle(is_per_prev_state_off);
>> omap_uart_resume_idle(2);
>> omap_uart_resume_idle(3);
>> }
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index 1da3233..10792b6 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -175,6 +175,9 @@ struct gpio_bank {
>> const char *pwrdm_name;
>> };
>>
>> +static void omap_gpio_save_context(struct gpio_bank *bank);
>> +static void omap_gpio_restore_context(struct gpio_bank *bank);
>> +
>> /*
>> * TODO: Cleanup gpio_bank usage as it is having information
>> * related to all instances of the device
>> @@ -1860,8 +1863,6 @@ static struct sys_device omap_gpio_device = {
>>
>> #endif
>>
>> -#ifdef CONFIG_ARCH_OMAP2PLUS
>> -
>
> Why remove this? This will just be bloat for OMAP1-only builds.
Just thought of removing all CONFIG_ARCH_* #ifdefs. Anyways, I will
not retain this check in my next version.
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-02-28 10:57 [PATCH 0/5] OMAP: GPIO: use PM runtime framework Charulatha V
` (3 preceding siblings ...)
2011-02-28 10:57 ` [PATCH 4/5] OMAP: GPIO: call save/restore ctxt from GPIO driver Charulatha V
@ 2011-02-28 10:57 ` Charulatha V
2011-03-04 22:59 ` Kevin Hilman
4 siblings, 1 reply; 20+ messages in thread
From: Charulatha V @ 2011-02-28 10:57 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, tony, paul, Charulatha V, Tarun Kanti DebBarma
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 <charu@ti.com>
Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
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 &= 0xFFFFFFFE;
- __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 &= ~(1 << offset);
- 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 disabled, clocks are gated */
- ctrl |= 1;
- __raw_writel(ctrl, reg);
- }
+ bank->mod_usage &= ~(1 << offset);
+ if (!bank->mod_usage) {
+ void __iomem *reg = bank->base;
+ u32 ctrl;
+
+ if (bank->method == METHOD_GPIO_24XX)
+ reg += OMAP24XX_GPIO_CTRL;
+ else if (bank->method == METHOD_GPIO_44XX)
+ reg += OMAP4_GPIO_CTRL;
+ else
+ goto reset_gpio;
+
+ ctrl = __raw_readl(reg);
+ /* Module is disabled, clocks are gated */
+ ctrl |= 1;
+ __raw_writel(ctrl, reg);
}
+reset_gpio:
_reset_gpio(bank, bank->chip.base + offset);
spin_unlock_irqrestore(&bank->lock, flags);
+
+ /*
+ * If this is the last gpio to be freed in the bank,
+ * disable the bank module
+ */
+ if (!bank->mod_usage) {
+ if (unlikely(pm_runtime_put(bank->dev) < 0)) {
+ struct platform_device *pdev =
+ to_platform_device(bank->dev);
+ dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
+ "failed\n", __func__, pdev->id);
+ }
+ }
}
/*
@@ -1143,10 +1164,17 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
struct gpio_bank *bank;
u32 retrigger = 0;
int unmasked = 0;
+ int enable_gpio = 0;
desc->irq_data.chip->irq_ack(&desc->irq_data);
bank = get_irq_data(irq);
+
+ if (pm_runtime_suspended(bank->dev)) {
+ if (unlikely(pm_runtime_get_sync(bank->dev) == 0))
+ enable_gpio = 1;
+ }
+
#ifdef CONFIG_ARCH_OMAP1
if (bank->method == METHOD_MPUIO)
isr_reg = bank->base +
@@ -1238,6 +1266,9 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
exit:
if (!unmasked)
desc->irq_data.chip->irq_unmask(&desc->irq_data);
+
+ if (enable_gpio)
+ pm_runtime_put(bank->dev);
}
static void gpio_irq_shutdown(struct irq_data *d)
@@ -1742,126 +1773,121 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
}
pm_runtime_enable(bank->dev);
- pm_runtime_get_sync(bank->dev);
+ pm_runtime_irq_safe(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__, id);
+ iounmap(bank->base);
+ return -EINVAL;
+ }
- omap_gpio_mod_init(bank, id);
omap_gpio_chip_init(bank);
omap_gpio_show_rev(bank);
+ if (unlikely(pm_runtime_put(bank->dev) < 0)) {
+ dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
+ "failed\n", __func__, id);
+ iounmap(bank->base);
+ return -EINVAL;
+ }
+
if (!gpio_init_done)
gpio_init_done = 1;
return 0;
}
-#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
-static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
+static int omap_gpio_suspend(struct device *dev)
{
- int i;
+ struct platform_device *pdev = to_platform_device(dev);
+ void __iomem *wake_status;
+ void __iomem *wake_clear;
+ void __iomem *wake_set;
+ unsigned long flags;
+ struct gpio_bank *bank = &gpio_bank[pdev->id];
- if (!cpu_class_is_omap2() && !cpu_is_omap16xx())
+ /* If the gpio bank is not used, do nothing */
+ if (!bank->mod_usage)
return 0;
- for (i = 0; i < gpio_bank_count; i++) {
- struct gpio_bank *bank = &gpio_bank[i];
- void __iomem *wake_status;
- void __iomem *wake_clear;
- void __iomem *wake_set;
- unsigned long flags;
+ switch (bank->method) {
+ case METHOD_GPIO_1610:
+ wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
+ wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
+ wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
+ break;
+ case METHOD_GPIO_24XX:
+ wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
+ wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
+ wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
+ break;
+ case METHOD_GPIO_44XX:
+ wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
+ wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
+ wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
+ break;
+ default:
+ return 0;
+ }
- switch (bank->method) {
-#ifdef CONFIG_ARCH_OMAP16XX
- case METHOD_GPIO_1610:
- wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
- wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
- wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
- break;
-#endif
-#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
- case METHOD_GPIO_24XX:
- wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
- wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
- wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
- break;
-#endif
-#ifdef CONFIG_ARCH_OMAP4
- case METHOD_GPIO_44XX:
- wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
- wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
- wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
- break;
-#endif
- default:
- continue;
- }
+ if (strcmp(bank->pwrdm_name, "wkup_pwrdm"))
+ omap_gpio_save_context(bank);
- spin_lock_irqsave(&bank->lock, flags);
- bank->saved_wakeup = __raw_readl(wake_status);
- __raw_writel(0xffffffff, wake_clear);
- __raw_writel(bank->suspend_wakeup, wake_set);
- spin_unlock_irqrestore(&bank->lock, flags);
- }
+ spin_lock_irqsave(&bank->lock, flags);
+ bank->saved_wakeup = __raw_readl(wake_status);
+ __raw_writel(0xffffffff, wake_clear);
+ __raw_writel(bank->suspend_wakeup, wake_set);
+ spin_unlock_irqrestore(&bank->lock, flags);
+
+ if (unlikely(pm_runtime_put(bank->dev) < 0))
+ return -EINVAL;
return 0;
}
-static int omap_gpio_resume(struct sys_device *dev)
+static int omap_gpio_resume(struct device *dev)
{
- int i;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = &gpio_bank[pdev->id];
+ void __iomem *wake_clear;
+ void __iomem *wake_set;
+ unsigned long flags;
- if (!cpu_class_is_omap2() && !cpu_is_omap16xx())
+ /* If the gpio bank is not used, do nothing */
+ if (!bank->mod_usage)
return 0;
- for (i = 0; i < gpio_bank_count; i++) {
- struct gpio_bank *bank = &gpio_bank[i];
- void __iomem *wake_clear;
- void __iomem *wake_set;
- unsigned long flags;
-
- switch (bank->method) {
-#ifdef CONFIG_ARCH_OMAP16XX
- case METHOD_GPIO_1610:
- wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
- wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
- break;
-#endif
-#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
- case METHOD_GPIO_24XX:
- wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
- wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
- break;
-#endif
-#ifdef CONFIG_ARCH_OMAP4
- case METHOD_GPIO_44XX:
- wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
- wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
- break;
-#endif
- default:
- continue;
- }
-
- spin_lock_irqsave(&bank->lock, flags);
- __raw_writel(0xffffffff, wake_clear);
- __raw_writel(bank->saved_wakeup, wake_set);
- spin_unlock_irqrestore(&bank->lock, flags);
+ switch (bank->method) {
+ case METHOD_GPIO_1610:
+ wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
+ wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
+ break;
+ case METHOD_GPIO_24XX:
+ wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
+ wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
+ break;
+ case METHOD_GPIO_44XX:
+ wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
+ wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
+ break;
+ default:
+ return 0;
}
- return 0;
-}
+ if (unlikely(pm_runtime_get_sync(bank->dev) < 0))
+ return -EINVAL;
-static struct sysdev_class omap_gpio_sysclass = {
- .name = "gpio",
- .suspend = omap_gpio_suspend,
- .resume = omap_gpio_resume,
-};
+ spin_lock_irqsave(&bank->lock, flags);
+ __raw_writel(0xffffffff, wake_clear);
+ __raw_writel(bank->saved_wakeup, wake_set);
+ spin_unlock_irqrestore(&bank->lock, flags);
-static struct sys_device omap_gpio_device = {
- .id = 0,
- .cls = &omap_gpio_sysclass,
-};
+ if (strcmp(bank->pwrdm_name, "wkup_pwrdm"))
+ omap_gpio_restore_context(bank);
-#endif
+ return 0;
+}
static int workaround_enabled;
@@ -1885,7 +1911,7 @@ void omap2_gpio_prepare_for_idle(int off_mode)
clk_disable(bank->dbck);
if (!off_mode)
- continue;
+ goto disable_gpio_bank;
/*
* If going to OFF, remove triggering for all
@@ -1930,6 +1956,10 @@ void omap2_gpio_prepare_for_idle(int off_mode)
save_gpio_ctx:
omap_gpio_save_context(bank);
+disable_gpio_bank:
+ if (unlikely(pm_runtime_put(bank->dev) < 0))
+ dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
+ "failed\n", __func__, i);
}
if (!c) {
workaround_enabled = 0;
@@ -1957,6 +1987,13 @@ void omap2_gpio_resume_after_idle(int off_mode)
for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
clk_enable(bank->dbck);
+ if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
+ dev_err(bank->dev, "%s: GPIO bank %d "
+ "pm_runtime_get_sync failed\n",
+ __func__, i);
+ continue;
+ }
+
if (!off_mode)
continue;
@@ -2040,7 +2077,6 @@ void omap2_gpio_resume_after_idle(int off_mode)
restore_gpio_ctx:
omap_gpio_restore_context(bank);
}
-
}
void omap_gpio_save_context(struct gpio_bank *bank)
@@ -2137,10 +2173,16 @@ void omap_gpio_restore_context(struct gpio_bank *bank)
}
}
+static const struct dev_pm_ops gpio_pm_ops = {
+ .suspend = omap_gpio_suspend,
+ .resume = omap_gpio_resume,
+};
+
static struct platform_driver omap_gpio_driver = {
.probe = omap_gpio_probe,
.driver = {
.name = "omap_gpio",
+ .pm = &gpio_pm_ops,
},
};
@@ -2157,21 +2199,8 @@ postcore_initcall(omap_gpio_drv_reg);
static int __init omap_gpio_sysinit(void)
{
- int ret = 0;
-
mpuio_init();
-
-#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
- if (cpu_is_omap16xx() || cpu_class_is_omap2()) {
- if (ret == 0) {
- ret = sysdev_class_register(&omap_gpio_sysclass);
- if (ret == 0)
- ret = sysdev_register(&omap_gpio_device);
- }
- }
-#endif
-
- return ret;
+ return 0;
}
arch_initcall(omap_gpio_sysinit);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-02-28 10:57 ` [PATCH 5/5] OMAP: GPIO: use PM runtime framework Charulatha V
@ 2011-03-04 22:59 ` Kevin Hilman
2011-03-05 5:10 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hilman @ 2011-03-04 22:59 UTC (permalink / raw)
To: Charulatha V; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
Charulatha V <charu@ti.com> 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 <charu@ti.com>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
> 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);
>
> + 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 &= 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.
This code also belongs in the ->runtime_resume() method so it happens
when the first GPIO in a bank is activated.
> +
> 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 &= ~(1 << offset);
> - 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 disabled, clocks are gated */
> - ctrl |= 1;
> - __raw_writel(ctrl, reg);
> - }
> + bank->mod_usage &= ~(1 << offset);
> + if (!bank->mod_usage) {
> + void __iomem *reg = bank->base;
> + u32 ctrl;
> +
> + if (bank->method == METHOD_GPIO_24XX)
> + reg += OMAP24XX_GPIO_CTRL;
> + else if (bank->method == METHOD_GPIO_44XX)
> + reg += OMAP4_GPIO_CTRL;
> + else
> + goto reset_gpio;
> +
> + ctrl = __raw_readl(reg);
> + /* Module is disabled, clocks are gated */
> + ctrl |= 1;
> + __raw_writel(ctrl, reg);
And here, rather than updating bank->mod_usage, just move this code
into a ->runtime_suspend hook which will then be called whenever the
bank is actually suspended.
> }
> +reset_gpio:
> _reset_gpio(bank, bank->chip.base + offset);
> spin_unlock_irqrestore(&bank->lock, flags);
> +
> + /*
> + * If this is the last gpio to be freed in the bank,
> + * disable the bank module
> + */
> + if (!bank->mod_usage) {
> + if (unlikely(pm_runtime_put(bank->dev) < 0)) {
> + struct platform_device *pdev =
> + to_platform_device(bank->dev);
> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
> + "failed\n", __func__, pdev->id);
> + }
> + }
and this can just be a pm_runtime_put(bank->dev)
> }
>
> /*
> @@ -1143,10 +1164,17 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> struct gpio_bank *bank;
> u32 retrigger = 0;
> int unmasked = 0;
> + int enable_gpio = 0;
>
> desc->irq_data.chip->irq_ack(&desc->irq_data);
>
> bank = get_irq_data(irq);
> +
> + if (pm_runtime_suspended(bank->dev)) {
Why do you need the check here?
If the device is already suspended, and you call _get_sync(), it
just increments the usecount, and returns.
> + if (unlikely(pm_runtime_get_sync(bank->dev) == 0))
> + enable_gpio = 1;
> + }
> +
> #ifdef CONFIG_ARCH_OMAP1
> if (bank->method == METHOD_MPUIO)
> isr_reg = bank->base +
> @@ -1238,6 +1266,9 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> exit:
> if (!unmasked)
> desc->irq_data.chip->irq_unmask(&desc->irq_data);
> +
> + if (enable_gpio)
> + pm_runtime_put(bank->dev);
Likewise, I think you could just always do a 'put' here without having
to have a flag.
> }
>
> static void gpio_irq_shutdown(struct irq_data *d)
> @@ -1742,126 +1773,121 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
> }
>
> pm_runtime_enable(bank->dev);
> - pm_runtime_get_sync(bank->dev);
> + pm_runtime_irq_safe(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__, id);
> + iounmap(bank->base);
> + return -EINVAL;
> + }
>
> - omap_gpio_mod_init(bank, id);
> omap_gpio_chip_init(bank);
> omap_gpio_show_rev(bank);
>
> + if (unlikely(pm_runtime_put(bank->dev) < 0)) {
use IS_ERR_VALUE() instead of '< 0'
> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
> + "failed\n", __func__, id);
> + iounmap(bank->base);
> + return -EINVAL;
> + }
> +
> if (!gpio_init_done)
> gpio_init_done = 1;
>
> return 0;
> }
>
> -#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
> -static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
> +static int omap_gpio_suspend(struct device *dev)
> {
> - int i;
> + struct platform_device *pdev = to_platform_device(dev);
> + void __iomem *wake_status;
> + void __iomem *wake_clear;
> + void __iomem *wake_set;
> + unsigned long flags;
> + struct gpio_bank *bank = &gpio_bank[pdev->id];
>
> - if (!cpu_class_is_omap2() && !cpu_is_omap16xx())
> + /* If the gpio bank is not used, do nothing */
> + if (!bank->mod_usage)
if (pm_runtime_suspended(bank-dev))
> return 0;
>
> - for (i = 0; i < gpio_bank_count; i++) {
> - struct gpio_bank *bank = &gpio_bank[i];
> - void __iomem *wake_status;
> - void __iomem *wake_clear;
> - void __iomem *wake_set;
> - unsigned long flags;
> + switch (bank->method) {
> + case METHOD_GPIO_1610:
> + wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
> + wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
> + wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
> + break;
> + case METHOD_GPIO_24XX:
> + wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
> + wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
> + wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
> + break;
> + case METHOD_GPIO_44XX:
> + wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
> + wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
> + wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
> + break;
> + default:
> + return 0;
> + }
>
> - switch (bank->method) {
> -#ifdef CONFIG_ARCH_OMAP16XX
> - case METHOD_GPIO_1610:
> - wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
> - wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
> - wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
> - break;
> -#endif
> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> - case METHOD_GPIO_24XX:
> - wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
> - wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
> - wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
> - break;
> -#endif
> -#ifdef CONFIG_ARCH_OMAP4
> - case METHOD_GPIO_44XX:
> - wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
> - wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
> - wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
> - break;
> -#endif
> - default:
> - continue;
> - }
> + if (strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> + omap_gpio_save_context(bank);
see comments on patch 2.
> - spin_lock_irqsave(&bank->lock, flags);
> - bank->saved_wakeup = __raw_readl(wake_status);
> - __raw_writel(0xffffffff, wake_clear);
> - __raw_writel(bank->suspend_wakeup, wake_set);
> - spin_unlock_irqrestore(&bank->lock, flags);
> - }
> + spin_lock_irqsave(&bank->lock, flags);
> + bank->saved_wakeup = __raw_readl(wake_status);
> + __raw_writel(0xffffffff, wake_clear);
> + __raw_writel(bank->suspend_wakeup, wake_set);
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + if (unlikely(pm_runtime_put(bank->dev) < 0))
> + return -EINVAL;
>
> return 0;
> }
>
> -static int omap_gpio_resume(struct sys_device *dev)
> +static int omap_gpio_resume(struct device *dev)
> {
> - int i;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct gpio_bank *bank = &gpio_bank[pdev->id];
> + void __iomem *wake_clear;
> + void __iomem *wake_set;
> + unsigned long flags;
>
> - if (!cpu_class_is_omap2() && !cpu_is_omap16xx())
> + /* If the gpio bank is not used, do nothing */
> + if (!bank->mod_usage)
if (pm_runtime_suspended(bank-dev))
> return 0;
>
> - for (i = 0; i < gpio_bank_count; i++) {
> - struct gpio_bank *bank = &gpio_bank[i];
> - void __iomem *wake_clear;
> - void __iomem *wake_set;
> - unsigned long flags;
> -
> - switch (bank->method) {
> -#ifdef CONFIG_ARCH_OMAP16XX
> - case METHOD_GPIO_1610:
> - wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
> - wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
> - break;
> -#endif
> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> - case METHOD_GPIO_24XX:
> - wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
> - wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
> - break;
> -#endif
> -#ifdef CONFIG_ARCH_OMAP4
> - case METHOD_GPIO_44XX:
> - wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
> - wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
> - break;
> -#endif
> - default:
> - continue;
> - }
> -
> - spin_lock_irqsave(&bank->lock, flags);
> - __raw_writel(0xffffffff, wake_clear);
> - __raw_writel(bank->saved_wakeup, wake_set);
> - spin_unlock_irqrestore(&bank->lock, flags);
> + switch (bank->method) {
> + case METHOD_GPIO_1610:
> + wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
> + wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
> + break;
> + case METHOD_GPIO_24XX:
> + wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
> + wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
> + break;
> + case METHOD_GPIO_44XX:
> + wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
> + wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
> + break;
> + default:
> + return 0;
> }
>
> - return 0;
> -}
> + if (unlikely(pm_runtime_get_sync(bank->dev) < 0))
> + return -EINVAL;
>
> -static struct sysdev_class omap_gpio_sysclass = {
> - .name = "gpio",
> - .suspend = omap_gpio_suspend,
> - .resume = omap_gpio_resume,
> -};
> + spin_lock_irqsave(&bank->lock, flags);
> + __raw_writel(0xffffffff, wake_clear);
> + __raw_writel(bank->saved_wakeup, wake_set);
> + spin_unlock_irqrestore(&bank->lock, flags);
>
> -static struct sys_device omap_gpio_device = {
> - .id = 0,
> - .cls = &omap_gpio_sysclass,
> -};
> + if (strcmp(bank->pwrdm_name, "wkup_pwrdm"))
> + omap_gpio_restore_context(bank);
>
> -#endif
> + return 0;
> +}
>
> static int workaround_enabled;
>
> @@ -1885,7 +1911,7 @@ void omap2_gpio_prepare_for_idle(int off_mode)
> clk_disable(bank->dbck);
>
> if (!off_mode)
> - continue;
> + goto disable_gpio_bank;
>
> /*
> * If going to OFF, remove triggering for all
> @@ -1930,6 +1956,10 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>
> save_gpio_ctx:
> omap_gpio_save_context(bank);
> +disable_gpio_bank:
> + if (unlikely(pm_runtime_put(bank->dev) < 0))
> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
> + "failed\n", __func__, i);
> }
> if (!c) {
> workaround_enabled = 0;
> @@ -1957,6 +1987,13 @@ void omap2_gpio_resume_after_idle(int off_mode)
> for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
> clk_enable(bank->dbck);
>
> + if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
> + dev_err(bank->dev, "%s: GPIO bank %d "
> + "pm_runtime_get_sync failed\n",
> + __func__, i);
> + continue;
> + }
> +
> if (!off_mode)
> continue;
>
> @@ -2040,7 +2077,6 @@ void omap2_gpio_resume_after_idle(int off_mode)
> restore_gpio_ctx:
> omap_gpio_restore_context(bank);
> }
> -
> }
>
> void omap_gpio_save_context(struct gpio_bank *bank)
> @@ -2137,10 +2173,16 @@ void omap_gpio_restore_context(struct gpio_bank *bank)
> }
> }
>
> +static const struct dev_pm_ops gpio_pm_ops = {
> + .suspend = omap_gpio_suspend,
> + .resume = omap_gpio_resume,
> +};
> +
> static struct platform_driver omap_gpio_driver = {
> .probe = omap_gpio_probe,
> .driver = {
> .name = "omap_gpio",
> + .pm = &gpio_pm_ops,
> },
> };
>
> @@ -2157,21 +2199,8 @@ postcore_initcall(omap_gpio_drv_reg);
>
> static int __init omap_gpio_sysinit(void)
> {
> - int ret = 0;
> -
> mpuio_init();
> -
> -#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
> - if (cpu_is_omap16xx() || cpu_class_is_omap2()) {
> - if (ret == 0) {
> - ret = sysdev_class_register(&omap_gpio_sysclass);
> - if (ret == 0)
> - ret = sysdev_register(&omap_gpio_device);
> - }
> - }
> -#endif
> -
> - return ret;
> + return 0;
> }
>
> arch_initcall(omap_gpio_sysinit);
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-03-04 22:59 ` Kevin Hilman
@ 2011-03-05 5:10 ` Varadarajan, Charulatha
2011-03-07 18:55 ` Kevin Hilman
0 siblings, 1 reply; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-05 5:10 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
Hi Kevin,
Thanks for the detailed review.
On Sat, Mar 5, 2011 at 03:29, Kevin Hilman <khilman@ti.com> wrote:
> Charulatha V <charu@ti.com> 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 <charu@ti.com>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> ---
>> 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.
The problem is only about the count mismatch I mentioned above.
>
>
>> 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 &= 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.
> This code also belongs in the ->runtime_resume() method so it happens
> when the first GPIO in a bank is activated.
Okay.
>
>> +
>> 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 &= ~(1 << offset);
>> - 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 disabled, clocks are gated */
>> - ctrl |= 1;
>> - __raw_writel(ctrl, reg);
>> - }
>> + bank->mod_usage &= ~(1 << offset);
>> + if (!bank->mod_usage) {
>> + void __iomem *reg = bank->base;
>> + u32 ctrl;
>> +
>> + if (bank->method == METHOD_GPIO_24XX)
>> + reg += OMAP24XX_GPIO_CTRL;
>> + else if (bank->method == METHOD_GPIO_44XX)
>> + reg += OMAP4_GPIO_CTRL;
>> + else
>> + goto reset_gpio;
>> +
>> + ctrl = __raw_readl(reg);
>> + /* Module is disabled, clocks are gated */
>> + ctrl |= 1;
>> + __raw_writel(ctrl, reg);
>
> And here, rather than updating bank->mod_usage, just move this code
> into a ->runtime_suspend hook which will then be called whenever the
> bank is actually suspended.
Pls see above explanation for bank->mod_usage.
>
>> }
>> +reset_gpio:
>> _reset_gpio(bank, bank->chip.base + offset);
>> spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> + /*
>> + * If this is the last gpio to be freed in the bank,
>> + * disable the bank module
>> + */
>> + if (!bank->mod_usage) {
>> + if (unlikely(pm_runtime_put(bank->dev) < 0)) {
>> + struct platform_device *pdev =
>> + to_platform_device(bank->dev);
>> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
>> + "failed\n", __func__, pdev->id);
>> + }
>> + }
>
> and this can just be a pm_runtime_put(bank->dev)
ditto.
>
>> }
>>
>> /*
>> @@ -1143,10 +1164,17 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> struct gpio_bank *bank;
>> u32 retrigger = 0;
>> int unmasked = 0;
>> + int enable_gpio = 0;
>>
>> desc->irq_data.chip->irq_ack(&desc->irq_data);
>>
>> bank = get_irq_data(irq);
>> +
>> + if (pm_runtime_suspended(bank->dev)) {
>
> Why do you need the check here?
>
> If the device is already suspended, and you call _get_sync(), it
> just increments the usecount, and returns.
Yes, you are right. I will remove pm_runtime_suspended() checks
and also the enable_gpio flag.
>
>> + if (unlikely(pm_runtime_get_sync(bank->dev) == 0))
>> + enable_gpio = 1;
>> + }
>> +
>> #ifdef CONFIG_ARCH_OMAP1
>> if (bank->method == METHOD_MPUIO)
>> isr_reg = bank->base +
>> @@ -1238,6 +1266,9 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> exit:
>> if (!unmasked)
>> desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> +
>> + if (enable_gpio)
>> + pm_runtime_put(bank->dev);
>
> Likewise, I think you could just always do a 'put' here without having
> to have a flag.
Sure, will change this.
>
>> }
>>
>> static void gpio_irq_shutdown(struct irq_data *d)
>> @@ -1742,126 +1773,121 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>> }
>>
>> pm_runtime_enable(bank->dev);
>> - pm_runtime_get_sync(bank->dev);
>> + pm_runtime_irq_safe(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__, id);
>> + iounmap(bank->base);
>> + return -EINVAL;
>> + }
>>
>> - omap_gpio_mod_init(bank, id);
>> omap_gpio_chip_init(bank);
>> omap_gpio_show_rev(bank);
>>
>> + if (unlikely(pm_runtime_put(bank->dev) < 0)) {
>
> use IS_ERR_VALUE() instead of '< 0'
Okay.
>
>> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
>> + "failed\n", __func__, id);
>> + iounmap(bank->base);
>> + return -EINVAL;
>> + }
>> +
>> if (!gpio_init_done)
>> gpio_init_done = 1;
>>
>> return 0;
>> }
>>
>> -#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
>> -static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
>> +static int omap_gpio_suspend(struct device *dev)
>> {
>> - int i;
>> + struct platform_device *pdev = to_platform_device(dev);
>> + void __iomem *wake_status;
>> + void __iomem *wake_clear;
>> + void __iomem *wake_set;
>> + unsigned long flags;
>> + struct gpio_bank *bank = &gpio_bank[pdev->id];
>>
>> - if (!cpu_class_is_omap2() && !cpu_is_omap16xx())
>> + /* If the gpio bank is not used, do nothing */
>> + if (!bank->mod_usage)
>
> if (pm_runtime_suspended(bank-dev))
>
>> return 0;
>>
>> - for (i = 0; i < gpio_bank_count; i++) {
>> - struct gpio_bank *bank = &gpio_bank[i];
>> - void __iomem *wake_status;
>> - void __iomem *wake_clear;
>> - void __iomem *wake_set;
>> - unsigned long flags;
>> + switch (bank->method) {
>> + case METHOD_GPIO_1610:
>> + wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
>> + wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> + wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
>> + break;
>> + case METHOD_GPIO_24XX:
>> + wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
>> + wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> + wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
>> + break;
>> + case METHOD_GPIO_44XX:
>> + wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> + wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> + wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> + break;
>> + default:
>> + return 0;
>> + }
>>
>> - switch (bank->method) {
>> -#ifdef CONFIG_ARCH_OMAP16XX
>> - case METHOD_GPIO_1610:
>> - wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
>> - wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> - wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
>> - break;
>> -#endif
>> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
>> - case METHOD_GPIO_24XX:
>> - wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
>> - wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> - wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
>> - break;
>> -#endif
>> -#ifdef CONFIG_ARCH_OMAP4
>> - case METHOD_GPIO_44XX:
>> - wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - break;
>> -#endif
>> - default:
>> - continue;
>> - }
>> + if (strcmp(bank->pwrdm_name, "wkup_pwrdm"))
>> + omap_gpio_save_context(bank);
>
> see comments on patch 2.
I agree.
>
>> - spin_lock_irqsave(&bank->lock, flags);
>> - bank->saved_wakeup = __raw_readl(wake_status);
>> - __raw_writel(0xffffffff, wake_clear);
>> - __raw_writel(bank->suspend_wakeup, wake_set);
>> - spin_unlock_irqrestore(&bank->lock, flags);
>> - }
>> + spin_lock_irqsave(&bank->lock, flags);
>> + bank->saved_wakeup = __raw_readl(wake_status);
>> + __raw_writel(0xffffffff, wake_clear);
>> + __raw_writel(bank->suspend_wakeup, wake_set);
>> + spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> + if (unlikely(pm_runtime_put(bank->dev) < 0))
>> + return -EINVAL;
>>
>> return 0;
>> }
>>
<<snip>>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-03-05 5:10 ` Varadarajan, Charulatha
@ 2011-03-07 18:55 ` Kevin Hilman
2011-03-08 15:05 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hilman @ 2011-03-07 18:55 UTC (permalink / raw)
To: Varadarajan, Charulatha; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
"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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-03-07 18:55 ` Kevin Hilman
@ 2011-03-08 15:05 ` Varadarajan, Charulatha
2011-03-08 18:23 ` Kevin Hilman
0 siblings, 1 reply; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-08 15:05 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
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
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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-03-08 15:05 ` Varadarajan, Charulatha
@ 2011-03-08 18:23 ` Kevin Hilman
2011-03-09 1:24 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hilman @ 2011-03-08 18:23 UTC (permalink / raw)
To: Varadarajan, Charulatha; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
"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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-03-08 18:23 ` Kevin Hilman
@ 2011-03-09 1:24 ` Varadarajan, Charulatha
2011-03-09 10:23 ` Varadarajan, Charulatha
0 siblings, 1 reply; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-09 1:24 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
Kevin,
On Tue, Mar 8, 2011 at 13:23, Kevin Hilman <khilman@ti.com> wrote:
> "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.
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, and save context (and do
> errata workarounds) accordingly.
Ok. I will modify the GPIO driver to access these APIs directly
from runtime_suspend hook of GPIO to read the next_state
of its powerdomain.
> 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.
Okay.
>
>> 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.
Thanks,
V Charulatha
<<snip>>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] OMAP: GPIO: use PM runtime framework
2011-03-09 1:24 ` Varadarajan, Charulatha
@ 2011-03-09 10:23 ` Varadarajan, Charulatha
0 siblings, 0 replies; 20+ messages in thread
From: Varadarajan, Charulatha @ 2011-03-09 10:23 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, tony, paul, Tarun Kanti DebBarma
Kevin,
On Wed, Mar 9, 2011 at 06:54, Varadarajan, Charulatha <charu@ti.com> wrote:
> On Tue, Mar 8, 2011 at 13:23, Kevin Hilman <khilman@ti.com> wrote:
>>> 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.
>
> 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, and save context (and do
>> errata workarounds) accordingly.
To read the next_state, the driver needs to be aware of the power
domain ptr and the power domain states. With this change, the
omap_gpio_runtime_suspend() would look like this:
{
...
...
pdev = to_platform_device(bank->dev);
od = to_omap_device(pdev);
pwrdm = omap_device_get_pwrdm (od);
/* or get the pwrdm via pdata during probe */
nxt_state = pwrdm_read_next_pwrst(pwrdm);
if (next_state == PWRDM_POWER_OFF) {
....
...
save_context();
}
....
....
}
IMO, the above doesn't look nice in a driver, as driver should not be
aware of power states. Also, a similar approach was taken in the
previous GPIO hwmod & PM runtime series and it was rejected.
Pls provide me some pointers on this.
Thanks,
V Charulatha
>
> Ok. I will modify the GPIO driver to access these APIs directly
> from runtime_suspend hook of GPIO to read the next_state
> of its powerdomain.
>
>> 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.
>
> Okay.
>
>>
>>> 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.
>
> Thanks,
> V Charulatha
>
> <<snip>>
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread