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

[Dmitry Osipenko]

06.01.2016 16:17, Peter Crosthwaite пишет:

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 
---
  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 .
   */

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

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

[Dmitry Osipenko]

06.01.2016 16:17, Peter Crosthwaite пишет:

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



Yes, timer runs based on current counter value. You can load = 0 and set counter 
!= 0 that would result in one-shot tick. Hmm... I need to fix it. Thanks for 
leading question!


Ughh... just noticed that test_timer_set_oneshot_count_to_0() on real HW sets IT 
bit if prescaler != 0. Looking into it..


--
Dmitry

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

[Peter Crosthwaite]

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 
> ---
>  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 .
>   */
>  
> +#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;
> -}
> -

[Qemu-devel] [PATCH v8 4/4] 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
- 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 | 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 .
  */
 
+#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)) {
+count = tb->load;
+}
+ptimer_set_count(tb->timer, count);
+}
+if ((tb->control & 1) && (count != 0)) {
+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);
 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.  */
+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) !=