[PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

Resend because of the content format issues in last mail---sorry for
inconvenience.

This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, and
also only enable WAKEUPEN When GPIO edge detection is enabled, 
also clear the gpio event triggering configurations to avoid Pending
interrupt status which is generated regardless of the IRQEN and WKUPEN
bit,
the pending IRQ status may prevent GPIO module acknowledge IDLE request
and prevent PER and thus CORE RETENTION.

This is only applied for OMAP2/3 GPIO IRQs.

Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
---
 arch/arm/plat-omap/gpio.c |   44
++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index bd04b40..19e0461 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int
enc_time)
 EXPORT_SYMBOL(omap_set_gpio_debounce_time);
 
 #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
-static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int
gpio,
+static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 						int trigger)
 {
 	void __iomem *base = bank->base;
@@ -597,7 +597,12 @@ static inline void set_24xx_gpio_triggering(struct
gpio_bank *bank, int gpio,
 		trigger & IRQ_TYPE_EDGE_FALLING);
 
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
-		if (trigger != 0)
+		/*
+		 * GPIO wakeup request can only be generated on edge
+		 * transitions, see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
+		 * wake-up request note for detail
+		 */
+		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
 			__raw_writel(1 << gpio, bank->base
 					+ OMAP24XX_GPIO_SETWKUENA);
 		else
@@ -1133,6 +1138,39 @@ static void gpio_ack_irq(unsigned int irq)
 	_clear_gpio_irqstatus(bank, gpio);
 }
 
+static void gpio_enable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+	int trigger = irq_desc[irq].status & IRQ_TYPE_SENSE_MASK;
+
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), trigger);
+#endif
+	_set_gpio_irqenable(bank, gpio, 1);
+}
+
+static void gpio_disable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+
+	_set_gpio_irqenable(bank, gpio, 0);
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	/*
+	 * Clear the event detect setting and IRQ status to prevent
+	 * IRQ staus been set or pending there While IRQ is disabled,
+	 * otherwise GPIO module will not acknowledge the IDLE request
+	 * from PER power domain, this may prevent System wide retention
+	 * See OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1 GPIO interrupt and
wakeup
+	 * enable note for detail
+	 */
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio),
IRQ_TYPE_NONE);
+	_clear_gpio_irqstatus(bank, gpio);
+#endif
+
+}
+
 static void gpio_mask_irq(unsigned int irq)
 {
 	unsigned int gpio = irq - IH_GPIO_BASE;
@@ -1160,6 +1198,8 @@ static void gpio_unmask_irq(unsigned int irq)
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.shutdown	= gpio_irq_shutdown,
+	.enable		= gpio_enable_irq,
+	.disable	= gpio_disable_irq,
 	.ack		= gpio_ack_irq,
 	.mask		= gpio_mask_irq,
 	.unmask		= gpio_unmask_irq,
-- 
1.5.4.3

--
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] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

"Wang Sawsd-A24013" <cqwang@motorola.com> writes:

> Resend because of the content format issues in last mail---sorry for
> inconvenience.

This version is line-wrapped and otherwise confusing as well so does
not apply cleanly.  Could you try using 'git format-patch' to generate
your patch?  It looks like you may have used 'git diff' and then
appended them together?

In addition, the many uses of "also" in this description suggest that
that it could be broken up in to several patches with more clear
descriptions/justifications..  Some are fixes, some are new
functionality.

> This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, 

Please elaborate on why you chose to implement the enable/disable
hooks instead of using the mask/unmask hooks.

> and also only enable WAKEUPEN When GPIO edge detection is enabled,

Can you send this fixa separate patch.  It is a bug fix that is
separate from the new stuff you're adding here.

> also clear the gpio event triggering configurations to avoid Pending
> interrupt status which is generated regardless of the IRQEN and
> WKUPEN bit, the pending IRQ status may prevent GPIO module
> acknowledge IDLE request and prevent PER and thus CORE RETENTION.

Have you tested the PM branch code?

I'm not sure I quite follow your description here, but IIUC, we are
already doing this in the [prepare|resume]_idle() hooks.

Kevin

> This is only applied for OMAP2/3 GPIO IRQs.
>
> Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
>
> ---
>  arch/arm/plat-omap/gpio.c |   44
> ++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index bd04b40..19e0461 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int
> enc_time)
>  EXPORT_SYMBOL(omap_set_gpio_debounce_time);
>  
>  #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> -static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int
> gpio,
> +static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>  						int trigger)
>  {
>  	void __iomem *base = bank->base;
> @@ -597,7 +597,12 @@ static inline void set_24xx_gpio_triggering(struct
> gpio_bank *bank, int gpio,
>  		trigger & IRQ_TYPE_EDGE_FALLING);
>  
>  	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
> -		if (trigger != 0)
> +		/*
> +		 * GPIO wakeup request can only be generated on edge
> +		 * transitions, see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
> +		 * wake-up request note for detail
> +		 */
> +		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
>  			__raw_writel(1 << gpio, bank->base
>  					+ OMAP24XX_GPIO_SETWKUENA);
>  		else
> @@ -1133,6 +1138,39 @@ static void gpio_ack_irq(unsigned int irq)
>  	_clear_gpio_irqstatus(bank, gpio);
>  }
>  
> +static void gpio_enable_irq(unsigned int irq)
> +{
> +	unsigned int gpio = irq - IH_GPIO_BASE;
> +	struct gpio_bank *bank = get_irq_chip_data(irq);
> +	int trigger = irq_desc[irq].status & IRQ_TYPE_SENSE_MASK;
> +
> +#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> +	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), trigger);
> +#endif
> +	_set_gpio_irqenable(bank, gpio, 1);
> +}
> +
> +static void gpio_disable_irq(unsigned int irq)
> +{
> +	unsigned int gpio = irq - IH_GPIO_BASE;
> +	struct gpio_bank *bank = get_irq_chip_data(irq);
> +
> +	_set_gpio_irqenable(bank, gpio, 0);
> +#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> +	/*
> +	 * Clear the event detect setting and IRQ status to prevent
> +	 * IRQ staus been set or pending there While IRQ is disabled,
> +	 * otherwise GPIO module will not acknowledge the IDLE request
> +	 * from PER power domain, this may prevent System wide retention
> +	 * See OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1 GPIO interrupt and
> wakeup
> +	 * enable note for detail
> +	 */
> +	set_24xx_gpio_triggering(bank, get_gpio_index(gpio),
> IRQ_TYPE_NONE);
> +	_clear_gpio_irqstatus(bank, gpio);
> +#endif
> +
> +}
> +
>  static void gpio_mask_irq(unsigned int irq)
>  {
>  	unsigned int gpio = irq - IH_GPIO_BASE;
> @@ -1160,6 +1198,8 @@ static void gpio_unmask_irq(unsigned int irq)
>  static struct irq_chip gpio_irq_chip = {
>  	.name		= "GPIO",
>  	.shutdown	= gpio_irq_shutdown,
> +	.enable		= gpio_enable_irq,
> +	.disable	= gpio_disable_irq,
>  	.ack		= gpio_ack_irq,
>  	.mask		= gpio_mask_irq,
>  	.unmask		= gpio_unmask_irq,
> -- 
> 1.5.4.3
>
> --
> 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
> --
> 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] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

[-- Attachment #1: Type: text/plain, Size: 4817 bytes --]


> "Wang Sawsd-A24013" <cqwang@motorola.com> writes:
> 
> > Resend because of the content format issues in last mail---sorry for
> > inconvenience.
> 
> This version is line-wrapped and otherwise confusing as well so does
> not apply cleanly.  Could you try using 'git format-patch' to generate
> your patch?  It looks like you may have used 'git diff' and then
> appended them together?
> 

I did use git-format-patch command, seems the issue is caused 
our mail server, this time I attached the whole patch in the mail,
and I didn't separate the change into sub-patchs, see explanation below.

> In addition, the many uses of "also" in this description suggest that
> that it could be broken up in to several patches with more clear
> descriptions/justifications..  Some are fixes, some are new
> functionality.

Yes, "also" is abused in my description, too spoken. :-) 
But the whole change is actually for the same issue which is 
OMAP GPIO IRQ/WAKEUP issue and I think they are all fixes
but not new functionality.

> 
> > This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, 
> 
> Please elaborate on why you chose to implement the enable/disable
> hooks instead of using the mask/unmask hooks.
> 
1: irq mask/unmask are typically used by the IRQ handling framework
or CHIP IRQ handler to disable and enable IRQ during the IRQ handing
procedure; While still many drivers are using irq_disable and
irq_disable
in their code especially in the case of suspend/resume and IOCTL to turn
on/off the peripherals, without the irq_disable in IRQ CHIP, the
default_disable
Is used but it is just an empty function now. This will cause an issue
that
The IRQ may still happen after irq_disable which may cause unexpected 
behavior in driver since they are not expecting any IRQ after
irq_disable.

2: I noticed that we have the mask and unmask already, but why I didn't
choose to use them directly for irq_disable and irq_enable is because
we need something special in irq_disable for OMAP. 

Since irq_disable typically means the driver wants to deactive the
peripherals
for a long period of time and may allow System going into low power
state, 
but for OMAP GPIO the IRQ status bit may still be set even when IRQEN
bit is cleared, that's why we need also clear the level detect and edge
detect
Configuration register to ensure after irq_disable, no IRQ status bit is
set,
since once the IRQ status bit is set while IRQ is disabled, then this
will
prevent GPIO module acknowledge IDLE request from PRCM, so PER 
and CORE will fail entering RET state. We are facing this issue now,
that is why I made this patch.


> > and also only enable WAKEUPEN When GPIO edge detection is enabled,
> 
> Can you send this fixa separate patch.  It is a bug fix that is
> separate from the new stuff you're adding here.
> 
This issue is also related with the OMAP GPIO IRQ wakeup configuration
So it is not a separate issue. But I can send it as separate patch if
the community
Think the rest of this patch is not applicable.

> > also clear the gpio event triggering configurations to avoid Pending
> > interrupt status which is generated regardless of the IRQEN and
> > WKUPEN bit, the pending IRQ status may prevent GPIO module
> > acknowledge IDLE request and prevent PER and thus CORE RETENTION.
> 
> Have you tested the PM branch code?
> 
Yes, I have verified the patch on the kernel which is based on google
omap
GIT which is based on linux-omap PM branch, and it does resolve the
issue.
The issue I am facing is the Touch controller on my HW will continously
report
Input interrupts fastly, so without the patch, the IRQ staus will be
pending there
Even after the touch driver calls irq_disable and prevents PER and CORE
going into RETENTION In suspend mode.

> I'm not sure I quite follow your description here, but IIUC, we are
> already doing this in the [prepare|resume]_idle() hooks.
> 
The code in GPIO prepare/resume for idle functions are made to address
another different issue for OMAP2, since on OMAP2 there are some silicon
bugs
that causes some GPIO to be non-wakeupable, so the workaround was made
to
not enable edge detection for those GPIOs and manually trigger IRQs
using level
detect based on input GPIO levels after idle. But for OMAP3, there is no
non-wakeup
gpio, all the GPIOs are wakeup gpios.

This patch is needed for both OMAP2 and OMAP3 since the GPIO module has
The similar silicon architecture, that is IRQ status will be set
regardless of IRQEN bit.

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

[-- Attachment #2: 0001-Fix-OMAP2-3-GPIO-pending-irq-status-blocking-RETENTI.patch --]
[-- Type: application/octet-stream, Size: 3121 bytes --]

From 6d76fe7d9b7c02d7f472e6edafff52ee549b7419 Mon Sep 17 00:00:00 2001
From: Chunqiu Wang <cqwang@motorola.com>
Date: Sat, 30 May 2009 07:38:40 +0800
Subject: [PATCH] Fix OMAP2/3 GPIO pending irq status blocking RETENTION issue


Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
---
 arch/arm/plat-omap/gpio.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index bd04b40..19e0461 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int enc_time)
 EXPORT_SYMBOL(omap_set_gpio_debounce_time);
 
 #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
-static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
+static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 						int trigger)
 {
 	void __iomem *base = bank->base;
@@ -597,7 +597,12 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 		trigger & IRQ_TYPE_EDGE_FALLING);
 
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
-		if (trigger != 0)
+		/*
+		 * GPIO wakeup request can only be generated on edge
+		 * transitions, see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
+		 * wake-up request note for detail
+		 */
+		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
 			__raw_writel(1 << gpio, bank->base
 					+ OMAP24XX_GPIO_SETWKUENA);
 		else
@@ -1133,6 +1138,39 @@ static void gpio_ack_irq(unsigned int irq)
 	_clear_gpio_irqstatus(bank, gpio);
 }
 
+static void gpio_enable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+	int trigger = irq_desc[irq].status & IRQ_TYPE_SENSE_MASK;
+
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), trigger);
+#endif
+	_set_gpio_irqenable(bank, gpio, 1);
+}
+
+static void gpio_disable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+
+	_set_gpio_irqenable(bank, gpio, 0);
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	/*
+	 * Clear the event detect setting and IRQ status to prevent
+	 * IRQ staus been set or pending there While IRQ is disabled,
+	 * otherwise GPIO module will not acknowledge the IDLE request
+	 * from PER power domain, this may prevent System wide retention
+	 * See OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1 GPIO interrupt and wakeup
+	 * enable note for detail
+	 */
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);
+	_clear_gpio_irqstatus(bank, gpio);
+#endif
+
+}
+
 static void gpio_mask_irq(unsigned int irq)
 {
 	unsigned int gpio = irq - IH_GPIO_BASE;
@@ -1160,6 +1198,8 @@ static void gpio_unmask_irq(unsigned int irq)
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.shutdown	= gpio_irq_shutdown,
+	.enable		= gpio_enable_irq,
+	.disable	= gpio_disable_irq,
 	.ack		= gpio_ack_irq,
 	.mask		= gpio_mask_irq,
 	.unmask		= gpio_unmask_irq,
-- 
1.5.4.3

Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

"Wang Sawsd-A24013" <cqwang@motorola.com> writes:

>> "Wang Sawsd-A24013" <cqwang@motorola.com> writes:
>> 
>> > Resend because of the content format issues in last mail---sorry for
>> > inconvenience.
>> 
>> This version is line-wrapped and otherwise confusing as well so does
>> not apply cleanly.  Could you try using 'git format-patch' to generate
>> your patch?  It looks like you may have used 'git diff' and then
>> appended them together?
>> 
>
> I did use git-format-patch command, seems the issue is caused 
> our mail server, this time I attached the whole patch in the mail,

Attached version applies fine, but still has problems.  More below.

In the future, you should also include the description/justifiction in
the git changelog so it shows up in the patch, and in the git history.

If you're having problems with your mailer or mail server, you might
try to bypass the mailer and use 'git send-email' to send directly to
an SMTP server.

> and I didn't separate the change into sub-patchs, see explanation below.

I still think there at least a couple different problems going on and
I think you are addressing some symptoms but not the root cause.

It would help if you described in more detail the problem(s) you are
having and which part of this patch fixes which part of your
problem(s).

This GPIO interrupt handling code is very sensitive to changes so we
need to really understand your problem before making changes here.

Also it's quite possible there are bugs in your driver as well.  Is
there any chance you can reproduce this on a public platform such as
the 3430SDP using the PM branch?  

If I use the PM branch on SDP, enable the touchscreen and go into
suspend, I do not see the GPIO driver keeping the system out of
retention.  In addition, if I add 

  enable_irq_wake(gpio_to_irq(ts_gpio));

to the board init code, I can also use the touchscreen as a wakeup
source and it does not prevent retention so you should be able to
reproduce there.

>> In addition, the many uses of "also" in this description suggest that
>> that it could be broken up in to several patches with more clear
>> descriptions/justifications..  Some are fixes, some are new
>> functionality.
>
> Yes, "also" is abused in my description, too spoken. :-) 
> But the whole change is actually for the same issue which is 
> OMAP GPIO IRQ/WAKEUP issue and I think they are all fixes
> but not new functionality.
>
>> 
>> > This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, 
>> 
>> Please elaborate on why you chose to implement the enable/disable
>> hooks instead of using the mask/unmask hooks.
>> 
> 1: irq mask/unmask are typically used by the IRQ handling framework
> or CHIP IRQ handler to disable and enable IRQ during the IRQ handing
> procedure; 

The mask/unmask are also used by the lazy disable feature of the
generic IRQ code.  More on this below...

> While still many drivers are using irq_disable and irq_disable in
> their code especially in the case of suspend/resume and IOCTL to
> turn on/off the peripherals, without the irq_disable in IRQ CHIP,
> the default_disable Is used but it is just an empty function
> now. This will cause an issue that The IRQ may still happen after
> irq_disable which may cause unexpected behavior in driver since they
> are not expecting any IRQ after irq_disable.

The driver's ISR should *never* be called after disable_irq().
However, the actual interrupt may still fire.  Here is what is
_supposed_ to happen.

The generic IRQ infrastructure was recently changed to use a "lazy
disable" which means the default disable_irq() (when no disable hook
is installed) doesn't actually disable the interrupt immediately.
Rather, it simply sets the IRQ_DISABLED flag.  Then, upon the next
interrupt the generic handler notices the flag, masks (and acks if
it's level triggered) the IRQ and does not call the interrupt handler.
Thus an extra interrupt happens, but the drivers ISR should not be
called.  If the drivers' ISR is being called after disable_irq() then
something bad is going on not related to GPIOs.

Now, that's what is "supposed" to happen.  That's not to say you
haven't found a real bug somewhere.  It's just not clear yet
because I still don't understand the bug you're trying to fix.

The chained handler for GPIOs is supposed to demux and than calls
either handle_edge_irq() or handle_level_irq() (via
generic_handle_irq()), the handler is set when the IRQ type is set.
Eiter of these handlers will handle the lazy disable and result
in a call to irq_chip->mask() on the next interrupt.

You could try the patch below[1] which will warn if the triggering
is not setup properly for a given GPIO IRQ.  This would result
in a broken lazy disable.

> 2: I noticed that we have the mask and unmask already, but why I didn't
> choose to use them directly for irq_disable and irq_enable is because
> we need something special in irq_disable for OMAP. 

This is where I disagree.  Basically, I think things you are doing in
disable are either done elsewhere already, or should be.

IOW, you're new disable hook does the equivalent of:

  irq_chip->mask(irq)
  irq_chip->set_wake(irq, 0); /* a.k.a. disable_irq_wake() */
  irq_chip->ack(irq);

The lazy disable will take care of the mask (and ack if it's level
triggered).  And if you don't want your driver's IRQ to be a wakeup
event, you should also call disable_irq_wake() in your driver if you
really want wakeup events disabled.

Your enable hook does he equivalent of

  irq_chip->set_wake(irq, 1); /* a.k.a. enable_irq_wake(irq); */
  irq_chip->unmask(irq);

Which is the same as your driver doing:

  enable_irq_wake(irq);
  enable_irq(irq);

So your driver should be simply add enable_irq_wake() if it wants to
(re)enable the wakeup feature.

Stated in another way, and possibly more simply, disable_irq() does
not imply disable_irq_wake() and enable_irq() does not imply
enable_irq_wake().

There is a possible bug here in the case of edge triggered interrupts:
a disable_irq() will not result in an irq_chip->ack().  Therfore,
pending IRQs that are not wakeup-enabled should probably be ACK'd in
the prepare_to_idle hook, or possibly in the set_wake() hook.  In the
disable case of set_wake(), if the IRQ is disabled, any pending IRQs
should be cleared.

> Since irq_disable typically means the driver wants to deactive the
> peripherals for a long period of time and may allow System going
> into low power state, but for OMAP GPIO the IRQ status bit may still
> be set even when IRQEN bit is cleared, that's why we need also clear
> the level detect and edge detect Configuration register to ensure
> after irq_disable, no IRQ status bit is set, since once the IRQ
> status bit is set while IRQ is disabled, then this will prevent GPIO
> module acknowledge IDLE request from PRCM, so PER and CORE will fail
> entering RET state. We are facing this issue now, that is why I made
> this patch.
>
>
>> > and also only enable WAKEUPEN When GPIO edge detection is enabled,
>> 
>> Can you send this fixa separate patch.  It is a bug fix that is
>> separate from the new stuff you're adding here.
>> 
> This issue is also related with the OMAP GPIO IRQ wakeup configuration
> So it is not a separate issue. But I can send it as separate patch if
> the community
> Think the rest of this patch is not applicable.

Yes, please send that part as a separate fix as it is a clear bugfix.
Also, it's probably better to put the TRM reference in the git
changelog instead of the code as those section numbers will become out
of date shortly.

>> > also clear the gpio event triggering configurations to avoid Pending
>> > interrupt status which is generated regardless of the IRQEN and
>> > WKUPEN bit, the pending IRQ status may prevent GPIO module
>> > acknowledge IDLE request and prevent PER and thus CORE RETENTION.
>> 
>> Have you tested the PM branch code?
>> 
> Yes, I have verified the patch on the kernel which is based on
> google omap GIT which is based on linux-omap PM branch, and it does
> resolve the issue.  The issue I am facing is the Touch controller on
> my HW will continously report Input interrupts fastly, so without
> the patch, the IRQ staus will be pending there Even after the touch
> driver calls irq_disable and prevents PER and CORE going into
> RETENTION In suspend mode.

So, IIUC, your drivers ISR is being called after disable_irq()?  If
so, something has gone horribly wrong.  This should not be possible
with the genirq framework.  Again, it would be nice to reproduce this
using the touchscreen on the SDP using the PM branch.  Please try
with my patch below[1] and tell me if you see any warnings.

>> I'm not sure I quite follow your description here, but IIUC, we are
>> already doing this in the [prepare|resume]_idle() hooks.
>> 
> The code in GPIO prepare/resume for idle functions are made to
> address another different issue for OMAP2, since on OMAP2 there are
> some silicon bugs that causes some GPIO to be non-wakeupable, so the
> workaround was made to not enable edge detection for those GPIOs and
> manually trigger IRQs using level detect based on input GPIO levels
> after idle. 

Sorry, I said the prepare_for/resume_from idle hooks but I was
thinking of the suspend/resume hooks....

> But for OMAP3, there is no non-wakeup gpio, all the GPIOs are wakeup
> gpios.

On OMAP3, all GPIOs are wakeup capable, but by non-wakeup GPIO is
meant GPIOs that are not *configured* for wakeup.   

The suspend hook will ensure that wakeups are disabled for any non
wake-enabled GPIO and restore them on resume.

> This patch is needed for both OMAP2 and OMAP3 since the GPIO module has
> The similar silicon architecture, that is IRQ status will be set
> regardless of IRQEN bit.
>

Hope this helps,

Kevin

[1]  Against current PM branch


diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 3b2054b..079f8a6 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -1098,9 +1098,15 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 
 		gpio_irq = bank->virtual_irq_start;
 		for (; isr != 0; isr >>= 1, gpio_irq++) {
+			struct irq_desc *desc;
+
 			if (!(isr & 1))
 				continue;
 
+			desc = irq_to_desc(gpio_irq);
+			WARN_ONCE(!(desc->status & IRQ_TYPE_SENSE_MASK),
+				  "%s: IRQ %d/%d has no triggering\n",
+				  __func__, irq, gpio_irq);
 			generic_handle_irq(gpio_irq);
 		}
 	}

RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

[-- Attachment #1: Type: text/plain, Size: 11426 bytes --]

Kevin,

See below for more comments/explanations:

> 
> I still think there at least a couple different problems going on and
> I think you are addressing some symptoms but not the root cause.
> 
I understand that combine these two changes in one patch may cause
Some confusion, so this time I separate into two patchs and 
they are attached.

I agree that I didn't describe the issue more in detail, but I think the
Second patch (also the original one I intended to make) does address
The root cause and not workaround the symptoms.
See below for the problem in detail.

> It would help if you described in more detail the problem(s) you are
> having and which part of this patch fixes which part of your
> problem(s).
> 
I met this problem when I am using a touch controller that has a
firmware
That does not work in a graceful way, the controller seems to report
Touch event very frequently and seems the HW is too sensitive, this does
not mean the issue Is caused by the controller, I think this just make
the issue exposed more easily.

The controller is using a GPIO to assert interrupt to OMAP, the
interrupt
Line will be dirven low until the OMAP driver reads out the data through
I2C.
On OMAP side, we configure the GPIO interrupt pin to be low level
detect,
The issue is, after the touch driver calls irq_disable, since it is
empty unless
Set the IRQ_DISABLED flag, so next time only the generic handler
function
handle_level_irq is called, this handler just ack to OMAP GPIO IRQ that
is
To clear the IRQ status and mask the GPIO on OMAP side, but since NO
Touch driver IRQ action is called, so the touch Controller can not gets
acknowledged, then the interrupt line will be always driven low by the
external controller, if we check the IRQ status for that GPIO pin, we
will find
The IRQ is always set. This will prevent PER enterring RET/OFF state
since GPIO
Will not acknowledge the IDLE request. The main purpose of the second
patch
Is to clear the level/edge detect registers for OMAP GPIO when their IRQ
is
Disabled, this can ensure the GPIO IRQ status will not be set and
prevent LPM.


> This GPIO interrupt handling code is very sensitive to changes so we
> need to really understand your problem before making changes here.
> 
I totally undersand this and I am glad to have a thorough understanding
By everyone and maybe we can find a better solution for this issue.
But now seems I need make you guys understand the issue first. :-)

> Also it's quite possible there are bugs in your driver as well.  Is
> there any chance you can reproduce this on a public platform such as
> the 3430SDP using the PM branch?  
> 
> If I use the PM branch on SDP, enable the touchscreen and go into
> suspend, I do not see the GPIO driver keeping the system out of
> retention.  In addition, if I add 
> 
>   enable_irq_wake(gpio_to_irq(ts_gpio));
> 
> to the board init code, I can also use the touchscreen as a wakeup
> source and it does not prevent retention so you should be able to
> reproduce there.
You can say that my touch controller is somehow buggy since it reports
Events very frequently and will always keep the interrupt line low
unless it is
Acknowledged by the touch driver, but I think more important is we can
not
Expect and *assume* all the peripherals to work as well as we expected,
The OMAP GPIO framework should be robust enough to handle such cases
Since it will act as a risky hole for PM on OMAP.

I understand that this will be hard to reproduce if the Touch controller
works
Very gracefully, even for us, we do not see the problem on some other
HWs
Since the controller will not report events/interupts so frequently.

Enable_irq_wake and disable_irq_wake is working regardless of irq
enabled or disabled
Since on OMAP3, they are using IOPAD wakeup(If PER is in RET/OFF) or
module level wakeup(if PER is ACTIVE), and the code is handled by gpio
suspend/resume functions, see more below.

> > 1: irq mask/unmask are typically used by the IRQ handling framework
> > or CHIP IRQ handler to disable and enable IRQ during the IRQ handing
> > procedure; 
> 
> The mask/unmask are also used by the lazy disable feature of the
> generic IRQ code.  More on this below...
I tried to figureout but didn't find any comment on what is the issue
fixed
By this lazy irq disable or what we can benefit from this lazy irq
disable,
Seems the change is mainly for x86.

> 
> The driver's ISR should *never* be called after disable_irq().
> However, the actual interrupt may still fire.  Here is what is
> _supposed_ to happen.
> ............snipped.................
> You could try the patch below[1] which will warn if the triggering
> is not setup properly for a given GPIO IRQ.  This would result
> in a broken lazy disable.
> 
I can confirm, the irq actions is not called in my case after
irq_disabled
Is called even without my patch, but the patch is not related
With the IRQ handling, what it tries to do is to simply find a place
To clear the level/edge detect settings.


> This is where I disagree.  Basically, I think things you are doing in
> disable are either done elsewhere already, or should be.
> 
> IOW, you're new disable hook does the equivalent of:
> 
>   irq_chip->mask(irq)
>   irq_chip->set_wake(irq, 0); /* a.k.a. disable_irq_wake() */
>   irq_chip->ack(irq);
> 
> The lazy disable will take care of the mask (and ack if it's level
> triggered).  And if you don't want your driver's IRQ to be a wakeup
> event, you should also call disable_irq_wake() in your driver if you
> really want wakeup events disabled.
> 
> Your enable hook does he equivalent of
> 
>   irq_chip->set_wake(irq, 1); /* a.k.a. enable_irq_wake(irq); */
>   irq_chip->unmask(irq);
> 
> Which is the same as your driver doing:
> 
>   enable_irq_wake(irq);
>   enable_irq(irq);
> 
> So your driver should be simply add enable_irq_wake() if it wants to
> (re)enable the wakeup feature.
I think you may ignored the most imortant change I want to make. :-)
That is to clear the GPIO level/edge detect settings, since for OMAP,
The GPIO module will set IRQ status based on the settings and
External GPIO input signals and regardless of the IRQ EN settings,
This is the main issue I want to address. Since most likely, some
peripherals
May drive the interrupt line to assert interrupts, there is no issue if
the GPIO
Is enabled by drivers, since the IRQ status will be cleared and the
controller
Will be also ACKed, but think about the case that the IRQ is disabled at
The GPIO HW level which means no even generic IRQ handler will be
Called, thus the IRQ status will be always pending there.

We met more than once such issues that pending GPIO IRQ status will
Prevent low power mode.

This is what I want to address, but there is no such API combination can
Do this unless we have the new disable_irq and enable_irq hooks. Though
Set_irq_type may change the level/detect settings, but we can not
Put such omap specfic hooks in the generic driver code. That is why
I added the two new hooks.In fact, the change does not provide any
new fundamental APIs, It just combine some existing APIs for OMAP
GPIO special settings. 

> 
> Stated in another way, and possibly more simply, disable_irq() does
> not imply disable_irq_wake() and enable_irq() does not imply
> enable_irq_wake().
> 
I unserstand this. Wakeup is indepent of irq enable/disable. The
_set_gpio_wakeup function just sets bank->suspend_wakeup
Which is used in gpio suspend/resume, and specially for OMAP3,
The code configures IOPAD wakeup.


> There is a possible bug here in the case of edge triggered interrupts:
> a disable_irq() will not result in an irq_chip->ack().  Therfore,
> pending IRQs that are not wakeup-enabled should probably be ACK'd in
> the prepare_to_idle hook, or possibly in the set_wake() hook.  In the
> disable case of set_wake(), if the IRQ is disabled, any pending IRQs
> should be cleared.
I think there is no issue with edge interrupt handling since the generic
Handler handle_edge_irq can still be called after disable_irq as long
As the IRQ is pending in the IRQ status register. The generic handler
will
ACK the IRQ(means clear the IRQ status).

I think current set_wake should be ok, no need try to clear IRQ status,
Since IRQ staus will be set *ANY TIME* when the external input signal
Meets the level/edge detect conditions. This is the key point, IRQ
status
Will be set even when IRQEN bit is cleared, that is why we need clear
The level/edge detect also.

> 
> Yes, please send that part as a separate fix as it is a clear bugfix.
> Also, it's probably better to put the TRM reference in the git
> changelog instead of the code as those section numbers will become out
> of date shortly.
Done. Sent in attachement. 0001-Only........patch

> 
> So, IIUC, your drivers ISR is being called after disable_irq()?  If
> so, something has gone horribly wrong.  This should not be possible
> with the genirq framework.  Again, it would be nice to reproduce this
> using the touchscreen on the SDP using the PM branch.  Please try
> with my patch below[1] and tell me if you see any warnings.
> 
No, my driver action handler is not called, but the generic handler is
called,
But the issue is the external interupt signal will not be released
unless
The driver's action handler ack's the external controller.

> 
> Sorry, I said the prepare_for/resume_from idle hooks but I was
> thinking of the suspend/resume hooks....
> 
> > But for OMAP3, there is no non-wakeup gpio, all the GPIOs are wakeup
> > gpios.
> 
> On OMAP3, all GPIOs are wakeup capable, but by non-wakeup GPIO is
> meant GPIOs that are not *configured* for wakeup.   
> 
> The suspend hook will ensure that wakeups are disabled for any non
> wake-enabled GPIO and restore them on resume.
> 
My understanding is we have two majors places that related with GPIO
WAKEUP.
1: suspend/resume hooks, I think this is just for the wakeup from system
wide suspend,
And will only enable wakeup for those GPIOs that the driver explictly
calls
enable_irq_wake, if driver does not call this explicitly, then no wakeup
will happen on that pin.
Suspend/resume hooks will not do anything else.
2: omap2_gpio_prepare_for_idle/omap2_gpio_resume_after_idle, for OMAP2,
It tries to workaround the non-wakeup gpio issue which may cause
spurious interrupts
If we enable edge detection on those GPIOs, but for OMAP3, the hooks are
used similarly
To avoid any edge GPIO interrupt lossing, since once PER enters RET/OFF,
only IOPAD
Wakeup events can happen and only IO wakeup interrupt can be seen by
ARM,
After GPIO context is restored, since the external GPIO input ls already
at a high/low level,
There is no edge can be detected by the GPIO module any more, so we
check the GPIO inputs
And compare it with the input level before IDLE, if it changes, then we
trigger a GPIO IRQ
By setting LEVEL detect register. This is needed for OMAP3 since we also
met the issue that
The edge interrupt is lost during PER RET/OFF.

None of the above hooks can address the issue I am facing now, since
none of the hooks
Will CLEAR the level/edge settings for those GPIO with IRQ disabled.

> >
> 
> Hope this helps,
> 
> Kevin
> 

[-- Attachment #2: 0001-Only-enable-WAKEUPEN-for-edge-detection-GPIOs.patch --]
[-- Type: application/octet-stream, Size: 1762 bytes --]

From c9e0cfc544bd30cf5ecfd77fe03fd52336344b0c Mon Sep 17 00:00:00 2001
From: Chunqiu Wang <cqwang@motorola.com>
Date: Thu, 4 Jun 2009 04:29:02 +0800
Subject: [PATCH] Only enable WAKEUPEN for edge detection GPIOs,
 see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
 For OMAP3, only edge GPIOs may lose interrupts
 when PER enters RET/OFF state, this is addressed
 by gpio prepare|resume idle functions


Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
---
 arch/arm/plat-omap/gpio.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index bd04b40..1335424 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -597,7 +597,11 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 		trigger & IRQ_TYPE_EDGE_FALLING);
 
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
-		if (trigger != 0)
+		/*
+		 * GPIO wakeup request can only be generated on edge
+		 * transitions
+		 */
+		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
 			__raw_writel(1 << gpio, bank->base
 					+ OMAP24XX_GPIO_SETWKUENA);
 		else
@@ -606,7 +610,13 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 	}
 	/* This part needs to be executed always for OMAP34xx */
 	if (cpu_is_omap34xx() || (bank->non_wakeup_gpios & gpio_bit)) {
-		if (trigger != 0)
+		/*
+		 * Log the edge gpio and manually trigger the IRQ
+		 * after resume if the input level changes
+		 * to avoid irq lost during PER RET/OFF mode
+		 * Applies for omap2 non-wakeup gpio and all omap3 gpios
+		 */
+		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
 			bank->enabled_non_wakeup_gpios |= gpio_bit;
 		else
 			bank->enabled_non_wakeup_gpios &= ~gpio_bit;
-- 
1.5.4.3


[-- Attachment #3: 0002-Clear-GPIO-level-edge-detect-settings-when-IRQ-is-di.patch --]
[-- Type: application/octet-stream, Size: 2819 bytes --]

From 68523adaca0a3e5bc7c6b1f1950ff40a41536e65 Mon Sep 17 00:00:00 2001
From: Chunqiu Wang <cqwang@motorola.com>
Date: Thu, 4 Jun 2009 04:40:10 +0800
Subject: [PATCH] Clear GPIO level/edge detect settings when IRQ is disabled by drivers
 Pending IRQ status will prevent PER and CORE transition to RET/OFF state
 GPIO IRQ status bit maybe set even when IRQ is disabled if the input
 signal meets the level/edge detect settings on OMAP2/3.
 See OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1 GPIO interrupt and wakeup enable note.


Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
---
 arch/arm/plat-omap/gpio.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 1335424..c356542 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int enc_time)
 EXPORT_SYMBOL(omap_set_gpio_debounce_time);
 
 #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
-static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
+static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 						int trigger)
 {
 	void __iomem *base = bank->base;
@@ -1143,6 +1143,38 @@ static void gpio_ack_irq(unsigned int irq)
 	_clear_gpio_irqstatus(bank, gpio);
 }
 
+static void gpio_enable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+	int trigger = irq_desc[irq].status & IRQ_TYPE_SENSE_MASK;
+
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), trigger);
+#endif
+	_set_gpio_irqenable(bank, gpio, 1);
+}
+
+static void gpio_disable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+
+	_set_gpio_irqenable(bank, gpio, 0);
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	/*
+	 * Clear the event detect setting and IRQ status to prevent
+	 * IRQ staus been set and pending there While IRQ is disabled
+	 * by drivers, otherwise GPIO module will not acknowledge
+	 * the IDLE request from PER power domain, this may prevent
+	 * System wide retention
+	 */
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);
+	_clear_gpio_irqstatus(bank, gpio);
+#endif
+
+}
+
 static void gpio_mask_irq(unsigned int irq)
 {
 	unsigned int gpio = irq - IH_GPIO_BASE;
@@ -1170,6 +1202,8 @@ static void gpio_unmask_irq(unsigned int irq)
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.shutdown	= gpio_irq_shutdown,
+	.enable		= gpio_enable_irq,
+	.disable	= gpio_disable_irq,
 	.ack		= gpio_ack_irq,
 	.mask		= gpio_mask_irq,
 	.unmask		= gpio_unmask_irq,
-- 
1.5.4.3

Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

"Wang Sawsd-A24013" <cqwang@motorola.com> writes:

> Kevin,
>
> See below for more comments/explanations:
>
>> 
>> I still think there at least a couple different problems going on and
>> I think you are addressing some symptoms but not the root cause.
>> 
> I understand that combine these two changes in one patch may cause
> Some confusion, so this time I separate into two patchs and 
> they are attached.
>
> I agree that I didn't describe the issue more in detail, but I think the
> Second patch (also the original one I intended to make) does address
> The root cause and not workaround the symptoms.
> See below for the problem in detail.
>
>> It would help if you described in more detail the problem(s) you are
>> having and which part of this patch fixes which part of your
>> problem(s).
>> 
> I met this problem when I am using a touch controller that has a
> firmware
> That does not work in a graceful way, the controller seems to report
> Touch event very frequently and seems the HW is too sensitive, this does
> not mean the issue Is caused by the controller, I think this just make
> the issue exposed more easily.
>
> The controller is using a GPIO to assert interrupt to OMAP, the
> interrupt
> Line will be dirven low until the OMAP driver reads out the data through
> I2C.
> On OMAP side, we configure the GPIO interrupt pin to be low level
> detect,

Dumb question: Why use level?  Why not use falling edge for this?

> The issue is, after the touch driver calls irq_disable, since it is
> empty unless
> Set the IRQ_DISABLED flag, so next time only the generic handler
> function
> handle_level_irq is called, this handler just ack to OMAP GPIO IRQ that
> is
> To clear the IRQ status and mask the GPIO on OMAP side, but since NO
> Touch driver IRQ action is called, so the touch Controller can not gets
> acknowledged, then the interrupt line will be always driven low by the
> external controller, 

If the GPIO is set to be edge triggered, the fact that it is still low
won't matter, the genirq layer will have noticed a pending interrupt.

> if we check the IRQ status for that GPIO pin, we
> will find
> The IRQ is always set. This will prevent PER enterring RET/OFF state
> since GPIO
> Will not acknowledge the IDLE request. The main purpose of the second
> patch
> Is to clear the level/edge detect registers for OMAP GPIO when their IRQ
> is
> Disabled, this can ensure the GPIO IRQ status will not be set and
> prevent LPM.

Thanks for the very clear explanation.  I'm pretty sure I understand
what is going on now.

I need to think some more about what you describe below, but am curious
what you think about using edge triggered GPIOs instead of level.

Kevin

>> This GPIO interrupt handling code is very sensitive to changes so we
>> need to really understand your problem before making changes here.
>> 
> I totally undersand this and I am glad to have a thorough understanding
> By everyone and maybe we can find a better solution for this issue.
> But now seems I need make you guys understand the issue first. :-)
>
>> Also it's quite possible there are bugs in your driver as well.  Is
>> there any chance you can reproduce this on a public platform such as
>> the 3430SDP using the PM branch?  
>> 
>> If I use the PM branch on SDP, enable the touchscreen and go into
>> suspend, I do not see the GPIO driver keeping the system out of
>> retention.  In addition, if I add 
>> 
>>   enable_irq_wake(gpio_to_irq(ts_gpio));
>> 
>> to the board init code, I can also use the touchscreen as a wakeup
>> source and it does not prevent retention so you should be able to
>> reproduce there.
> You can say that my touch controller is somehow buggy since it reports
> Events very frequently and will always keep the interrupt line low
> unless it is
> Acknowledged by the touch driver, but I think more important is we can
> not
> Expect and *assume* all the peripherals to work as well as we expected,
> The OMAP GPIO framework should be robust enough to handle such cases
> Since it will act as a risky hole for PM on OMAP.
>
> I understand that this will be hard to reproduce if the Touch controller
> works
> Very gracefully, even for us, we do not see the problem on some other
> HWs
> Since the controller will not report events/interupts so frequently.
>
> Enable_irq_wake and disable_irq_wake is working regardless of irq
> enabled or disabled
> Since on OMAP3, they are using IOPAD wakeup(If PER is in RET/OFF) or
> module level wakeup(if PER is ACTIVE), and the code is handled by gpio
> suspend/resume functions, see more below.
>
>> > 1: irq mask/unmask are typically used by the IRQ handling framework
>> > or CHIP IRQ handler to disable and enable IRQ during the IRQ handing
>> > procedure; 
>> 
>> The mask/unmask are also used by the lazy disable feature of the
>> generic IRQ code.  More on this below...
> I tried to figureout but didn't find any comment on what is the issue
> fixed
> By this lazy irq disable or what we can benefit from this lazy irq
> disable,
> Seems the change is mainly for x86.

The lazy disable is also used so that wakeup interrupts will not be
lost between driver suspend and actual HW suspend.  In addition, the
genirq subsystem allows interrupts between HW wakeup and driver resume
to be resent to the driver.


>> 
>> The driver's ISR should *never* be called after disable_irq().
>> However, the actual interrupt may still fire.  Here is what is
>> _supposed_ to happen.
>> ............snipped.................
>> You could try the patch below[1] which will warn if the triggering
>> is not setup properly for a given GPIO IRQ.  This would result
>> in a broken lazy disable.
>> 
> I can confirm, the irq actions is not called in my case after
> irq_disabled
> Is called even without my patch, but the patch is not related
> With the IRQ handling, what it tries to do is to simply find a place
> To clear the level/edge detect settings.
>
>
>> This is where I disagree.  Basically, I think things you are doing in
>> disable are either done elsewhere already, or should be.
>> 
>> IOW, you're new disable hook does the equivalent of:
>> 
>>   irq_chip->mask(irq)
>>   irq_chip->set_wake(irq, 0); /* a.k.a. disable_irq_wake() */
>>   irq_chip->ack(irq);
>> 
>> The lazy disable will take care of the mask (and ack if it's level
>> triggered).  And if you don't want your driver's IRQ to be a wakeup
>> event, you should also call disable_irq_wake() in your driver if you
>> really want wakeup events disabled.
>> 
>> Your enable hook does he equivalent of
>> 
>>   irq_chip->set_wake(irq, 1); /* a.k.a. enable_irq_wake(irq); */
>>   irq_chip->unmask(irq);
>> 
>> Which is the same as your driver doing:
>> 
>>   enable_irq_wake(irq);
>>   enable_irq(irq);
>> 
>> So your driver should be simply add enable_irq_wake() if it wants to
>> (re)enable the wakeup feature.
> I think you may ignored the most imortant change I want to make. :-)
> That is to clear the GPIO level/edge detect settings, since for OMAP,
> The GPIO module will set IRQ status based on the settings and
> External GPIO input signals and regardless of the IRQ EN settings,
> This is the main issue I want to address. Since most likely, some
> peripherals
> May drive the interrupt line to assert interrupts, there is no issue if
> the GPIO
> Is enabled by drivers, since the IRQ status will be cleared and the
> controller
> Will be also ACKed, but think about the case that the IRQ is disabled at
> The GPIO HW level which means no even generic IRQ handler will be
> Called, thus the IRQ status will be always pending there.
>
> We met more than once such issues that pending GPIO IRQ status will
> Prevent low power mode.
>
> This is what I want to address, but there is no such API combination can
> Do this unless we have the new disable_irq and enable_irq hooks. Though
> Set_irq_type may change the level/detect settings, but we can not
> Put such omap specfic hooks in the generic driver code. That is why
> I added the two new hooks.In fact, the change does not provide any
> new fundamental APIs, It just combine some existing APIs for OMAP
> GPIO special settings. 
>
>> 
>> Stated in another way, and possibly more simply, disable_irq() does
>> not imply disable_irq_wake() and enable_irq() does not imply
>> enable_irq_wake().
>> 
> I unserstand this. Wakeup is indepent of irq enable/disable. The
> _set_gpio_wakeup function just sets bank->suspend_wakeup
> Which is used in gpio suspend/resume, and specially for OMAP3,
> The code configures IOPAD wakeup.
>
>
>> There is a possible bug here in the case of edge triggered interrupts:
>> a disable_irq() will not result in an irq_chip->ack().  Therfore,
>> pending IRQs that are not wakeup-enabled should probably be ACK'd in
>> the prepare_to_idle hook, or possibly in the set_wake() hook.  In the
>> disable case of set_wake(), if the IRQ is disabled, any pending IRQs
>> should be cleared.
> I think there is no issue with edge interrupt handling since the generic
> Handler handle_edge_irq can still be called after disable_irq as long
> As the IRQ is pending in the IRQ status register. The generic handler
> will
> ACK the IRQ(means clear the IRQ status).
>
> I think current set_wake should be ok, no need try to clear IRQ status,
> Since IRQ staus will be set *ANY TIME* when the external input signal
> Meets the level/edge detect conditions. This is the key point, IRQ
> status
> Will be set even when IRQEN bit is cleared, that is why we need clear
> The level/edge detect also.
>
>> 
>> Yes, please send that part as a separate fix as it is a clear bugfix.
>> Also, it's probably better to put the TRM reference in the git
>> changelog instead of the code as those section numbers will become out
>> of date shortly.
> Done. Sent in attachement. 0001-Only........patch
>
>> 
>> So, IIUC, your drivers ISR is being called after disable_irq()?  If
>> so, something has gone horribly wrong.  This should not be possible
>> with the genirq framework.  Again, it would be nice to reproduce this
>> using the touchscreen on the SDP using the PM branch.  Please try
>> with my patch below[1] and tell me if you see any warnings.
>> 
> No, my driver action handler is not called, but the generic handler is
> called,
> But the issue is the external interupt signal will not be released
> unless
> The driver's action handler ack's the external controller.
>
>> 
>> Sorry, I said the prepare_for/resume_from idle hooks but I was
>> thinking of the suspend/resume hooks....
>> 
>> > But for OMAP3, there is no non-wakeup gpio, all the GPIOs are wakeup
>> > gpios.
>> 
>> On OMAP3, all GPIOs are wakeup capable, but by non-wakeup GPIO is
>> meant GPIOs that are not *configured* for wakeup.   
>> 
>> The suspend hook will ensure that wakeups are disabled for any non
>> wake-enabled GPIO and restore them on resume.
>> 
> My understanding is we have two majors places that related with GPIO
> WAKEUP.
> 1: suspend/resume hooks, I think this is just for the wakeup from system
> wide suspend,
> And will only enable wakeup for those GPIOs that the driver explictly
> calls
> enable_irq_wake, if driver does not call this explicitly, then no wakeup
> will happen on that pin.
> Suspend/resume hooks will not do anything else.
> 2: omap2_gpio_prepare_for_idle/omap2_gpio_resume_after_idle, for OMAP2,
> It tries to workaround the non-wakeup gpio issue which may cause
> spurious interrupts
> If we enable edge detection on those GPIOs, but for OMAP3, the hooks are
> used similarly
> To avoid any edge GPIO interrupt lossing, since once PER enters RET/OFF,
> only IOPAD
> Wakeup events can happen and only IO wakeup interrupt can be seen by
> ARM,
> After GPIO context is restored, since the external GPIO input ls already
> at a high/low level,
> There is no edge can be detected by the GPIO module any more, so we
> check the GPIO inputs
> And compare it with the input level before IDLE, if it changes, then we
> trigger a GPIO IRQ
> By setting LEVEL detect register. This is needed for OMAP3 since we also
> met the issue that
> The edge interrupt is lost during PER RET/OFF.
>
> None of the above hooks can address the issue I am facing now, since
> none of the hooks
> Will CLEAR the level/edge settings for those GPIO with IRQ disabled.
>
>> >
>> 
>> Hope this helps,
>> 
>> Kevin
>> 

RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: 2009年6月5日 1:04
> 
> Dumb question: Why use level?  Why not use falling edge for this?
> 
A good question, :-) We did use edge interrupt before, see the reason below.

> > The issue is, after the touch driver calls irq_disable, since it is
> > empty unless
> > Set the IRQ_DISABLED flag, so next time only the generic handler
> > function
> > handle_level_irq is called, this handler just ack to OMAP 
> GPIO IRQ that
> > is
> > To clear the IRQ status and mask the GPIO on OMAP side, but since NO
> > Touch driver IRQ action is called, so the touch Controller 
> can not gets
> > acknowledged, then the interrupt line will be always driven 
> low by the
> > external controller, 
> 
> If the GPIO is set to be edge triggered, the fact that it is still low
> won't matter, the genirq layer will have noticed a pending interrupt.
> 
If we use edge interrupt here, the potential issue is still existing, and also
We are liky to lose the interrupt.
After irq_disable and before HW suspend, if the interrupt line is driven low,
Then OMAP GPIO can catch this edge transition, so the IRQ is set,
Then the handle_edge_irq will clear the IRQ staus and mask the IRQ.
Since the controller is not ACKed, then the interrupt line is always driven low,
Then from then on, no edge can happen, and no more Touch interrupt can happen
Even when irq_enable is called, though we have the prepare for idle hooks,
But that only work when the edge transition happens after that prepare call,
Since it saves the GPIO data IN at that time, if the input level already changes
Before that time, then the workaround does not work. 

We ever made another patch to not only compare the data in, but also check
It is rising or falling edge, and check the CURRENT input level to decide whether to
Use LEVEL detect to manually trigger the interrupt, it works fine with our HW.
But later on, touch cotroller driver is updated to use level detect, then we
Met this issue. I think the patch we made to workaround the edge int lost is also needed
In current pm branch, currently we may still face the issue I mentioned above.

But think more generically, the kernel should be robust enough to handle such cases,
We can not let driver to do the workaround since it is specially OMAP platform issue,
Even we have the patch to workaround the edge GPIO IRQ lost issue, but the
IRQ staus maybe set again if the interrupt line is driven a second edge.
The generic handler will only be called once after irq_disable is called, and
If the interrupt happens more than once after that, then we still face the issue that
The IRQ status is set and pending there and prevent LPM.

I tried to summarize the root cause that we need this change, I think it can be simply
Two points:
1: on OMAP, GPIO IRQ status may be set as long as LEVEL/EDGE 
detect register is configured, regardless of IRQEN or WKEN
2: Pending GPIO IRQ status will impact LPM transition

Hope this can explain more on this patch.

Thanks,
Chunqiu


> > if we check the IRQ status for that GPIO pin, we
> > will find
> > The IRQ is always set. This will prevent PER enterring RET/OFF state
> > since GPIO
> > Will not acknowledge the IDLE request. The main purpose of 
> the second
> > patch
> > Is to clear the level/edge detect registers for OMAP GPIO 
> when their IRQ
> > is
> > Disabled, this can ensure the GPIO IRQ status will not be set and
> > prevent LPM.
> 
> Thanks for the very clear explanation.  I'm pretty sure I understand
> what is going on now.
> 
> I need to think some more about what you describe below, but 
> am curious
> what you think about using edge triggered GPIOs instead of level.
> 
See the above explanation


> 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

Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [090805 17:36]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > Hi,
> >
> > Sorry for the long delay on replying to this one.
> 
> Tony, this one has been superceded, and the irq_enable/disable stuff no
> longer is needed due to the patch
> 
>   OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask
> 
> from my PM fixes queue.
> 
> An updated version (currently in the PM branch) is here:
> http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commit;h=c80a909697ad931ffad9d30ed321469fa699b208

OK, thanks.


> 
> but...
> 

<snip>

> > To me it looks like the right way to deal with the level gpio
> > would be to change it temporarily to be edge for the duration of
> > idle, then change it back to be level after returning from idle.
> 
> Not sure about that.  That would have to be experimented with to 
> see if there are any other side effects.
> 
> In any case, that kind of change would be independent of this change
> as this change just fixes a bug so wake-enables are only set on
> edge-triggered GPIOs.

OK. Just FYI, I think I experimented on that for 2420 at some point
and it worked, but that was several years ago.

Tony

Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

Tony Lindgren <tony@atomide.com> writes:

> Hi,
>
> Sorry for the long delay on replying to this one.

Tony, this one has been superceded, and the irq_enable/disable stuff no
longer is needed due to the patch

  OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask

from my PM fixes queue.

An updated version (currently in the PM branch) is here:
http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commit;h=c80a909697ad931ffad9d30ed321469fa699b208

but...

> * Wang Sawsd-A24013 <cqwang@motorola.com> [090602 02:25]:
>> This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, and
>> also only enable WAKEUPEN
>> When GPIO edge detection is enabled, also clear the gpio event
>> triggering configurations to avoid
>> Pending interrupt status which is generated regardless of the IRQEN and
>> WKUPEN bit, the pending
>> IRQ status may prevent GPIO module acknowledge IDLE request and prevent
>> PER and thus RETENTION.
>> 
>> This is only applied for OMAP2/3 GPIO IRQs.
>> 
>> Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
>> ---
>>  arch/arm/plat-omap/gpio.c |   44
>> ++++++++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 42 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index bd04b40..19e0461 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int
>> enc_time)
>>  EXPORT_SYMBOL(omap_set_gpio_debounce_time);
>>  
>>  #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
>> -static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int
>> gpio,
>> +static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>>  						int trigger)
>>  {
>>  	void __iomem *base = bank->base;
>> @@ -597,7 +597,12 @@ static inline void set_24xx_gpio_triggering(struct
>> gpio_bank *bank, int gpio,
>>  		trigger & IRQ_TYPE_EDGE_FALLING);
>>  
>>  	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
>> -		if (trigger != 0)
>> +		/*
>> +		 * GPIO wakeup request can only be generated on edge
>> +		 * transitions, see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
>> +		 * wake-up request note for detail
>> +		 */
>> +		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
>>  			__raw_writel(1 << gpio, bank->base
>>  					+ OMAP24XX_GPIO_SETWKUENA);
>>  		else
>
> To me it looks like the right way to deal with the level gpio
> would be to change it temporarily to be edge for the duration of
> idle, then change it back to be level after returning from idle.

Not sure about that.  That would have to be experimented with to 
see if there are any other side effects.

In any case, that kind of change would be independent of this change
as this change just fixes a bug so wake-enables are only set on
edge-triggered GPIOs.

Kevin

Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Tony Lindgren]

Hi,

Sorry for the long delay on replying to this one.

* Wang Sawsd-A24013 <cqwang@motorola.com> [090602 02:25]:
> This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, and
> also only enable WAKEUPEN
> When GPIO edge detection is enabled, also clear the gpio event
> triggering configurations to avoid
> Pending interrupt status which is generated regardless of the IRQEN and
> WKUPEN bit, the pending
> IRQ status may prevent GPIO module acknowledge IDLE request and prevent
> PER and thus RETENTION.
> 
> This is only applied for OMAP2/3 GPIO IRQs.
> 
> Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
> ---
>  arch/arm/plat-omap/gpio.c |   44
> ++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index bd04b40..19e0461 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int
> enc_time)
>  EXPORT_SYMBOL(omap_set_gpio_debounce_time);
>  
>  #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> -static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int
> gpio,
> +static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
>  						int trigger)
>  {
>  	void __iomem *base = bank->base;
> @@ -597,7 +597,12 @@ static inline void set_24xx_gpio_triggering(struct
> gpio_bank *bank, int gpio,
>  		trigger & IRQ_TYPE_EDGE_FALLING);
>  
>  	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
> -		if (trigger != 0)
> +		/*
> +		 * GPIO wakeup request can only be generated on edge
> +		 * transitions, see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
> +		 * wake-up request note for detail
> +		 */
> +		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
>  			__raw_writel(1 << gpio, bank->base
>  					+ OMAP24XX_GPIO_SETWKUENA);
>  		else

To me it looks like the right way to deal with the level gpio
would be to change it temporarily to be edge for the duration of
idle, then change it back to be level after returning from idle.


> @@ -1133,6 +1138,39 @@ static void gpio_ack_irq(unsigned int irq)
>  	_clear_gpio_irqstatus(bank, gpio);
>  }
>  
> +static void gpio_enable_irq(unsigned int irq)
> +{
> +	unsigned int gpio = irq - IH_GPIO_BASE;
> +	struct gpio_bank *bank = get_irq_chip_data(irq);
> +	int trigger = irq_desc[irq].status & IRQ_TYPE_SENSE_MASK;
> +
> +#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> +	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), trigger);
> +#endif
> +	_set_gpio_irqenable(bank, gpio, 1);
> +}
> +
> +static void gpio_disable_irq(unsigned int irq)
> +{
> +	unsigned int gpio = irq - IH_GPIO_BASE;
> +	struct gpio_bank *bank = get_irq_chip_data(irq);
> +
> +	_set_gpio_irqenable(bank, gpio, 0);
> +#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> +	/*
> +	 * Clear the event detect setting and IRQ status to prevent
> +	 * IRQ staus been set or pending there While IRQ is disabled,
> +	 * otherwise GPIO module will not acknowledge the IDLE request
> +	 * from PER power domain, this may prevent System wide retention
> +	 * See OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1 GPIO interrupt and
> wakeup
> +	 * enable note for detail
> +	 */
> +	set_24xx_gpio_triggering(bank, get_gpio_index(gpio),
> IRQ_TYPE_NONE);
> +	_clear_gpio_irqstatus(bank, gpio);
> +#endif
> +
> +}
> +
>  static void gpio_mask_irq(unsigned int irq)
>  {
>  	unsigned int gpio = irq - IH_GPIO_BASE;
> @@ -1160,6 +1198,8 @@ static void gpio_unmask_irq(unsigned int irq)
>  static struct irq_chip gpio_irq_chip = {
>  	.name		= "GPIO",
>  	.shutdown	= gpio_irq_shutdown,
> +	.enable		= gpio_enable_irq,
> +	.disable	= gpio_disable_irq,
>  	.ack		= gpio_ack_irq,
>  	.mask		= gpio_mask_irq,
>  	.unmask		= gpio_unmask_irq,

Also, this irq_enable/disable should be a separate patch.

Regards,

Tony

RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: 2009年6月6日 5:35
> To: Wang Sawsd-A24013
> Cc: linux-omap@vger.kernel.org; nm@ti.com; Mike Chan
> Subject: Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status 
> been set after irq_disable
> 
> "Wang Sawsd-A24013" <cqwang@motorola.com> writes:
> 
> >> Could you try this patch with your TS GPIO configured as 
> >> level-triggered?
> >> 
> >
> > I tried the below patch, it can solve the issue also.
> >
> 
> Thanks for testing.
> 
> This whole time, you're probably wondering... "why doesn't Kevin just
> add the disable and enable hooks and be done with it."  I don't think
> I've explained myself there yet so let me try to explain so I don't
> come across as refusing your original idea for no good reason other
> than being stubborn.
> 
> We basically have the option of doing it in my proposed patch, which
> adds additional overhead to the mask/unmask hooks or we can just add
> disable/enable hooks as you did in your original patch.
> 
> The reason I prefer not to add disable/enable hooks is to take
> advantage of the lazy disable feature.  With lazy disable, by waiting
> to actually disable, it allows us to handle potentially lost
> interrupts and this is especially important for wakeup interrupts.  An
> interesting example this is interrupts that are lazy disabled during
> suspend/resume.
> 
> For example, if a wakeup interrupt happens between the drivers
> disable_irq(), what you want is for this interrupt to cancel suspend.
> If you you implement a ->disable hook, and thus mask interrupts in HW
> immediately, you will loose this interrupt.  With lazy disable, the
> drivers ISR will not be called, but genirq will see this interrupt and
> set IRQ_PENDING.  Late in the suspend path, IRQs that are IRQ_WAKEUP
> and IRQ_PENDING will cancel suspend (see
> kernel/irq/pm.c:check_wakeup_irqs())
> 
> Another possible scenario is an interrupt that happens between HW
> resume and the drivers enable_irq() hook.  With lazy disable, this
> interrupt will be IRQ_PENDING when enable_irq() gets called and be
> triggered/handled immedately.
> 
> Hope that helps explain my stubborness, ;)
> 
> Of course this doesn't mean it's an absolute decision.  As always, I'm
> certainly open to being pursuaded that there are other better reasons
> for doing it differently.
> 
> Kevin
> 

Kevin, I am totally ok with your change. :-)

Why I  didn't modify mask/unmask is only because I won't to impact the generic
IRQ handler with overhead, but as you said, lazy irq disable is good
To not lose interrupt before suspend, we should benefit from it as much as possible.

Your change is pretty good, more important is I am glad to see the issue
Is understood by others and we found a solution for it, I care nothing about
Whether it is addressed by using my original patch or not. :-)

Thanks,
Chunqiu
--
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] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

"Wang Sawsd-A24013" <cqwang@motorola.com> writes:

>> Could you try this patch with your TS GPIO configured as 
>> level-triggered?
>> 
>
> I tried the below patch, it can solve the issue also.
>

Thanks for testing.

This whole time, you're probably wondering... "why doesn't Kevin just
add the disable and enable hooks and be done with it."  I don't think
I've explained myself there yet so let me try to explain so I don't
come across as refusing your original idea for no good reason other
than being stubborn.

We basically have the option of doing it in my proposed patch, which
adds additional overhead to the mask/unmask hooks or we can just add
disable/enable hooks as you did in your original patch.

The reason I prefer not to add disable/enable hooks is to take
advantage of the lazy disable feature.  With lazy disable, by waiting
to actually disable, it allows us to handle potentially lost
interrupts and this is especially important for wakeup interrupts.  An
interesting example this is interrupts that are lazy disabled during
suspend/resume.

For example, if a wakeup interrupt happens between the drivers
disable_irq(), what you want is for this interrupt to cancel suspend.
If you you implement a ->disable hook, and thus mask interrupts in HW
immediately, you will loose this interrupt.  With lazy disable, the
drivers ISR will not be called, but genirq will see this interrupt and
set IRQ_PENDING.  Late in the suspend path, IRQs that are IRQ_WAKEUP
and IRQ_PENDING will cancel suspend (see
kernel/irq/pm.c:check_wakeup_irqs())

Another possible scenario is an interrupt that happens between HW
resume and the drivers enable_irq() hook.  With lazy disable, this
interrupt will be IRQ_PENDING when enable_irq() gets called and be
triggered/handled immedately.

Hope that helps explain my stubborness, ;)

Of course this doesn't mean it's an absolute decision.  As always, I'm
certainly open to being pursuaded that there are other better reasons
for doing it differently.

Kevin

RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

> 
> Could you try this patch with your TS GPIO configured as 
> level-triggered?
> 
> Kevin
> 
I tried the below patch, it can solve the issue also.

> commit f8eb69a2edd684c9e0b72bc3c84c6af9718bd4a4
> Author: Kevin Hilman <khilman@deeprootsystems.com>
> Date:   Thu Jun 4 15:57:10 2009 -0700
> 
>     OMAP: GPIO: clear/restore level/edge detect settings on 
> mask/unmask
>     
>     <needs detailed description>
> 
>     Signed-off-by: Kevin Hilman <khilman@ti.deeprootsystems.com>
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 3b2054b..83ac494 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -1135,6 +1135,7 @@ static void gpio_mask_irq(unsigned int irq)
>  	struct gpio_bank *bank = get_irq_chip_data(irq);
>  
>  	_set_gpio_irqenable(bank, gpio, 0);
> +	_set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);
>  }
>  
>  static void gpio_unmask_irq(unsigned int irq)
> @@ -1142,6 +1143,11 @@ static void gpio_unmask_irq(unsigned int irq)
>  	unsigned int gpio = irq - IH_GPIO_BASE;
>  	struct gpio_bank *bank = get_irq_chip_data(irq);
>  	unsigned int irq_mask = 1 << get_gpio_index(gpio);
> +	struct irq_desc *desc = irq_to_desc(irq);
> +	u32 trigger = desc->status & IRQ_TYPE_SENSE_MASK;
> +
> +	if (trigger)
> +		_set_gpio_triggering(bank, 
> get_gpio_index(gpio), trigger);
>  
>  	/* For level-triggered GPIOs, the clearing must be done after
>  	 * the HW source is cleared, thus after the handler has run */
> --
> 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] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

"Wang Sawsd-A24013" <cqwang@motorola.com> writes:

>  
>> What do you think about disabling the level/edge detection when
>> disable_irq_wake() is called instead?  This seems more logical
>> and expected.
>
> Kevin, if we look at the current code, enable_irq_wake and
> disable_irq_wake Does not even touch any GPIO WAKEEN register, it
> seems it is intended To just log the gpio bit and enable its WAKEUP
> and IOPAD wakeup when suspend happens.

Correct.

> And also, enable_irq_wake/disable_irq_wake
> Are designed to be able used when both IRQ is enabled AND disabled,
> In another words, enable_irq_wake may be called after irq_disable,
> Disable_irq_wake may be called after irq_enable, if we change
> Level/edge detect then it may cause either IRQ never happen

Good point.

> After irq_enable, or IRQ staus bit also set after irq_disable. Since
> The root reason is the level/edge detect can cause IRQ status, it
> Is related with IRQ, not wakeup.

Correct again.

> What do you think?

I'm thinking I'm not thinking very clearly on the subject today.  It's
too hot in Seattle today.  ;)

I'm also thinking that this isn't just going to be a problem with
suspend/resume but also for hitting retention in idle.  Any
level-triggered GPIO IRQ that is masked, yet still has level/edge
detect configured can prevent retention during idle since it will
cause IRQ status as you've pointed out.

Can you think of any reason not to disable the level/edge detect in
the ->mask() hook and to re-enable it in the ->unmask hook?  Something
like the patch below?

Could you try this patch with your TS GPIO configured as level-triggered?

Kevin

commit f8eb69a2edd684c9e0b72bc3c84c6af9718bd4a4
Author: Kevin Hilman <khilman@deeprootsystems.com>
Date:   Thu Jun 4 15:57:10 2009 -0700

    OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask
    
    <needs detailed description>

    Signed-off-by: Kevin Hilman <khilman@ti.deeprootsystems.com>

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 3b2054b..83ac494 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -1135,6 +1135,7 @@ static void gpio_mask_irq(unsigned int irq)
 	struct gpio_bank *bank = get_irq_chip_data(irq);
 
 	_set_gpio_irqenable(bank, gpio, 0);
+	_set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);
 }
 
 static void gpio_unmask_irq(unsigned int irq)
@@ -1142,6 +1143,11 @@ static void gpio_unmask_irq(unsigned int irq)
 	unsigned int gpio = irq - IH_GPIO_BASE;
 	struct gpio_bank *bank = get_irq_chip_data(irq);
 	unsigned int irq_mask = 1 << get_gpio_index(gpio);
+	struct irq_desc *desc = irq_to_desc(irq);
+	u32 trigger = desc->status & IRQ_TYPE_SENSE_MASK;
+
+	if (trigger)
+		_set_gpio_triggering(bank, get_gpio_index(gpio), trigger);
 
 	/* For level-triggered GPIOs, the clearing must be done after
 	 * the HW source is cleared, thus after the handler has run */

RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

 
> What do you think about disabling the level/edge detection when
> disable_irq_wake() is called instead?  This seems more logical
> and expected.
Kevin, if we look at the current code, enable_irq_wake and
disable_irq_wake
Does not even touch any GPIO WAKEEN register, it seems it is intended
To just log the gpio bit and enable its WAKEUP and IOPAD wakeup
when suspend happens. And also, enable_irq_wake/disable_irq_wake
Are designed to be able used when both IRQ is enabled AND disabled,
In another words, enable_irq_wake may be called after irq_disable,
Disable_irq_wake may be called after irq_enable, if we change
Level/edge detect then it may cause either IRQ never happen
After irq_enable, or IRQ staus bit also set after irq_disable. Since
The root reason is the level/edge detect can cause IRQ status, it
Is related with IRQ, not wakeup.

What do you think?
> 
> Kevin
> 
> P.S., are you wanting to use your touchscreen as a wakeup source?
> 

Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Kevin Hilman]

"Wang Sawsd-A24013" <cqwang@motorola.com> writes:

>  
>
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org 
>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Wang 
>> Sawsd-A24013
>> Sent: 2009年6月5日 1:43
>> To: Kevin Hilman
>> Cc: linux-omap@vger.kernel.org; nm@ti.com; Mike Chan
>> Subject: RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status 
>> been set after irq_disable
>> 
>> 
>> 
>> > -----Original Message-----
>> > From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> > Sent: 2009年6月5日 1:04
>> > 
>> > Dumb question: Why use level?  Why not use falling edge for this?
>> > 
>> A good question, :-) We did use edge interrupt before, see 
>> the reason below.
>> 
>> > > The issue is, after the touch driver calls irq_disable, 
>> since it is
>> > > empty unless
>> > > Set the IRQ_DISABLED flag, so next time only the generic handler
>> > > function
>> > > handle_level_irq is called, this handler just ack to OMAP 
>> > GPIO IRQ that
>> > > is
>> > > To clear the IRQ status and mask the GPIO on OMAP side, 
>> but since NO
>> > > Touch driver IRQ action is called, so the touch Controller 
>> > can not gets
>> > > acknowledged, then the interrupt line will be always driven 
>> > low by the
>> > > external controller, 
>> > 
>> > If the GPIO is set to be edge triggered, the fact that it 
>> is still low
>> > won't matter, the genirq layer will have noticed a pending 
>> interrupt.
>> > 
>> If we use edge interrupt here, the potential issue is still 
>> existing, and also
>> We are liky to lose the interrupt.
>> After irq_disable and before HW suspend, if the interrupt 
>> line is driven low,
>> Then OMAP GPIO can catch this edge transition, so the IRQ is set,
>> Then the handle_edge_irq will clear the IRQ staus and mask the IRQ.
>> Since the controller is not ACKed, then the interrupt line is 
>> always driven low,
>> Then from then on, no edge can happen, and no more Touch 
>> interrupt can happen
>> Even when irq_enable is called, though we have the prepare 
>> for idle hooks,
>> But that only work when the edge transition happens after 
>> that prepare call,
>> Since it saves the GPIO data IN at that time, if the input 
>> level already changes
>> Before that time, then the workaround does not work. 
>> 
>> We ever made another patch to not only compare the data in, 
>> but also check
>> It is rising or falling edge, and check the CURRENT input 
>> level to decide whether to
>> Use LEVEL detect to manually trigger the interrupt, it works 
>> fine with our HW.
>> But later on, touch cotroller driver is updated to use level 
>> detect, then we
>> Met this issue. I think the patch we made to workaround the 
>> edge int lost is also needed
>> In current pm branch, currently we may still face the issue I 
>> mentioned above.
>
> I rechecked the code, seems the issue will not be there since
> The handle_edge_irq can resend irq during resume time, then

Yes, I was actually replying to ask you to check why the retrigger
wasn't happening in your kernel.  

> I checked our issue log and found, the reason that we lose
> The edge interrupt is because we were using omapzoom kernel

I was starting to think you were not actually using the linux-omap
kernel (looks like I was right.)

> And put PER to fully HW supervised mode and we didn't use
> The prepare idle hooks in our idle function call, then the issues
> Happens when PER is in RET/OFF state but the touch interrupt happens.
>
> With linux-omap kernel, seems using edge interrupt can just workaround
> The touch issue, 

Good.

>but I think the issue is still there, we can not expect All the GPIO
>interrupts to be edge type, and we can not expect all the edge
>Interrupts to fire only once, with open platforms, every kind of
>peripherals We may use, 

I completely agree.  You have definitely found a robustness problem in
the GPIO core that will be relatively easy to run into in the future.

>the change to root fix this problem should be still clearing The
>level/edge detection when irq_disable is called. This will not cause
>Extra interrupt loss since we still have the prepare/resume hooks to
>check Gpio input and retrigger the interrupts.

What do you think about disabling the level/edge detection when
disable_irq_wake() is called instead?  This seems more logical
and expected.

Kevin

P.S., are you wanting to use your touchscreen as a wakeup source?
--
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

[PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable

[Wang Sawsd-A24013]

This patch adds irq_enable and irq_disable for OMAP GPIO IRQ chip, and
also only enable WAKEUPEN
When GPIO edge detection is enabled, also clear the gpio event
triggering configurations to avoid
Pending interrupt status which is generated regardless of the IRQEN and
WKUPEN bit, the pending
IRQ status may prevent GPIO module acknowledge IDLE request and prevent
PER and thus RETENTION.

This is only applied for OMAP2/3 GPIO IRQs.

Signed-off-by: Chunqiu Wang <cqwang@motorola.com>
---
 arch/arm/plat-omap/gpio.c |   44
++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index bd04b40..19e0461 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -581,7 +581,7 @@ void omap_set_gpio_debounce_time(int gpio, int
enc_time)
 EXPORT_SYMBOL(omap_set_gpio_debounce_time);
 
 #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
-static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int
gpio,
+static void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 						int trigger)
 {
 	void __iomem *base = bank->base;
@@ -597,7 +597,12 @@ static inline void set_24xx_gpio_triggering(struct
gpio_bank *bank, int gpio,
 		trigger & IRQ_TYPE_EDGE_FALLING);
 
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
-		if (trigger != 0)
+		/*
+		 * GPIO wakeup request can only be generated on edge
+		 * transitions, see OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1
+		 * wake-up request note for detail
+		 */
+		if ((trigger & IRQ_TYPE_EDGE_BOTH) != 0)
 			__raw_writel(1 << gpio, bank->base
 					+ OMAP24XX_GPIO_SETWKUENA);
 		else
@@ -1133,6 +1138,39 @@ static void gpio_ack_irq(unsigned int irq)
 	_clear_gpio_irqstatus(bank, gpio);
 }
 
+static void gpio_enable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+	int trigger = irq_desc[irq].status & IRQ_TYPE_SENSE_MASK;
+
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio), trigger);
+#endif
+	_set_gpio_irqenable(bank, gpio, 1);
+}
+
+static void gpio_disable_irq(unsigned int irq)
+{
+	unsigned int gpio = irq - IH_GPIO_BASE;
+	struct gpio_bank *bank = get_irq_chip_data(irq);
+
+	_set_gpio_irqenable(bank, gpio, 0);
+#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
+	/*
+	 * Clear the event detect setting and IRQ status to prevent
+	 * IRQ staus been set or pending there While IRQ is disabled,
+	 * otherwise GPIO module will not acknowledge the IDLE request
+	 * from PER power domain, this may prevent System wide retention
+	 * See OMAP34xx_ES3.1_TRM_V_Q G25.5.3.1 GPIO interrupt and
wakeup
+	 * enable note for detail
+	 */
+	set_24xx_gpio_triggering(bank, get_gpio_index(gpio),
IRQ_TYPE_NONE);
+	_clear_gpio_irqstatus(bank, gpio);
+#endif
+
+}
+
 static void gpio_mask_irq(unsigned int irq)
 {
 	unsigned int gpio = irq - IH_GPIO_BASE;
@@ -1160,6 +1198,8 @@ static void gpio_unmask_irq(unsigned int irq)
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.shutdown	= gpio_irq_shutdown,
+	.enable		= gpio_enable_irq,
+	.disable	= gpio_disable_irq,
 	.ack		= gpio_ack_irq,
 	.mask		= gpio_mask_irq,
 	.unmask		= gpio_unmask_irq,
-- 
1.5.4.3