[PATCH] OMAP: GPIO: Avoid generating extra IRQs

[PATCH] OMAP: GPIO: Avoid generating extra IRQs

[ext-eero.nurkkala]

From: Eero Nurkkala <ext-eero.nurkkala@nokia.com>

It is possible for GPIO IRQ lines configured with
falling edge triggering only to get IRQs at the
rising edge upon the exit of offmode. And vice
versa. Prevent such IRQs to arrive by generating
the IRQ obeying the detection scheme.

Signed-off-by: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
---
 arch/arm/plat-omap/gpio.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 26b387c..7a270c0 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -1804,7 +1804,7 @@ void omap2_gpio_resume_after_retention(void)
 		return;
 	for (i = 0; i < gpio_bank_count; i++) {
 		struct gpio_bank *bank = &gpio_bank[i];
-		u32 l;
+		u32 l, gen, gen0, gen1;
 
 		if (!(bank->enabled_non_wakeup_gpios))
 			continue;
@@ -1825,14 +1825,33 @@ void omap2_gpio_resume_after_retention(void)
 #endif
 		l ^= bank->saved_datain;
 		l &= bank->non_wakeup_gpios;
-		if (l) {
+
+		/*
+		 * No need to generate IRQs for the rising edge for gpio IRQs
+		 * configured with falling edge only; and vice versa.
+		 */
+		gen0 = l & bank->saved_fallingdetect;
+		gen0 &= bank->saved_datain;
+
+		gen1 = l & bank->saved_risingdetect;
+		gen1 &= ~(bank->saved_datain);
+
+		/* FIXME: Consider GPIO IRQs with level detections properly! */
+		gen = l & (~(bank->saved_fallingdetect) &
+				~(bank->saved_risingdetect));
+		/* Consider all GPIO IRQs needed to be updated */
+		gen |= gen0 | gen1;
+
+		if (gen) {
 			u32 old0, old1;
 #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX) || \
 			defined(CONFIG_ARCH_OMAP4)
 			old0 = __raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT0);
 			old1 = __raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT1);
-			__raw_writel(old0 | l, bank->base + OMAP24XX_GPIO_LEVELDETECT0);
-			__raw_writel(old1 | l, bank->base + OMAP24XX_GPIO_LEVELDETECT1);
+			__raw_writel(old0 | gen, bank->base +
+					OMAP24XX_GPIO_LEVELDETECT0);
+			__raw_writel(old1 | gen, bank->base +
+					OMAP24XX_GPIO_LEVELDETECT1);
 			__raw_writel(old0, bank->base + OMAP24XX_GPIO_LEVELDETECT0);
 			__raw_writel(old1, bank->base + OMAP24XX_GPIO_LEVELDETECT1);
 #endif
-- 
1.6.0.4

Re: [PATCH] OMAP: GPIO: Avoid generating extra IRQs

[Tony Lindgren]

Hi,

* ext-eero.nurkkala@nokia.com <ext-eero.nurkkala@nokia.com> [090703 09:27]:
> From: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
> 
> It is possible for GPIO IRQ lines configured with
> falling edge triggering only to get IRQs at the
> rising edge upon the exit of offmode. And vice
> versa. Prevent such IRQs to arrive by generating
> the IRQ obeying the detection scheme.

Is the situation still the same if we temporarily change the
level lines to edge for the duration of idle? This would allow
waking up even if the line is level.

Also one question below.

> Signed-off-by: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
> ---
>  arch/arm/plat-omap/gpio.c |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 26b387c..7a270c0 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -1804,7 +1804,7 @@ void omap2_gpio_resume_after_retention(void)
>  		return;
>  	for (i = 0; i < gpio_bank_count; i++) {
>  		struct gpio_bank *bank = &gpio_bank[i];
> -		u32 l;
> +		u32 l, gen, gen0, gen1;
>  
>  		if (!(bank->enabled_non_wakeup_gpios))
>  			continue;
> @@ -1825,14 +1825,33 @@ void omap2_gpio_resume_after_retention(void)
>  #endif
>  		l ^= bank->saved_datain;
>  		l &= bank->non_wakeup_gpios;
> -		if (l) {
> +
> +		/*
> +		 * No need to generate IRQs for the rising edge for gpio IRQs
> +		 * configured with falling edge only; and vice versa.
> +		 */
> +		gen0 = l & bank->saved_fallingdetect;
> +		gen0 &= bank->saved_datain;
> +
> +		gen1 = l & bank->saved_risingdetect;
> +		gen1 &= ~(bank->saved_datain);
> +

Is the gen0 &= correct or should it also clear the bits?

Also, have you checked the code flow on omap1 when l, gen, gen0 & gen1
are not initialized? Just wondering ;)

Tony


> +		/* FIXME: Consider GPIO IRQs with level detections properly! */
> +		gen = l & (~(bank->saved_fallingdetect) &
> +				~(bank->saved_risingdetect));
> +		/* Consider all GPIO IRQs needed to be updated */
> +		gen |= gen0 | gen1;
> +
> +		if (gen) {
>  			u32 old0, old1;
>  #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX) || \
>  			defined(CONFIG_ARCH_OMAP4)
>  			old0 = __raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT0);
>  			old1 = __raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT1);
> -			__raw_writel(old0 | l, bank->base + OMAP24XX_GPIO_LEVELDETECT0);
> -			__raw_writel(old1 | l, bank->base + OMAP24XX_GPIO_LEVELDETECT1);
> +			__raw_writel(old0 | gen, bank->base +
> +					OMAP24XX_GPIO_LEVELDETECT0);
> +			__raw_writel(old1 | gen, bank->base +
> +					OMAP24XX_GPIO_LEVELDETECT1);
>  			__raw_writel(old0, bank->base + OMAP24XX_GPIO_LEVELDETECT0);
>  			__raw_writel(old1, bank->base + OMAP24XX_GPIO_LEVELDETECT1);
>  #endif
> -- 
> 1.6.0.4
> 
> --
> 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

RE: [PATCH] OMAP: GPIO: Avoid generating extra IRQs

[ext-Eero.Nurkkala]

>>               l ^= bank->saved_datain;
>>               l &= bank->non_wakeup_gpios;
>> -             if (l) {
>> +
>> +             /*
>> +              * No need to generate IRQs for the rising edge for gpio IRQs
>> +              * configured with falling edge only; and vice versa.
>> +              */
>> +             gen0 = l & bank->saved_fallingdetect;
>> +             gen0 &= bank->saved_datain;
>> +
>> +             gen1 = l & bank->saved_risingdetect;
>> +             gen1 &= ~(bank->saved_datain);
>> +

> Is the gen0 &= correct or should it also clear the bits?

What do you mean, eg, what bits should be cleared?

gen0 = (bit has changed) and is falling_edge
gen0 &= (the bit has changed) is falling edge and former state was "HIGH"
(transition from HIGH to LOW has occurred)

for rising edge, needed is LOW to HIGH transition.

- Eero

Re: [PATCH] OMAP: GPIO: Avoid generating extra IRQs

[Tony Lindgren]

* ext-Eero.Nurkkala@nokia.com <ext-Eero.Nurkkala@nokia.com> [090805 17:45]:
> 
> >>               l ^= bank->saved_datain;
> >>               l &= bank->non_wakeup_gpios;
> >> -             if (l) {
> >> +
> >> +             /*
> >> +              * No need to generate IRQs for the rising edge for gpio IRQs
> >> +              * configured with falling edge only; and vice versa.
> >> +              */
> >> +             gen0 = l & bank->saved_fallingdetect;
> >> +             gen0 &= bank->saved_datain;
> >> +
> >> +             gen1 = l & bank->saved_risingdetect;
> >> +             gen1 &= ~(bank->saved_datain);
> >> +
> 
> > Is the gen0 &= correct or should it also clear the bits?
> 
> What do you mean, eg, what bits should be cleared?
> 
> gen0 = (bit has changed) and is falling_edge
> gen0 &= (the bit has changed) is falling edge and former state was "HIGH"
> (transition from HIGH to LOW has occurred)
> 
> for rising edge, needed is LOW to HIGH transition.

OK, my comment was just based on looking at the patch, sounds like
you have verified that it's correct. 

Tony

RE: [PATCH] OMAP: GPIO: Avoid generating extra IRQs

[ext-Eero.Nurkkala]

>> It is possible for GPIO IRQ lines configured with
>> falling edge triggering only to get IRQs at the
>> rising edge upon the exit of offmode. And vice
>> versa. Prevent such IRQs to arrive by generating
>> the IRQ obeying the detection scheme.
>
> Is the situation still the same if we temporarily change the
> level lines to edge for the duration of idle? This would allow
> waking up even if the line is level.

I admit that I haven't tested such a schenario. But I guess
it's not different than what it used to be ... Is there a sample
to try out?

However, if there's no edge detection (nor and or level detection)
set at all, it will still generate soft IRQs, which possibly needs
some rethinking (maybe WARN if no egde nor level detection on
the GPIO IRQ). But it's been that way, so I figured no need to
change that.


> Also, have you checked the code flow on omap1 when l, gen, gen0 & gen1
> are not initialized? Just wondering ;)
> 
> Tony

Right, possibly a compile warning.

- Eero

[APPLIED] [PATCH] OMAP: GPIO: Avoid generating extra IRQs

[Tony Lindgren]

This patch has been applied to the linux-omap
by youw fwiendly patch wobot.

Branch in linux-omap: omap-upstream

Initial commit ID (Likely to change): ad23735d6744e7eeec4d2f012aa2ed2c1f8d64a9

PatchWorks
http://patchwork.kernel.org/patch/33855/

Git (Likely to change, and takes a while to get mirrored)
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=ad23735d6744e7eeec4d2f012aa2ed2c1f8d64a9