Re: [Qemu-devel] [PATCH v3 4/7] allwinner-a10-pit: use level triggered interrupts

[Peter Crosthwaite]

On Sat, Mar 15, 2014 at 11:01 PM, Beniamino Galvani <address@hidden> wrote:
> Convert the interrupt generation logic to the use of level triggered
> interrupts.
>
> Signed-off-by: Beniamino Galvani <address@hidden>
> ---
>  hw/timer/allwinner-a10-pit.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index 696b7d9..f8c9236 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -19,6 +19,15 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/timer/allwinner-a10-pit.h"
>
> +static void a10_pit_update_irq(AwA10PITState *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
> +        qemu_set_irq(s->irq[i], s->irq_status & s->irq_enable & (1 << i));

People do sometimes use a !! on the level arg to qemu_set_irq to force
0/1 behavior but I think our most recent on-list conclusion is it's
not a requirement. I'm all for the change however, as it makes the
eventual cleanup of qemu_set_irq perhaps a little easier for someone.

> +    }
> +}
> +
>  static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      AwA10PITState *s = AW_A10_PIT(opaque);
> @@ -74,9 +83,11 @@ static void a10_pit_write(void *opaque, hwaddr offset, 
> uint64_t value,
>      switch (offset) {
>      case AW_A10_PIT_TIMER_IRQ_EN:
>          s->irq_enable = value;
> +        a10_pit_update_irq(s);
>          break;
>      case AW_A10_PIT_TIMER_IRQ_ST:
>          s->irq_status &= ~value;
> +        a10_pit_update_irq(s);
>          break;
>      case AW_A10_PIT_TIMER_BASE ... AW_A10_PIT_TIMER_BASE_END:
>          index = offset & 0xf0;
> @@ -203,7 +214,7 @@ static void a10_pit_timer_cb(void *opaque)
>              ptimer_stop(s->timer[i]);
>              s->control[i] &= ~AW_A10_PIT_TIMER_EN;
>          }
> -        qemu_irq_pulse(s->irq[i]);
> +        a10_pit_update_irq(s);
>      }
>  }
>

I think you need to a10_pit_update_irq() in your reset handler.
Otherwise your level sensitive IRQs can stay high through your
peripheral's (or system's) hard reset.

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> --
> 1.7.10.4
>
>