Re: [Qemu-devel] [PATCH v2 2/7] allwinner-a10-pic: update pending register when an irq is cleared

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <address@hidden> wrote:
> The value of pending register was updated only when an irq was raised
> from a device; it should also be updated when an interrupt is cleared.
>

So the reason for doing this is obviously the need for level sensitive
interrupts. Current implementation works under the assumption that all
interrupts are edge sensitive. This patch goes full on the other way
and mandates that all interrupts the edge-sensitive. In the absence of
definitive documentation, I guess that ok, but you do effecitively
defeature the interrupt controller transforming and edge to a level
(which is a very common interrupt controller feature). It is possible
to implement both schemes concurrently with some extra state. I.e. one
set of registers for the external pin state (no latching) and one set
for interrupts that have been detected and not serviced yet (via wtc
to IRQ_PENDING). If an interrupt is serviced while the pin state is
still high then it insta-retriggers (correct level sens. behav.).

You have left in the support for clearing the register from software.
Although I can see some wierd use cases of this. I.e. an interrupt can
be triggered and "serviced" (i.e. irq_pending wtc'd) by the intc then
the device level service is delayed while the device irq line remains
high. To, me this actually corresponds to a disabling of the interrupt
(assuming it is level sensitive!) rather than a servicing of a pending
interrupt. This match the kernel driver?

Regards,
Peter

> Signed-off-by: Beniamino Galvani <address@hidden>
> ---
>  hw/intc/allwinner-a10-pic.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 00f3c11..9011c82 100644
> --- a/hw/intc/allwinner-a10-pic.c
> +++ b/hw/intc/allwinner-a10-pic.c
> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int 
> level)
>
>      if (level) {
>          set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> +    } else {
> +        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>      }
>      aw_a10_pic_update(s);
>  }
> --
> 1.7.10.4
>
>