Re: [Qemu-arm] [PATCH] hw/timer: Add value matching support to aspeed_timer

[Joel Stanley]

On Fri, May 27, 2016 at 12:08 AM, Andrew Jeffery <address@hidden> wrote:
> Value matching allows Linux to boot with CONFIG_NO_HZ_IDLE=y on the
> palmetto-bmc machine. Two match registers are provided for each timer.

Thanks for doing this. We now boot faster in my testing.

>
> Signed-off-by: Andrew Jeffery <address@hidden>

Acked-by: Joel Stanley <address@hidden>

> ---
>
> The change pulls out ptimer in favour of the regular timer infrastructure. As 
> a
> consequence it implements the conversions between ticks and time which feels a
> little tedious. Any comments there would be appreciated.
>
>  hw/timer/aspeed_timer.c         | 135 
> ++++++++++++++++++++++++++++++----------
>  include/hw/timer/aspeed_timer.h |   6 +-
>  2 files changed, 105 insertions(+), 36 deletions(-)
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 4b94808821b4..905de0f788b2 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -9,13 +9,12 @@
>   * the COPYING file in the top-level directory.
>   */
>
> +#include <math.h>
>  #include "qemu/osdep.h"
> -#include "hw/ptimer.h"
>  #include "hw/sysbus.h"
>  #include "hw/timer/aspeed_timer.h"
>  #include "qemu-common.h"
>  #include "qemu/bitops.h"
> -#include "qemu/main-loop.h"
>  #include "qemu/timer.h"
>  #include "qemu/log.h"
>  #include "trace.h"
> @@ -77,21 +76,99 @@ static inline bool timer_can_pulse(AspeedTimer *t)
>      return t->id >= TIMER_FIRST_CAP_PULSE;
>  }
>
> +static inline bool timer_external_clock(AspeedTimer *t)
> +{
> +    return timer_ctrl_status(t, op_external_clock);
> +}
> +
> +static double clock_rates[] = { TIMER_CLOCK_APB_HZ, TIMER_CLOCK_EXT_HZ };
> +
> +static inline double calculate_rate(struct AspeedTimer *t)
> +{
> +    return clock_rates[timer_external_clock(t)];
> +}
> +
> +static inline double calculate_period(struct AspeedTimer *t)
> +{
> +    return NANOSECONDS_PER_SECOND / calculate_rate(t);
> +}
> +
> +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t 
> now_ns)
> +{
> +    uint64_t delta_ns = now_ns - MIN(now_ns, t->start);
> +    uint32_t ticks = (uint32_t) floor(delta_ns / calculate_period(t));
> +
> +    return t->reload - MIN(t->reload, ticks);
> +}
> +
> +static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks)
> +{
> +    uint64_t delta_ns;
> +
> +    ticks = MIN(t->reload, ticks);
> +    delta_ns = (uint64_t) floor((t->reload - ticks) * calculate_period(t));
> +
> +    return t->start + delta_ns;
> +}
> +
> +static uint64_t calculate_next(struct AspeedTimer *t)
> +{
> +    uint64_t now;
> +    uint64_t next;
> +    int i;
> +    /* We don't know the relationship between the values in the match
> +     * registers, so sort using MAX/MIN/zero. We sort in that order as the
> +     * timer counts down to zero. */
> +    uint64_t seq[] = {
> +        MAX(t->match[0], t->match[1]),
> +        MIN(t->match[0], t->match[1]),
> +        0,
> +    };
> +
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    for (i = 0; i < ARRAY_SIZE(seq); i++) {
> +        next = calculate_time(t, seq[i]);
> +        if (now < next) {
> +            return next;
> +        }
> +    }
> +
> +    {
> +        uint64_t reload_ns;
> +
> +        reload_ns = (uint64_t) floor(t->reload * calculate_period(t));
> +        t->start = now - ((now - t->start) % reload_ns);
> +    }
> +
> +    return calculate_next(t);
> +}
> +
>  static void aspeed_timer_expire(void *opaque)
>  {
>      AspeedTimer *t = opaque;
> +    bool interrupt = false;
> +    uint32_t ticks;
>
> -    /* Only support interrupts on match values of zero for the moment - this 
> is
> -     * sufficient to boot an aspeed_defconfig Linux kernel.
> -     *
> -     * TODO: matching on arbitrary values (see e.g. hw/timer/a9gtimer.c)
> -     */
> -    bool match = !(t->match[0] && t->match[1]);
> -    bool interrupt = timer_overflow_interrupt(t) || match;
> -    if (timer_enabled(t) && interrupt) {
> +    if (!timer_enabled(t)) {
> +        return;
> +    }
> +
> +    ticks = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +
> +    if (!ticks) {
> +        interrupt = timer_overflow_interrupt(t) || !t->match[0] || 
> !t->match[1];
> +    } else if (ticks <= MIN(t->match[0], t->match[1])) {
> +        interrupt = true;
> +    } else if (ticks <= MAX(t->match[0], t->match[1])) {
> +        interrupt = true;
> +    }
> +
> +    if (interrupt) {
>          t->level = !t->level;
>          qemu_set_irq(t->irq, t->level);
>      }
> +
> +    timer_mod(&t->timer, calculate_next(t));
>  }
>
>  static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
> @@ -100,7 +177,7 @@ static uint64_t aspeed_timer_get_value(AspeedTimer *t, 
> int reg)
>
>      switch (reg) {
>      case TIMER_REG_STATUS:
> -        value = ptimer_get_count(t->timer);
> +        value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>          break;
>      case TIMER_REG_RELOAD:
>          value = t->reload;
> @@ -160,24 +237,21 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState 
> *s, int timer, int reg,
>      switch (reg) {
>      case TIMER_REG_STATUS:
>          if (timer_enabled(t)) {
> -            ptimer_set_count(t->timer, value);
> +            uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +            int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, 
> now);
> +
> +            t->start += delta * calculate_period(t);
> +            timer_mod(&t->timer, calculate_next(t));
>          }
>          break;
>      case TIMER_REG_RELOAD:
>          t->reload = value;
> -        ptimer_set_limit(t->timer, value, 1);
>          break;
>      case TIMER_REG_MATCH_FIRST:
>      case TIMER_REG_MATCH_SECOND:
> -        if (value) {
> -            /* Non-zero match values are unsupported. As such an interrupt 
> will
> -             * always be triggered when the timer reaches zero even if the
> -             * overflow interrupt control bit is clear.
> -             */
> -            qemu_log_mask(LOG_UNIMP, "%s: Match value unsupported by device: 
> "
> -                    "0x%" PRIx32 "\n", __func__, value);
> -        } else {
> -            t->match[reg - 2] = value;
> +        t->match[reg - 2] = value;
> +        if (timer_enabled(t)) {
> +            timer_mod(&t->timer, calculate_next(t));
>          }
>          break;
>      default:
> @@ -196,21 +270,16 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, 
> bool enable)
>  {
>      trace_aspeed_timer_ctrl_enable(t->id, enable);
>      if (enable) {
> -        ptimer_run(t->timer, 0);
> +        t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        timer_mod(&t->timer, calculate_next(t));
>      } else {
> -        ptimer_stop(t->timer);
> -        ptimer_set_limit(t->timer, t->reload, 1);
> +        timer_del(&t->timer);
>      }
>  }
>
>  static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
>  {
>      trace_aspeed_timer_ctrl_external_clock(t->id, enable);
> -    if (enable) {
> -        ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
> -    } else {
> -        ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
> -    }
>  }
>
>  static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
> @@ -351,12 +420,10 @@ static const MemoryRegionOps aspeed_timer_ops = {
>
>  static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
>  {
> -    QEMUBH *bh;
>      AspeedTimer *t = &s->timers[id];
>
>      t->id = id;
> -    bh = qemu_bh_new(aspeed_timer_expire, t);
> -    t->timer = ptimer_init(bh);
> +    timer_init_ns(&t->timer, QEMU_CLOCK_VIRTUAL, aspeed_timer_expire, t);
>  }
>
>  static void aspeed_timer_realize(DeviceState *dev, Error **errp)
> @@ -404,7 +471,7 @@ static const VMStateDescription vmstate_aspeed_timer = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(id, AspeedTimer),
>          VMSTATE_INT32(level, AspeedTimer),
> -        VMSTATE_PTIMER(timer, AspeedTimer),
> +        VMSTATE_TIMER(timer, AspeedTimer),
>          VMSTATE_UINT32(reload, AspeedTimer),
>          VMSTATE_UINT32_ARRAY(match, AspeedTimer, 2),
>          VMSTATE_END_OF_LIST()
> diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
> index 44dc2f89d5c6..970fea83143c 100644
> --- a/include/hw/timer/aspeed_timer.h
> +++ b/include/hw/timer/aspeed_timer.h
> @@ -22,7 +22,8 @@
>  #ifndef ASPEED_TIMER_H
>  #define ASPEED_TIMER_H
>
> -#include "hw/ptimer.h"
> +#include "qemu/typedefs.h"
> +#include "qemu/timer.h"
>
>  #define ASPEED_TIMER(obj) \
>      OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
> @@ -33,15 +34,16 @@ typedef struct AspeedTimer {
>      qemu_irq irq;
>
>      uint8_t id;
> +    QEMUTimer timer;
>
>      /**
>       * Track the line level as the ASPEED timers implement edge triggered
>       * interrupts, signalling with both the rising and falling edge.
>       */
>      int32_t level;
> -    ptimer_state *timer;
>      uint32_t reload;
>      uint32_t match[2];
> +    uint64_t start;
>  } AspeedTimer;
>
>  typedef struct AspeedTimerCtrlState {
> --
> 2.7.4
>