| Message ID | 20170321001732.gs525wb5klq7fuq7@lenoch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Ladislav Michl <ladis@linux-mips.org> [170320 17:19]: > On Mon, Mar 20, 2017 at 04:16:33PM -0500, Grygorii Strashko wrote: > > > > > > On 03/20/2017 06:21 AM, Ladislav Michl wrote: > > > On Thu, Mar 16, 2017 at 12:17:45PM -0700, Tony Lindgren wrote: > > >> Hmm maybe we need to flush posted writes when re-enabling the GPIO interrupts? > > >> > > >> Below is an untested patch that might help if that's the case. > > > > > > Unfortunately that's not the case. Even writing set&clear version of > > > interrupt demux handler did not make it any better. And idea from > > > ancient patch I initially sent is not easily extensible when we > > > need to trigger interrupt on both edges. So far I'm clueless... > > > > So, just to be sure - can you reproduce it with LKML? > > Do you mean mainline kernel? I'm on 4.11-rc3 now... > > > As per code, the possible problem could be with double acking of edge irqs > > (theoretically): > > - omap_gpio_irq_handler > > - "isr" = read irq status > > - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask); --- clear edge status, so new irq can be accepted > > - loop while "isr" > > generic_handle_irq() > > - handle_edge_irq() > > - desc->irq_data.chip->irq_ack(&desc->irq_data); > > - omap_gpio_ack_irq() > > it might be that at this moment edge IRQ was triggered again and it will be cleared. > > > > just as an experiment, could you try to update omap_gpio_ack_irq() > > as below: > > > > if (irq type is not edge) > > omap_clear_gpio_irqstatus(bank, offset); > > Rewritten as: > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index efc85a279d54..9381763e1fec 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -806,7 +806,8 @@ 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); > + if (bank->level_mask & BIT(offset)) > + omap_clear_gpio_irqstatus(bank, offset); > } > > static void omap_gpio_mask_irq(struct irq_data *d) > > And that did the trick. So far I tried IR decoder and decoding sometimes > fails, but I cannot say anything certain until I check with scope (which > I do tomorrow) Another good code review catch by Grygorii! :) Can you guys please add a comment there to the code when posting the proper patch? Something like: /* * Edge GPIOs are already cleared during handling, clearing * them here again will cause lost interrupts. */ And also add a proper Fixes tag so this gets propagated to the stable kernels. It really seems we've had this for a long time. Regards, Tony -- 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
On Wed, Mar 22, 2017 at 10:17:14AM -0700, Tony Lindgren wrote: > Another good code review catch by Grygorii! :) > > Can you guys please add a comment there to the code when posting the > proper patch? Something like: > > /* > * Edge GPIOs are already cleared during handling, clearing > * them here again will cause lost interrupts. > */ Just a though, as we are acking edge irq in demux handler handle_simple_irq should do the job as well. > And also add a proper Fixes tag so this gets propagated to the > stable kernels. It really seems we've had this for a long time. Well, it makes things better, but it is not good enough, yet... Bellow is a little test code - I'm feeding gpio159 from signal generator and expecting to see nearly the same on gpio161. Now, if you connect speaker to output gpio, you do not even need a scope and you can easily debug with your ears. ladis _ ,/' (_). ,/' __ :: (__)' `\. `\. #include <linux/kernel.h> #include <linux/init.h> #include <linux/module.h> #include <linux/interrupt.h> #include <linux/gpio.h> #include <linux/slab.h> #include <linux/platform_device.h> #include <linux/irq.h> struct gpio_test_dev { int gpio_in; int gpio_out; }; static struct gpio_test_dev dev; static irqreturn_t gpio_irq(int irq, void *dev_id) { struct gpio_test_dev *gpio_dev = dev_id; int val; val = gpio_get_value(gpio_dev->gpio_in); if (val < 0) goto err_get_value; gpio_set_value(gpio_dev->gpio_out, val); err_get_value: return IRQ_HANDLED; } static int __init gpio_test_init(void) { int rc; dev.gpio_in = 159; dev.gpio_out = 161; rc = gpio_request(dev.gpio_in, "gpio-in"); if (rc < 0) goto err; rc = gpio_direction_input(dev.gpio_in); if (rc < 0) goto err_free_input; rc = gpio_request(dev.gpio_out, "gpio-out"); if (rc < 0) goto err_free_input; rc = gpio_direction_output(dev.gpio_out, 0); if (rc < 0) goto err_free_output; rc = request_irq(gpio_to_irq(dev.gpio_in), gpio_irq, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, "gpio-irq", &dev); if (rc < 0) goto err_free_output; return 0; err_free_output: gpio_free(dev.gpio_out); err_free_input: gpio_free(dev.gpio_in); err: return rc; } static void __exit gpio_test_exit(void) { free_irq(gpio_to_irq(dev.gpio_in), &dev); gpio_free(dev.gpio_in); gpio_free(dev.gpio_out); } module_init(gpio_test_init); module_exit(gpio_test_exit); MODULE_DESCRIPTION("GPIO interrupt test driver"); MODULE_LICENSE("GPL v2"); -- 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
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index efc85a279d54..9381763e1fec 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -806,7 +806,8 @@ 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); + if (bank->level_mask & BIT(offset)) + omap_clear_gpio_irqstatus(bank, offset); } static void omap_gpio_mask_irq(struct irq_data *d)