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

[Beniamino Galvani <b.galvani@gmail.com>]

On Mon, Feb 24, 2014 at 02:45:06PM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >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<b.galvani@gmail.com>
> >>>>>---
> >>>>>  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.
> >
> 
> Hmm..., sorry,  I also can't test this operation on A10 board now,
> but why not they just wipe out these writings(kernel 3.12)?

I don't know, there was a proposed patch that removed those writes in
the lkml discussion but probably it never reached mainline.

To be on the safe side I can restore the writability of the register
and then, when we will figure out how it really works, correct it if
needed.

Beniamino