Eduardo Valentin <eduardo.valentin@xxxxxxxxx> writes:
> On Wed, Jan 05, 2011 at 03:22:51PM -0800, ext Kevin Hilman wrote:
>> Eduardo Valentin <eduardo.valentin@xxxxxxxxx> writes:
>>
>> > Hello Russell,
>> >
>> > On Wed, Jan 05, 2011 at 06:19:18PM +0000, Russell King wrote:
>> >> On Wed, Jan 05, 2011 at 07:58:03PM +0200, Eduardo Valentin wrote:
>> >> > 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.
>> >>
>> >> 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.
>
> Right. I'm convinced that the handler won't be called because of the lazy
> disable mechanism.
>
>>
>> >>
>> >> 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.
>>
>> 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..
>
>
>>
>> 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?
>
> 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.
>
> 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 <khilman@xxxxxx>
Date: Thu Jan 6 09:45:24 2011 -0800
GPIO: enable/disable wakeups immediately
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index ccf2660..1418423 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -977,34 +977,39 @@ static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int ena
static 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 = {
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
