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

Message ID 87d49jeh7p.fsf@deeprootsystems.com
State Superseded
Delegated to: Kevin Hilman
Headers show

Commit Message

Kevin Hilman June 4, 2009, 11:01 p.m. UTC
"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>

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

Comments

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

Patch
diff mbox

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