Re: [Qemu-devel] [PATCH 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Li Guang]

Beniamino Galvani wrote:
> On Tue, Feb 18, 2014 at 12:17:04PM +0800, Li Guang wrote:
>    
> > Beniamino Galvani wrote:
> >      
> > > The model was generating interrupts for all enabled timers after the
> > > expiration of one of them. Avoid this by passing to the timer callback
> > > function a structure containing the index of the expired timer.
> > >
> > >        
> > did you detect spurious?
> > didn't by my limited test,
> > code will disable any expired timer, unless be re-armed,
> > so it will never generate interrupt any more.
> >      
> Yes, when both timer0 and timer1 were enabled and timer0 expired, the
> previous implementation raised two interrupts because the callback
> function iterated over the array of timers. Instead it should consider
> only the timer that generated the event.
>
>    

Ok, I like it this if it really can eliminate spurious interrupts.

Thanks!

>    
> > > Signed-off-by: Beniamino Galvani<address@hidden>
> > > ---
> > >   hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
> > >   1 file changed, 19 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > > index b27fce8..3e1c183 100644
> > > --- a/hw/timer/allwinner-a10-pit.c
> > > +++ b/hw/timer/allwinner-a10-pit.c
> > > @@ -19,6 +19,11 @@
> > >   #include "sysemu/sysemu.h"
> > >   #include "hw/timer/allwinner-a10-pit.h"
> > >
> > > +typedef struct TimerContext {
> > > +    AwA10PITState *state;
> > > +    int index;
> > > +} TimerContext;
> > > +
> > >   static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
> > >   {
> > >       AwA10PITState *s = AW_A10_PIT(opaque);
> > > @@ -193,18 +198,17 @@ static void a10_pit_reset(DeviceState *dev)
> > >
> > >   static void a10_pit_timer_cb(void *opaque)
> > >   {
> > > -    AwA10PITState *s = AW_A10_PIT(opaque);
> > > -    uint8_t i;
> > > +    TimerContext *tc = opaque;
> > > +    AwA10PITState *s = tc->state;
> > > +    uint8_t i = tc->index;
> > >
> > > -    for (i = 0; i<   AW_A10_PIT_TIMER_NR; i++) {
> > > -        if (s->control[i]&   AW_A10_PIT_TIMER_EN) {
> > > -            s->irq_status |= 1<<   i;
> > > -            if (s->control[i]&   AW_A10_PIT_TIMER_MODE) {
> > > -                ptimer_stop(s->timer[i]);
> > > -                s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> > > -            }
> > > -            qemu_irq_pulse(s->irq[i]);
> > > +    if (s->control[i]&   AW_A10_PIT_TIMER_EN) {
> > > +        s->irq_status |= 1<<   i;
> > > +        if (s->control[i]&   AW_A10_PIT_TIMER_MODE) {
> > > +            ptimer_stop(s->timer[i]);
> > > +            s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> > >           }
> > > +        qemu_irq_pulse(s->irq[i]);
> > >       }
> > >   }
> > >
> > > @@ -213,6 +217,7 @@ static void a10_pit_init(Object *obj)
> > >       AwA10PITState *s = AW_A10_PIT(obj);
> > >       SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > >       QEMUBH * bh[AW_A10_PIT_TIMER_NR];
> > > +    TimerContext *tc;
> > >       uint8_t i;
> > >
> > >       for (i = 0; i<   AW_A10_PIT_TIMER_NR; i++) {
> > > @@ -223,7 +228,10 @@ static void a10_pit_init(Object *obj)
> > >       sysbus_init_mmio(sbd,&s->iomem);
> > >
> > >       for (i = 0; i<   AW_A10_PIT_TIMER_NR; i++) {
> > > -        bh[i] = qemu_bh_new(a10_pit_timer_cb, s);
> > > +        tc = g_malloc(sizeof(TimerContext));
> > > +        tc->state = s;
> > > +        tc->index = i;
> > > +        bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
> > >           s->timer[i] = ptimer_init(bh[i]);
> > >           ptimer_set_freq(s->timer[i], 240000);
> > >       }
> > >        
> >      
>
>