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.
However, after trying for several minutes, I'm not sure there's an easy
way to improve it.
> +
> 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>
signature.asc
Description: This is a digitally signed message part
