[PATCH qemu] hw/timer: Add clock value matching support to aspeed_timer

[Joel Stanley]

On Fri, May 20, 2016 at 12:34 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> Clock 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 implementing this.

We also boot much faster now.

>
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> ---
>  hw/timer/aspeed_timer.c         | 132 +++++++++++++++++++++++++++++-----------
>  include/hw/timer/aspeed_timer.h |   5 +-
>  2 files changed, 101 insertions(+), 36 deletions(-)
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 51e8303cdaa3..e88234e7823f 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 "trace.h"
>
> @@ -76,21 +75,96 @@ 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 inline double calculate_period(struct AspeedTimer *t)
> +{
> +    double rate;
> +
> +    rate = timer_external_clock(t) ? TIMER_CLOCK_EXT_HZ : TIMER_CLOCK_APB_HZ;

Would it make more sense return the rate from timer_external_clock
(perhaps get_timer_clock?).

> +
> +    return NANOSECONDS_PER_SECOND / rate;
> +}
> +
> +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t now_ns)
> +{
> +    double ticks;
> +
> +    ticks = floor((now_ns - t->start) / calculate_period(t));
> +
> +    return t->reload - MIN(t->reload, (uint32_t) ticks);

Ugh, Casty McCastface.

Perhaps make ticks the uint32_t:

 uint32_t ticks = (uint32_t)floor( ... );

> +}
> +
> +static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks)
> +{
> +    double delta_ns;
> +
> +    ticks = MIN(t->reload, ticks);
> +    delta_ns = floor((t->reload - ticks) * calculate_period(t));

As above.

> +
> +    return t->start + (uint64_t) delta_ns;
> +}
> +
> +static uint64_t calculate_next(struct AspeedTimer *t)
> +{
> +    uint64_t now;
> +    uint64_t next;
> +    int i;
> +    uint64_t seq[] = {
> +        MAX(t->match[0], t->match[1]),
> +        MIN(t->match[0], t->match[1]),
> +        0,
> +    };

Perhaps a comment on why you chose to do it like this? I only
understand because you explained it to me.

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

Why is this scoped like this?

> +
> +    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));
> +
> +    interrupt = !ticks && timer_overflow_interrupt(t);

I think it's clearer to say

if (!ticks && timer_overflow_interrupt)
   interrupt = true;


> +
> +    if (!interrupt) {
> +        interrupt = ticks <= MIN(t->match[0], t->match[1]);
> +    }

if (!interrupt && ticks <=  MIN(t->match[0], t->match[1]))
    interrupt = true;

> +
> +    if (!interrupt) {
> +        interrupt = ticks <= MAX(t->match[0], t->match[1]);
> +    }

As above. And the advantage of structuring it like this is these
else..else if statements can be represented as such.

> +
> +    if (interrupt) {
>          t->level = !t->level;
>          qemu_set_irq(t->irq, t->level);
>      }
> +
> +    timer_mod(&t->timer, calculate_next(t));
>  }