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

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

[Andrew Jeffery]

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.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---

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

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

[Joel Stanley]

On Fri, May 27, 2016 at 12:08 AM, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au>

Acked-by: Joel Stanley <joel@jms.id.au>

> ---
>
> 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
>

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

[Cédric Le Goater]

On 06/05/2016 06:20 PM, Joel Stanley wrote:
> On Fri, May 27, 2016 at 12:08 AM, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au>
> 
> Acked-by: Joel Stanley <joel@jms.id.au>

Yes. I should have mentioned it also. This is a must have for the SMC 
patches (soon to come) which let us boot directly for the OpenBMC flash 
images.

Acked-by: Cédric Le Goater <clg@kaod.org>

Thanks

C.
 
>> ---
>>
>> 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
>>

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

[Peter Maydell]

On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>
> 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.

So what would you need from ptimer to be able to implement value
matching with it; or is ptimer just too far away from what this
timer device needs to make that worthwhile ?

thanks
-- PMM

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On Mon, 2016-06-06 at 15:01 +0100, Peter Maydell wrote:
> On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > 
> > 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.
> So what would you need from ptimer to be able to implement value
> matching with it; or is ptimer just too far away from what this
> timer device needs to make that worthwhile ?

I gave expanding the ptimer API some (quick) consideration. It feels
like it might be a departure from "simple" and depending on your view a
departure from "periodic"; though an interrupt for a given match value
is at least periodic with respect to itself. In this case the hardware
supports two match values, so if we add something to the ptimer API it
would need to support an arbitrary number of values
(ptimer_{add,del}_match(...)?). In this case the hardware counts down,
but just as we're doing currently we can fix the values to give the
right behaviour.

I guess the final thought was that you queried me on the #include
"qemu/main-loop.h" in the original patch, and that moving away from
ptimer would eliminate it.

If we come up with an acceptable match value API for ptimer I can
implement it and resend.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Maydell]

On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>
> 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.

Couple of minor comments below.

>  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"

osdep.h must always be the first include.

> -#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));

Do we really need to do this with floating point ?

> +
> +    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);

Why is this recursing ?

> +}
> +
>  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()

You need to bump the vmstate version_id and minimum_version_id if
you change the format (and then the version number you use in the
VMSTATE_STRUCT_ARRAY that refers to this one).

> 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"

osdep.h gives you typedefs.h, so don't include it here.

> +#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

thanks
-- PMM

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 11724 bytes --]

On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
> On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > 
> > 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.
> Couple of minor comments below.
> 
> > 
> >  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 
> >  #include "qemu/osdep.h"
> osdep.h must always be the first include.

Thanks for picking that up.

> 
> > 
> > -#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));
> Do we really need to do this with floating point ?

I went with floating point to avoid accumulating errors from truncation
by integer division. At eg 24MHz truncation increases the tick rate by
approximately 1 in 63. We're using floor() here, but that only
truncates at the end of the calculation, so the fractional current
tick.

Having said that, grep'ing around under {,include/}hw/ doesn't show a
lot of use of floating point. It looks like we are explicitly avoiding
it, however with a quick search I didn't find it mentioned in any of
the docs. What's the reasoning? Should I use a fixed-point approach
like ptimer?

> 
> > 
> > +
> > +    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);
> Why is this recursing ?

The common case is that we return during iterating over seq array i.e.
we're anticipating another match event (either from the match values or
the timer reaching zero). If not then we've overrun, so we reinitialise
the timer by resetting the 'start' reference point then searching again
for the next event (iterating over seq). As the search for the next
event is encoded in the function, I've recursed to reuse it.

Would you prefer a loop here?

Considering the two approaches (recursion vs loop), if TCO doesn't
apply we could blow the stack, and with loops or TCO it's possible we
could spin here indefinitely if the timer period is shorter than the
time it takes to recalculate. Arguably, not crashing is better so I'm
happy to rework it.

As an aside, the approach for reinitialising start skips countdown
periods that were completely missed through high service latency, and
there will be no interrupts issued for the missing events. Is that
reasonable?

> 
> > 
> > +}
> > +
> >  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()
> You need to bump the vmstate version_id and minimum_version_id if
> you change the format (and then the version number you use in the
> VMSTATE_STRUCT_ARRAY that refers to this one).

Will do, thanks for clarifying.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Maydell]

On 10 June 2016 at 01:59, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
>> On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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.
>> >
>> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> > ---
>> >
>> > 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.
>> Couple of minor comments below.
>>
>> >
>> >  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
>> >  #include "qemu/osdep.h"
>> osdep.h must always be the first include.
>
> Thanks for picking that up.

If you like you can run scripts/clean-includes file ...
and it will automatically make any necessary changes like this to
your files.


>> > +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));
>> Do we really need to do this with floating point ?
>
> I went with floating point to avoid accumulating errors from truncation
> by integer division. At eg 24MHz truncation increases the tick rate by
> approximately 1 in 63. We're using floor() here, but that only
> truncates at the end of the calculation, so the fractional current
> tick.

Right, but you only have a risk of truncation because of the way
you've structured the calculation. You have

floor(delta_ns / calculate_period())
== floor(delta_ns / (calculate_rate() / NS_PER_SEC))
== floor((delta_ns * NS_PER_SEC) / calculate_rate())

and calculate_rate() returns either 1000000 or 24000000.

So you have a 64 bit delta_ns, a 32 bit NANOSECONDS_PER_SECOND
and a 32 bit frequency. We have a function for doing this
accurately with integer arithmetic: muldiv64() (which takes
a 64 bit a, 32 bit b and 32 bit c and returns (a*b)/c using
an intermediate 96 bit precision to avoid overflow).

Doing ticks-to-ns and ns-to-ticks with muldiv64() is pretty
standard (grep the codebase and you'll see a lot of it).

> Having said that, grep'ing around under {,include/}hw/ doesn't show a
> lot of use of floating point. It looks like we are explicitly avoiding
> it, however with a quick search I didn't find it mentioned in any of
> the docs. What's the reasoning? Should I use a fixed-point approach
> like ptimer?

My view is that hardware doesn't generally use floating point
so it's odd to see it in a software model of hardware. Double
doesn't actually get you any more bits than uint64_t anyway...

>> > +    return calculate_next(t);

>> Why is this recursing ?
>
> The common case is that we return during iterating over seq array i.e.
> we're anticipating another match event (either from the match values or
> the timer reaching zero). If not then we've overrun, so we reinitialise
> the timer by resetting the 'start' reference point then searching again
> for the next event (iterating over seq). As the search for the next
> event is encoded in the function, I've recursed to reuse it.
>
> Would you prefer a loop here?
>
> Considering the two approaches (recursion vs loop), if TCO doesn't
> apply we could blow the stack, and with loops or TCO it's possible we
> could spin here indefinitely if the timer period is shorter than the
> time it takes to recalculate. Arguably, not crashing is better so I'm
> happy to rework it.

Yes, I'd prefer a loop -- TCO is an optimization, not a guarantee
by the compiler.

> As an aside, the approach for reinitialising start skips countdown
> periods that were completely missed through high service latency, and
> there will be no interrupts issued for the missing events. Is that
> reasonable?

If the countdown period is so short that we can't service it
in time then the system is probably not going to be functional
anyway, so I don't think it matters very much. (In ptimer we
impose an artificial limit on the timeout rate for a periodic timer
and just refuse to fire any faster than that: see ptimer_reload().)

thanks
-- PMM

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 5568 bytes --]

On Fri, 2016-06-10 at 11:42 +0100, Peter Maydell wrote:
> On 10 June 2016 at 01:59, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 
> > On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
> > > 
> > > On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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.
> > > > 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > > 
> > > > 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.
> > > Couple of minor comments below.
> > > 
> > > > 
> > > > 
> > > >  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
> > > >  #include "qemu/osdep.h"
> > > osdep.h must always be the first include.
> > Thanks for picking that up.
> If you like you can run scripts/clean-includes file ...
> and it will automatically make any necessary changes like this to
> your files.

Thanks, I'll add that to my submission checklist

> 
> 
> > 
> > > 
> > > > 
> > > > +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));
> > > Do we really need to do this with floating point ?
> > I went with floating point to avoid accumulating errors from truncation
> > by integer division. At eg 24MHz truncation increases the tick rate by
> > approximately 1 in 63. We're using floor() here, but that only
> > truncates at the end of the calculation, so the fractional current
> > tick.
> Right, but you only have a risk of truncation because of the way
> you've structured the calculation. You have
> 
> floor(delta_ns / calculate_period())
> == floor(delta_ns / (calculate_rate() / NS_PER_SEC))
> == floor((delta_ns * NS_PER_SEC) / calculate_rate())
> 
> and calculate_rate() returns either 1000000 or 24000000.
> 
> So you have a 64 bit delta_ns, a 32 bit NANOSECONDS_PER_SECOND
> and a 32 bit frequency. We have a function for doing this
> accurately with integer arithmetic: muldiv64() (which takes
> a 64 bit a, 32 bit b and 32 bit c and returns (a*b)/c using
> an intermediate 96 bit precision to avoid overflow).
> 
> Doing ticks-to-ns and ns-to-ticks with muldiv64() is pretty
> standard (grep the codebase and you'll see a lot of it).

Right! As I didn't see a concern with floating point prior to sending
the patch I didn't try to rearrange the calculation. I'll rework to use
muldiv64().

> 
> > 
> > Having said that, grep'ing around under {,include/}hw/ doesn't show a
> > lot of use of floating point. It looks like we are explicitly avoiding
> > it, however with a quick search I didn't find it mentioned in any of
> > the docs. What's the reasoning? Should I use a fixed-point approach
> > like ptimer?
> My view is that hardware doesn't generally use floating point
> so it's odd to see it in a software model of hardware.

Fair enough.

>  Double
> doesn't actually get you any more bits than uint64_t anyway...
> 
> > 
> > > 
> > > > 
> > > > +    return calculate_next(t);
> > 
> > > 
> > > Why is this recursing ?
> > The common case is that we return during iterating over seq array i.e.
> > we're anticipating another match event (either from the match values or
> > the timer reaching zero). If not then we've overrun, so we reinitialise
> > the timer by resetting the 'start' reference point then searching again
> > for the next event (iterating over seq). As the search for the next
> > event is encoded in the function, I've recursed to reuse it.
> > 
> > Would you prefer a loop here?
> > 
> > Considering the two approaches (recursion vs loop), if TCO doesn't
> > apply we could blow the stack, and with loops or TCO it's possible we
> > could spin here indefinitely if the timer period is shorter than the
> > time it takes to recalculate. Arguably, not crashing is better so I'm
> > happy to rework it.
> Yes, I'd prefer a loop -- TCO is an optimization, not a guarantee
> by the compiler.

Yes, on reflection I agree. I'll rework it.

> 
> > 
> > As an aside, the approach for reinitialising start skips countdown
> > periods that were completely missed through high service latency, and
> > there will be no interrupts issued for the missing events. Is that
> > reasonable?
> If the countdown period is so short that we can't service it
> in time then the system is probably not going to be functional
> anyway, so I don't think it matters very much. (In ptimer we
> impose an artificial limit on the timeout rate for a periodic timer
> and just refuse to fire any faster than that: see ptimer_reload().)

I'll leave it as is for v2.

Thanks for the feedback!

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]