Messages in this thread | | | Date | Mon, 04 Jan 2021 10:03:47 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH v3 03/10] irqchip/sun6i-r: Use a stacked irqchip driver |
| |
On 2021-01-04 03:46, Samuel Holland wrote: > On 1/3/21 7:10 AM, Marc Zyngier wrote: >> On Sun, 03 Jan 2021 12:08:43 +0000, >> Samuel Holland <samuel@sholland.org> wrote: >>> >>> On 1/3/21 5:27 AM, Marc Zyngier wrote:
[...]
>>> For edge interrupts, don't you want to ack as early as possible, >>> before the handler clears the source of the interrupt? That way if a >>> second interrupt comes in while you're handling the first one, you >>> don't ack the second one without handling it? >> >> It completely depends on what this block does. If, as I expect, it >> latches the interrupt, then it needs clearing after the GIC has acked >> the incoming interrupt. > > Yes, there is an internal S/R latch. > - For edge interrupts, the latch is set once for each pulse. > - For level interrupts, it gets set continuously as long as the > pin is high/low. > - Writing a "1" to bit 0 of PENDING resets the latch. > - The output of the latch goes to the GIC. > >>>> It also begs the question: why would you want to clear the signal to >>>> the GIC on mask (or unmask)? The expectations are that a pending >>>> interrupt is preserved across a mask/unmask sequence. >>> >>> I hadn't thought about anything masking the IRQ outside of the >>> handler; but you're right, this breaks that case. I'm trying to work >>> within the constraints of stacking the GIC driver, which assumes >>> handle_fasteoi_irq, so it sounds like I should switch back to >>> handle_fasteoi_ack_irq and use .irq_ack. Or based on your previous >>> paragraph, maybe I'm missing some other consideration? >> >> handle_fasteoi_ack_irq() sounds like a good match for edge >> interrupts. Do you actually need to do anything for level signals? If >> you do, piggybacking on .irq_eoi would do the trick. > > For level interrupts, I have to reset the latch (see above) after the > source of > the interrupt is cleared.
Right, so that is definitely to be done in .irq_eoi, at least in the non-threaded case (as it doesn't involve masking/unmasking).
> That was the bug with v2: I set IRQ_EOI_THREADED so .irq_eoi would run > after the > thread. But with GICv2 EOImode==0, that blocked other interrupts from > being > received during the IRQ thread. Which is why I moved it to .irq_unmask > and > removed the flag: so .irq_eoi runs at the end of the hardirq > (unblocking further > interrupts at the GIC), and .irq_unmask resets the latch at the end of > the thread. > > With the flag removed, but still clearing the latch in .irq_eoi, every > edge IRQ
edge? Didn't you mean level here? Edge interrupts really should clear the latch in .irq_ack.
> was followed by a second, spurious IRQ after the thread finished. > > Does that make sense?
It does. It is a bit of a kludge, but hey, silly HW (if only this could be turned into a bypass, it'd all be simpler).
To sum it up, this is what I'd expect to see:
For edge interrupts: - clear latch in .irq_ack and .irq_set_irqchip_state(PENDING) - interrupt flow set to fasteoi_ack
For level interrupts - clear latch in .irq_eoi (non-threaded) and .irq_unmask (threaded) - interrupt flow set to fasteoi (though leaving to the _ack version should not hurt).
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |