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

[Dmitry Osipenko]

24.01.2016 17:59, Dmitry Osipenko пишет:

Hello Peter,

24.01.2016 08:25, Peter Crosthwaite пишет:

[snip]


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


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



It does the following:

mode | prescaler | reload if counter == 0 | tick immediately if counter == 0

oneshot == 0   0 0
oneshot  != 0   0 1
periodic == 0   1 0
periodic != 0   0 1

If writing control register with prescaler = 0, then for one-shot timer with
counter == 0 or periodic timer with load = counter == 0 this is NOP. Will add a
comment.

Thanks for review!



Just realized that I missed timer re-run on prescaler change. So it was a good 
question! =)


--
Dmitry

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

[Dmitry Osipenko]

Hello Peter,

24.01.2016 08:25, Peter Crosthwaite пишет:

[snip]


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


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



It does the following:

mode | prescaler | reload if counter == 0 | tick immediately if counter == 0

oneshot  == 0   0 0
oneshot  != 0   0 1
periodic == 0   1 0
periodic != 0   0 1

If writing control register with prescaler = 0, then for one-shot timer with 
counter == 0 or periodic timer with load = counter == 0 this is NOP. Will add a 
comment.


Thanks for review!

--
Dmitry

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

[Peter Crosthwaite]

On Thu, Jan 21, 2016 at 11:03 AM, 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
> - 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 
> ---
>  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 .
>   */
>
> +#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));
> +

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

[Dmitry Osipenko]

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 
---
 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 .
  */
 
+#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.  */
+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)
+{
+if (cond) {
+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.  */
+if ((control & 1) && (value == 0) && (control & 0xff00) == 0) {
+if (!(control & 2) || ptimer_get_limit(tb->timer) == 0) {
+ptimer_stop(tb->timer);
+control &= ~1;
+}