From patchwork Wed Sep 20 12:54:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ladislav Michl X-Patchwork-Id: 9961377 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7D5DE600C5 for ; Wed, 20 Sep 2017 12:54:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6E82D29102 for ; Wed, 20 Sep 2017 12:54:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6349C29104; Wed, 20 Sep 2017 12:54:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 77E1929102 for ; Wed, 20 Sep 2017 12:54:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751762AbdITMyc (ORCPT ); Wed, 20 Sep 2017 08:54:32 -0400 Received: from eddie.linux-mips.org ([148.251.95.138]:40682 "EHLO cvs.linux-mips.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751529AbdITMyb (ORCPT ); Wed, 20 Sep 2017 08:54:31 -0400 Received: (from localhost user: 'ladis' uid#1021 fake: STDIN (ladis@eddie.linux-mips.org)) by eddie.linux-mips.org id S23993968AbdITMy27eIln (ORCPT ); Wed, 20 Sep 2017 14:54:28 +0200 Date: Wed, 20 Sep 2017 14:54:27 +0200 From: Ladislav Michl To: Grygorii Strashko Cc: linux-omap@vger.kernel.org, Tony Lindgren Subject: Re: [PATCH] gpio: omap: Fix lost edge interrupts Message-ID: <20170920125427.szszm357rme7wmsm@lenoch> References: <20170915214552.h54qifw2hxyvgmue@lenoch> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Sep 15, 2017 at 06:21:53PM -0500, Grygorii Strashko wrote: > On 09/15/2017 04:45 PM, Ladislav Michl wrote: > > From: Grygorii Strashko > > > > Edge interrupts are already cleared in omap_gpio_irq_handler, > > so clearing them again in omap_gpio_ack_irq causes lost > > interrupts. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Grygorii Strashko > > Signed-off-by: Ladislav Michl > > --- > > drivers/gpio/gpio-omap.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index dbf869fb63ce..a25738862129 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -811,7 +811,12 @@ static void omap_gpio_ack_irq(struct irq_data *d) > > struct gpio_bank *bank = omap_irq_data_get_bank(d); > > unsigned offset = d->hwirq; > > > > - omap_clear_gpio_irqstatus(bank, offset); > > + /* > > + * Edge GPIOs are already cleared during handling, clearing > > + * them here again will cause lost interrupts. > > + */ > > + if (bank->level_mask & BIT(offset)) > > + omap_clear_gpio_irqstatus(bank, offset); > > } > > > > static void omap_gpio_mask_irq(struct irq_data *d) > > > > Huh. You are back :) Thanks a lot for testing this and for your time. > > Actually, I'd like you to add one more change in this patch as it make no sense > to enable/disable edge IRQs while clearing status. > > /* clear edge sensitive interrupts before handler(s) are > called so that we don't miss any interrupt occurred while > executing them */ > - omap_disable_gpio_irqbank(bank, isr_saved & ~level_mask); > - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask); > - omap_enable_gpio_irqbank(bank, isr_saved & ~level_mask); > + if (isr_saved & ~level_mask) > + omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask); > > Also, It seems your idea to use handle_simple_irq() for edge IRQ should also work. like > > @@ -518,7 +518,12 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type) > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) > irq_set_handler_locked(d, handle_level_irq); > else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) > - irq_set_handler_locked(d, handle_edge_irq); > + /* > + * Edge GPIOs are already cleared during handling and not > + * masked, clearing them here again will cause lost interrupts. > + * So just use handle_simple_irq. > + */ > + irq_set_handler_locked(d, handle_simple_irq); > > > So, I think it will look better If you can check above and it'll works the same. Squashed into appended patch, verified using signal generator and scope. I'll send v2 after objections timeout expires. Btw, what is wa_lock protecting? diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index dbf869fb63ce..bb280b9a7f9f 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -518,7 +518,7 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type) if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) irq_set_handler_locked(d, handle_level_irq); else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) - irq_set_handler_locked(d, handle_edge_irq); + irq_set_handler_locked(d, handle_simple_irq); return 0; @@ -678,7 +678,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) { void __iomem *isr_reg = NULL; - u32 isr; + u32 enabled, isr, level_mask; unsigned int bit; struct gpio_bank *bank = gpiobank; unsigned long wa_lock_flags; @@ -691,23 +691,21 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) pm_runtime_get_sync(bank->chip.parent); while (1) { - u32 isr_saved, level_mask = 0; - u32 enabled; - raw_spin_lock_irqsave(&bank->lock, lock_flags); enabled = omap_get_gpio_irqbank_mask(bank); - isr_saved = isr = readl_relaxed(isr_reg) & enabled; + isr = readl_relaxed(isr_reg) & enabled; if (bank->level_mask) level_mask = bank->level_mask & enabled; + else + level_mask = 0; /* clear edge sensitive interrupts before handler(s) are called so that we don't miss any interrupt occurred while executing them */ - omap_disable_gpio_irqbank(bank, isr_saved & ~level_mask); - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask); - omap_enable_gpio_irqbank(bank, isr_saved & ~level_mask); + if (isr & ~level_mask) + omap_clear_gpio_irqbank(bank, isr & ~level_mask); raw_spin_unlock_irqrestore(&bank->lock, lock_flags); @@ -811,7 +809,12 @@ static void omap_gpio_ack_irq(struct irq_data *d) struct gpio_bank *bank = omap_irq_data_get_bank(d); unsigned offset = d->hwirq; - omap_clear_gpio_irqstatus(bank, offset); + /* + * Edge GPIOs are already cleared during handling, clearing + * them here again will cause lost interrupts. + */ + if (bank->level_mask & BIT(offset)) + omap_clear_gpio_irqstatus(bank, offset); } static void omap_gpio_mask_irq(struct irq_data *d)