Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965183AbcDLQow (ORCPT ); Tue, 12 Apr 2016 12:44:52 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:31924 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933631AbcDLQou (ORCPT ); Tue, 12 Apr 2016 12:44:50 -0400 Subject: Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode To: Grygorii Strashko , Linus Walleij , Alexandre Courbot , Santosh Shilimkar , tony@atomide.com References: <1460458351-24187-1-git-send-email-grygorii.strashko@ti.com> Cc: nsekhar@ti.com, linux-omap@vger.kernel.org, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Roger Quadros From: santosh shilimkar Organization: Oracle Corporation Message-ID: <570D25EB.1010709@oracle.com> Date: Tue, 12 Apr 2016 09:44:27 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1460458351-24187-1-git-send-email-grygorii.strashko@ti.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5288 Lines: 132 On 4/12/2016 3:52 AM, Grygorii Strashko wrote: > Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle > in the following case: > extcon_usb1(id_irq) -> pcf8575.gpio1 -> omapgpio6.gpio11 -> gic > > the extcon_usb1 is wake up source and it enables IRQ wake up for > id_irq by calling enable/disable_irq_wake() during suspend/resume > which, in turn, causes execution of omap_gpio_wake_enable(). And > omap_gpio_wake_enable() will set/clear corresponding bit in > GPIO_IRQWAKEN_x register. > > omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs > by default from omap_gpio_irq_type: > GPIO_IRQSTATUS_SET_0 | 0x00000400 > GPIO_IRQSTATUS_CLR_0 | 0x00000400 > GPIO_IRQWAKEN_0 | 0x00000400 > GPIO_RISINGDETECT | 0x00000000 > GPIO_FALLINGDETECT | 0x00000400 > > omapgpio6 configuration after after suspend/resume cycle: > GPIO_IRQSTATUS_SET_0 | 0x00000400 > GPIO_IRQSTATUS_CLR_0 | 0x00000400 > GPIO_IRQWAKEN_0 | 0x00000000 <--- > GPIO_RISINGDETECT | 0x00000000 > GPIO_FALLINGDETECT | 0x00000400 > > As result, system will start to lose interrupts from pcf8575 GPIO > expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there > is no guarantee that transition(s) on input non wake up GPIO pin will > trigger asynchronous wake-up request to PRCM and then IRQ generation. > IRQ will be generated when GPIO is in active mode - for example, some > time after accessing GPIO bank registers IRQs will be generated > normally, but issue will happen again once PRCM will put GPIO in low > power smart-idle wakeup mode. > > Note 1. Issue is not reproduced if debounce clk is enabled for GPIO > bank. > > Note 2. Issue hardly reproducible if GPIO pins group contains both > wakeup/non-wakeup gpios - for example, it will be hard to reproduce > issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3 > GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level > Detection"). > > Note 3. There nothing common bitween System wake up and OMAP GPIO bank > IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON > transition will happen inside SoC under control of PRCM. > > Hence, fix the problem by removing omap_set_gpio_wakeup() function > completely and so keeping always in sync GPIO IRQ mask/unmask > (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding > IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup > GPIO IRQs will be properly masked/unmask by IRQ PM core during > suspend/resume cycle. > > Cc: Roger Quadros > Signed-off-by: Grygorii Strashko > --- GPIO IP has two levels of controls for wakeups and you are just removing the SYSCFG wakeup and relying on the IRQ line wakeup. I like usage of "IRQCHIP_MASK_ON_SUSPEND" but please be acreful this change which might break older OMAPs. > drivers/gpio/gpio-omap.c | 42 ++---------------------------------------- > 1 file changed, 2 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 551dfa9..b98ede7 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -611,51 +611,12 @@ static inline void omap_set_gpio_irqenable(struct gpio_bank *bank, > omap_disable_gpio_irqbank(bank, BIT(offset)); > } > > -/* > - * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register. > - * 1510 does not seem to have a wake-up register. If JTAG is connected > - * to the target, system will wake up always on GPIO events. While > - * system is running all registered GPIO interrupts need to have wake-up > - * enabled. When system is suspended, only selected GPIO interrupts need > - * to have wake-up enabled. > - */ > -static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset, > - int enable) > -{ > - u32 gpio_bit = BIT(offset); > - unsigned long flags; > - > - if (bank->non_wakeup_gpios & gpio_bit) { > - dev_err(bank->chip.parent, > - "Unable to modify wakeup on non-wakeup GPIO%d\n", > - offset); > - return -EINVAL; > - } > - > - raw_spin_lock_irqsave(&bank->lock, flags); > - if (enable) > - bank->context.wake_en |= gpio_bit; > - else > - bank->context.wake_en &= ~gpio_bit; > - > - writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en); > - raw_spin_unlock_irqrestore(&bank->lock, flags); > - > - return 0; > -} > - > /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */ > static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable) > { > struct gpio_bank *bank = omap_irq_data_get_bank(d); > - unsigned offset = d->hwirq; > - int ret; > > - ret = omap_set_gpio_wakeup(bank, offset, enable); > - if (!ret) > - ret = irq_set_irq_wake(bank->irq, enable); > - > - return ret; > + return irq_set_irq_wake(bank->irq, enable); > } > > static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) > @@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct platform_device *pdev) > irqc->irq_bus_lock = omap_gpio_irq_bus_lock, > irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock, > irqc->name = dev_name(&pdev->dev); > + irqc->flags = IRQCHIP_MASK_ON_SUSPEND; > > bank->irq = platform_get_irq(pdev, 0); > if (bank->irq <= 0) { >