| Message ID | 20170310230430.6pfljo5rr7esn5qb@lenoch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, * Ladislav Michl <ladis@linux-mips.org> [170310 15:06]: > Hi there, > > I'm using drivers/media/rc/gpio-ir-recv.c on DM3730 based board and getting > a lot of IR protocol decoding failures: > RC5(x/sz) decode failed at state 0 count 7 (855us space) > RC5(x/sz) decode failed at state 1 count 4 (1740us space) > RC5(x/sz) decode failed at state 4 count 1 (125000us space) > RC5(x/sz) decode failed at state 0 count 1 (182348us space) > RC5(x/sz) decode failed at state 0 count 1 (366us pulse) > etc... > Hacking gpio-ir-recv code to add debounce on GPIO pin makes situation > a bit better, but still bad enough. > It seems this problem is there since dawn of linux-omap time [1] and > can be easily reproduced requesting edge trigerred gpio interrupt, > feeding that gpio pin from signal generator and toggling another gpio > in interrupt hangler with scope hooked. > Last time patch [1] was updated to kernel 2.6.32 and you can find it > below. > > So it seems noone is really using edge triggered interrupts, right? Interesting, adding Grygorii to cc as well. > Now, patch bellow is for ancient omap1 variant (and I'd love to give it > a try with recent kernel, but everything I compiled so far is insanely > huge) and does not support both rising and falling edge. > > To make interrupt code reliable I do not see other option than to read > gpio input register and make loop similar as in patch bellow. However, > this was repeatedly rejected in the past. So, better ideas, anyone? > [1] http://marc.info/?l=linux-omap&m=119634410025393&w=2 Oh that thing yeah I remember testing it too. If we don't have any other options for edge interrupts I guess the loop is what we need to do. If you have some experimental patch to play with against the current mainline, please post it and I'll give it a try too. Regards, Tony > --- linux-2.6.32/arch/arm/plat-omap/gpio.c.orig 2009-12-29 20:56:29.000000000 +0100 > +++ linux-2.6.32/arch/arm/plat-omap/gpio.c 2009-12-29 20:56:44.000000000 +0100 > @@ -192,6 +192,7 @@ > u32 saved_risingdetect; > #endif > u32 level_mask; > + u32 serviced; > spinlock_t lock; > struct gpio_chip chip; > struct clk *dbck; > @@ -1204,6 +1205,28 @@ > spin_unlock_irqrestore(&bank->lock, flags); > } > > +static u32 _get_gpio_irq_status(struct gpio_bank *bank) > +{ > + u32 isr, ism, in, re; > + u32 base = bank->base; > + > + switch (bank->method) { > +#ifdef CONFIG_ARCH_OMAP15XX > + case METHOD_GPIO_1510: > + in = __raw_readl(base + OMAP1510_GPIO_DATA_INPUT); > + isr = __raw_readl(base + OMAP1510_GPIO_INT_STATUS); > + ism = __raw_readl(base + OMAP1510_GPIO_INT_MASK) | 0xffff0000; > + re = __raw_readl(base + OMAP1510_GPIO_INT_CONTROL); > + isr |= (~(in^re)) & (~ism); > + break; > +#endif > + default: > + BUG(); > + return 0; > + } > + return isr; > +} > + > /* > * We need to unmask the GPIO bank interrupt as soon as possible to > * avoid missing GPIO interrupts for other lines in the bank. > @@ -1218,11 +1241,13 @@ > void __iomem *isr_reg = NULL; > u32 isr; > unsigned int gpio_irq; > + unsigned long flags; > struct gpio_bank *bank; > u32 retrigger = 0; > int unmasked = 0; > > desc->chip->ack(irq); > + desc->chip->unmask(irq); > > bank = get_irq_data(irq); > #ifdef CONFIG_ARCH_OMAP1 > @@ -1249,54 +1274,29 @@ > if (bank->method == METHOD_GPIO_24XX) > isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0; > #endif > + spin_lock_irqsave(&bank->lock, flags); > while(1) { > - u32 isr_saved, level_mask = 0; > - u32 enabled; > + u32 isr; > + unsigned long serviced; > > - enabled = _get_gpio_irqbank_mask(bank); > - isr_saved = isr = __raw_readl(isr_reg) & enabled; > - > - if (cpu_is_omap15xx() && (bank->method == METHOD_MPUIO)) > - isr &= 0x0000ffff; > - > - if (cpu_class_is_omap2()) { > - level_mask = bank->level_mask & enabled; > - } > - > - /* clear edge sensitive interrupts before handler(s) are > - called so that we don't miss any interrupt occurred while > - executing them */ > - _enable_gpio_irqbank(bank, isr_saved & ~level_mask, 0); > - _clear_gpio_irqbank(bank, isr_saved & ~level_mask); > - _enable_gpio_irqbank(bank, isr_saved & ~level_mask, 1); > - > - /* if there is only edge sensitive GPIO pin interrupts > - configured, we could unmask GPIO bank interrupt immediately */ > - if (!level_mask && !unmasked) { > - unmasked = 1; > - desc->chip->unmask(irq); > - } > - > - isr |= retrigger; > - retrigger = 0; > + isr = _get_gpio_irq_status(bank) & ~bank->serviced; > if (!isr) > break; > > gpio_irq = bank->virtual_irq_start; > - for (; isr != 0; isr >>= 1, gpio_irq++) { > - if (!(isr & 1)) > - continue; > - > - generic_handle_irq(gpio_irq); > + serviced = 1; > + for (; isr != 0; gpio_irq++, serviced <<= 1 ) { > + if (isr & serviced) { > + bank->serviced |= serviced; > + spin_unlock_irqrestore(&bank->lock, flags); > + generic_handle_irq(gpio_irq); > + spin_lock_irqsave(&bank->lock, flags); > + bank->serviced &= ~serviced; > + isr &= ~serviced; > + } > } > } > - /* if bank has any level sensitive GPIO pin interrupt > - configured, we must unmask the bank interrupt only after > - handler(s) are executed in order to avoid spurious bank > - interrupt */ > - if (!unmasked) > - desc->chip->unmask(irq); > - > + spin_unlock_irqrestore(&bank->lock, flags); > } > > static void gpio_irq_shutdown(unsigned int irq) > @@ -1781,6 +1781,7 @@ > #endif > > bank->mod_usage = 0; > + bank->serviced = 0; > /* REVISIT eventually switch from OMAP-specific gpio structs > * over to the generic ones > */ > > -- > 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 > -- 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
* Tony Lindgren <tony@atomide.com> [170311 12:13]: > Hi, > > * Ladislav Michl <ladis@linux-mips.org> [170310 15:06]: > > Hi there, > > > > I'm using drivers/media/rc/gpio-ir-recv.c on DM3730 based board and getting > > a lot of IR protocol decoding failures: > > RC5(x/sz) decode failed at state 0 count 7 (855us space) > > RC5(x/sz) decode failed at state 1 count 4 (1740us space) > > RC5(x/sz) decode failed at state 4 count 1 (125000us space) > > RC5(x/sz) decode failed at state 0 count 1 (182348us space) > > RC5(x/sz) decode failed at state 0 count 1 (366us pulse) > > etc... > > Hacking gpio-ir-recv code to add debounce on GPIO pin makes situation > > a bit better, but still bad enough. > > It seems this problem is there since dawn of linux-omap time [1] and > > can be easily reproduced requesting edge trigerred gpio interrupt, > > feeding that gpio pin from signal generator and toggling another gpio > > in interrupt hangler with scope hooked. > > Last time patch [1] was updated to kernel 2.6.32 and you can find it > > below. > > > > So it seems noone is really using edge triggered interrupts, right? > > Interesting, adding Grygorii to cc as well. > > > Now, patch bellow is for ancient omap1 variant (and I'd love to give it > > a try with recent kernel, but everything I compiled so far is insanely > > huge) and does not support both rising and falling edge. > > > > To make interrupt code reliable I do not see other option than to read > > gpio input register and make loop similar as in patch bellow. However, > > this was repeatedly rejected in the past. So, better ideas, anyone? > > > [1] http://marc.info/?l=linux-omap&m=119634410025393&w=2 > > Oh that thing yeah I remember testing it too. If we don't have any > other options for edge interrupts I guess the loop is what we need to > do. If you have some experimental patch to play with against the > current mainline, please post it and I'll give it a try too. Hmm and we already seem to have a similar loop in the mainline kernel? At least I could not easily spot what we might be missing. Regards, Tony > > --- linux-2.6.32/arch/arm/plat-omap/gpio.c.orig 2009-12-29 20:56:29.000000000 +0100 > > +++ linux-2.6.32/arch/arm/plat-omap/gpio.c 2009-12-29 20:56:44.000000000 +0100 > > @@ -192,6 +192,7 @@ > > u32 saved_risingdetect; > > #endif > > u32 level_mask; > > + u32 serviced; > > spinlock_t lock; > > struct gpio_chip chip; > > struct clk *dbck; > > @@ -1204,6 +1205,28 @@ > > spin_unlock_irqrestore(&bank->lock, flags); > > } > > > > +static u32 _get_gpio_irq_status(struct gpio_bank *bank) > > +{ > > + u32 isr, ism, in, re; > > + u32 base = bank->base; > > + > > + switch (bank->method) { > > +#ifdef CONFIG_ARCH_OMAP15XX > > + case METHOD_GPIO_1510: > > + in = __raw_readl(base + OMAP1510_GPIO_DATA_INPUT); > > + isr = __raw_readl(base + OMAP1510_GPIO_INT_STATUS); > > + ism = __raw_readl(base + OMAP1510_GPIO_INT_MASK) | 0xffff0000; > > + re = __raw_readl(base + OMAP1510_GPIO_INT_CONTROL); > > + isr |= (~(in^re)) & (~ism); > > + break; > > +#endif > > + default: > > + BUG(); > > + return 0; > > + } > > + return isr; > > +} > > + > > /* > > * We need to unmask the GPIO bank interrupt as soon as possible to > > * avoid missing GPIO interrupts for other lines in the bank. > > @@ -1218,11 +1241,13 @@ > > void __iomem *isr_reg = NULL; > > u32 isr; > > unsigned int gpio_irq; > > + unsigned long flags; > > struct gpio_bank *bank; > > u32 retrigger = 0; > > int unmasked = 0; > > > > desc->chip->ack(irq); > > + desc->chip->unmask(irq); > > > > bank = get_irq_data(irq); > > #ifdef CONFIG_ARCH_OMAP1 > > @@ -1249,54 +1274,29 @@ > > if (bank->method == METHOD_GPIO_24XX) > > isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0; > > #endif > > + spin_lock_irqsave(&bank->lock, flags); > > while(1) { > > - u32 isr_saved, level_mask = 0; > > - u32 enabled; > > + u32 isr; > > + unsigned long serviced; > > > > - enabled = _get_gpio_irqbank_mask(bank); > > - isr_saved = isr = __raw_readl(isr_reg) & enabled; > > - > > - if (cpu_is_omap15xx() && (bank->method == METHOD_MPUIO)) > > - isr &= 0x0000ffff; > > - > > - if (cpu_class_is_omap2()) { > > - level_mask = bank->level_mask & enabled; > > - } > > - > > - /* clear edge sensitive interrupts before handler(s) are > > - called so that we don't miss any interrupt occurred while > > - executing them */ > > - _enable_gpio_irqbank(bank, isr_saved & ~level_mask, 0); > > - _clear_gpio_irqbank(bank, isr_saved & ~level_mask); > > - _enable_gpio_irqbank(bank, isr_saved & ~level_mask, 1); > > - > > - /* if there is only edge sensitive GPIO pin interrupts > > - configured, we could unmask GPIO bank interrupt immediately */ > > - if (!level_mask && !unmasked) { > > - unmasked = 1; > > - desc->chip->unmask(irq); > > - } > > - > > - isr |= retrigger; > > - retrigger = 0; > > + isr = _get_gpio_irq_status(bank) & ~bank->serviced; > > if (!isr) > > break; > > > > gpio_irq = bank->virtual_irq_start; > > - for (; isr != 0; isr >>= 1, gpio_irq++) { > > - if (!(isr & 1)) > > - continue; > > - > > - generic_handle_irq(gpio_irq); > > + serviced = 1; > > + for (; isr != 0; gpio_irq++, serviced <<= 1 ) { > > + if (isr & serviced) { > > + bank->serviced |= serviced; > > + spin_unlock_irqrestore(&bank->lock, flags); > > + generic_handle_irq(gpio_irq); > > + spin_lock_irqsave(&bank->lock, flags); > > + bank->serviced &= ~serviced; > > + isr &= ~serviced; > > + } > > } > > } > > - /* if bank has any level sensitive GPIO pin interrupt > > - configured, we must unmask the bank interrupt only after > > - handler(s) are executed in order to avoid spurious bank > > - interrupt */ > > - if (!unmasked) > > - desc->chip->unmask(irq); > > - > > + spin_unlock_irqrestore(&bank->lock, flags); > > } > > > > static void gpio_irq_shutdown(unsigned int irq) > > @@ -1781,6 +1781,7 @@ > > #endif > > > > bank->mod_usage = 0; > > + bank->serviced = 0; > > /* REVISIT eventually switch from OMAP-specific gpio structs > > * over to the generic ones > > */ > > > > -- > > 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 > > > -- > 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 -- 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
Hi Tony, On Tue, Mar 14, 2017 at 11:21:25AM -0700, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [170311 12:13]: > > Hi, > > > > * Ladislav Michl <ladis@linux-mips.org> [170310 15:06]: > > > Hi there, > > > > > > I'm using drivers/media/rc/gpio-ir-recv.c on DM3730 based board and getting > > > a lot of IR protocol decoding failures: > > > RC5(x/sz) decode failed at state 0 count 7 (855us space) > > > RC5(x/sz) decode failed at state 1 count 4 (1740us space) > > > RC5(x/sz) decode failed at state 4 count 1 (125000us space) > > > RC5(x/sz) decode failed at state 0 count 1 (182348us space) > > > RC5(x/sz) decode failed at state 0 count 1 (366us pulse) > > > etc... > > > Hacking gpio-ir-recv code to add debounce on GPIO pin makes situation > > > a bit better, but still bad enough. > > > It seems this problem is there since dawn of linux-omap time [1] and > > > can be easily reproduced requesting edge trigerred gpio interrupt, > > > feeding that gpio pin from signal generator and toggling another gpio > > > in interrupt hangler with scope hooked. > > > Last time patch [1] was updated to kernel 2.6.32 and you can find it > > > below. > > > > > > So it seems noone is really using edge triggered interrupts, right? > > > > Interesting, adding Grygorii to cc as well. > > > > > Now, patch bellow is for ancient omap1 variant (and I'd love to give it > > > a try with recent kernel, but everything I compiled so far is insanely > > > huge) and does not support both rising and falling edge. > > > > > > To make interrupt code reliable I do not see other option than to read > > > gpio input register and make loop similar as in patch bellow. However, > > > this was repeatedly rejected in the past. So, better ideas, anyone? > > > > > [1] http://marc.info/?l=linux-omap&m=119634410025393&w=2 > > > > Oh that thing yeah I remember testing it too. If we don't have any > > other options for edge interrupts I guess the loop is what we need to > > do. If you have some experimental patch to play with against the > > current mainline, please post it and I'll give it a try too. > > Hmm and we already seem to have a similar loop in the mainline kernel? > At least I could not easily spot what we might be missing. I'm sorry for not answering in timely manner, I cannot find a time to work on this, however I at least tried to write special handler for chips with set and clear registers, which improved handling, but still not working completely reliably. I'll try to find some time to clean it up and post. ladis -- 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
* Ladislav Michl <ladis@linux-mips.org> [170314 23:20]: > Hi Tony, > > On Tue, Mar 14, 2017 at 11:21:25AM -0700, Tony Lindgren wrote: > > * Tony Lindgren <tony@atomide.com> [170311 12:13]: > > > Hi, > > > > > > * Ladislav Michl <ladis@linux-mips.org> [170310 15:06]: > > > > Hi there, > > > > > > > > I'm using drivers/media/rc/gpio-ir-recv.c on DM3730 based board and getting > > > > a lot of IR protocol decoding failures: > > > > RC5(x/sz) decode failed at state 0 count 7 (855us space) > > > > RC5(x/sz) decode failed at state 1 count 4 (1740us space) > > > > RC5(x/sz) decode failed at state 4 count 1 (125000us space) > > > > RC5(x/sz) decode failed at state 0 count 1 (182348us space) > > > > RC5(x/sz) decode failed at state 0 count 1 (366us pulse) > > > > etc... > > > > Hacking gpio-ir-recv code to add debounce on GPIO pin makes situation > > > > a bit better, but still bad enough. > > > > It seems this problem is there since dawn of linux-omap time [1] and > > > > can be easily reproduced requesting edge trigerred gpio interrupt, > > > > feeding that gpio pin from signal generator and toggling another gpio > > > > in interrupt hangler with scope hooked. > > > > Last time patch [1] was updated to kernel 2.6.32 and you can find it > > > > below. > > > > > > > > So it seems noone is really using edge triggered interrupts, right? > > > > > > Interesting, adding Grygorii to cc as well. > > > > > > > Now, patch bellow is for ancient omap1 variant (and I'd love to give it > > > > a try with recent kernel, but everything I compiled so far is insanely > > > > huge) and does not support both rising and falling edge. > > > > > > > > To make interrupt code reliable I do not see other option than to read > > > > gpio input register and make loop similar as in patch bellow. However, > > > > this was repeatedly rejected in the past. So, better ideas, anyone? > > > > > > > [1] http://marc.info/?l=linux-omap&m=119634410025393&w=2 > > > > > > Oh that thing yeah I remember testing it too. If we don't have any > > > other options for edge interrupts I guess the loop is what we need to > > > do. If you have some experimental patch to play with against the > > > current mainline, please post it and I'll give it a try too. > > > > Hmm and we already seem to have a similar loop in the mainline kernel? > > At least I could not easily spot what we might be missing. > > I'm sorry for not answering in timely manner, I cannot find a time to work > on this, however I at least tried to write special handler for chips with set > and clear registers, which improved handling, but still not working > completely reliably. I'll try to find some time to clean it up and post. OK sure no problem. Thanks, 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
--- linux-2.6.32/arch/arm/plat-omap/gpio.c.orig 2009-12-29 20:56:29.000000000 +0100 +++ linux-2.6.32/arch/arm/plat-omap/gpio.c 2009-12-29 20:56:44.000000000 +0100 @@ -192,6 +192,7 @@ u32 saved_risingdetect; #endif u32 level_mask; + u32 serviced; spinlock_t lock; struct gpio_chip chip; struct clk *dbck; @@ -1204,6 +1205,28 @@ spin_unlock_irqrestore(&bank->lock, flags); } +static u32 _get_gpio_irq_status(struct gpio_bank *bank) +{ + u32 isr, ism, in, re; + u32 base = bank->base; + + switch (bank->method) { +#ifdef CONFIG_ARCH_OMAP15XX + case METHOD_GPIO_1510: + in = __raw_readl(base + OMAP1510_GPIO_DATA_INPUT); + isr = __raw_readl(base + OMAP1510_GPIO_INT_STATUS); + ism = __raw_readl(base + OMAP1510_GPIO_INT_MASK) | 0xffff0000; + re = __raw_readl(base + OMAP1510_GPIO_INT_CONTROL); + isr |= (~(in^re)) & (~ism); + break; +#endif + default: + BUG(); + return 0; + } + return isr; +} + /* * We need to unmask the GPIO bank interrupt as soon as possible to * avoid missing GPIO interrupts for other lines in the bank. @@ -1218,11 +1241,13 @@ void __iomem *isr_reg = NULL; u32 isr; unsigned int gpio_irq; + unsigned long flags; struct gpio_bank *bank; u32 retrigger = 0; int unmasked = 0; desc->chip->ack(irq); + desc->chip->unmask(irq); bank = get_irq_data(irq); #ifdef CONFIG_ARCH_OMAP1 @@ -1249,54 +1274,29 @@ if (bank->method == METHOD_GPIO_24XX) isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0; #endif + spin_lock_irqsave(&bank->lock, flags); while(1) { - u32 isr_saved, level_mask = 0; - u32 enabled; + u32 isr; + unsigned long serviced; - enabled = _get_gpio_irqbank_mask(bank); - isr_saved = isr = __raw_readl(isr_reg) & enabled; - - if (cpu_is_omap15xx() && (bank->method == METHOD_MPUIO)) - isr &= 0x0000ffff; - - if (cpu_class_is_omap2()) { - level_mask = bank->level_mask & enabled; - } - - /* clear edge sensitive interrupts before handler(s) are - called so that we don't miss any interrupt occurred while - executing them */ - _enable_gpio_irqbank(bank, isr_saved & ~level_mask, 0); - _clear_gpio_irqbank(bank, isr_saved & ~level_mask); - _enable_gpio_irqbank(bank, isr_saved & ~level_mask, 1); - - /* if there is only edge sensitive GPIO pin interrupts - configured, we could unmask GPIO bank interrupt immediately */ - if (!level_mask && !unmasked) { - unmasked = 1; - desc->chip->unmask(irq); - } - - isr |= retrigger; - retrigger = 0; + isr = _get_gpio_irq_status(bank) & ~bank->serviced; if (!isr) break; gpio_irq = bank->virtual_irq_start; - for (; isr != 0; isr >>= 1, gpio_irq++) { - if (!(isr & 1)) - continue; - - generic_handle_irq(gpio_irq); + serviced = 1; + for (; isr != 0; gpio_irq++, serviced <<= 1 ) { + if (isr & serviced) { + bank->serviced |= serviced; + spin_unlock_irqrestore(&bank->lock, flags); + generic_handle_irq(gpio_irq); + spin_lock_irqsave(&bank->lock, flags); + bank->serviced &= ~serviced; + isr &= ~serviced; + } } } - /* if bank has any level sensitive GPIO pin interrupt - configured, we must unmask the bank interrupt only after - handler(s) are executed in order to avoid spurious bank - interrupt */ - if (!unmasked) - desc->chip->unmask(irq); - + spin_unlock_irqrestore(&bank->lock, flags); } static void gpio_irq_shutdown(unsigned int irq) @@ -1781,6 +1781,7 @@ #endif bank->mod_usage = 0; + bank->serviced = 0; /* REVISIT eventually switch from OMAP-specific gpio structs * over to the generic ones */