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

[Andrew Jeffery]

On Mon, 2016-05-23 at 17:00 +0930, Joel Stanley wrote:
> 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.

A double win!

> 
> > 
> > 
> > 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 
> >  #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?).

I'll look into it.

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

Hah!

> 
> Perhaps make ticks the uint32_t:
> 
>  uint32_t ticks = (uint32_t)floor( ... );

I'll try clean up the casts.

> 
> > 
> > +}
> > +
> > +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.

Fair point. I'll add some words.

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

Because reload_ns wasn't relevant for the loop above, so to make it's
useful lifetime explicit I scoped it to where I needed it.

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

Okay, I'll look at restructuring it.

Thanks for the review.

Andrew

> 
> > 
> > +
> > +    if (interrupt) {
> >          t->level = !t->level;
> >          qemu_set_irq(t->irq, t->level);
> >      }
> > +
> > +    timer_mod(&t->timer, calculate_next(t));
> >  }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160525/705d2daf/attachment.sig>