On Fri, 2017-06-09 at 07:40 +0200, Cédric Le Goater wrote: > On 06/09/2017 04:26 AM, Andrew Jeffery wrote: > > On Tue, 2017-06-06 at 10:55 +0200, Cédric Le Goater wrote: > > > When a timer is enabled before a reload value is set, the controller > > > waits for a reload value to be set before starting decrementing. This > > > fix tries to cover that case by changing the timer expiry only when > > > a reload value is valid. > > > > > > > Signed-off-by: Cédric Le Goater <c...@kaod.org> > > > > > > --- > > > hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++-------- > > > 1 file changed, 29 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > > > index 9b70ee09b07f..50acbf530a3a 100644 > > > --- a/hw/timer/aspeed_timer.c > > > +++ b/hw/timer/aspeed_timer.c > > > @@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer > > > *t) > > > next = seq[1]; > > > } else if (now < seq[2]) { > > > next = seq[2]; > > > - } else { > > > + } else if (t->reload) { > > > reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, > > > rate); > > > t->start = now - ((now - t->start) % reload_ns); > > > + } else { > > > + /* no reload value, return 0 */ > > > + break; > > > } > > > } > > > > > > return next; > > > } > > > > > > +static void aspeed_timer_mod(AspeedTimer *t) > > > +{ > > > + uint64_t next = calculate_next(t); > > > + if (next) { > > > + timer_mod(&t->timer, next); > > > + } > > > +} > > > + > > > static void aspeed_timer_expire(void *opaque) > > > { > > > AspeedTimer *t = opaque; > > > @@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque) > > > qemu_set_irq(t->irq, t->level); > > > } > > > > > > - timer_mod(&t->timer, calculate_next(t)); > > > + aspeed_timer_mod(t); > > > } > > > > > > static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg) > > > @@ -227,10 +238,23 @@ static void > > > aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, > > > uint32_t value) > > > { > > > AspeedTimer *t; > > > + uint32_t old_reload; > > > > > > trace_aspeed_timer_set_value(timer, reg, value); > > > t = &s->timers[timer]; > > > switch (reg) { > > > + case TIMER_REG_RELOAD: > > > + old_reload = t->reload; > > > + t->reload = value; > > > + > > > + /* If the reload value was not previously set, or zero, and > > > + * the current value is valid, try to start the timer if it is > > > + * enabled. > > > + */ > > > + if (old_reload || !t->reload) { > > > + break; > > > + } > > > > Maybe I need more caffeine, but I initially struggled to reconcile the > > condition with the comment, as the condition checks the inverse in > > order to break while the comment discusses the non-breaking case. > > I agree. The reload "value" is used in a hidden way to the activate the > timer. > > > However, after trying for several minutes, I'm not sure there's an easy > > way to improve it. > > I tried a few things. May be, we could move the following code in > its own routine and call it twice ?
I don't think it's necessary. The comment serves as enough warning - it should at least make people think before modifying the code. Cheers, Andrew > > > > + > > > case TIMER_REG_STATUS: > > > if (timer_enabled(t)) { > > > uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > > @@ -238,17 +262,14 @@ static void > > > aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, > > > uint32_t rate = calculate_rate(t); > > > > > > t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); > > > - timer_mod(&t->timer, calculate_next(t)); > > > + aspeed_timer_mod(t); > > > } > > > break; > > > - case TIMER_REG_RELOAD: > > > - t->reload = value; > > > - break; > > > case TIMER_REG_MATCH_FIRST: > > > case TIMER_REG_MATCH_SECOND: > > > t->match[reg - 2] = value; > > > if (timer_enabled(t)) { > > > - timer_mod(&t->timer, calculate_next(t)); > > > + aspeed_timer_mod(t); > > > } > > > break; > > > default: > > > @@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, > > > bool enable) > > > trace_aspeed_timer_ctrl_enable(t->id, enable); > > > if (enable) { > > > t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > > - timer_mod(&t->timer, calculate_next(t)); > > > + aspeed_timer_mod(t); > > > } else { > > > timer_del(&t->timer); > > > } > > > > Reviewed-by: Andrew Jeffery <and...@aj.id.au> > > Thanks, > > C.
signature.asc
Description: This is a digitally signed message part