| Message ID | 1392659003-8264-4-git-send-email-b.galvani@gmail.com |
|---|---|
| State | New |
| Headers | show |
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. Thanks! 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. > > 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); > } >
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. Beniamino > >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); > > } >
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); >>> } >>> >> > >
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); }
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. Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> --- hw/timer/allwinner-a10-pit.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)