Re: [Qemu-arm] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer

[Peter Crosthwaite]

On Thu, Jan 21, 2016 at 11:03 AM, Dmitry Osipenko <address@hidden> 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
>         - Correctly handles prescaler != 0 cases
>         - 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 <address@hidden>
> ---
>  hw/timer/arm_mptimer.c         | 131 
> +++++++++++++++++++++--------------------
>  include/hw/timer/arm_mptimer.h |   5 +-
>  2 files changed, 67 insertions(+), 69 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 3e59c2a..a5f46df 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
> @@ -42,33 +43,34 @@ static inline void timerblock_update_irq(TimerBlock *tb)
>  }
>
>  /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
> -static inline uint32_t timerblock_scale(TimerBlock *tb)
> +static inline uint32_t timerblock_scale(uint32_t control)
>  {
> -    return (((tb->control >> 8) & 0xff) + 1) * 10;
> +    return (((control >> 8) & 0xff) + 1) * 10;
>  }
>
> -static void timerblock_reload(TimerBlock *tb, int restart)
> +static inline void timerblock_set_count(struct ptimer_state *timer,
> +                                        uint32_t control, uint64_t *count)
>  {
> -    if (tb->count == 0) {
> -        return;
> +    /* PTimer would immediately trigger interrupt for periodic timer
> +     * when counter set to 0, MPtimer under certain condition only.  */

newline before */

> +    if ((control & 3) == 3 && (*count == 0) && (control & 0xff00) == 0) {
> +        *count = ptimer_get_limit(timer);
>      }
> -    if (restart) {
> -        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    ptimer_set_count(timer, *count);
> +}
> +
> +static inline void timerblock_run(struct ptimer_state *timer, uint32_t 
> control,
> +                                  bool cond)

drop cond ...

> +{
> +    if (cond) {

... control & 1 ...

> +        ptimer_run(timer, !(control & 2));
>      }
> -    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 +78,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;
> +        return ptimer_get_limit(tb->timer);
>      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.  */
> @@ -104,39 +96,51 @@ static void timerblock_write(void *opaque, hwaddr addr,
>                               uint64_t value, unsigned size)
>  {
>      TimerBlock *tb = (TimerBlock *)opaque;
> -    int64_t old;
> +    uint32_t control = tb->control;
>      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 prescaler == 0.  */
> +        if ((control & 1) && (value == 0) && (control & 0xff00) == 0) {
> +            ptimer_stop(tb->timer);
> +            control &= ~1;
>          }
> -        tb->count = value;
> -        if (tb->control & 1) {
> -            timerblock_reload(tb, 1);
> +        ptimer_set_limit(tb->timer, value, 1);
> +        timerblock_run(tb->timer, control, (control & 1));
> +        break;
> +    case 4: /* Counter.  */
> +        /* Setting counter to 0 stops the one-shot timer if prescaler == 0
> +         * and periodic if load = 0.  */

newline before */

> +        if ((control & 1) && (value == 0) && (control & 0xff00) == 0) {
> +            if (!(control & 2) || ptimer_get_limit(tb->timer) == 0) {

comment doesn't look to match code. The code looks like:

"setting counter to 0 while prescaler == 0 will disable the timer if
it is either oneshot, or periodic with load == 0",

whearas your comment read as if it only applies to one-shot timers.

ifs can be merged.

> +                ptimer_stop(tb->timer);
> +                control &= ~1;
> +            }
>          }
> +        timerblock_set_count(tb->timer, control, &value);

... and just add in the extra if in here with if (value != 0). The
boolean cond argument that just conditionalizes the whole thing is a
bit un-usual and the three call sites all pass in (control & 1) in
addition to the full raw control.

> +        timerblock_run(tb->timer, control, (value != 0) && (control & 1));
>          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 ((value & 1) && (control & 3) != (value & 3)) {
> +            uint64_t count = (value & 0xff00) ? 1 : 
> ptimer_get_count(tb->timer);
> +            if ((count == 0) && (value & 2)) {
> +                timerblock_set_count(tb->timer, value, &count);

This looks like a weird corner-case, what does it do exactly? I can't
follow it so it needs a comment :)

Regards,
Peter

>              }
> -            if (tb->control & 2) {
> -                tb->count = tb->load;
> -            }
> -            timerblock_reload(tb, 1);
> -        } else if (old & 1) {
> -            /* Shutdown the timer.  */
> -            timer_del(tb->timer);
> +            timerblock_run(tb->timer, value, (count != 0));
> +        } else if ((control & 1) && !(value & 1)) {
> +            ptimer_stop(tb->timer);
> +        }
> +        if ((control & 0xff00) != (value & 0xff00)) {
> +            ptimer_set_period(tb->timer, timerblock_scale(value));
>          }
> +        tb->control = value;
>          break;
>      case 12: /* Interrupt status.  */
> +        /* Simulate continuous IRQ gen.  */
> +        if ((control & 3) == 3 && (control & 0xff00) != 0) {
> +            if (ptimer_get_limit(tb->timer) == 0) {
> +                break;
> +            }
> +        }
>          tb->status &= ~value;
>          timerblock_update_irq(tb);
>          break;
> @@ -184,13 +188,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);
> +        ptimer_set_period(tb->timer, timerblock_scale(0));
>      }
>  }
>
> @@ -235,7 +238,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 +249,23 @@ 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..c46d8d2 100644
> --- a/include/hw/timer/arm_mptimer.h
> +++ b/include/hw/timer/arm_mptimer.h
> @@ -27,12 +27,9 @@
>
>  /* 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.7.0
>