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

[Li Guang <lig.fnst@cn.fujitsu.com>]

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<b.galvani@gmail.com>
>>> ---
>>>   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);
>>>       }
>>>        
>>      
>
>