| Message ID | 20171019131003.9684-1-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 19, 2017 at 03:10:03PM +0200, Gregory CLEMENT wrote: > From: Ken Ma <make@marvell.com> > > Current edge both type gpio irqs which need to swap polarity in each > interrupt are not supported, this patch adds edge both type gpio irq > support. So is the assumption here that you can handle the interrupt and flip the edge, faster than it takes the signal to change? If the software is too slow, you loose the following edge? And you might loose the edge after that as well, since the software will at some point handle the interrupt and reconfigure the edge, potentially for the wrong edge? Or am i missing something which makes this race free? Andrew
Hi Andrew, On jeu., oct. 19 2017, Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Oct 19, 2017 at 03:10:03PM +0200, Gregory CLEMENT wrote: >> From: Ken Ma <make@marvell.com> >> >> Current edge both type gpio irqs which need to swap polarity in each >> interrupt are not supported, this patch adds edge both type gpio irq >> support. > > So is the assumption here that you can handle the interrupt and flip > the edge, faster than it takes the signal to change? > > If the software is too slow, you loose the following edge? And you > might loose the edge after that as well, since the software will at > some point handle the interrupt and reconfigure the edge, potentially > for the wrong edge? > > Or am i missing something which makes this race free? To put more context, this patch is just the port of what is currently done in gpio-mvebu.c. I needed this support for the SD detect pin and didn't have any issue because it was not an intensive use at all. Going back to your concern, yes if the interrupt frequency is too high we might loose 2 events. But the same things could happen with a single edge, we can still miss an event. But I agree that with the "both edge" mode then we will loose 2 events instead of one. It is the problem for the edge interrupt in general. So as soon as you use edge interrupt you must be ready to loose some interrupt. In the end I don't think it is a problem. Gregory > > Andrew
Hi Gregory
> In the end I don't think it is a problem.
I think it should be clearly documented somewhere. Hardware which can
do both edges in hardware won't have this problem. If i'm using this
generic feature, i want an idea if it is mostly going to work, or
always going to work.
Andrew
On Thu, Oct 19, 2017 at 3:10 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > From: Ken Ma <make@marvell.com> > > Current edge both type gpio irqs which need to swap polarity in each > interrupt are not supported, this patch adds edge both type gpio irq > support. > > Signed-off-by: Ken Ma <make@marvell.com> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Patch applied. The discussion here is interesting, it is customary for GPIO drivers to implement double-edge detection emulation by swapping the edge detector around like this. It might be possible to collect some generic information about this in the Documentation/gpio/driver.txt document. Yours, Linus Walleij
> Patch applied. > > The discussion here is interesting, it is customary for GPIO drivers > to implement double-edge detection emulation by swapping the > edge detector around like this. Hi Linus I was not aware this was customary. > It might be possible to collect some generic information about > this in the Documentation/gpio/driver.txt document. Yes, i think it should be documented somewhere. Even in the use case here, detecting an SD card being inserted/removed, you could get some bounce on the microswitch, miss an edge, and be in the wrong state. Andrew
On Tue, Oct 31, 2017 at 2:16 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> The discussion here is interesting, it is customary for GPIO drivers >> to implement double-edge detection emulation by swapping the >> edge detector around like this. > > Hi Linus > > I was not aware this was customary. > >> It might be possible to collect some generic information about >> this in the Documentation/gpio/driver.txt document. > > Yes, i think it should be documented somewhere. Even in the use case > here, detecting an SD card being inserted/removed, you could get some > bounce on the microswitch, miss an edge, and be in the wrong state. Indeed :/ This is I guess why it is a good idea to combine this with the debounce feature if the hardware supports it. I'll think about some writeup. Yours, Linus Walleij
On Tue, Oct 31, 2017 at 02:16:17PM +0100, Andrew Lunn wrote: > > Patch applied. > > > > The discussion here is interesting, it is customary for GPIO drivers > > to implement double-edge detection emulation by swapping the > > edge detector around like this. > > Hi Linus > > I was not aware this was customary. > > > It might be possible to collect some generic information about > > this in the Documentation/gpio/driver.txt document. > > Yes, i think it should be documented somewhere. Even in the use case > here, detecting an SD card being inserted/removed, you could get some > bounce on the microswitch, miss an edge, and be in the wrong state. Maybe I'm wrong, but I wonder if there could be a set of helper functions provided by the gpio core that helps implementing this software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as possible in software) to prevent common mistakes. First draft: disable_irq_nosync(...); level = gpio_get(...); retry: if (level) configure_for_falling_edge(); else configure_for_raising_edge(); postlevel = gpio_get(...); if (level != postlevel) { mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */ level = postlevel; goto retry; } enable_irq(...); /* this resends the irq */ I think this only looses an event if there is an edge between gpio_get and the configure_for_${some}_edge and another before postlevel = ... that make the two events invisible. But I think this is okish, as a short spike might also be missed by a hw-edge-detector. And compared to the current code there should be no way to end in a state where we configured for raising edge and the level is already high. When the gpio toggles quickly this might keep the cpu busy in an endless loop, but such a sequence would also block a controller that can trigger on both edges in hardware. Not sure if breaking the loop at some point is sensible anyhow. Also calling the irq handlers would be beneficial, but I don't know if/how this works without (more) racing. A similar approach would be great to have to "simulate" level sensitive irqs if the hardware only implements edge logic (which affects armada-37xx, too, which annoys me). Best regards Uwe
On Tue, Mar 20, 2018 at 10:56 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Maybe I'm wrong, but I wonder if there could be a set of helper > functions provided by the gpio core that helps implementing this > software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as > possible in software) to prevent common mistakes. > > First draft: > > disable_irq_nosync(...); > level = gpio_get(...); > retry: > if (level) > configure_for_falling_edge(); > else > configure_for_raising_edge(); > postlevel = gpio_get(...); > > if (level != postlevel) { > mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */ > level = postlevel; > goto retry; > } > > enable_irq(...); /* this resends the irq */ > > I think this only looses an event if there is an edge between gpio_get > and the configure_for_${some}_edge and another before postlevel = ... > that make the two events invisible. But I think this is okish, as a > short spike might also be missed by a hw-edge-detector. And compared to > the current code there should be no way to end in a state where we > configured for raising edge and the level is already high. This is looking good compared to the solutions people have hacked up. > When the gpio toggles quickly this might keep the cpu busy in an endless > loop, but such a sequence would also block a controller that can trigger > on both edges in hardware. Not sure if breaking the loop at some point > is sensible anyhow. Also calling the irq handlers would be beneficial, > but I don't know if/how this works without (more) racing. What would make sense (if you want a perfect solution) is to enforce some reasonable debouncing on double edges. That may seem hard to do since not all HW has debounce. In the past I had the idea to implement also generic debounce with a timer in gpiolib, so that gpiod_set_debounce() would never fail, so in effect to factor the code from drivers/input/keyboard/gpio_keys.c over to gpiolib so they don't need a fallback at all, and then with double edges, enforce some debouncing based on HZ. At one point I tried to bring the debounce code over from the input driver, but I hit some snag, I don't remember what though. An optional per-gpiod timer can be created in struct gpio_desc when needed. > A similar approach would be great to have to "simulate" level sensitive > irqs if the hardware only implements edge logic (which affects > armada-37xx, too, which annoys me). Yes that would be neat too... Yours, Linus Walleij
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c index 71b944748304..4e8d836a8c6f 100644 --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c @@ -576,6 +576,19 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type) case IRQ_TYPE_EDGE_FALLING: val |= (BIT(d->hwirq % GPIO_PER_REG)); break; + case IRQ_TYPE_EDGE_BOTH: { + u32 in_val, in_reg = INPUT_VAL; + + armada_37xx_irq_update_reg(&in_reg, d); + regmap_read(info->regmap, in_reg, &in_val); + + /* Set initial polarity based on current input level. */ + if (in_val & d->mask) + val |= d->mask; /* falling */ + else + val &= ~d->mask; /* rising */ + break; + } default: spin_unlock_irqrestore(&info->irq_lock, flags); return -EINVAL; @@ -586,6 +599,40 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type) return 0; } +static int armada_37xx_edge_both_irq_swap_pol(struct armada_37xx_pinctrl *info, + u32 pin_idx) +{ + u32 reg_idx = pin_idx / GPIO_PER_REG; + u32 bit_num = pin_idx % GPIO_PER_REG; + u32 p, l, ret; + unsigned long flags; + + regmap_read(info->regmap, INPUT_VAL + 4*reg_idx, &l); + + spin_lock_irqsave(&info->irq_lock, flags); + p = readl(info->base + IRQ_POL + 4 * reg_idx); + if ((p ^ l) & (1 << bit_num)) { + /* + * For the gpios which are used for both-edge irqs, when their + * interrupts happen, their input levels are changed, + * yet their interrupt polarities are kept in old values, we + * should synchronize their interrupt polarities; for example, + * at first a gpio's input level is low and its interrupt + * polarity control is "Detect rising edge", then the gpio has + * a interrupt , its level turns to high, we should change its + * polarity control to "Detect falling edge" correspondingly. + */ + p ^= 1 << bit_num; + writel(p, info->base + IRQ_POL + 4 * reg_idx); + ret = 0; + } else { + /* Spurious irq */ + ret = -1; + } + + spin_unlock_irqrestore(&info->irq_lock, flags); + return ret; +} static void armada_37xx_irq_handler(struct irq_desc *desc) { @@ -609,6 +656,23 @@ static void armada_37xx_irq_handler(struct irq_desc *desc) u32 hwirq = ffs(status) - 1; u32 virq = irq_find_mapping(d, hwirq + i * GPIO_PER_REG); + u32 t = irq_get_trigger_type(virq); + + if ((t & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) { + /* Swap polarity (race with GPIO line) */ + if (armada_37xx_edge_both_irq_swap_pol(info, + hwirq + i * GPIO_PER_REG)) { + /* + * For spurious irq, which gpio level + * is not as expected after incoming + * edge, just ack the gpio irq. + */ + writel(1 << hwirq, + info->base + + IRQ_STATUS + 4 * i); + continue; + } + } generic_handle_irq(virq);