Re: [PATCH v3 03/10] irqchip/sun6i-r: Use a stacked irqchip driver

[Marc Zyngier]

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@xxxxxxxxxxxx> 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...