OMAP2/3 Avoid GPIO pending irq status been set after irq_disable
diff mbox

Message ID B00E06E2766C2744B022DE9BAF3C59D532D19D@zmy16exm69.ds.mot.com
State Superseded
Delegated to: Kevin Hilman
Headers show

Commit Message

Wang Sawsd-A24013 June 1, 2009, 11:49 p.m. UTC
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(-)

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

Comments

Kevin Hilman June 2, 2009, 3:11 p.m. UTC | #1
"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
--
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
Wang Sawsd-A24013 June 2, 2009, 5:18 p.m. UTC | #2
> "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
>

Patch
diff mbox

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)