On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galv...@gmail.com> 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.
>
Nice catch! I think there was a conditional in the earlier versions of
the original series that caught this but it got lost in the later
reivew stages. I do however prefer this de-looped/coreified
implementation.
> Signed-off-by: Beniamino Galvani <b.galv...@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;
"state" is a bit ambiguous for a variable name. I have seen "parent"
used in this context before (although I must confess that may get
confused with QOM terms)."container"?
> + 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;
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++) {
... is a local variable to this for loop iteration. So it can be
defined here for easier reading.
Otherwise:
Reviewed-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
Regards,
Peter
> - 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);
> }
> --
> 1.7.10.4
>
>
