[v2,2/7] allwinner-a10-pic: update pending register when an irq is cleared

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.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/intc/allwinner-a10-pic.c |    2 ++
 1 file changed, 2 insertions(+)

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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 <b.galvani@gmail.com>
> ---
>  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
>
>

On Mon, Mar 03, 2014 at 09:56:07PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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?

Hi,
the kernel driver uses this function to ack an interrupt [1]:

static void sun4i_irq_ack(struct irq_data *irqd)
{
        unsigned int irq = irqd_to_hwirq(irqd);
        unsigned int irq_off = irq % 32;
        int reg = irq / 32;
        u32 val;

        val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
        writel(val | (1 << irq_off),
                   sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
}

I don't understand how this function can work, because if we assume
that the SUN4I_IRQ_PENDING register is a wtc register as specified in
the datasheet [2] at p.114 (and ignoring that the register is also
described as read-only), the function would clear all the bits every
time is called.

As reported in the comment for v1, there was a thread on LKML [3] in
which a drop of all the writes to the pending register was proposed,
and a mail from the driver author explained that writes to the
register are basically uneffective.

So my initial idea was to remove the writability of pending register
and to change all interrupts to level-triggered; but since I was not
sure about the first point, I kept the writability.

Beniamino

[1] http://lxr.free-electrons.com/source/drivers/irqchip/irq-sun4i.c#L41
[2] http://dl.linux-sunxi.org/A10/A10%20User%20Manual%20-%20v1.20%20%282012-04-09,%20DECRYPTED%29.pdf
[3] https://lkml.org/lkml/2013/7/6/59

> Regards,
> Peter
> 
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  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
> >
> >