| Message ID | 2c265b6bcfcde7d2327b94c4f6e3ad6d4f1e2de7.1716967982.git.matthias.schiffer@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-tqmx86 fixes | expand |
On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > The TQMx86 GPIO controller only supports falling and rising edge > triggers, but not both. Fix this by implementing a software both-edge > mode that toggles the edge type after every interrupt. Do you have a real use case for this, one that will handle lost interrupts because it cannot swap edge quick enough? I personally would not do this, because it is dangerous, it gives the impression the device can do both, when in fact it cannot reliably. For me, the correct fix is to return EOPNOTSUPP or EINVAL for BOTH. Andrew
On Wed, 2024-05-29 at 14:37 +0200, Andrew Lunn wrote: > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > > The TQMx86 GPIO controller only supports falling and rising edge > > triggers, but not both. Fix this by implementing a software both-edge > > mode that toggles the edge type after every interrupt. > > Do you have a real use case for this, one that will handle lost > interrupts because it cannot swap edge quick enough? > > I personally would not do this, because it is dangerous, it gives the > impression the device can do both, when in fact it cannot reliably. > > For me, the correct fix is to return EOPNOTSUPP or EINVAL for BOTH. > This was the first thing I tried as well, but it seems that supporting IRQ_TYPE_EDGE_BOTH is mandatory for all GPIO drivers (not doing so results in various error messages when attemting to use *any* type of interrupt for the GPIO; I don't remember the exact errors). For this reason, several drivers implement a similar software solution when the hardware doesn't support it. Many drivers implement this in a fragile way that will easily break when an edge is missed. On the TQMx86 we are lucky, and this software implementation is actually robust and will not stop reporting edges when one is missed. The reason is explained in detail in the long comment added by this patch. Matthias
On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > index c957be3341774..400415676ad5d 100644 > --- a/drivers/gpio/gpio-tqmx86.c > +++ b/drivers/gpio/gpio-tqmx86.c > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq) > unsigned int offset = hwirq - TQMX86_NGPO; > u8 type = TQMX86_INT_TRIG_NONE, mask, val; > > - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) > + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) { > type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK; > > + if (type == TQMX86_INT_TRIG_BOTH) > + type = tqmx86_gpio_get(&gpio->chip, hwirq) ^^^^^ > + ? TQMX86_INT_TRIG_FALLING > + : TQMX86_INT_TRIG_RISING; > + } > + > mask = TQMX86_GPII_MASK(offset); ^^^^^^ > val = TQMX86_GPII_CONFIG(offset, type); ^^^^^^ > _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); The offset stuff wasn't beautiful and I'm glad you are deleting it. My understanding is that a hwirq is 0-3 for output or 4-7 input. An offset is "hwirq % 4"? There are a bunch of places which are still marked as taking an offset but they all actually take a hwirq. For example, tqmx86_gpio_get() above. The only things which still actually take an offset are the TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros. Could you: 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq? 2) Rename all the "offset" variables to "hwirq"? regards, dan carpenter
On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote: > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > > index c957be3341774..400415676ad5d 100644 > > --- a/drivers/gpio/gpio-tqmx86.c > > +++ b/drivers/gpio/gpio-tqmx86.c > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq) > > unsigned int offset = hwirq - TQMX86_NGPO; > > u8 type = TQMX86_INT_TRIG_NONE, mask, val; > > > > - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) > > + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) { > > type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK; > > > > + if (type == TQMX86_INT_TRIG_BOTH) > > + type = tqmx86_gpio_get(&gpio->chip, hwirq) > ^^^^^ > > > + ? TQMX86_INT_TRIG_FALLING > > + : TQMX86_INT_TRIG_RISING; > > + } > > + > > mask = TQMX86_GPII_MASK(offset); > ^^^^^^ > > val = TQMX86_GPII_CONFIG(offset, type); > ^^^^^^ > > _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); > > The offset stuff wasn't beautiful and I'm glad you are deleting it. My > understanding is that a hwirq is 0-3 for output or 4-7 input. An offset > is "hwirq % 4"? > > There are a bunch of places which are still marked as taking an offset > but they all actually take a hwirq. For example, tqmx86_gpio_get() > above. The only things which still actually take an offset are the > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros. > > Could you: > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq? > 2) Rename all the "offset" variables to "hwirq"? Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and hwirq numbers differ, is it? Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7, while GPIOs 0-3 don't have interrupt support. We will soon be mainlining support for our SMARC modules, which have up to 14 GPIOs, and (on some families) IRQ support for all GPIOs (IRQs 0-13 correspond to GPIOs 0-13). New interrupt config and status registers have been introduced to support more IRQs - up to 4 config registers (2 bits for each IRQ) and 3 status registers (IRQs 0-3 in the first one, 4-11 in the second one, 12-13 in the third one... so this part is a bit more convoluted than just "hwirq % 4") As the mapping between GPIOs and IRQs will become dynamic with these changes, I'd rather keep TQMX86_GPII_* using IRQ numbers instead of GPIO numbers. We will be introducing helpers for accessing the interrupt registers; the macros deal with individual register bits, and I think they should be agnostic of the mapping to GPIO/hwirq numbers. Matthias > > regards, > dan carpenter >
On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote: > On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote: > > > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > > > index c957be3341774..400415676ad5d 100644 > > > --- a/drivers/gpio/gpio-tqmx86.c > > > +++ b/drivers/gpio/gpio-tqmx86.c > > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq) > > > unsigned int offset = hwirq - TQMX86_NGPO; > > > u8 type = TQMX86_INT_TRIG_NONE, mask, val; > > > > > > - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) > > > + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) { > > > type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK; > > > > > > + if (type == TQMX86_INT_TRIG_BOTH) > > > + type = tqmx86_gpio_get(&gpio->chip, hwirq) > > ^^^^^ > > > > > + ? TQMX86_INT_TRIG_FALLING > > > + : TQMX86_INT_TRIG_RISING; > > > + } > > > + > > > mask = TQMX86_GPII_MASK(offset); > > ^^^^^^ > > > val = TQMX86_GPII_CONFIG(offset, type); > > ^^^^^^ > > > _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); > > > > The offset stuff wasn't beautiful and I'm glad you are deleting it. My > > understanding is that a hwirq is 0-3 for output or 4-7 input. An offset > > is "hwirq % 4"? > > > > There are a bunch of places which are still marked as taking an offset > > but they all actually take a hwirq. For example, tqmx86_gpio_get() > > above. The only things which still actually take an offset are the > > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros. > > > > Could you: > > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq? > > 2) Rename all the "offset" variables to "hwirq"? > > Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers > depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and > hwirq numbers differ, is it? > > Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7, > while GPIOs 0-3 don't have interrupt support. I'm so confused. So "offset" is the GPIO number and "hwirq" is the IRQ number? If the IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in _tqmx86_gpio_irq_config()? unsigned int offset = hwirq - TQMX86_NGPO; And again, it's just weird to call: type = tqmx86_gpio_get(&gpio->chip, hwirq); where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as an argument. regards, dan carpenter
On Thu, 2024-05-30 at 13:22 +0300, Dan Carpenter wrote: > > On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote: > > On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote: > > > > > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > > > > index c957be3341774..400415676ad5d 100644 > > > > --- a/drivers/gpio/gpio-tqmx86.c > > > > +++ b/drivers/gpio/gpio-tqmx86.c > > > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq) > > > > unsigned int offset = hwirq - TQMX86_NGPO; > > > > u8 type = TQMX86_INT_TRIG_NONE, mask, val; > > > > > > > > - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) > > > > + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) { > > > > type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK; > > > > > > > > + if (type == TQMX86_INT_TRIG_BOTH) > > > > + type = tqmx86_gpio_get(&gpio->chip, hwirq) > > > ^^^^^ > > > > > > > + ? TQMX86_INT_TRIG_FALLING > > > > + : TQMX86_INT_TRIG_RISING; > > > > + } > > > > + > > > > mask = TQMX86_GPII_MASK(offset); > > > ^^^^^^ > > > > val = TQMX86_GPII_CONFIG(offset, type); > > > ^^^^^^ > > > > _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); > > > > > > The offset stuff wasn't beautiful and I'm glad you are deleting it. My > > > understanding is that a hwirq is 0-3 for output or 4-7 input. An offset > > > is "hwirq % 4"? > > > > > > There are a bunch of places which are still marked as taking an offset > > > but they all actually take a hwirq. For example, tqmx86_gpio_get() > > > above. The only things which still actually take an offset are the > > > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros. > > > > > > Could you: > > > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq? > > > 2) Rename all the "offset" variables to "hwirq"? > > > > Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers > > depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and > > hwirq numbers differ, is it? > > > > Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7, > > while GPIOs 0-3 don't have interrupt support. > > I'm so confused. > > So "offset" is the GPIO number and "hwirq" is the IRQ number? If the > IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in The current naming in the driver is confusing and I'll fix that in the next round of refactoring patches. Generally, hwirq == GPIO number (I have not found a way to change this mapping - if there is one, I'd be interested to try if it makes the code less confusing). "offset" currently always refers to some shift in a hardware register. In tqmx86_gpio_get and tqmx86_gpio_set, offset is a GPIO number. In all functions dealing with IRQs, offset is an IRQ number (which is different from the hwirq number). Matthias > _tqmx86_gpio_irq_config()? > > unsigned int offset = hwirq - TQMX86_NGPO; > > And again, it's just weird to call: > > type = tqmx86_gpio_get(&gpio->chip, hwirq); > > where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as > an argument. > > regards, > dan carpenter >
On Thu, 2024-05-30 at 13:15 +0200, Matthias Schiffer wrote: > On Thu, 2024-05-30 at 13:22 +0300, Dan Carpenter wrote: > > > > On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote: > > > On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote: > > > > > > > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote: > > > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > > > > > index c957be3341774..400415676ad5d 100644 > > > > > --- a/drivers/gpio/gpio-tqmx86.c > > > > > +++ b/drivers/gpio/gpio-tqmx86.c > > > > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq) > > > > > unsigned int offset = hwirq - TQMX86_NGPO; > > > > > u8 type = TQMX86_INT_TRIG_NONE, mask, val; > > > > > > > > > > - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) > > > > > + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) { > > > > > type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK; > > > > > > > > > > + if (type == TQMX86_INT_TRIG_BOTH) > > > > > + type = tqmx86_gpio_get(&gpio->chip, hwirq) > > > > ^^^^^ > > > > > > > > > + ? TQMX86_INT_TRIG_FALLING > > > > > + : TQMX86_INT_TRIG_RISING; > > > > > + } > > > > > + > > > > > mask = TQMX86_GPII_MASK(offset); > > > > ^^^^^^ > > > > > val = TQMX86_GPII_CONFIG(offset, type); > > > > ^^^^^^ > > > > > _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); > > > > > > > > The offset stuff wasn't beautiful and I'm glad you are deleting it. My > > > > understanding is that a hwirq is 0-3 for output or 4-7 input. An offset > > > > is "hwirq % 4"? > > > > > > > > There are a bunch of places which are still marked as taking an offset > > > > but they all actually take a hwirq. For example, tqmx86_gpio_get() > > > > above. The only things which still actually take an offset are the > > > > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros. > > > > > > > > Could you: > > > > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq? > > > > 2) Rename all the "offset" variables to "hwirq"? > > > > > > Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers > > > depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and > > > hwirq numbers differ, is it? > > > > > > Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7, > > > while GPIOs 0-3 don't have interrupt support. > > > > I'm so confused. > > > > So "offset" is the GPIO number and "hwirq" is the IRQ number? If the > > IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in > > The current naming in the driver is confusing and I'll fix that in the next round of refactoring > patches. > > Generally, hwirq == GPIO number (I have not found a way to change this mapping - if there is one, > I'd be interested to try if it makes the code less confusing). "offset" currently always refers to > some shift in a hardware register. In tqmx86_gpio_get and tqmx86_gpio_set, offset is a GPIO number. > In all functions dealing with IRQs, offset is an IRQ number (which is different from the hwirq > number). > > Matthias Ah, I just noticed the "to_irq" function in struct gpio_chip, I guess I simply didn't look in the right place before. Will have a closer look when I rebase the refactoring patches. Matthias > > > > _tqmx86_gpio_irq_config()? > > > > unsigned int offset = hwirq - TQMX86_NGPO; > > > > And again, it's just weird to call: > > > > type = tqmx86_gpio_get(&gpio->chip, hwirq); > > > > where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as > > an argument. > > > > regards, > > dan carpenter > > >
> Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7, > while GPIOs 0-3 don't have interrupt support. We will soon be mainlining support for our SMARC > modules, which have up to 14 GPIOs, and (on some families) IRQ support for all GPIOs (IRQs 0-13 > correspond to GPIOs 0-13). > > New interrupt config and status registers have been introduced to support more IRQs - up to 4 config > registers (2 bits for each IRQ) and 3 status registers (IRQs 0-3 in the first one, 4-11 in the > second one, 12-13 in the third one... so this part is a bit more convoluted than just "hwirq % 4") Depending on how different it is, you could consider a second driver, rather than make this driver more complex. Andrew
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c index c957be3341774..400415676ad5d 100644 --- a/drivers/gpio/gpio-tqmx86.c +++ b/drivers/gpio/gpio-tqmx86.c @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq) unsigned int offset = hwirq - TQMX86_NGPO; u8 type = TQMX86_INT_TRIG_NONE, mask, val; - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) { type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK; + if (type == TQMX86_INT_TRIG_BOTH) + type = tqmx86_gpio_get(&gpio->chip, hwirq) + ? TQMX86_INT_TRIG_FALLING + : TQMX86_INT_TRIG_RISING; + } + mask = TQMX86_GPII_MASK(offset); val = TQMX86_GPII_CONFIG(offset, type); _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); @@ -200,8 +206,8 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc) struct gpio_chip *chip = irq_desc_get_handler_data(desc); struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip); struct irq_chip *irq_chip = irq_desc_get_chip(desc); - unsigned long irq_bits; - int i = 0; + unsigned long irq_bits, flags; + int i, hwirq; u8 irq_status; chained_irq_enter(irq_chip, desc); @@ -210,6 +216,36 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc) tqmx86_gpio_write(gpio, TQMX86_GPIIS, irq_status); irq_bits = irq_status; + + raw_spin_lock_irqsave(&gpio->spinlock, flags); + for_each_set_bit(i, &irq_bits, TQMX86_NGPI) { + hwirq = i + TQMX86_NGPO; + + /* + * Edge-both triggers are implemented by flipping the edge + * trigger after each interrupt, as the controller only supports + * either rising or falling edge triggers, but not both. + * + * Internally, the TQMx86 GPIO controller has separate status + * registers for rising and falling edge interrupts. GPIIC + * configures which bits from which register are visible in the + * interrupt status register GPIIS and defines what triggers the + * parent IRQ line. Writing to GPIIS always clears both rising + * and falling interrupt flags internally, regardless of the + * currently configured trigger. + * + * In consequence, we can cleanly implement the edge-both + * trigger in software by first clearing the interrupt and then + * setting the new trigger based on the current GPIO input in + * _tqmx86_gpio_irq_config() - even if an edge arrives between + * reading the input and setting the trigger, we will have a new + * interrupt pending. + */ + if ((gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK) == TQMX86_INT_TRIG_BOTH) + _tqmx86_gpio_irq_config(gpio, hwirq); + } + raw_spin_unlock_irqrestore(&gpio->spinlock, flags); + for_each_set_bit(i, &irq_bits, TQMX86_NGPI) generic_handle_domain_irq(gpio->chip.irq.domain, i + TQMX86_NGPO);
The TQMx86 GPIO controller only supports falling and rising edge triggers, but not both. Fix this by implementing a software both-edge mode that toggles the edge type after every interrupt. Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller") Co-developed-by: Gregor Herburger <gregor.herburger@tq-group.com> Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/gpio/gpio-tqmx86.c | 42 +++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)