arm: omap: gpio: define .disable callback for gpio irq chip
Subsystems (2)
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Currently, if one calls disable_irq(gpio_irq), the irq won't get disabled.
This is happening because the omap gpio code defines only a .mask callback. And the default_disable function is just a stub. The result is that, when someone calls disable_irq for an irq in a gpio line, it will be kept enabled.
This patch solves this issue by setting the .disable callback to point to the same .mask callback.
--- arch/arm/plat-omap/gpio.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/plat-omap/gpio.c +2 -0
@@ -1433,6 +1433,7 @@ static struct irq_chip gpio_irq_chip = { .shutdown = gpio_irq_shutdown, .ack = gpio_ack_irq, .mask = gpio_mask_irq, + .disable = gpio_mask_irq, .unmask = gpio_unmask_irq, .set_type = gpio_irq_type, .set_wake = gpio_wake_enable,
@@ -1469,6 +1470,7 @@ static struct irq_chip mpuio_irq_chip = { .name = "MPUIO", .ack = mpuio_ack_irq, .mask = mpuio_mask_irq, + .disable = mpuio_mask_irq, .unmask = mpuio_unmask_irq, .set_type = gpio_irq_type,#ifdef CONFIG_ARCH_OMAP16XX
Signature · 1 line
1.6.3.GIT
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Quoted 10 lines · Eduardo Valentin
Currently, if one calls
disable_irq(gpio_irq), the irq won't get disabled.This is happening because the omap gpio code defines only a .mask callback. And the
default_disablefunction is just a stub. The result is that, when someone callsdisable_irqfor an irq in a gpio line, it will be kept enabled.This patch solves this issue by setting the .disable callback to point to the same .mask callback.
Amd this is a problem because?
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Hello Russell,
Quoted 9 lines · Russell King
Quoted 8 lines · Eduardo Valentin
Currently, if one calls
disable_irq(gpio_irq), the irq won't get disabled.This is happening because the omap gpio code defines only a .mask callback. And the
default_disablefunction is just a stub. The result is that, when someone callsdisable_irqfor an irq in a gpio line, it will be kept enabled.
Quoted 4 lines
Quoted 2 lines
This patch solves this issue by setting the .disable callback to point to the same .mask callback.
Amd this is a problem because?
errr.. because the interrupt is enabled when it was supposed to be disabled?
Quoted 5 lines
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
Right.. I didn't see from this point. I will check how that gets unmasked. But even so, if I understood correctly what you described, it would still open a time window which the system would see at least 1 interrupt during the time it was not suppose to. And that can wakeup a system which is in deep sleep mode, either via dynamic idle or static suspend.
It is unlikely, I know. But it can still happen. And can be avoided.
Regards,
Signature · 1 line
Eduardo Valentin
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Quoted 5 lines · Eduardo Valentin
Quoted 4 lines
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
Quoted 7 lines
Right.. I didn't see from this point. I will check how that gets unmasked. But even so, if I understood correctly what you described, it would still open a time window which the system would see at least 1 interrupt during the time it was not suppose to. And that can wakeup a system which is in deep sleep mode, either via dynamic idle or static suspend.
It is unlikely, I know. But it can still happen. And can be avoided.
Maybe a system going into deep sleep mode should update the masked state of the interrupts to reflect the lazy-disable state?
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Quoted 2 lines
o lazy-disable state
This is an odd state, and confusion regularly comes up ... I've never been a fan of having the imperatively named disable_irq() act like a
disable_irq_at a_random_later_time_ _but_nyet(). If
one must have the latter function, clearer IMO
to name it better and have disable_irq()
do exactly that by the time it returns ... that
is after all what folk expect given its name and conventional interpretation of "disable".
(ergo confusion when that's not what happens)
lazy_disable_irq() would be accurate, butI'm not sure many folk would choose to use it.
- Dave
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
In reply to Eduardo ValentinQuoted 12 lines · Eduardo Valentin
Hello Russell,
Quoted 9 lines · Russell King
Quoted 8 lines · Eduardo Valentin
Currently, if one calls
disable_irq(gpio_irq), the irq won't get disabled.This is happening because the omap gpio code defines only a .mask callback. And the
default_disablefunction is just a stub. The result is that, when someone callsdisable_irqfor an irq in a gpio line, it will be kept enabled.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
This patch solves this issue by setting the .disable callback to point to the same .mask callback.
Amd this is a problem because?
errr.. because the interrupt is enabled when it was supposed to be disabled?
As Russell pointed out, it's not actually "supposed" to be.
With lazy disable, what disable_irq() does is prevent the *handler* from ever being called. If another interrupt arrives, it will be caught by the genirq core, marked as IRQ_PENDING and then masked. This "don't disable unless we really have to" is the desired behavior of the lazy disable feature.
Quoted 6 lines
Quoted 5 lines
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
Quoted 7 lines
Right.. I didn't see from this point. I will check how that gets unmasked. But even so, if I understood correctly what you described, it would still open a time window which the system would see at least 1 interrupt during the time it was not suppose to. And that can wakeup a system which is in deep sleep mode, either via dynamic idle or static suspend.
It is unlikely, I know. But it can still happen. And can be avoided.
If the GPIO is configured as a wakeup source, then wouldn't you want activity on that GPIO to wake up the system?
If you don't want wakeups on that GPIO, then the driver should probably be using disable_irq_wake().
Kevin
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Quoted 22 lines · ext Kevin Hilman
Quoted 12 lines · Eduardo Valentin
Hello Russell,
Quoted 9 lines · Russell King
Quoted 8 lines · Eduardo Valentin
Currently, if one calls
disable_irq(gpio_irq), the irq won't get disabled.This is happening because the omap gpio code defines only a .mask callback. And the
default_disablefunction is just a stub. The result is that, when someone callsdisable_irqfor an irq in a gpio line, it will be kept enabled.Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
This patch solves this issue by setting the .disable callback to point to the same .mask callback.
Amd this is a problem because?
errr.. because the interrupt is enabled when it was supposed to be disabled?
Quoted 7 lines
As Russell pointed out, it's not actually "supposed" to be.
With lazy disable, what
disable_irq()does is prevent the *handler* from ever being called. If another interrupt arrives, it will be caught by the genirq core, marked asIRQ_PENDINGand then masked. This "don't disable unless we really have to" is the desired behavior of the lazy disable feature.
Right. I'm convinced that the handler won't be called because of the lazy disable mechanism.
Quoted 13 lines
Quoted 13 lines
Quoted 6 lines
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
Right.. I didn't see from this point. I will check how that gets unmasked. But even so, if I understood correctly what you described, it would still open a time window which the system would see at least 1 interrupt during the time it was not suppose to. And that can wakeup a system which is in deep sleep mode, either via dynamic idle or static suspend.
Quoted 4 lines
Quoted 1 line
It is unlikely, I know. But it can still happen. And can be avoided.
If the GPIO is configured as a wakeup source, then wouldn't you want activity on that GPIO to wake up the system?
Yes I would want it.. of course, if the interrupt is enabled though..
I'm still trying to find a valid situation where someone disables an irq but still wants its activity to be a wakeup source. I couldn't find so far..
Quoted 3 lines
If you don't want wakeups on that GPIO, then the driver should probably be using
disable_irq_wake().
Yes. Let's take this situation. Let's assume a driver, at its suspend callback, explicitly reports to the system that its irq can be disabled and also should not be seen as a wakeup source, by calling disable_irq(gpio_irq) and disable_irq_wake(gpio_irq).
What should be the expected system behavior when the user says echo mem > /sys/power/state?
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Quoted 80 lines · Eduardo Valentin
Quoted 22 lines · ext Kevin Hilman
Quoted 12 lines · Eduardo Valentin
Hello Russell,
Quoted 9 lines · Russell King
Quoted 8 lines · Eduardo Valentin
Currently, if one calls
disable_irq(gpio_irq), the irq won't get disabled.This is happening because the omap gpio code defines only a .mask callback. And the
default_disablefunction is just a stub. The result is that, when someone callsdisable_irqfor an irq in a gpio line, it will be kept enabled.Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
This patch solves this issue by setting the .disable callback to point to the same .mask callback.
Amd this is a problem because?
errr.. because the interrupt is enabled when it was supposed to be disabled?
Quoted 7 lines
As Russell pointed out, it's not actually "supposed" to be.
With lazy disable, what
disable_irq()does is prevent the *handler* from ever being called. If another interrupt arrives, it will be caught by the genirq core, marked asIRQ_PENDINGand then masked. This "don't disable unless we really have to" is the desired behavior of the lazy disable feature.Right. I'm convinced that the handler won't be called because of the lazy disable mechanism.
Quoted 13 lines
Quoted 13 lines
Quoted 6 lines
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
Right.. I didn't see from this point. I will check how that gets unmasked. But even so, if I understood correctly what you described, it would still open a time window which the system would see at least 1 interrupt during the time it was not suppose to. And that can wakeup a system which is in deep sleep mode, either via dynamic idle or static suspend.
Quoted 4 lines
Quoted 1 line
It is unlikely, I know. But it can still happen. And can be avoided.
If the GPIO is configured as a wakeup source, then wouldn't you want activity on that GPIO to wake up the system?
Yes I would want it.. of course, if the interrupt is enabled though..
I'm still trying to find a valid situation where someone disables an irq but still wants its activity to be a wakeup source. I couldn't find so far..
Quoted 3 lines
If you don't want wakeups on that GPIO, then the driver should probably be using
disable_irq_wake().Yes. Let's take this situation. Let's assume a driver, at its suspend callback, explicitly reports to the system that its irq can be disabled and also should not be seen as a wakeup source, by calling
disable_irq(gpio_irq)anddisable_irq_wake(gpio_irq).What should be the expected system behavior when the user says echo mem >
/sys/power/state?From the point we are done with devices suspend, that gpio will still be marked as an irq, at the bank level. But its corresponding pad will have its wakeup bit disabled. Would that work? I think yes, in most cases.
Now let's take the WFI instruction as a divisor for 2 situations:
A - common / working case: an interrupt on that gpio still happens after the WFI instruction. Since the system is in sleep mode and the pad for that gpio is not wakeup capable, then nothing would happen and the system won't wakeup. Then, I think everyone is happy here.
Quoted 6 lines
B - corner case: an interrupt on that gpio happens before the WFI instruction. Since the system is active, the gpio bank can still report this interrupt and will do it. The suspend won't happen due to a irq which has been explicitly marked as disabled and wakeup incapable.
Then, would that be the expected behavior? Assuming that the driver has explicitly said to the system, you should not bother about this at all.
Well, expected behaviour would be that GPIO bank should not be reporting the this interrupt at all since it has been disabled.
However, since you're asking, I assume that you're not seeing this expected behavior.
Ignoring wakeups for a second, if this corner case happens on a non-wakeup capable GPIO using lazy disable, I would not expect suspend to be prevented. The genirq core would see the IRQ, mark it as IRQ_PENDING, mask it and return. and suspend should continue.
hmm... however, if the IRQ happens after interrupts are disabled, the genirq core won't handle it, and our PM core will see a pending interrupt and abort idle/suspend.
Are you seeing this corner case for a wakeup-enable GPIO or a non wakeup-enabled GPIO?
/me looks at code
I'm assuming now it's for a wakeup-enabled GPIO.
Another more likely possibility is that the IRQ arrives between the time the driver does its disable_irq_wake() and when the GPIO driver actually suspends. We currently defer disalbing the bank-level IRQ wakeup capabilities until the suspend method of the GPIO driver is run (which is very late in the suspend cycle, since it is a sysdev.) Thinking about this some more, I'm not sure exactly why we do this. The current code seems to only manage GPIO wakeups for suspend/resume but not for idle, so it defers the actual register writes until the suspend hook. Since we presumably also want wakeup-enabled GPIOs to wake up from idle, we probably shouldn't be deferring the wakeup enable/disable.
Here's an expirement. If you have a use case that is preventing a suspend in this corner case, let's try to immediately enable/disable wakeups instead of waiting for the suspend/resume hooks.
Below is a test patch only briefly tested on Zoom3 which has it's network interface on a GPIO IRQ. It does not have wakeups enabled, and under ping flood from a host (ping -f), it suspended just fine. I added an enable_irq_wake() the driver and it was still able to suspend during ping flood.
If you suspect the above might be happening in your corner case, can you give this a try to see if it changes?
It's also worth noting that we are currently not managing IO pad wakeups in the GPIO core. We are only mananging GPIO module level wakeups, which only are in effect if CORE is active. Now that we have a mux framework that can probably handle this, we need a mapping of GPIO lines to pads so we can also manage IO pad level wakeups from the GPIO core code. However, if your corner case is arriving before WFI, than I suspect CORE is active, and module level wakeups is what you're running into.
Kevin
commit 3af8a1051a44462c62c5a5d47a8256626e32fbba
Author: Kevin Hilman <[email protected]> Date: Thu Jan 6 09:45:24 2011 -0800
GPIO: enable/disable wakeups immediately
arch/arm/plat-omap/gpio.c +28 -109
@@ -977,34 +977,39 @@ static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enastatic int _set_gpio_wakeup (struct gpio_bank *bank,int gpio,int enable) {unsigned long uninitialized_var (flags ); +void __iomem *wake_status; +void __iomem *wake_clear; +void __iomem *wake_set; + +printk ("KJH: %s: bank IRQ %d, GPIO %d enable=%d\n" , __func__, + bank->irq, gpio, enable);switch (bank->method) {#ifdef CONFIG_ARCH_OMAP16XX case METHOD_MPUIO :case METHOD_GPIO_1610 : -spin_lock_irqsave (&bank->lock, flags); -if (enable) - bank->suspend_wakeup |= (1 << gpio); -else - bank->suspend_wakeup &= ~(1 << gpio); -spin_unlock_irqrestore (&bank->lock, flags); + wake_status = bank->base +OMAP1610_GPIO_WAKEUPENABLE ; + wake_clear = bank->base +OMAP1610_GPIO_CLEAR_WAKEUPENA ; + wake_set = bank->base +OMAP1610_GPIO_SET_WAKEUPENA ;return 0 ;#endif #ifdef CONFIG_ARCH_OMAP2PLUS case METHOD_GPIO_24XX : -case METHOD_GPIO_44XX : + wake_status = bank->base +OMAP24XX_GPIO_WAKE_EN ; + wake_clear = bank->base +OMAP24XX_GPIO_CLEARWKUENA ; + wake_set = bank->base +OMAP24XX_GPIO_SETWKUENA ; +if (bank->non_wakeup_gpios & (1 << gpio)) {printk (KERN_ERR "Unable to modify wakeup on " "non-wakeup GPIO%d\n" , (bank - gpio_bank) *32 + gpio);return -EINVAL ; } -spin_lock_irqsave (&bank->lock, flags); -if (enable) - bank->suspend_wakeup |= (1 << gpio); -else - bank->suspend_wakeup &= ~(1 << gpio); -spin_unlock_irqrestore (&bank->lock, flags); +break ; +case METHOD_GPIO_44XX : + wake_status = bank->base +OMAP4_GPIO_IRQWAKEN0 ; + wake_clear = bank->base +OMAP4_GPIO_IRQWAKEN0 ; + wake_set = bank->base +OMAP4_GPIO_IRQWAKEN0 ;return 0 ;#endif default :
@@ -1012,6 +1017,16 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable) bank->method);return -EINVAL ; } + +spin_lock_irqsave (&bank->lock, flags); +if (enable) +__raw_writel ((1 << gpio), wake_set); +else +__raw_writel ((1 << gpio), wake_clear); + +spin_unlock_irqrestore (&bank->lock, flags); + +return 0 ; }static void _reset_gpio (struct gpio_bank *bank,int gpio)
@@ -1755,105 +1770,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) }#if defined(CONFIG_ARCH_OMAP16XX ) || defined(CONFIG_ARCH_OMAP2PLUS ) -static int omap_gpio_suspend (struct sys_device *dev,pm_message_t mesg) -{ -int i; - -if (!cpu_class_is_omap2 () && !cpu_is_omap16xx ()) -return 0 ; - -for (i =0 ; i < gpio_bank_count; i++) { -struct gpio_bank *bank = &gpio_bank[i]; -void __iomem *wake_status; -void __iomem *wake_clear; -void __iomem *wake_set; -unsigned long flags; - -switch (bank->method) { -#ifdef CONFIG_ARCH_OMAP16XX -case METHOD_GPIO_1610 : - wake_status = bank->base +OMAP1610_GPIO_WAKEUPENABLE ; - wake_clear = bank->base +OMAP1610_GPIO_CLEAR_WAKEUPENA ; - wake_set = bank->base +OMAP1610_GPIO_SET_WAKEUPENA ; -break ; -#endif -#if defined(CONFIG_ARCH_OMAP2 ) || defined(CONFIG_ARCH_OMAP3 ) -case METHOD_GPIO_24XX : - wake_status = bank->base +OMAP24XX_GPIO_WAKE_EN ; - wake_clear = bank->base +OMAP24XX_GPIO_CLEARWKUENA ; - wake_set = bank->base +OMAP24XX_GPIO_SETWKUENA ; -break ; -#endif -#ifdef CONFIG_ARCH_OMAP4 -case METHOD_GPIO_44XX : - wake_status = bank->base +OMAP4_GPIO_IRQWAKEN0 ; - wake_clear = bank->base +OMAP4_GPIO_IRQWAKEN0 ; - wake_set = bank->base +OMAP4_GPIO_IRQWAKEN0 ; -break ; -#endif -default : -continue ; - } - -spin_lock_irqsave (&bank->lock, flags); - bank->saved_wakeup =__raw_readl (wake_status); -__raw_writel (0xffffffff , wake_clear); -__raw_writel (bank->suspend_wakeup, wake_set); -spin_unlock_irqrestore (&bank->lock, flags); - } - -return 0 ; -} - -static int omap_gpio_resume (struct sys_device *dev) -{ -int i; - -if (!cpu_class_is_omap2 () && !cpu_is_omap16xx ()) -return 0 ; - -for (i =0 ; i < gpio_bank_count; i++) { -struct gpio_bank *bank = &gpio_bank[i]; -void __iomem *wake_clear; -void __iomem *wake_set; -unsigned long flags; - -switch (bank->method) { -#ifdef CONFIG_ARCH_OMAP16XX -case METHOD_GPIO_1610 : - wake_clear = bank->base +OMAP1610_GPIO_CLEAR_WAKEUPENA ; - wake_set = bank->base +OMAP1610_GPIO_SET_WAKEUPENA ; -break ; -#endif -#if defined(CONFIG_ARCH_OMAP2 ) || defined(CONFIG_ARCH_OMAP3 ) -case METHOD_GPIO_24XX : - wake_clear = bank->base +OMAP24XX_GPIO_CLEARWKUENA ; - wake_set = bank->base +OMAP24XX_GPIO_SETWKUENA ; -break ; -#endif -#ifdef CONFIG_ARCH_OMAP4 -case METHOD_GPIO_44XX : - wake_clear = bank->base +OMAP4_GPIO_IRQWAKEN0 ; - wake_set = bank->base +OMAP4_GPIO_IRQWAKEN0 ; -break ; -#endif -default : -continue ; - } - -spin_lock_irqsave (&bank->lock, flags); -__raw_writel (0xffffffff , wake_clear); -__raw_writel (bank->saved_wakeup, wake_set); -spin_unlock_irqrestore (&bank->lock, flags); - } - -return 0 ; -}static struct sysdev_class omap_gpio_sysclass = { .name ="gpio" , - .suspend = omap_gpio_suspend, - .resume = omap_gpio_resume, }; static struct sys_device omap_gpio_device = {
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Hello Kevin,
Quoted 89 lines · ext Kevin Hilman
Quoted 80 lines · Eduardo Valentin
Quoted 22 lines · ext Kevin Hilman
Quoted 12 lines · Eduardo Valentin
Hello Russell,
Quoted 9 lines · Russell King
Quoted 8 lines · Eduardo Valentin
Currently, if one calls
disable_irq(gpio_irq), the irq won't get disabled.This is happening because the omap gpio code defines only a .mask callback. And the
default_disablefunction is just a stub. The result is that, when someone callsdisable_irqfor an irq in a gpio line, it will be kept enabled.Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
This patch solves this issue by setting the .disable callback to point to the same .mask callback.
Amd this is a problem because?
errr.. because the interrupt is enabled when it was supposed to be disabled?
Quoted 7 lines
As Russell pointed out, it's not actually "supposed" to be.
With lazy disable, what
disable_irq()does is prevent the *handler* from ever being called. If another interrupt arrives, it will be caught by the genirq core, marked asIRQ_PENDINGand then masked. This "don't disable unless we really have to" is the desired behavior of the lazy disable feature.Right. I'm convinced that the handler won't be called because of the lazy disable mechanism.
Quoted 13 lines
Quoted 13 lines
Quoted 6 lines
The way this works is that although it isn't disabled at that point, if it never triggers, then everything remains happy. However, if it does trigger, the genirq code will then mask the interrupt and won't call the handler.
Right.. I didn't see from this point. I will check how that gets unmasked. But even so, if I understood correctly what you described, it would still open a time window which the system would see at least 1 interrupt during the time it was not suppose to. And that can wakeup a system which is in deep sleep mode, either via dynamic idle or static suspend.
Quoted 4 lines
Quoted 1 line
It is unlikely, I know. But it can still happen. And can be avoided.
If the GPIO is configured as a wakeup source, then wouldn't you want activity on that GPIO to wake up the system?
Yes I would want it.. of course, if the interrupt is enabled though..
I'm still trying to find a valid situation where someone disables an irq but still wants its activity to be a wakeup source. I couldn't find so far..
Quoted 3 lines
If you don't want wakeups on that GPIO, then the driver should probably be using
disable_irq_wake().Yes. Let's take this situation. Let's assume a driver, at its suspend callback, explicitly reports to the system that its irq can be disabled and also should not be seen as a wakeup source, by calling
disable_irq(gpio_irq)anddisable_irq_wake(gpio_irq).What should be the expected system behavior when the user says echo mem >
/sys/power/state?From the point we are done with devices suspend, that gpio will still be marked as an irq, at the bank level. But its corresponding pad will have its wakeup bit disabled. Would that work? I think yes, in most cases.
Now let's take the WFI instruction as a divisor for 2 situations:
A - common / working case: an interrupt on that gpio still happens after the WFI instruction. Since the system is in sleep mode and the pad for that gpio is not wakeup capable, then nothing would happen and the system won't wakeup. Then, I think everyone is happy here.
Quoted 6 lines
B - corner case: an interrupt on that gpio happens before the WFI instruction. Since the system is active, the gpio bank can still report this interrupt and will do it. The suspend won't happen due to a irq which has been explicitly marked as disabled and wakeup incapable.
Then, would that be the expected behavior? Assuming that the driver has explicitly said to the system, you should not bother about this at all.
Quoted 5 lines
Well, expected behaviour would be that GPIO bank should not be reporting the this interrupt at all since it has been disabled.
However, since you're asking, I assume that you're not seeing this expected behavior.
Yeah that's right. But it is rather difficult to reproduce.
Quoted 5 lines
Ignoring wakeups for a second, if this corner case happens on a non-wakeup capable GPIO using lazy disable, I would not expect suspend to be prevented. The genirq core would see the IRQ, mark it as
IRQ_PENDING, mask it and return. and suspend should continue.
True.
Quoted 4 lines
hmm... however, if the IRQ happens after interrupts are disabled, the genirq core won't handle it, and our PM core will see a pending interrupt and abort idle/suspend.
True again. And that's what think it is happening.
Quoted 3 lines
Are you seeing this corner case for a wakeup-enable GPIO or a non wakeup-enabled GPIO?
It is in wakeup-enable gpio. But the driver removes the wakeup flag from the gpio on its suspend function right after disabling the irq.
disable_irq(gpio_irq);
disable_irq_wake(gpio_irq);
In this case, the device uses gpio 61 as its irq line.
Quoted 4 lines
/me looks at code
I'm assuming now it's for a wakeup-enabled GPIO.
Right..
Quoted 11 lines
Another more likely possibility is that the IRQ arrives between the time the driver does its
disable_irq_wake()and when the GPIO driver actually suspends. We currently defer disalbing the bank-level IRQ wakeup capabilities until the suspend method of the GPIO driver is run (which is very late in the suspend cycle, since it is a sysdev.) Thinking about this some more, I'm not sure exactly why we do this. The current code seems to only manage GPIO wakeups for suspend/resume but not for idle, so it defers the actual register writes until the suspend hook. Since we presumably also want wakeup-enabled GPIOs to wake up from idle, we probably shouldn't be deferring the wakeup enable/disable.
I'm not sure if this is related to wakeup at all. Problem that I see is that the suspend is aborted due to an interrupt. And that interrupt has been disabled and marked as non-wakeup in the previous device suspend step.
I've checked that by verifying the gpio bank irqenable1 register and the pad conf for the gpio in use, right before entering omap_sram_idle in the suspend path.
[ 10.360321] GPIO BANK2 - 0x0 0x60200800 [ 10.360321] GPIO61 PAD conf 0x11C
The line starting with GPIO BANK2 tells me that GPIO_IRQSTATUS1 is 0x0 at that point. And GPIO_IRQENABLE1 is 0x60200800. Meaning, channel of GPIO 61 is still enabled at that point. The second line tells me that wakeup bit for GPIO61 is off. And of course, the suspend fails when we have a match of IRQSTATUS1 with IRQENABLE1, in this case for GPIO 61, which is my trigger here. But on this specific printing I didn't see the issue. However, just the fact that we reach this point with GPIO bank still seeing the IRQ for GPIO 61 enabled is already an indication that something is asking for failure.
So, that's the problematic situation. The gpio is still seen as an interrupt, although it has been set to be disabled.
The wakeup is not an issue here I'd say. Mainly because the system is still active, in the process to enter sleep, so no need to wakeup in order to see that interrupt coming from bank2.
After applying the patch I sent here, I see then the interrupts getting disabled by the time we are entering the omap_sram_idle.
[ 13.671142] GPIO BANK2 - 0x0 0x0 [ 13.671142] GPIO61 PAD conf 0x11C
The fact that no interrupts are enabled after this patch points that not only gpio 61 could be triggering the suspend failure. I still couldn't reproduce it on dynamic idle, but I can imagine that it is actually possible to reach this issue also in dyn. idle.
Quoted 5 lines
Here's an expirement. If you have a use case that is preventing a suspend in this corner case, let's try to immediately enable/disable wakeups instead of waiting for the suspend/resume hooks.
Quoted 8 lines
Below is a test patch only briefly tested on Zoom3 which has it's network interface on a GPIO IRQ. It does not have wakeups enabled, and under ping flood from a host (ping -f), it suspended just fine. I added an
enable_irq_wake()the driver and it was still able to suspend during ping flood.If you suspect the above might be happening in your corner case, can you give this a try to see if it changes?
Yes sure. I will try to test that, even though I still think the wakeups are not the problem. And another issue is that, as you probably have noticed, it is a very rare situation. Since it's rather difficult to reproduce I might take some time to give you some feedback here.
Quoted 197 lines
It's also worth noting that we are currently not managing IO pad wakeups in the GPIO core. We are only mananging GPIO module level wakeups, which only are in effect if CORE is active. Now that we have a mux framework that can probably handle this, we need a mapping of GPIO lines to pads so we can also manage IO pad level wakeups from the GPIO core code. However, if your corner case is arriving before WFI, than I suspect CORE is active, and module level wakeups is what you're running into.
Kevin
commit
3af8a1051a44462c62c5a5d47a8256626e32fbbaAuthor: Kevin Hilman <[email protected]> Date: Thu Jan 6 09:45:24 2011 -0800GPIO: enable/disable wakeups immediately
arch/arm/plat-omap/gpio.c +28
-109@@ -977,34 +977,39 @@ static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enastatic int _set_gpio_wakeup (struct gpio_bank *bank,int gpio,int enable) {unsigned long uninitialized_var (flags ); +void __iomem *wake_status; +void __iomem *wake_clear; +void __iomem *wake_set; + +printk ("KJH: %s: bank IRQ %d, GPIO %d enable=%d\n" , __func__, + bank->irq, gpio, enable);switch (bank->method) {#ifdef CONFIG_ARCH_OMAP16XX case METHOD_MPUIO :case METHOD_GPIO_1610 : -spin_lock_irqsave (&bank->lock, flags); -if (enable) - bank->suspend_wakeup |= (1 << gpio); -else - bank->suspend_wakeup &= ~(1 << gpio); -spin_unlock_irqrestore (&bank->lock, flags); + wake_status = bank->base +OMAP1610_GPIO_WAKEUPENABLE ; + wake_clear = bank->base +OMAP1610_GPIO_CLEAR_WAKEUPENA ; + wake_set = bank->base +OMAP1610_GPIO_SET_WAKEUPENA ;return 0 ;#endif #ifdef CONFIG_ARCH_OMAP2PLUS case METHOD_GPIO_24XX : -case METHOD_GPIO_44XX : + wake_status = bank->base +OMAP24XX_GPIO_WAKE_EN ; + wake_clear = bank->base +OMAP24XX_GPIO_CLEARWKUENA ; + wake_set = bank->base +OMAP24XX_GPIO_SETWKUENA ; +if (bank->non_wakeup_gpios & (1 << gpio)) {printk (KERN_ERR "Unable to modify wakeup on " "non-wakeup GPIO%d\n" , (bank - gpio_bank) *32 + gpio);return -EINVAL ; } -spin_lock_irqsave (&bank->lock, flags); -if (enable) - bank->suspend_wakeup |= (1 << gpio); -else - bank->suspend_wakeup &= ~(1 << gpio); -spin_unlock_irqrestore (&bank->lock, flags); +break ; +case METHOD_GPIO_44XX : + wake_status = bank->base +OMAP4_GPIO_IRQWAKEN0 ; + wake_clear = bank->base +OMAP4_GPIO_IRQWAKEN0 ; + wake_set = bank->base +OMAP4_GPIO_IRQWAKEN0 ;return 0 ;#endif default :@@ -1012,6 +1017,16 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable) bank->method);return -EINVAL ; } + +spin_lock_irqsave (&bank->lock, flags); +if (enable) +__raw_writel ((1 << gpio), wake_set); +else +__raw_writel ((1 << gpio), wake_clear); + +spin_unlock_irqrestore (&bank->lock, flags); + +return 0 ; }static void _reset_gpio (struct gpio_bank *bank,int gpio)@@ -1755,105 +1770,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) }#if defined(CONFIG_ARCH_OMAP16XX ) || defined(CONFIG_ARCH_OMAP2PLUS ) -static int omap_gpio_suspend (struct sys_device *dev,pm_message_t mesg) -{ -int i; - -if (!cpu_class_is_omap2 () && !cpu_is_omap16xx ()) -return 0 ; - -for (i =0 ; i < gpio_bank_count; i++) { -struct gpio_bank *bank = &gpio_bank[i]; -void __iomem *wake_status; -void __iomem *wake_clear; -void __iomem *wake_set; -unsigned long flags; - -switch (bank->method) { -#ifdef CONFIG_ARCH_OMAP16XX -case METHOD_GPIO_1610 : - wake_status = bank->base +OMAP1610_GPIO_WAKEUPENABLE ; - wake_clear = bank->base +OMAP1610_GPIO_CLEAR_WAKEUPENA ; - wake_set = bank->base +OMAP1610_GPIO_SET_WAKEUPENA ; -break ; -#endif -#if defined(CONFIG_ARCH_OMAP2 ) || defined(CONFIG_ARCH_OMAP3 ) -case METHOD_GPIO_24XX : - wake_status = bank->base +OMAP24XX_GPIO_WAKE_EN ; - wake_clear = bank->base +OMAP24XX_GPIO_CLEARWKUENA ; - wake_set = bank->base +OMAP24XX_GPIO_SETWKUENA ; -break ; -#endif -#ifdef CONFIG_ARCH_OMAP4 -case METHOD_GPIO_44XX : - wake_status = bank->base +OMAP4_GPIO_IRQWAKEN0 ; - wake_clear = bank->base +OMAP4_GPIO_IRQWAKEN0 ; - wake_set = bank->base +OMAP4_GPIO_IRQWAKEN0 ; -break ; -#endif -default : -continue ; - } - -spin_lock_irqsave (&bank->lock, flags); - bank->saved_wakeup =__raw_readl (wake_status); -__raw_writel (0xffffffff , wake_clear); -__raw_writel (bank->suspend_wakeup, wake_set); -spin_unlock_irqrestore (&bank->lock, flags); - } - -return 0 ; -} - -static int omap_gpio_resume (struct sys_device *dev) -{ -int i; - -if (!cpu_class_is_omap2 () && !cpu_is_omap16xx ()) -return 0 ; - -for (i =0 ; i < gpio_bank_count; i++) { -struct gpio_bank *bank = &gpio_bank[i]; -void __iomem *wake_clear; -void __iomem *wake_set; -unsigned long flags; - -switch (bank->method) { -#ifdef CONFIG_ARCH_OMAP16XX -case METHOD_GPIO_1610 : - wake_clear = bank->base +OMAP1610_GPIO_CLEAR_WAKEUPENA ; - wake_set = bank->base +OMAP1610_GPIO_SET_WAKEUPENA ; -break ; -#endif -#if defined(CONFIG_ARCH_OMAP2 ) || defined(CONFIG_ARCH_OMAP3 ) -case METHOD_GPIO_24XX : - wake_clear = bank->base +OMAP24XX_GPIO_CLEARWKUENA ; - wake_set = bank->base +OMAP24XX_GPIO_SETWKUENA ; -break ; -#endif -#ifdef CONFIG_ARCH_OMAP4 -case METHOD_GPIO_44XX : - wake_clear = bank->base +OMAP4_GPIO_IRQWAKEN0 ; - wake_set = bank->base +OMAP4_GPIO_IRQWAKEN0 ; -break ; -#endif -default : -continue ; - } - -spin_lock_irqsave (&bank->lock, flags); -__raw_writel (0xffffffff , wake_clear); -__raw_writel (bank->saved_wakeup, wake_set); -spin_unlock_irqrestore (&bank->lock, flags); - } - -return 0 ; -}static struct sysdev_class omap_gpio_sysclass = { .name ="gpio" , - .suspend = omap_gpio_suspend, - .resume = omap_gpio_resume, }; static struct sys_device omap_gpio_device = {
Signature · 1 line
Eduardo Valentin
PATCH 1/1 arm: omap: gpio: define .disable callback for gpio irq chip
Quoted 6 lines · Eduardo Valentin
It is in wakeup-enable gpio. But the driver removes the wakeup flag from the gpio on its suspend function right after disabling the irq.
disable_irq(gpio_irq);
disable_irq_wake(gpio_irq);In this case, the device uses gpio 61 as its irq line.
I think the solution to this is to have genirq synchronize the hardware state with the lazy state on suspend transitions rather than trying to fix this on a case-by-case basis.