Re: [Qemu-devel] [PATCH v5 11/14] hw/timer/nrf51_timer: Add nRF51 Timer peripheral

[Peter Maydell]

On 12 November 2018 at 21:42, Steffen Görtz <address@hidden> wrote:
> This patch adds the model for the nRF51 timer peripheral.
> Currently, only the TIMER mode is implemented.
>
> Signed-off-by: Steffen Görtz <address@hidden>

> +static void set_prescaler(NRF51TimerState *s, uint32_t prescaler)
> +{
> +    uint64_t period;
> +    s->prescaler = prescaler;
> +
> +    period = ((1UL << s->prescaler) * TIMER_TICK_PS) / 1000;
> +    /* Limit minimum timeout period to 10us to allow some progress */
> +    if (period < MINIMUM_PERIOD) {
> +        s->tick_period = MINIMUM_PERIOD;
> +        s->counter_inc = MINIMUM_PERIOD / period;
> +    } else {
> +        s->tick_period = period;
> +        s->counter_inc = 1;
> +    }
> +}
> +
> +static void update_irq(NRF51TimerState *s)
> +{
> +    bool flag = false;
> +    size_t i;
> +
> +    for (i = 0; i < NRF51_TIMER_REG_COUNT; i++) {
> +        flag |= s->events_compare[i] && extract32(s->inten, 16 + i, 1);
> +    }
> +    qemu_set_irq(s->irq, flag);
> +}
> +
> +static void timer_expire(void *opaque)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(opaque);
> +    bool should_stop = false;
> +    uint32_t counter = s->counter;
> +    size_t i;
> +    uint64_t diff;
> +
> +    if (s->running) {
> +        for (i = 0; i < NRF51_TIMER_REG_COUNT; i++) {
> +            if (counter < s->cc[i]) {
> +                diff = s->cc[i] - counter;
> +            } else {
> +                diff = (s->cc[i] + BIT(bitwidths[s->bitmode])) - counter;
> +            }
> +
> +            if (diff <= s->counter_inc) {
> +                s->events_compare[i] = true;
> +
> +                if (s->shorts & BIT(i)) {
> +                    s->counter = 0;
> +                }
> +
> +                should_stop |= s->shorts & BIT(i + 8);
> +            }
> +        }
> +
> +        s->counter += s->counter_inc;
> +        s->counter &= (BIT(bitwidths[s->bitmode]) - 1);
> +
> +        update_irq(s);
> +
> +        if (should_stop) {
> +            s->running = false;
> +            timer_del(&s->timer);
> +        } else {
> +            s->time_offset += s->tick_period;
> +            timer_mod_ns(&s->timer, s->time_offset);
> +        }
> +    } else {
> +        timer_del(&s->timer);
> +    }
> +}

This looks like it's setting the timer to fire every tick_period,
whether that is going to hit a comparison point or not. This
is not recommended because it's a lot of overhead. It's much
better to identify when the next interesting event is going to
happen and set the timer for that point, and then evaluate
what has happened at the point when your timer has fired.
(If the guest reads a register in the meantime you can
calculate what its value should be at the point when the guest
does the read.)

thanks
-- PMM