Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Beniamino Galvani]

On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
> >>Beniamino Galvani wrote:
> >>>According to this mail thread [1], writing to pending register seems
> >>>to have no effect on actual pending status of interrupts. This means
> >>>that the only way to clear a pending interrupt is to clear the
> >>>interrupt source. This patch implements such behaviour.
> >>>
> >>>[1] http://lkml.org/lkml/2013/7/6/59
> >>>
> >>>Signed-off-by: Beniamino Galvani<address@hidden>
> >>>---
> >>>  hw/intc/allwinner-a10-pic.c |    6 ++++--
> >>>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> >>>index bb2351f..afd57ef 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);
> >>>  }
> >>>@@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr 
> >>>offset, uint64_t value,
> >>>          s->nmi = value;
> >>>          break;
> >>>      case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
> >>>-        s->irq_pending[index]&= ~value;
> >>>+        /* Nothing to do */
> >>>          break;
> >>>      case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
> >>>-        s->fiq_pending[index]&= ~value;
> >>>+        /* Ditto */
> >>>          break;
> >>>      case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
> >>>          s->select[index] = value;
> >>pending registers are also clear registers by a10 datasheet,
> >>also you found bits are marked as 'R', so, ..., contradict itself.
> >Yes, the datasheet is inconsistent about this because the register
> >can't be read-only and 'clear' at the same time.
> >
> >Unfortunately at the moment I cannot test if the clearing
> >functionality of the pending register works on real hardware but the
> >idea I got from the linked discussion is that it's either not
> >implemented or broken and therefore interrupts remain pending until
> >they are disabled at the source.
> >
> >Do you have a chance to try it on a real board?
> >
> Ah? even kernel code from allwinner wrote pending registers
> to clear pending interrupt, didn't you see it?
> so should be no doubt that these registers are writable.

Well, if you look closely at that code, it's a bit strange:

void sw_irq_ack(struct irq_data *irqd)
{
    unsigned int irq = irqd->irq;

    [...]
    writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<<irq), SW_INT_IRQ_PENDING_REG0);
    [...]
}

In order to clear a single interrupt, it is or'ing the irq bit with
the previous value of the register, so it's basically clearing all
pending interrupts. 

This is discussed in the mentioned thread from lkml and the final
explanation is that the pending register is always updated with the
actual status of irq lines, even if you write to it. In other words,
writes to the register are ignored, otherwise the code above would
produce random loss of interrupts.

Beniamino