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

[Marc Zyngier <maz@kernel.org>]

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