[PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

Pad wakeups are now enabled if the corresponding GPIO interrupt is enabled.

Applies on top of PM branch.

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
---
 arch/arm/plat-omap/gpio.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index e242112..fa79db2 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -267,6 +267,7 @@ static struct gpio_bank gpio_bank_34xx[6] = {
 #define OMAP34XX_PAD_SAFE_MODE 0x7
 #define OMAP34XX_PAD_IN_PU_GPIO 0x11c
 #define OMAP34XX_PAD_IN_PD_GPIO 0x10c
+#define OMAP34XX_PAD_WAKE_EN (1 << 14)
 
 struct omap3_gpio_regs {
 	u32 sysconfig;
@@ -713,6 +714,8 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 {
 	void __iomem *base = bank->base;
 	u32 gpio_bit = 1 << gpio;
+	struct gpio_pad *pad;
+	int gpio_num;
 	u32 val;
 
 	if (cpu_is_omap44xx()) {
@@ -750,6 +753,23 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 			 * GPIO wakeup request can only be generated on edge
 			 * transitions
 			 */
+			pad = gpio_pads;
+
+			gpio_num = bank->virtual_irq_start - IH_GPIO_BASE +
+				gpio;
+			/* Find the pad corresponding the GPIO */
+			while (pad->gpio >= 0 && pad->gpio != gpio_num)
+				pad++;
+			/* Enable / disable pad wakeup */
+			if (pad->gpio == gpio_num) {
+				val = omap_ctrl_readw(pad->offset);
+				if (trigger & IRQ_TYPE_EDGE_BOTH)
+					val |= OMAP34XX_PAD_WAKE_EN;
+				else
+					val &= ~(u16)OMAP34XX_PAD_WAKE_EN;
+				omap_ctrl_writew(val, pad->offset);
+			}
+
 			if (trigger & IRQ_TYPE_EDGE_BOTH)
 				__raw_writel(1 << gpio, bank->base
 					+ OMAP24XX_GPIO_SETWKUENA);
@@ -1654,7 +1674,7 @@ static int __init omap3_gpio_pads_init(void)
 	gpio_pads[gpio_amt].gpio = -1;
 	return 0;
 }
-late_initcall(omap3_gpio_pads_init);
+early_initcall(omap3_gpio_pads_init);
 #endif
 
 /* This lock class tells lockdep that GPIO irqs are in a different
-- 
1.5.4.3

Re: [PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Kevin Hilman]

Tero Kristo <tero.kristo@nokia.com> writes:

> From: Tero Kristo <tero.kristo@nokia.com>
>
> Pad wakeups are now enabled if the corresponding GPIO interrupt is enabled.
>
> Applies on top of PM branch.
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>

I just discovered this one in patchwork... sorry for the delay.

Changes in wakeup state should not be directly correlated to interrupt
enabled GPIOs.  Rather, this should only be done for GPIOs that are
explicitly wakeup enabled (via enable_irq_wake(), which in turn
calls gpio_wake_enable()).

A couple other minor comments below...

> ---
>  arch/arm/plat-omap/gpio.c |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index e242112..fa79db2 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -267,6 +267,7 @@ static struct gpio_bank gpio_bank_34xx[6] = {
>  #define OMAP34XX_PAD_SAFE_MODE 0x7
>  #define OMAP34XX_PAD_IN_PU_GPIO 0x11c
>  #define OMAP34XX_PAD_IN_PD_GPIO 0x10c
> +#define OMAP34XX_PAD_WAKE_EN (1 << 14)

Can use BIT(14) here

>  struct omap3_gpio_regs {
>  	u32 sysconfig;
> @@ -713,6 +714,8 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>  {
>  	void __iomem *base = bank->base;
>  	u32 gpio_bit = 1 << gpio;
> +	struct gpio_pad *pad;
> +	int gpio_num;
>  	u32 val;
>  
>  	if (cpu_is_omap44xx()) {
> @@ -750,6 +753,23 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>  			 * GPIO wakeup request can only be generated on edge
>  			 * transitions
>  			 */

The comment above needs to stay with the original code.

> +			pad = gpio_pads;
> +
> +			gpio_num = bank->virtual_irq_start - IH_GPIO_BASE +
> +				gpio;
> +			/* Find the pad corresponding the GPIO */
> +			while (pad->gpio >= 0 && pad->gpio != gpio_num)
> +				pad++;
> +			/* Enable / disable pad wakeup */
> +			if (pad->gpio == gpio_num) {
> +				val = omap_ctrl_readw(pad->offset);
> +				if (trigger & IRQ_TYPE_EDGE_BOTH)
> +					val |= OMAP34XX_PAD_WAKE_EN;
> +				else
> +					val &= ~(u16)OMAP34XX_PAD_WAKE_EN;
> +				omap_ctrl_writew(val, pad->offset);
> +			}
> +
>  			if (trigger & IRQ_TYPE_EDGE_BOTH)
>  				__raw_writel(1 << gpio, bank->base
>  					+ OMAP24XX_GPIO_SETWKUENA);
> @@ -1654,7 +1674,7 @@ static int __init omap3_gpio_pads_init(void)
>  	gpio_pads[gpio_amt].gpio = -1;
>  	return 0;
>  }
> -late_initcall(omap3_gpio_pads_init);
> +early_initcall(omap3_gpio_pads_init);

This change isn't explained in the changelog and appears unrelated to
this patch.


Kevin

RE: [PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Tero.Kristo]

 

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>Sent: 08 March, 2010 19:06
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCH] OMAP3: GPIO: Added dynamic control logic 
>for pad wakeups
>
>Tero Kristo <tero.kristo@nokia.com> writes:
>
>> From: Tero Kristo <tero.kristo@nokia.com>
>>
>> Pad wakeups are now enabled if the corresponding GPIO 
>interrupt is enabled.
>>
>> Applies on top of PM branch.
>>
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>
>I just discovered this one in patchwork... sorry for the delay.
>
>Changes in wakeup state should not be directly correlated to interrupt
>enabled GPIOs.  Rather, this should only be done for GPIOs that are
>explicitly wakeup enabled (via enable_irq_wake(), which in turn
>calls gpio_wake_enable()).

This logic somehow escapes me... I would guess drivers should not care during dynamic idle whether the device is in off/ret/ina and interrupts should just work. This is done to make this happen. Also, I understood that gpio wakeup logic is needed for the suspend wakeup, which is quite different from dynamic idle wakeup.

However, if this is intended behavior for the kernel, then I will accept it. You are saying the code below should be moved into the gpio_wake_enable() / disable() calls?

>
>A couple other minor comments below...
>
>> ---
>>  arch/arm/plat-omap/gpio.c |   22 +++++++++++++++++++++-
>>  1 files changed, 21 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index e242112..fa79db2 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -267,6 +267,7 @@ static struct gpio_bank gpio_bank_34xx[6] = {
>>  #define OMAP34XX_PAD_SAFE_MODE 0x7
>>  #define OMAP34XX_PAD_IN_PU_GPIO 0x11c
>>  #define OMAP34XX_PAD_IN_PD_GPIO 0x10c
>> +#define OMAP34XX_PAD_WAKE_EN (1 << 14)
>
>Can use BIT(14) here
>
>>  struct omap3_gpio_regs {
>>  	u32 sysconfig;
>> @@ -713,6 +714,8 @@ static inline void 
>set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>>  {
>>  	void __iomem *base = bank->base;
>>  	u32 gpio_bit = 1 << gpio;
>> +	struct gpio_pad *pad;
>> +	int gpio_num;
>>  	u32 val;
>>  
>>  	if (cpu_is_omap44xx()) {
>> @@ -750,6 +753,23 @@ static inline void 
>set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>>  			 * GPIO wakeup request can only be 
>generated on edge
>>  			 * transitions
>>  			 */
>
>The comment above needs to stay with the original code.
>
>> +			pad = gpio_pads;
>> +
>> +			gpio_num = bank->virtual_irq_start - 
>IH_GPIO_BASE +
>> +				gpio;
>> +			/* Find the pad corresponding the GPIO */
>> +			while (pad->gpio >= 0 && pad->gpio != gpio_num)
>> +				pad++;
>> +			/* Enable / disable pad wakeup */
>> +			if (pad->gpio == gpio_num) {
>> +				val = omap_ctrl_readw(pad->offset);
>> +				if (trigger & IRQ_TYPE_EDGE_BOTH)
>> +					val |= OMAP34XX_PAD_WAKE_EN;
>> +				else
>> +					val &= 
>~(u16)OMAP34XX_PAD_WAKE_EN;
>> +				omap_ctrl_writew(val, pad->offset);
>> +			}
>> +
>>  			if (trigger & IRQ_TYPE_EDGE_BOTH)
>>  				__raw_writel(1 << gpio, bank->base
>>  					+ OMAP24XX_GPIO_SETWKUENA);
>> @@ -1654,7 +1674,7 @@ static int __init omap3_gpio_pads_init(void)
>>  	gpio_pads[gpio_amt].gpio = -1;
>>  	return 0;
>>  }
>> -late_initcall(omap3_gpio_pads_init);
>> +early_initcall(omap3_gpio_pads_init);
>
>This change isn't explained in the changelog and appears unrelated to
>this patch.

The reason for this change is that we need the gpio->pad mapping early now to enable wakeups properly. Otherwise some components can enable gpio interrupts early in the boot cycle and they will miss their wakeup setting because the map does not exist yet. I think another way to do this would be to enable wakeups for all enabled interrupts during the omap3_gpio_pads_init().

-Tero

Re: [PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Kevin Hilman]

<Tero.Kristo@nokia.com> writes:

>>-----Original Message-----
>>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>>Sent: 08 March, 2010 19:06
>>To: Kristo Tero (Nokia-D/Tampere)
>>Cc: linux-omap@vger.kernel.org
>>Subject: Re: [PATCH] OMAP3: GPIO: Added dynamic control logic 
>>for pad wakeups
>>
>>Tero Kristo <tero.kristo@nokia.com> writes:
>>
>>> From: Tero Kristo <tero.kristo@nokia.com>
>>>
>>> Pad wakeups are now enabled if the corresponding GPIO 
>>interrupt is enabled.
>>>
>>> Applies on top of PM branch.
>>>
>>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>>
>>I just discovered this one in patchwork... sorry for the delay.
>>
>>Changes in wakeup state should not be directly correlated to interrupt
>>enabled GPIOs.  Rather, this should only be done for GPIOs that are
>>explicitly wakeup enabled (via enable_irq_wake(), which in turn
>>calls gpio_wake_enable()).
>
> This logic somehow escapes me... I would guess drivers should not
> care during dynamic idle whether the device is in off/ret/ina and
> interrupts should just work.  This is done to make this
> happen. Also, I understood that gpio wakeup logic is needed for the
> suspend wakeup, which is quite different from dynamic idle wakeup.

>From a wider kernel perspecitve, wakeup from idle and suspend are two
different things.  But from the OMAP perspecitve, they're identical.
Unfortunately, the kernel has no generic way to generically tell the
difference, so my preference for readability and maintainability is to
keep them coupled.  If a driver wants wakeups (from either idle or
suspend) it should use enable_irq_wake().

The only real difference is whether the wakeup is a IO pad wakeup
(CORE in INA/RET/OFF) or a module-level wakeup.

Your current approach decouples those, and that's primarily what I
don't like.  With the current approach, IO pad wakeups are always
enabled for all GPIO IRQs, but not module level wakeups which
are only enabled by enable_irq_wake().

> However, if this is intended behavior for the kernel, then I will
> accept it. You are saying the code below should be moved into the
> gpio_wake_enable() / disable() calls?

Yes.  That will essentially ensure that both module-level wakeups
and IO pad wakeups for a GPIO are enabled/disabled together, and
controllable by enable|disable_irq_wake()

Kevin


>>
>>A couple other minor comments below...
>>
>>> ---
>>>  arch/arm/plat-omap/gpio.c |   22 +++++++++++++++++++++-
>>>  1 files changed, 21 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>>> index e242112..fa79db2 100644
>>> --- a/arch/arm/plat-omap/gpio.c
>>> +++ b/arch/arm/plat-omap/gpio.c
>>> @@ -267,6 +267,7 @@ static struct gpio_bank gpio_bank_34xx[6] = {
>>>  #define OMAP34XX_PAD_SAFE_MODE 0x7
>>>  #define OMAP34XX_PAD_IN_PU_GPIO 0x11c
>>>  #define OMAP34XX_PAD_IN_PD_GPIO 0x10c
>>> +#define OMAP34XX_PAD_WAKE_EN (1 << 14)
>>
>>Can use BIT(14) here
>>
>>>  struct omap3_gpio_regs {
>>>  	u32 sysconfig;
>>> @@ -713,6 +714,8 @@ static inline void 
>>set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>>>  {
>>>  	void __iomem *base = bank->base;
>>>  	u32 gpio_bit = 1 << gpio;
>>> +	struct gpio_pad *pad;
>>> +	int gpio_num;
>>>  	u32 val;
>>>  
>>>  	if (cpu_is_omap44xx()) {
>>> @@ -750,6 +753,23 @@ static inline void 
>>set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>>>  			 * GPIO wakeup request can only be 
>>generated on edge
>>>  			 * transitions
>>>  			 */
>>
>>The comment above needs to stay with the original code.
>>
>>> +			pad = gpio_pads;
>>> +
>>> +			gpio_num = bank->virtual_irq_start - 
>>IH_GPIO_BASE +
>>> +				gpio;
>>> +			/* Find the pad corresponding the GPIO */
>>> +			while (pad->gpio >= 0 && pad->gpio != gpio_num)
>>> +				pad++;
>>> +			/* Enable / disable pad wakeup */
>>> +			if (pad->gpio == gpio_num) {
>>> +				val = omap_ctrl_readw(pad->offset);
>>> +				if (trigger & IRQ_TYPE_EDGE_BOTH)
>>> +					val |= OMAP34XX_PAD_WAKE_EN;
>>> +				else
>>> +					val &= 
>>~(u16)OMAP34XX_PAD_WAKE_EN;
>>> +				omap_ctrl_writew(val, pad->offset);
>>> +			}
>>> +
>>>  			if (trigger & IRQ_TYPE_EDGE_BOTH)
>>>  				__raw_writel(1 << gpio, bank->base
>>>  					+ OMAP24XX_GPIO_SETWKUENA);
>>> @@ -1654,7 +1674,7 @@ static int __init omap3_gpio_pads_init(void)
>>>  	gpio_pads[gpio_amt].gpio = -1;
>>>  	return 0;
>>>  }
>>> -late_initcall(omap3_gpio_pads_init);
>>> +early_initcall(omap3_gpio_pads_init);
>>
>>This change isn't explained in the changelog and appears unrelated to
>>this patch.
>
> The reason for this change is that we need the gpio->pad mapping early now to enable wakeups properly. Otherwise some components can enable gpio interrupts early in the boot cycle and they will miss their wakeup setting because the map does not exist yet. I think another way to do this would be to enable wakeups for all enabled interrupts during the omap3_gpio_pads_init().
>
> -Tero

Re: [PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Tony Lindgren]

* Tero.Kristo@nokia.com <Tero.Kristo@nokia.com> [100309 00:20]:
> >
> >Changes in wakeup state should not be directly correlated to interrupt
> >enabled GPIOs.  Rather, this should only be done for GPIOs that are
> >explicitly wakeup enabled (via enable_irq_wake(), which in turn
> >calls gpio_wake_enable()).
> 
> This logic somehow escapes me... I would guess drivers should not care during dynamic idle whether the device is in off/ret/ina and interrupts should just work. This is done to make this happen. Also, I understood that gpio wakeup logic is needed for the suspend wakeup, which is quite different from dynamic idle wakeup.
> 
> However, if this is intended behavior for the kernel, then I will accept it. You are saying the code below should be moved into the gpio_wake_enable() / disable() calls?

I agree. I'd assume during the idle modes we want everything to
automatically wake the system up. Otherwise we again have non-standard
Linux behaviour that's mysterious to track down. The enable_irq_wake
should only be needed for suspend states.
 
> >This change isn't explained in the changelog and appears unrelated to
> >this patch.
> 
> The reason for this change is that we need the gpio->pad mapping early now to enable wakeups properly. Otherwise some components can enable gpio interrupts early in the boot cycle and they will miss their wakeup setting because the map does not exist yet. I think another way to do this would be to enable wakeups for all enabled interrupts during the omap3_gpio_pads_init().

Don't have the original patch, but it smells like you're trying
to do gpio to mux register mapping?

If so, your already have that available via omap_mux_get/set_gpio()
for 34xx in the new muxcode already in mainline. Those work even
if you don't have CONFIG_OMAP_MUX set.

And I still need to convert 24xx to the new mux code so we can
get rid of the old omap1 style code..

Regards,

Tony

Re: [PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Kevin Hilman]

Tony Lindgren <tony@atomide.com> writes:

> * Tero.Kristo@nokia.com <Tero.Kristo@nokia.com> [100309 00:20]:
>> >
>> >Changes in wakeup state should not be directly correlated to interrupt
>> >enabled GPIOs.  Rather, this should only be done for GPIOs that are
>> >explicitly wakeup enabled (via enable_irq_wake(), which in turn
>> >calls gpio_wake_enable()).
>> 
>> This logic somehow escapes me... I would guess drivers should not care during dynamic idle whether the device is in off/ret/ina and interrupts should just work. This is done to make this happen. Also, I understood that gpio wakeup logic is needed for the suspend wakeup, which is quite different from dynamic idle wakeup.
>> 
>> However, if this is intended behavior for the kernel, then I will accept it. You are saying the code below should be moved into the gpio_wake_enable() / disable() calls?
>
> I agree. I'd assume during the idle modes we want everything to
> automatically wake the system up. Otherwise we again have non-standard
> Linux behaviour that's mysterious to track down. The enable_irq_wake
> should only be needed for suspend states.

OK, then essentially all GPIO IRQs need to be configured in the
equivalent of an enable_irq_wake'd state by default.  That means
IO pad wakeups *and* module-level wakeups.

Upon suspend, the ability to wakeup should be removed for all except
for those that have been explicitly enabled via enable_irq_wake().

Kevin

Re: [PATCH] OMAP3: GPIO: Added dynamic control logic for pad wakeups

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [100309 11:23]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Tero.Kristo@nokia.com <Tero.Kristo@nokia.com> [100309 00:20]:
> >> >
> >> >Changes in wakeup state should not be directly correlated to interrupt
> >> >enabled GPIOs.  Rather, this should only be done for GPIOs that are
> >> >explicitly wakeup enabled (via enable_irq_wake(), which in turn
> >> >calls gpio_wake_enable()).
> >> 
> >> This logic somehow escapes me... I would guess drivers should not care during dynamic idle whether the device is in off/ret/ina and interrupts should just work. This is done to make this happen. Also, I understood that gpio wakeup logic is needed for the suspend wakeup, which is quite different from dynamic idle wakeup.
> >> 
> >> However, if this is intended behavior for the kernel, then I will accept it. You are saying the code below should be moved into the gpio_wake_enable() / disable() calls?
> >
> > I agree. I'd assume during the idle modes we want everything to
> > automatically wake the system up. Otherwise we again have non-standard
> > Linux behaviour that's mysterious to track down. The enable_irq_wake
> > should only be needed for suspend states.
> 
> OK, then essentially all GPIO IRQs need to be configured in the
> equivalent of an enable_irq_wake'd state by default.  That means
> IO pad wakeups *and* module-level wakeups.
> 
> Upon suspend, the ability to wakeup should be removed for all except
> for those that have been explicitly enabled via enable_irq_wake().

That sounds pretty good to me.

Tony