Re: [Qemu-devel] [PATCH v8 4/4] arm_mptimer: Convert to use ptimer

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Tue, Jan 05, 2016 at 05:33:29AM +0300, Dmitry Osipenko wrote:
> Current ARM MPTimer implementation uses QEMUTimer for the actual timer,
> this implementation isn't complete and mostly tries to duplicate of what
> generic ptimer is already doing fine.
> 
> Conversion to ptimer brings the following benefits and fixes:
> 	- Simple timer pausing implementation
> 	- Fixes counter value preservation after stopping the timer
> 	- Code simplification and reduction
> 
> Bump VMSD to version 3, since VMState is changed and is not compatible
> with the previous implementation.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/timer/arm_mptimer.c         | 110 ++++++++++++++++++-----------------------
>  include/hw/timer/arm_mptimer.h |   4 +-
>  2 files changed, 49 insertions(+), 65 deletions(-)
> 
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 3e59c2a..c06da5e 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -19,8 +19,9 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "hw/ptimer.h"
>  #include "hw/timer/arm_mptimer.h"
> -#include "qemu/timer.h"
> +#include "qemu/main-loop.h"
>  #include "qom/cpu.h"
>  
>  /* This device implements the per-cpu private timer and watchdog block
> @@ -47,28 +48,10 @@ static inline uint32_t timerblock_scale(TimerBlock *tb)
>      return (((tb->control >> 8) & 0xff) + 1) * 10;
>  }
>  
> -static void timerblock_reload(TimerBlock *tb, int restart)
> -{
> -    if (tb->count == 0) {
> -        return;
> -    }
> -    if (restart) {
> -        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -    }
> -    tb->tick += (int64_t)tb->count * timerblock_scale(tb);
> -    timer_mod(tb->timer, tb->tick);
> -}
> -
>  static void timerblock_tick(void *opaque)
>  {
>      TimerBlock *tb = (TimerBlock *)opaque;
>      tb->status = 1;
> -    if (tb->control & 2) {
> -        tb->count = tb->load;
> -        timerblock_reload(tb, 0);
> -    } else {
> -        tb->count = 0;
> -    }
>      timerblock_update_irq(tb);
>  }
>  
> @@ -76,21 +59,11 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
>                                  unsigned size)
>  {
>      TimerBlock *tb = (TimerBlock *)opaque;
> -    int64_t val;
>      switch (addr) {
>      case 0: /* Load */
>          return tb->load;
>      case 4: /* Counter.  */
> -        if (((tb->control & 1) == 0) || (tb->count == 0)) {
> -            return 0;
> -        }
> -        /* Slow and ugly, but hopefully won't happen too often.  */
> -        val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -        val /= timerblock_scale(tb);
> -        if (val < 0) {
> -            val = 0;
> -        }
> -        return val;
> +        return ptimer_get_count(tb->timer);
>      case 8: /* Control.  */
>          return tb->control;
>      case 12: /* Interrupt status.  */
> @@ -100,6 +73,19 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
>      }
>  }
>  
> +static void timerblock_run(TimerBlock *tb, uint64_t count, int set_count)
> +{
> +    if (set_count) {
> +        if (((tb->control & 3) == 3) && (count == 0)) {

Parentheses around == expressions should not be needed.

> +            count = tb->load;
> +        }
> +        ptimer_set_count(tb->timer, count);
> +    }
> +    if ((tb->control & 1) && (count != 0)) {

This can check against tb->load instead of count to avoid dummy
pass of tb->load to this fn ...

> +        ptimer_run(tb->timer, !(tb->control & 2));
> +    }
> +}
> +
>  static void timerblock_write(void *opaque, hwaddr addr,
>                               uint64_t value, unsigned size)
>  {
> @@ -108,32 +94,34 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      switch (addr) {
>      case 0: /* Load */
>          tb->load = value;
> -        /* Fall through.  */
> -    case 4: /* Counter.  */
> -        if ((tb->control & 1) && tb->count) {
> -            /* Cancel the previous timer.  */
> -            timer_del(tb->timer);
> +        /* Setting load to 0 stops the timer.  */
> +        if (tb->load == 0) {
> +            ptimer_stop(tb->timer);
>          }
> -        tb->count = value;
> -        if (tb->control & 1) {
> -            timerblock_reload(tb, 1);
> +        ptimer_set_limit(tb->timer, tb->load, 1);
> +        timerblock_run(tb, tb->load, 0);
> +        break;
> +    case 4: /* Counter.  */
> +        /* Setting counter to 0 stops the one-shot timer.  */
> +        if (!(tb->control & 2) && (value == 0)) {
> +            ptimer_stop(tb->timer);
>          }
> +        timerblock_run(tb, value, 1);

... then this would just need to be elsed.

>          break;
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> -        if (value & 1) {
> -            if ((old & 1) && (tb->count != 0)) {
> -                /* Do nothing if timer is ticking right now.  */
> -                break;
> -            }
> -            if (tb->control & 2) {
> -                tb->count = tb->load;
> -            }
> -            timerblock_reload(tb, 1);
> -        } else if (old & 1) {
> -            /* Shutdown the timer.  */
> -            timer_del(tb->timer);
> +        /* Timer mode switch requires ptimer to be stopped.  */

Is it worth adding this to ptimer? It seems ptimer can now handle
every other case of running configuration change except this one
case.

> +        if ((old & 3) != (tb->control & 3)) {
> +            ptimer_stop(tb->timer);
> +        }
> +        if (!(tb->control & 1)) {
> +            break;
> +        }
> +        ptimer_set_period(tb->timer, timerblock_scale(tb));
> +        if ((old & 3) != (tb->control & 3)) {
> +            value = ptimer_get_count(tb->timer);
> +            timerblock_run(tb, value, 1);

Is this reachable when the load value is still 0?

Following on from the suggested refactor before, I think timerblock_run
should split off the count set to a new helper. Then this is

timerblock_setcount(tb, value);
timerblock_run(tb);

and the boolean arg and dummy pass of tb->load as count are unneeded.

Regards,
Peter

>          }
>          break;
>      case 12: /* Interrupt status.  */
> @@ -184,13 +172,12 @@ static const MemoryRegionOps timerblock_ops = {
>  
>  static void timerblock_reset(TimerBlock *tb)
>  {
> -    tb->count = 0;
>      tb->load = 0;
>      tb->control = 0;
>      tb->status = 0;
> -    tb->tick = 0;
>      if (tb->timer) {
> -        timer_del(tb->timer);
> +        ptimer_stop(tb->timer);
> +        ptimer_set_limit(tb->timer, 0, 1);
>      }
>  }
>  
> @@ -235,7 +222,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
>       */
>      for (i = 0; i < s->num_cpu; i++) {
>          TimerBlock *tb = &s->timerblock[i];
> -        tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb);
> +        QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
> +        tb->timer = ptimer_init(bh);
>          sysbus_init_irq(sbd, &tb->irq);
>          memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
>                                "arm_mptimer_timerblock", 0x20);
> @@ -245,26 +233,24 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
>  
>  static const VMStateDescription vmstate_timerblock = {
>      .name = "arm_mptimer_timerblock",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(count, TimerBlock),
>          VMSTATE_UINT32(load, TimerBlock),
>          VMSTATE_UINT32(control, TimerBlock),
>          VMSTATE_UINT32(status, TimerBlock),
> -        VMSTATE_INT64(tick, TimerBlock),
> -        VMSTATE_TIMER_PTR(timer, TimerBlock),
> +        VMSTATE_PTIMER(timer, TimerBlock),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>  
>  static const VMStateDescription vmstate_arm_mptimer = {
>      .name = "arm_mptimer",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>      .fields = (VMStateField[]) {
>          VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
> -                                     2, vmstate_timerblock, TimerBlock),
> +                                     3, vmstate_timerblock, TimerBlock),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
> index b34cba0..93db61b 100644
> --- a/include/hw/timer/arm_mptimer.h
> +++ b/include/hw/timer/arm_mptimer.h
> @@ -27,12 +27,10 @@
>  
>  /* State of a single timer or watchdog block */
>  typedef struct {
> -    uint32_t count;
>      uint32_t load;
>      uint32_t control;
>      uint32_t status;
> -    int64_t tick;
> -    QEMUTimer *timer;
> +    struct ptimer_state *timer;
>      qemu_irq irq;
>      MemoryRegion iomem;
>  } TimerBlock;
> -- 
> 2.6.4
>