Messages in this thread | | | Date | Thu, 13 Oct 2022 09:30:25 +0200 | From | Michael Walle <> | Subject | Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller |
| |
Hi Horatiu,
Am 2022-10-07 11:49, schrieb Horatiu Vultur: > The 10/06/2022 13:43, Michael Walle wrote: > > Hi Walle, > >> Seeing 20 was definitely fishy, seeing two instead of one maybe not >> so much. I guess it will create one spurious interrupt if none of >> the registered handlers will care. >> >> OTOH, the code below won't work in all cases anyway, right? It's just >> best effort. > > I was expecting to work in all cases, but if you found some cases that > would not work, please point them out. > >> >> > Below I have a diff that I tried with LAN8814 PHYs and I could see that >> > count in /proc/interrupts is increasing correctly. >> > >> > > I've verified that there is only one low pulse on the interrupt line. >> > > I've >> > > noticed though, that the number of interrupts seem to be correlating >> > > with >> > > the length of the low pulse. >> > --- >> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c >> > b/drivers/pinctrl/pinctrl-ocelot.c >> > index c7df8c5fe5854..105771ff82e62 100644 >> > --- a/drivers/pinctrl/pinctrl-ocelot.c >> > +++ b/drivers/pinctrl/pinctrl-ocelot.c >> > @@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct >> > irq_data *data) >> > if (val & bit) >> > ack = true; >> > >> > + /* Try to clear any rising edges */ >> > + if (!active && ack) >> > + regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio), >> > + bit, bit); >> >> Might we lose interrupts here, if the line would go active again right >> after the read of the line state and before reading the "ack" bit? > > We lose the interrupt here, as the HW will not generate another one > but at later point we read again the line status. And if the line is > active then we kick again the interrupt handler again.
Ahh, thanks for explaining. That also explains the read below.
Will you send a proper patch?
-michael
> >> >> > + >> > /* Enable the interrupt now */ >> > gpiochip_enable_irq(chip, gpio); >> > regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio), >> > bit, bit); >> > >> > /* >> > - * In case the interrupt line is still active and the interrupt >> > - * controller has not seen any changes in the interrupt line, then it >> > - * means that there happen another interrupt while the line was >> > active. >> > + * In case the interrupt line is still active then it means that >> > + * there happen another interrupt while the line was active. >> > * So we missed that one, so we need to kick the interrupt again >> > * handler. >> > */ >> > - if (active && !ack) { >> > + regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val); >> > + if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) || >> > + (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH)) >> > + active = true; >> >> Why do you read the line state twice? What happens if the line state >> changes right after you've read it? > > Here we need to read again the status because we might have clear the > ack of interrupt. > If the line becomes active right after this read, then the HW will > generate another interrupt as the interrupt is enabled and ack is > cleared. > >> >> > + >> > + if (active) { >> > struct ocelot_irq_work *work; >> > >> > work = kmalloc(sizeof(*work), GFP_ATOMIC); >> >> So yes, maybe the trade-off that there will be two interrupts are >> better than this additional patch. But it should be documented >> somewhere, even if it's just a comment in this driver. >> >> -michael
| |