[PATCH 0/8] Convert misc-arch devices to new ptimer API

[PATCH 0/8] Convert misc-arch devices to new ptimer API

[Peter Maydell]

This patchset converts the devices used by the miscellaneous minor
architecture machines to the new ptimer API. More specifically:

cris:
 hw/timer/etraxfs_timer.c

lm32:
 hw/timer/lm32_timer.c
 hw/timer/milkymist-sysctl.c

nios2:
 hw/timer/altera_timer.c

sh4:
 hw/timer/sh_timer.c

unicore32:
 hw/timer/puv3_ost.c

m68k:
 hw/m68k/mcf5206.c
 hw/m68k/mcf5208.c

Currently the ptimer design uses a QEMU bottom-half as its mechanism
for calling back into the device model using the ptimer when the
timer has expired.  Unfortunately this design is fatally flawed,
because it means that there is a lag between the ptimer updating its
own state and the device callback function updating device state, and
guest accesses to device registers between the two can return
inconsistent device state. This was reported as a bug in a specific
timer device but it's a problem with the generic ptimer code:
https://bugs.launchpad.net/qemu/+bug/1777777

The updates to the individual ptimer devices are straightforward:
we need to add begin/commit calls around the various places that
modify the ptimer state, and use the new ptimer_init() function
to create the timer.

Testing I have done:
 * 'make check'
 * running a milkymist system image I had lying around
 * running an sh4 system image

This doesn't exercise the devices very much, so more specific testing
would be appreciated. I plan to collect these patches up and
get them into the tree with other ptimer-related changes (probably
via target-arm just for convenience) unless anybody would specifically
like to take a patch via some other tree.

thanks
--PMM


Peter Maydell (8):
  hw/timer/puv3_ost.c: Switch to transaction-based ptimer API
  hw/timer/sh_timer: Switch to transaction-based ptimer API
  hw/timer/lm32_timer: Switch to transaction-based ptimer API
  hw/watchdog/milkymist-sysctl.c: Switch to transaction-based ptimer API
  hw/timer/altera_timer.c: Switch to transaction-based ptimer API
  hw/watchdog/etraxfs_timer.c: Switch to transaction-based ptimer API
  hw/m68k/mcf5206.c: Switch to transaction-based ptimer API
  hw/m68k/mcf5208.c: Switch to transaction-based ptimer API

 hw/m68k/mcf5206.c           |  9 +++++----
 hw/m68k/mcf5208.c           |  9 +++++----
 hw/timer/altera_timer.c     | 13 +++++++++----
 hw/timer/etraxfs_timer.c    | 23 +++++++++++++----------
 hw/timer/lm32_timer.c       | 13 +++++++++----
 hw/timer/milkymist-sysctl.c | 25 ++++++++++++++++++-------
 hw/timer/puv3_ost.c         |  9 +++++----
 hw/timer/sh_timer.c         | 13 +++++++++----
 8 files changed, 73 insertions(+), 41 deletions(-)

-- 
2.20.1

[PATCH 1/8] hw/timer/puv3_ost.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the puv3_ost code away from bottom-half based ptimers to the
new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/puv3_ost.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/timer/puv3_ost.c b/hw/timer/puv3_ost.c
index 0898da5ce97..697519593bb 100644
--- a/hw/timer/puv3_ost.c
+++ b/hw/timer/puv3_ost.c
@@ -13,7 +13,6 @@
 #include "hw/sysbus.h"
 #include "hw/irq.h"
 #include "hw/ptimer.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 #undef DEBUG_PUV3
@@ -27,7 +26,6 @@ typedef struct PUV3OSTState {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    QEMUBH *bh;
     qemu_irq irq;
     ptimer_state *ptimer;
 
@@ -68,6 +66,7 @@ static void puv3_ost_write(void *opaque, hwaddr offset,
     DPRINTF("offset 0x%x, value 0x%x\n", offset, value);
     switch (offset) {
     case 0x00: /* Match Register 0 */
+        ptimer_transaction_begin(s->ptimer);
         s->reg_OSMR0 = value;
         if (s->reg_OSMR0 > s->reg_OSCR) {
             ptimer_set_count(s->ptimer, s->reg_OSMR0 - s->reg_OSCR);
@@ -76,6 +75,7 @@ static void puv3_ost_write(void *opaque, hwaddr offset,
                     (0xffffffff - s->reg_OSCR));
         }
         ptimer_run(s->ptimer, 2);
+        ptimer_transaction_commit(s->ptimer);
         break;
     case 0x14: /* Status Register */
         assert(value == 0);
@@ -128,9 +128,10 @@ static void puv3_ost_realize(DeviceState *dev, Error **errp)
 
     sysbus_init_irq(sbd, &s->irq);
 
-    s->bh = qemu_bh_new(puv3_ost_tick, s);
-    s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init(puv3_ost_tick, s, PTIMER_POLICY_DEFAULT);
+    ptimer_transaction_begin(s->ptimer);
     ptimer_set_freq(s->ptimer, 50 * 1000 * 1000);
+    ptimer_transaction_commit(s->ptimer);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &puv3_ost_ops, s, "puv3_ost",
             PUV3_REGS_OFFSET);
-- 
2.20.1

Re: [PATCH 1/8] hw/timer/puv3_ost.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:28 AM, Peter Maydell wrote:
> Switch the puv3_ost code away from bottom-half based ptimers to the
> new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/puv3_ost.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/8] hw/timer/puv3_ost.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:28 PM, Peter Maydell wrote:
> Switch the puv3_ost code away from bottom-half based ptimers to the
> new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/puv3_ost.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/timer/puv3_ost.c b/hw/timer/puv3_ost.c
> index 0898da5ce97..697519593bb 100644
> --- a/hw/timer/puv3_ost.c
> +++ b/hw/timer/puv3_ost.c
> @@ -13,7 +13,6 @@
>   #include "hw/sysbus.h"
>   #include "hw/irq.h"
>   #include "hw/ptimer.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   #undef DEBUG_PUV3
> @@ -27,7 +26,6 @@ typedef struct PUV3OSTState {
>       SysBusDevice parent_obj;
>   
>       MemoryRegion iomem;
> -    QEMUBH *bh;
>       qemu_irq irq;
>       ptimer_state *ptimer;
>   
> @@ -68,6 +66,7 @@ static void puv3_ost_write(void *opaque, hwaddr offset,
>       DPRINTF("offset 0x%x, value 0x%x\n", offset, value);
>       switch (offset) {
>       case 0x00: /* Match Register 0 */
> +        ptimer_transaction_begin(s->ptimer);
>           s->reg_OSMR0 = value;
>           if (s->reg_OSMR0 > s->reg_OSCR) {
>               ptimer_set_count(s->ptimer, s->reg_OSMR0 - s->reg_OSCR);
> @@ -76,6 +75,7 @@ static void puv3_ost_write(void *opaque, hwaddr offset,
>                       (0xffffffff - s->reg_OSCR));
>           }
>           ptimer_run(s->ptimer, 2);
> +        ptimer_transaction_commit(s->ptimer);
>           break;
>       case 0x14: /* Status Register */
>           assert(value == 0);
> @@ -128,9 +128,10 @@ static void puv3_ost_realize(DeviceState *dev, Error **errp)
>   
>       sysbus_init_irq(sbd, &s->irq);
>   
> -    s->bh = qemu_bh_new(puv3_ost_tick, s);
> -    s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);
> +    s->ptimer = ptimer_init(puv3_ost_tick, s, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(s->ptimer);
>       ptimer_set_freq(s->ptimer, 50 * 1000 * 1000);
> +    ptimer_transaction_commit(s->ptimer);
>   
>       memory_region_init_io(&s->iomem, OBJECT(s), &puv3_ost_ops, s, "puv3_ost",
>               PUV3_REGS_OFFSET);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 2/8] hw/timer/sh_timer: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the sh_timer code away from bottom-half based ptimers to the
new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/sh_timer.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index 48a81b4dc79..13c4051808f 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -13,7 +13,6 @@
 #include "hw/irq.h"
 #include "hw/sh4/sh.h"
 #include "qemu/timer.h"
-#include "qemu/main-loop.h"
 #include "hw/ptimer.h"
 
 //#define DEBUG_TIMER
@@ -91,13 +90,18 @@ static void sh_timer_write(void *opaque, hwaddr offset,
     switch (offset >> 2) {
     case OFFSET_TCOR:
         s->tcor = value;
+        ptimer_transaction_begin(s->timer);
         ptimer_set_limit(s->timer, s->tcor, 0);
+        ptimer_transaction_commit(s->timer);
         break;
     case OFFSET_TCNT:
         s->tcnt = value;
+        ptimer_transaction_begin(s->timer);
         ptimer_set_count(s->timer, s->tcnt);
+        ptimer_transaction_commit(s->timer);
         break;
     case OFFSET_TCR:
+        ptimer_transaction_begin(s->timer);
         if (s->enabled) {
             /* Pause the timer if it is running.  This may cause some
                inaccuracy dure to rounding, but avoids a whole lot of other
@@ -148,6 +152,7 @@ static void sh_timer_write(void *opaque, hwaddr offset,
             /* Restart the timer if still enabled.  */
             ptimer_run(s->timer, 0);
         }
+        ptimer_transaction_commit(s->timer);
         break;
     case OFFSET_TCPR:
         if (s->feat & TIMER_FEAT_CAPT) {
@@ -168,12 +173,14 @@ static void sh_timer_start_stop(void *opaque, int enable)
     printf("sh_timer_start_stop %d (%d)\n", enable, s->enabled);
 #endif
 
+    ptimer_transaction_begin(s->timer);
     if (s->enabled && !enable) {
         ptimer_stop(s->timer);
     }
     if (!s->enabled && enable) {
         ptimer_run(s->timer, 0);
     }
+    ptimer_transaction_commit(s->timer);
     s->enabled = !!enable;
 
 #ifdef DEBUG_TIMER
@@ -191,7 +198,6 @@ static void sh_timer_tick(void *opaque)
 static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
 {
     sh_timer_state *s;
-    QEMUBH *bh;
 
     s = (sh_timer_state *)g_malloc0(sizeof(sh_timer_state));
     s->freq = freq;
@@ -203,8 +209,7 @@ static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
     s->enabled = 0;
     s->irq = irq;
 
-    bh = qemu_bh_new(sh_timer_tick, s);
-    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init(sh_timer_tick, s, PTIMER_POLICY_DEFAULT);
 
     sh_timer_write(s, OFFSET_TCOR >> 2, s->tcor);
     sh_timer_write(s, OFFSET_TCNT >> 2, s->tcnt);
-- 
2.20.1

Re: [PATCH 2/8] hw/timer/sh_timer: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:28 AM, Peter Maydell wrote:
> Switch the sh_timer code away from bottom-half based ptimers to the
> new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/sh_timer.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


> @@ -168,12 +173,14 @@ static void sh_timer_start_stop(void *opaque, int enable)
>      printf("sh_timer_start_stop %d (%d)\n", enable, s->enabled);
>  #endif
>  
> +    ptimer_transaction_begin(s->timer);
>      if (s->enabled && !enable) {
>          ptimer_stop(s->timer);
>      }
>      if (!s->enabled && enable) {
>          ptimer_run(s->timer, 0);
>      }
> +    ptimer_transaction_commit(s->timer);
>      s->enabled = !!enable;

Ew.  Future cleanup should perhaps be

- static void sh_timer_start_stop(void *opaque, int enable)
+ static void sh_timer_start_stop(void *opaque, bool enable)

    if (s->enabled != enable) {
        s->enabled = enable;
        ptimer_transaction_begin(s->timer);
        if (enable) {
            ptimer_run(s->timer, 0);
        } else {
            ptimer_stop(s->timer);
        }
        ptimer_transaction_commit(s->timer);
    }


r~

Re: [PATCH 2/8] hw/timer/sh_timer: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:28 PM, Peter Maydell wrote:
> Switch the sh_timer code away from bottom-half based ptimers to the
> new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/sh_timer.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 3/8] hw/timer/lm32_timer: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the lm32_timer code away from bottom-half based ptimers to the
new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the ytimer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/lm32_timer.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/timer/lm32_timer.c b/hw/timer/lm32_timer.c
index fabde760b2d..3fdecd09fe2 100644
--- a/hw/timer/lm32_timer.c
+++ b/hw/timer/lm32_timer.c
@@ -30,7 +30,6 @@
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 #define DEFAULT_FREQUENCY (50*1000000)
@@ -63,7 +62,6 @@ struct LM32TimerState {
 
     MemoryRegion iomem;
 
-    QEMUBH *bh;
     ptimer_state *ptimer;
 
     qemu_irq irq;
@@ -119,6 +117,7 @@ static void timer_write(void *opaque, hwaddr addr,
         s->regs[R_SR] &= ~SR_TO;
         break;
     case R_CR:
+        ptimer_transaction_begin(s->ptimer);
         s->regs[R_CR] = value;
         if (s->regs[R_CR] & CR_START) {
             ptimer_run(s->ptimer, 1);
@@ -126,10 +125,13 @@ static void timer_write(void *opaque, hwaddr addr,
         if (s->regs[R_CR] & CR_STOP) {
             ptimer_stop(s->ptimer);
         }
+        ptimer_transaction_commit(s->ptimer);
         break;
     case R_PERIOD:
         s->regs[R_PERIOD] = value;
+        ptimer_transaction_begin(s->ptimer);
         ptimer_set_count(s->ptimer, value);
+        ptimer_transaction_commit(s->ptimer);
         break;
     case R_SNAPSHOT:
         error_report("lm32_timer: write access to read only register 0x"
@@ -176,7 +178,9 @@ static void timer_reset(DeviceState *d)
     for (i = 0; i < R_MAX; i++) {
         s->regs[i] = 0;
     }
+    ptimer_transaction_begin(s->ptimer);
     ptimer_stop(s->ptimer);
+    ptimer_transaction_commit(s->ptimer);
 }
 
 static void lm32_timer_init(Object *obj)
@@ -195,10 +199,11 @@ static void lm32_timer_realize(DeviceState *dev, Error **errp)
 {
     LM32TimerState *s = LM32_TIMER(dev);
 
-    s->bh = qemu_bh_new(timer_hit, s);
-    s->ptimer = ptimer_init_with_bh(s->bh, PTIMER_POLICY_DEFAULT);
+    s->ptimer = ptimer_init(timer_hit, s, PTIMER_POLICY_DEFAULT);
 
+    ptimer_transaction_begin(s->ptimer);
     ptimer_set_freq(s->ptimer, s->freq_hz);
+    ptimer_transaction_commit(s->ptimer);
 }
 
 static const VMStateDescription vmstate_lm32_timer = {
-- 
2.20.1

Re: [PATCH 3/8] hw/timer/lm32_timer: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the lm32_timer code away from bottom-half based ptimers to the
> new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the ytimer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/lm32_timer.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 3/8] hw/timer/lm32_timer: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the lm32_timer code away from bottom-half based ptimers to the
> new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the ytimer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/lm32_timer.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 4/8] hw/watchdog/milkymist-sysctl.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the milkymist-sysctl code away from bottom-half based
ptimers to the new transaction-based ptimer API.  This just requires
adding begin/commit calls around the various places that modify the
ptimer state, and using the new ptimer_init() function to create the
timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/milkymist-sysctl.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/timer/milkymist-sysctl.c b/hw/timer/milkymist-sysctl.c
index 5193c038501..66f86541114 100644
--- a/hw/timer/milkymist-sysctl.c
+++ b/hw/timer/milkymist-sysctl.c
@@ -31,7 +31,6 @@
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 enum {
@@ -71,8 +70,6 @@ struct MilkymistSysctlState {
 
     MemoryRegion regs_region;
 
-    QEMUBH *bh0;
-    QEMUBH *bh1;
     ptimer_state *ptimer0;
     ptimer_state *ptimer1;
 
@@ -161,14 +158,19 @@ static void sysctl_write(void *opaque, hwaddr addr, uint64_t value,
         s->regs[addr] = value;
         break;
     case R_TIMER0_COMPARE:
+        ptimer_transaction_begin(s->ptimer0);
         ptimer_set_limit(s->ptimer0, value, 0);
         s->regs[addr] = value;
+        ptimer_transaction_commit(s->ptimer0);
         break;
     case R_TIMER1_COMPARE:
+        ptimer_transaction_begin(s->ptimer1);
         ptimer_set_limit(s->ptimer1, value, 0);
         s->regs[addr] = value;
+        ptimer_transaction_commit(s->ptimer1);
         break;
     case R_TIMER0_CONTROL:
+        ptimer_transaction_begin(s->ptimer0);
         s->regs[addr] = value;
         if (s->regs[R_TIMER0_CONTROL] & CTRL_ENABLE) {
             trace_milkymist_sysctl_start_timer0();
@@ -179,8 +181,10 @@ static void sysctl_write(void *opaque, hwaddr addr, uint64_t value,
             trace_milkymist_sysctl_stop_timer0();
             ptimer_stop(s->ptimer0);
         }
+        ptimer_transaction_commit(s->ptimer0);
         break;
     case R_TIMER1_CONTROL:
+        ptimer_transaction_begin(s->ptimer0);
         s->regs[addr] = value;
         if (s->regs[R_TIMER1_CONTROL] & CTRL_ENABLE) {
             trace_milkymist_sysctl_start_timer1();
@@ -191,6 +195,7 @@ static void sysctl_write(void *opaque, hwaddr addr, uint64_t value,
             trace_milkymist_sysctl_stop_timer1();
             ptimer_stop(s->ptimer1);
         }
+        ptimer_transaction_commit(s->ptimer0);
         break;
     case R_ICAP:
         sysctl_icap_write(s, value);
@@ -263,8 +268,12 @@ static void milkymist_sysctl_reset(DeviceState *d)
         s->regs[i] = 0;
     }
 
+    ptimer_transaction_begin(s->ptimer0);
     ptimer_stop(s->ptimer0);
+    ptimer_transaction_commit(s->ptimer0);
+    ptimer_transaction_begin(s->ptimer1);
     ptimer_stop(s->ptimer1);
+    ptimer_transaction_commit(s->ptimer1);
 
     /* defaults */
     s->regs[R_ICAP] = ICAP_READY;
@@ -292,13 +301,15 @@ static void milkymist_sysctl_realize(DeviceState *dev, Error **errp)
 {
     MilkymistSysctlState *s = MILKYMIST_SYSCTL(dev);
 
-    s->bh0 = qemu_bh_new(timer0_hit, s);
-    s->bh1 = qemu_bh_new(timer1_hit, s);
-    s->ptimer0 = ptimer_init_with_bh(s->bh0, PTIMER_POLICY_DEFAULT);
-    s->ptimer1 = ptimer_init_with_bh(s->bh1, PTIMER_POLICY_DEFAULT);
+    s->ptimer0 = ptimer_init(timer0_hit, s, PTIMER_POLICY_DEFAULT);
+    s->ptimer1 = ptimer_init(timer1_hit, s, PTIMER_POLICY_DEFAULT);
 
+    ptimer_transaction_begin(s->ptimer0);
     ptimer_set_freq(s->ptimer0, s->freq_hz);
+    ptimer_transaction_commit(s->ptimer0);
+    ptimer_transaction_begin(s->ptimer1);
     ptimer_set_freq(s->ptimer1, s->freq_hz);
+    ptimer_transaction_commit(s->ptimer1);
 }
 
 static const VMStateDescription vmstate_milkymist_sysctl = {
-- 
2.20.1

Re: [PATCH 4/8] hw/watchdog/milkymist-sysctl.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the milkymist-sysctl code away from bottom-half based
> ptimers to the new transaction-based ptimer API.  This just requires
> adding begin/commit calls around the various places that modify the
> ptimer state, and using the new ptimer_init() function to create the
> timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/milkymist-sysctl.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 4/8] hw/watchdog/milkymist-sysctl.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the milkymist-sysctl code away from bottom-half based
> ptimers to the new transaction-based ptimer API.  This just requires
> adding begin/commit calls around the various places that modify the
> ptimer state, and using the new ptimer_init() function to create the
> timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/milkymist-sysctl.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/timer/milkymist-sysctl.c b/hw/timer/milkymist-sysctl.c
> index 5193c038501..66f86541114 100644
> --- a/hw/timer/milkymist-sysctl.c
> +++ b/hw/timer/milkymist-sysctl.c
> @@ -31,7 +31,6 @@
>   #include "hw/ptimer.h"
>   #include "hw/qdev-properties.h"
>   #include "qemu/error-report.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   enum {
> @@ -71,8 +70,6 @@ struct MilkymistSysctlState {
>   
>       MemoryRegion regs_region;
>   
> -    QEMUBH *bh0;
> -    QEMUBH *bh1;
>       ptimer_state *ptimer0;
>       ptimer_state *ptimer1;
>   
> @@ -161,14 +158,19 @@ static void sysctl_write(void *opaque, hwaddr addr, uint64_t value,
>           s->regs[addr] = value;
>           break;
>       case R_TIMER0_COMPARE:
> +        ptimer_transaction_begin(s->ptimer0);
>           ptimer_set_limit(s->ptimer0, value, 0);
>           s->regs[addr] = value;
> +        ptimer_transaction_commit(s->ptimer0);
>           break;
>       case R_TIMER1_COMPARE:
> +        ptimer_transaction_begin(s->ptimer1);
>           ptimer_set_limit(s->ptimer1, value, 0);
>           s->regs[addr] = value;
> +        ptimer_transaction_commit(s->ptimer1);
>           break;
>       case R_TIMER0_CONTROL:
> +        ptimer_transaction_begin(s->ptimer0);
>           s->regs[addr] = value;
>           if (s->regs[R_TIMER0_CONTROL] & CTRL_ENABLE) {
>               trace_milkymist_sysctl_start_timer0();
> @@ -179,8 +181,10 @@ static void sysctl_write(void *opaque, hwaddr addr, uint64_t value,
>               trace_milkymist_sysctl_stop_timer0();
>               ptimer_stop(s->ptimer0);
>           }
> +        ptimer_transaction_commit(s->ptimer0);
>           break;
>       case R_TIMER1_CONTROL:
> +        ptimer_transaction_begin(s->ptimer0);

Copy/paste error I suppose, ptimer1 :)

>           s->regs[addr] = value;
>           if (s->regs[R_TIMER1_CONTROL] & CTRL_ENABLE) {
>               trace_milkymist_sysctl_start_timer1();
> @@ -191,6 +195,7 @@ static void sysctl_write(void *opaque, hwaddr addr, uint64_t value,
>               trace_milkymist_sysctl_stop_timer1();
>               ptimer_stop(s->ptimer1);
>           }
> +        ptimer_transaction_commit(s->ptimer0);

Ditto.

With it fixed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>           break;
>       case R_ICAP:
>           sysctl_icap_write(s, value);
> @@ -263,8 +268,12 @@ static void milkymist_sysctl_reset(DeviceState *d)
>           s->regs[i] = 0;
>       }
>   
> +    ptimer_transaction_begin(s->ptimer0);
>       ptimer_stop(s->ptimer0);
> +    ptimer_transaction_commit(s->ptimer0);
> +    ptimer_transaction_begin(s->ptimer1);
>       ptimer_stop(s->ptimer1);
> +    ptimer_transaction_commit(s->ptimer1);
>   
>       /* defaults */
>       s->regs[R_ICAP] = ICAP_READY;
> @@ -292,13 +301,15 @@ static void milkymist_sysctl_realize(DeviceState *dev, Error **errp)
>   {
>       MilkymistSysctlState *s = MILKYMIST_SYSCTL(dev);
>   
> -    s->bh0 = qemu_bh_new(timer0_hit, s);
> -    s->bh1 = qemu_bh_new(timer1_hit, s);
> -    s->ptimer0 = ptimer_init_with_bh(s->bh0, PTIMER_POLICY_DEFAULT);
> -    s->ptimer1 = ptimer_init_with_bh(s->bh1, PTIMER_POLICY_DEFAULT);
> +    s->ptimer0 = ptimer_init(timer0_hit, s, PTIMER_POLICY_DEFAULT);
> +    s->ptimer1 = ptimer_init(timer1_hit, s, PTIMER_POLICY_DEFAULT);
>   
> +    ptimer_transaction_begin(s->ptimer0);
>       ptimer_set_freq(s->ptimer0, s->freq_hz);
> +    ptimer_transaction_commit(s->ptimer0);
> +    ptimer_transaction_begin(s->ptimer1);
>       ptimer_set_freq(s->ptimer1, s->freq_hz);
> +    ptimer_transaction_commit(s->ptimer1);
>   }
>   
>   static const VMStateDescription vmstate_milkymist_sysctl = {
> 

[PATCH 5/8] hw/timer/altera_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the altera_timer code away from bottom-half based ptimers to
the new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/altera_timer.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c
index ee32e0ec1ff..79fc381252d 100644
--- a/hw/timer/altera_timer.c
+++ b/hw/timer/altera_timer.c
@@ -19,7 +19,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
 
@@ -53,7 +52,6 @@ typedef struct AlteraTimer {
     MemoryRegion  mmio;
     qemu_irq      irq;
     uint32_t      freq_hz;
-    QEMUBH       *bh;
     ptimer_state *ptimer;
     uint32_t      regs[R_MAX];
 } AlteraTimer;
@@ -105,6 +103,7 @@ static void timer_write(void *opaque, hwaddr addr,
         break;
 
     case R_CONTROL:
+        ptimer_transaction_begin(t->ptimer);
         t->regs[R_CONTROL] = value & (CONTROL_ITO | CONTROL_CONT);
         if ((value & CONTROL_START) &&
             !(t->regs[R_STATUS] & STATUS_RUN)) {
@@ -115,10 +114,12 @@ static void timer_write(void *opaque, hwaddr addr,
             ptimer_stop(t->ptimer);
             t->regs[R_STATUS] &= ~STATUS_RUN;
         }
+        ptimer_transaction_commit(t->ptimer);
         break;
 
     case R_PERIODL:
     case R_PERIODH:
+        ptimer_transaction_begin(t->ptimer);
         t->regs[addr] = value & 0xFFFF;
         if (t->regs[R_STATUS] & STATUS_RUN) {
             ptimer_stop(t->ptimer);
@@ -126,6 +127,7 @@ static void timer_write(void *opaque, hwaddr addr,
         }
         tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
         ptimer_set_limit(t->ptimer, tvalue + 1, 1);
+        ptimer_transaction_commit(t->ptimer);
         break;
 
     case R_SNAPL:
@@ -183,9 +185,10 @@ static void altera_timer_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    t->bh = qemu_bh_new(timer_hit, t);
-    t->ptimer = ptimer_init_with_bh(t->bh, PTIMER_POLICY_DEFAULT);
+    t->ptimer = ptimer_init(timer_hit, t, PTIMER_POLICY_DEFAULT);
+    ptimer_transaction_begin(t->ptimer);
     ptimer_set_freq(t->ptimer, t->freq_hz);
+    ptimer_transaction_commit(t->ptimer);
 
     memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t,
                           TYPE_ALTERA_TIMER, R_MAX * sizeof(uint32_t));
@@ -204,8 +207,10 @@ static void altera_timer_reset(DeviceState *dev)
 {
     AlteraTimer *t = ALTERA_TIMER(dev);
 
+    ptimer_transaction_begin(t->ptimer);
     ptimer_stop(t->ptimer);
     ptimer_set_limit(t->ptimer, 0xffffffff, 1);
+    ptimer_transaction_commit(t->ptimer);
     memset(t->regs, 0, sizeof(t->regs));
 }
 
-- 
2.20.1

Re: [PATCH 5/8] hw/timer/altera_timer.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the altera_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/altera_timer.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 5/8] hw/timer/altera_timer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the altera_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/altera_timer.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c
> index ee32e0ec1ff..79fc381252d 100644
> --- a/hw/timer/altera_timer.c
> +++ b/hw/timer/altera_timer.c
> @@ -19,7 +19,6 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   #include "qapi/error.h"
>   
> @@ -53,7 +52,6 @@ typedef struct AlteraTimer {
>       MemoryRegion  mmio;
>       qemu_irq      irq;
>       uint32_t      freq_hz;
> -    QEMUBH       *bh;
>       ptimer_state *ptimer;
>       uint32_t      regs[R_MAX];
>   } AlteraTimer;
> @@ -105,6 +103,7 @@ static void timer_write(void *opaque, hwaddr addr,
>           break;
>   
>       case R_CONTROL:
> +        ptimer_transaction_begin(t->ptimer);
>           t->regs[R_CONTROL] = value & (CONTROL_ITO | CONTROL_CONT);
>           if ((value & CONTROL_START) &&
>               !(t->regs[R_STATUS] & STATUS_RUN)) {
> @@ -115,10 +114,12 @@ static void timer_write(void *opaque, hwaddr addr,
>               ptimer_stop(t->ptimer);
>               t->regs[R_STATUS] &= ~STATUS_RUN;
>           }
> +        ptimer_transaction_commit(t->ptimer);
>           break;
>   
>       case R_PERIODL:
>       case R_PERIODH:
> +        ptimer_transaction_begin(t->ptimer);
>           t->regs[addr] = value & 0xFFFF;
>           if (t->regs[R_STATUS] & STATUS_RUN) {
>               ptimer_stop(t->ptimer);
> @@ -126,6 +127,7 @@ static void timer_write(void *opaque, hwaddr addr,
>           }
>           tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
>           ptimer_set_limit(t->ptimer, tvalue + 1, 1);
> +        ptimer_transaction_commit(t->ptimer);
>           break;
>   
>       case R_SNAPL:
> @@ -183,9 +185,10 @@ static void altera_timer_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    t->bh = qemu_bh_new(timer_hit, t);
> -    t->ptimer = ptimer_init_with_bh(t->bh, PTIMER_POLICY_DEFAULT);
> +    t->ptimer = ptimer_init(timer_hit, t, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(t->ptimer);
>       ptimer_set_freq(t->ptimer, t->freq_hz);
> +    ptimer_transaction_commit(t->ptimer);

This looks odd because timers are not running at this point (REALIZE),
but if we don't protect it, ptimer_set_freq() will trigger the assertion.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   
>       memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t,
>                             TYPE_ALTERA_TIMER, R_MAX * sizeof(uint32_t));
> @@ -204,8 +207,10 @@ static void altera_timer_reset(DeviceState *dev)
>   {
>       AlteraTimer *t = ALTERA_TIMER(dev);
>   
> +    ptimer_transaction_begin(t->ptimer);
>       ptimer_stop(t->ptimer);
>       ptimer_set_limit(t->ptimer, 0xffffffff, 1);
> +    ptimer_transaction_commit(t->ptimer);
>       memset(t->regs, 0, sizeof(t->regs));
>   }
>   
> 

Re: [PATCH 5/8] hw/timer/altera_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 17:09, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 10/17/19 3:29 PM, Peter Maydell wrote:
> > -    t->bh = qemu_bh_new(timer_hit, t);
> > -    t->ptimer = ptimer_init_with_bh(t->bh, PTIMER_POLICY_DEFAULT);
> > +    t->ptimer = ptimer_init(timer_hit, t, PTIMER_POLICY_DEFAULT);
> > +    ptimer_transaction_begin(t->ptimer);
> >       ptimer_set_freq(t->ptimer, t->freq_hz);
> > +    ptimer_transaction_commit(t->ptimer);
>
> This looks odd because timers are not running at this point (REALIZE),
> but if we don't protect it, ptimer_set_freq() will trigger the assertion.

Yep. The same pattern is used in several of the other ptimer
users where a fixed frequency or period is set immediately after
init.

thanks
-- PMM

[PATCH 6/8] hw/watchdog/etraxfs_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the etraxfs_timer code away from bottom-half based ptimers to
the new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/etraxfs_timer.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index ab27fe1895b..afe3d30a8ea 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -26,7 +26,6 @@
 #include "hw/sysbus.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qemu/timer.h"
 #include "hw/irq.h"
@@ -59,9 +58,6 @@ typedef struct ETRAXTimerState {
     qemu_irq irq;
     qemu_irq nmi;
 
-    QEMUBH *bh_t0;
-    QEMUBH *bh_t1;
-    QEMUBH *bh_wd;
     ptimer_state *ptimer_t0;
     ptimer_state *ptimer_t1;
     ptimer_state *ptimer_wd;
@@ -155,6 +151,7 @@ static void update_ctrl(ETRAXTimerState *t, int tnum)
     }
 
     D(printf ("freq_hz=%d div=%d\n", freq_hz, div));
+    ptimer_transaction_begin(timer);
     ptimer_set_freq(timer, freq_hz);
     ptimer_set_limit(timer, div, 0);
 
@@ -176,6 +173,7 @@ static void update_ctrl(ETRAXTimerState *t, int tnum)
             abort();
             break;
     }
+    ptimer_transaction_commit(timer);
 }
 
 static void timer_update_irq(ETRAXTimerState *t)
@@ -240,6 +238,7 @@ static inline void timer_watchdog_update(ETRAXTimerState *t, uint32_t value)
 
     t->wd_hits = 0;
 
+    ptimer_transaction_begin(t->ptimer_wd);
     ptimer_set_freq(t->ptimer_wd, 760);
     if (wd_cnt == 0)
         wd_cnt = 256;
@@ -250,6 +249,7 @@ static inline void timer_watchdog_update(ETRAXTimerState *t, uint32_t value)
         ptimer_stop(t->ptimer_wd);
 
     t->rw_wd_ctrl = value;
+    ptimer_transaction_commit(t->ptimer_wd);
 }
 
 static void
@@ -311,9 +311,15 @@ static void etraxfs_timer_reset(void *opaque)
 {
     ETRAXTimerState *t = opaque;
 
+    ptimer_transaction_begin(t->ptimer_t0);
     ptimer_stop(t->ptimer_t0);
+    ptimer_transaction_commit(t->ptimer_t0);
+    ptimer_transaction_begin(t->ptimer_t1);
     ptimer_stop(t->ptimer_t1);
+    ptimer_transaction_commit(t->ptimer_t1);
+    ptimer_transaction_begin(t->ptimer_wd);
     ptimer_stop(t->ptimer_wd);
+    ptimer_transaction_commit(t->ptimer_wd);
     t->rw_wd_ctrl = 0;
     t->r_intr = 0;
     t->rw_intr_mask = 0;
@@ -325,12 +331,9 @@ static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
     ETRAXTimerState *t = ETRAX_TIMER(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    t->bh_t0 = qemu_bh_new(timer0_hit, t);
-    t->bh_t1 = qemu_bh_new(timer1_hit, t);
-    t->bh_wd = qemu_bh_new(watchdog_hit, t);
-    t->ptimer_t0 = ptimer_init_with_bh(t->bh_t0, PTIMER_POLICY_DEFAULT);
-    t->ptimer_t1 = ptimer_init_with_bh(t->bh_t1, PTIMER_POLICY_DEFAULT);
-    t->ptimer_wd = ptimer_init_with_bh(t->bh_wd, PTIMER_POLICY_DEFAULT);
+    t->ptimer_t0 = ptimer_init(timer0_hit, t, PTIMER_POLICY_DEFAULT);
+    t->ptimer_t1 = ptimer_init(timer1_hit, t, PTIMER_POLICY_DEFAULT);
+    t->ptimer_wd = ptimer_init(watchdog_hit, t, PTIMER_POLICY_DEFAULT);
 
     sysbus_init_irq(sbd, &t->irq);
     sysbus_init_irq(sbd, &t->nmi);
-- 
2.20.1

Re: [PATCH 6/8] hw/watchdog/etraxfs_timer.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the etraxfs_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/etraxfs_timer.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 6/8] hw/watchdog/etraxfs_timer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the etraxfs_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/timer/etraxfs_timer.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
> index ab27fe1895b..afe3d30a8ea 100644
> --- a/hw/timer/etraxfs_timer.c
> +++ b/hw/timer/etraxfs_timer.c
> @@ -26,7 +26,6 @@
>   #include "hw/sysbus.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/runstate.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   #include "qemu/timer.h"
>   #include "hw/irq.h"
> @@ -59,9 +58,6 @@ typedef struct ETRAXTimerState {
>       qemu_irq irq;
>       qemu_irq nmi;
>   
> -    QEMUBH *bh_t0;
> -    QEMUBH *bh_t1;
> -    QEMUBH *bh_wd;
>       ptimer_state *ptimer_t0;
>       ptimer_state *ptimer_t1;
>       ptimer_state *ptimer_wd;
> @@ -155,6 +151,7 @@ static void update_ctrl(ETRAXTimerState *t, int tnum)
>       }
>   
>       D(printf ("freq_hz=%d div=%d\n", freq_hz, div));
> +    ptimer_transaction_begin(timer);
>       ptimer_set_freq(timer, freq_hz);
>       ptimer_set_limit(timer, div, 0);
>   
> @@ -176,6 +173,7 @@ static void update_ctrl(ETRAXTimerState *t, int tnum)
>               abort();
>               break;
>       }
> +    ptimer_transaction_commit(timer);
>   }
>   
>   static void timer_update_irq(ETRAXTimerState *t)
> @@ -240,6 +238,7 @@ static inline void timer_watchdog_update(ETRAXTimerState *t, uint32_t value)
>   
>       t->wd_hits = 0;
>   
> +    ptimer_transaction_begin(t->ptimer_wd);
>       ptimer_set_freq(t->ptimer_wd, 760);
>       if (wd_cnt == 0)
>           wd_cnt = 256;
> @@ -250,6 +249,7 @@ static inline void timer_watchdog_update(ETRAXTimerState *t, uint32_t value)
>           ptimer_stop(t->ptimer_wd);
>   
>       t->rw_wd_ctrl = value;
> +    ptimer_transaction_commit(t->ptimer_wd);
>   }
>   
>   static void
> @@ -311,9 +311,15 @@ static void etraxfs_timer_reset(void *opaque)
>   {
>       ETRAXTimerState *t = opaque;
>   
> +    ptimer_transaction_begin(t->ptimer_t0);
>       ptimer_stop(t->ptimer_t0);
> +    ptimer_transaction_commit(t->ptimer_t0);
> +    ptimer_transaction_begin(t->ptimer_t1);
>       ptimer_stop(t->ptimer_t1);
> +    ptimer_transaction_commit(t->ptimer_t1);
> +    ptimer_transaction_begin(t->ptimer_wd);
>       ptimer_stop(t->ptimer_wd);
> +    ptimer_transaction_commit(t->ptimer_wd);
>       t->rw_wd_ctrl = 0;
>       t->r_intr = 0;
>       t->rw_intr_mask = 0;
> @@ -325,12 +331,9 @@ static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
>       ETRAXTimerState *t = ETRAX_TIMER(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>   
> -    t->bh_t0 = qemu_bh_new(timer0_hit, t);
> -    t->bh_t1 = qemu_bh_new(timer1_hit, t);
> -    t->bh_wd = qemu_bh_new(watchdog_hit, t);
> -    t->ptimer_t0 = ptimer_init_with_bh(t->bh_t0, PTIMER_POLICY_DEFAULT);
> -    t->ptimer_t1 = ptimer_init_with_bh(t->bh_t1, PTIMER_POLICY_DEFAULT);
> -    t->ptimer_wd = ptimer_init_with_bh(t->bh_wd, PTIMER_POLICY_DEFAULT);
> +    t->ptimer_t0 = ptimer_init(timer0_hit, t, PTIMER_POLICY_DEFAULT);
> +    t->ptimer_t1 = ptimer_init(timer1_hit, t, PTIMER_POLICY_DEFAULT);
> +    t->ptimer_wd = ptimer_init(watchdog_hit, t, PTIMER_POLICY_DEFAULT);
>   
>       sysbus_init_irq(sbd, &t->irq);
>       sysbus_init_irq(sbd, &t->nmi);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the mcf5206 code away from bottom-half based ptimers to
the new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/mcf5206.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index a49096367cb..c05401e0e50 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -8,7 +8,6 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/irq.h"
@@ -57,6 +56,7 @@ static void m5206_timer_recalibrate(m5206_timer_state *s)
     int prescale;
     int mode;
 
+    ptimer_transaction_begin(s->timer);
     ptimer_stop(s->timer);
 
     if ((s->tmr & TMR_RST) == 0)
@@ -78,6 +78,7 @@ static void m5206_timer_recalibrate(m5206_timer_state *s)
     ptimer_set_limit(s->timer, s->trr, 0);
 
     ptimer_run(s->timer, 0);
+    ptimer_transaction_commit(s->timer);
 }
 
 static void m5206_timer_trigger(void *opaque)
@@ -123,7 +124,9 @@ static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)
         s->tcr = val;
         break;
     case 0xc:
+        ptimer_transaction_begin(s->timer);
         ptimer_set_count(s->timer, val);
+        ptimer_transaction_commit(s->timer);
         break;
     case 0x11:
         s->ter &= ~val;
@@ -137,11 +140,9 @@ static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)
 static m5206_timer_state *m5206_timer_init(qemu_irq irq)
 {
     m5206_timer_state *s;
-    QEMUBH *bh;
 
     s = g_new0(m5206_timer_state, 1);
-    bh = qemu_bh_new(m5206_timer_trigger, s);
-    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init(m5206_timer_trigger, s, PTIMER_POLICY_DEFAULT);
     s->irq = irq;
     m5206_timer_reset(s);
     return s;
-- 
2.20.1

Re: [PATCH 7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the mcf5206 code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/mcf5206.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the mcf5206 code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/m68k/mcf5206.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
> index a49096367cb..c05401e0e50 100644
> --- a/hw/m68k/mcf5206.c
> +++ b/hw/m68k/mcf5206.c
> @@ -8,7 +8,6 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/error-report.h"
> -#include "qemu/main-loop.h"
>   #include "cpu.h"
>   #include "hw/hw.h"
>   #include "hw/irq.h"
> @@ -57,6 +56,7 @@ static void m5206_timer_recalibrate(m5206_timer_state *s)
>       int prescale;
>       int mode;
>   
> +    ptimer_transaction_begin(s->timer);
>       ptimer_stop(s->timer);
>   
>       if ((s->tmr & TMR_RST) == 0)
> @@ -78,6 +78,7 @@ static void m5206_timer_recalibrate(m5206_timer_state *s)
>       ptimer_set_limit(s->timer, s->trr, 0);
>   
>       ptimer_run(s->timer, 0);
> +    ptimer_transaction_commit(s->timer);
>   }
>   
>   static void m5206_timer_trigger(void *opaque)
> @@ -123,7 +124,9 @@ static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)
>           s->tcr = val;
>           break;
>       case 0xc:
> +        ptimer_transaction_begin(s->timer);
>           ptimer_set_count(s->timer, val);
> +        ptimer_transaction_commit(s->timer);
>           break;
>       case 0x11:
>           s->ter &= ~val;
> @@ -137,11 +140,9 @@ static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)
>   static m5206_timer_state *m5206_timer_init(qemu_irq irq)
>   {
>       m5206_timer_state *s;
> -    QEMUBH *bh;
>   
>       s = g_new0(m5206_timer_state, 1);
> -    bh = qemu_bh_new(m5206_timer_trigger, s);
> -    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
> +    s->timer = ptimer_init(m5206_timer_trigger, s, PTIMER_POLICY_DEFAULT);
>       s->irq = irq;
>       m5206_timer_reset(s);
>       return s;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

[Thomas Huth]

Am Thu, 17 Oct 2019 14:29:04 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Switch the mcf5206 code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/mcf5206.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

After applying the patch, I now get an assertion:

$ qemu-system-m68k -nographic -kernel
~/tmp/images/image-an5206-big-20000706.bin -M an5206

uClinux/COLDFIRE(m5206)
COLDFIRE port done by Greg Ungerer, gerg@moreton.com.au
Flat model support (C) 1998,1999 Kenneth Albanowski, D. Jeff Dionne
KERNEL -> TEXT=0x010000-0x077cb8 DATA=0x077cb8-0x08b0d0 BSS=0x08b0d0-0x0a2d58
KERNEL -> ROMFS=0x0a2d58-0x183b10 MEM=0x183b10-0x1fff000 STACK=0x1fff000-0x2000000
qemu-system-m68k: hw/core/ptimer.c:410: ptimer_transaction_begin: Assertion `!s->in_transaction || !s->callback' failed.

Looks like something is still wrong here...

 Thomas

Re: [PATCH 7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

[Thomas Huth]

Am Sat, 19 Oct 2019 12:48:59 +0200
schrieb Thomas Huth <huth@tuxfamily.org>:

> Am Thu, 17 Oct 2019 14:29:04 +0100
> schrieb Peter Maydell <peter.maydell@linaro.org>:
> 
> > Switch the mcf5206 code away from bottom-half based ptimers to
> > the new transaction-based ptimer API.  This just requires adding
> > begin/commit calls around the various places that modify the ptimer
> > state, and using the new ptimer_init() function to create the timer.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/m68k/mcf5206.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)  
> 
> After applying the patch, I now get an assertion:
> 
> $ qemu-system-m68k -nographic -kernel
> ~/tmp/images/image-an5206-big-20000706.bin -M an5206
> 
> uClinux/COLDFIRE(m5206)
> COLDFIRE port done by Greg Ungerer, gerg@moreton.com.au
> Flat model support (C) 1998,1999 Kenneth Albanowski, D. Jeff Dionne
> KERNEL -> TEXT=0x010000-0x077cb8 DATA=0x077cb8-0x08b0d0
> BSS=0x08b0d0-0x0a2d58 KERNEL -> ROMFS=0x0a2d58-0x183b10
> MEM=0x183b10-0x1fff000 STACK=0x1fff000-0x2000000 qemu-system-m68k:
> hw/core/ptimer.c:410: ptimer_transaction_begin: Assertion
> `!s->in_transaction || !s->callback' failed.
> 
> Looks like something is still wrong here...

FWIW, the image for testing can be found here:

https://web.archive.org/web/20181018135822/http://www.uclinux.org/ports/coldfire/image-an5206-big-20000706.bin.gz

 Thomas

Re: [PATCH 7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

[Thomas Huth]

Am Sat, 19 Oct 2019 13:10:27 +0200
schrieb Thomas Huth <huth@tuxfamily.org>:

> Am Sat, 19 Oct 2019 12:48:59 +0200
> schrieb Thomas Huth <huth@tuxfamily.org>:
> 
> > Am Thu, 17 Oct 2019 14:29:04 +0100
> > schrieb Peter Maydell <peter.maydell@linaro.org>:
> > 
> > > Switch the mcf5206 code away from bottom-half based ptimers to
> > > the new transaction-based ptimer API.  This just requires adding
> > > begin/commit calls around the various places that modify the
> > > ptimer state, and using the new ptimer_init() function to create
> > > the timer.
> > > 
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >  hw/m68k/mcf5206.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)  
> > 
> > After applying the patch, I now get an assertion:
> > 
> > $ qemu-system-m68k -nographic -kernel
> > ~/tmp/images/image-an5206-big-20000706.bin -M an5206
> > 
> > uClinux/COLDFIRE(m5206)
> > COLDFIRE port done by Greg Ungerer, gerg@moreton.com.au
> > Flat model support (C) 1998,1999 Kenneth Albanowski, D. Jeff Dionne
> > KERNEL -> TEXT=0x010000-0x077cb8 DATA=0x077cb8-0x08b0d0
> > BSS=0x08b0d0-0x0a2d58 KERNEL -> ROMFS=0x0a2d58-0x183b10
> > MEM=0x183b10-0x1fff000 STACK=0x1fff000-0x2000000 qemu-system-m68k:
> > hw/core/ptimer.c:410: ptimer_transaction_begin: Assertion
> > `!s->in_transaction || !s->callback' failed.
> > 
> > Looks like something is still wrong here...

Looking at the code a little bit, I think you missed the early return
in m5206_timer_recalibrate() :

    if ((s->tmr & TMR_RST) == 0)
        return;

That needs a ptimer_transaction_commit() now, too.

 Thomas

[PATCH 8/8] hw/m68k/mcf5208.c: Switch to transaction-based ptimer API

[Peter Maydell]

Switch the mcf5208 code away from bottom-half based ptimers to
the new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/mcf5208.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index 34d34eba17c..158c5e4be75 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -9,7 +9,6 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "cpu.h"
@@ -79,6 +78,7 @@ static void m5208_timer_write(void *opaque, hwaddr offset,
             return;
         }
 
+        ptimer_transaction_begin(s->timer);
         if (s->pcsr & PCSR_EN)
             ptimer_stop(s->timer);
 
@@ -94,8 +94,10 @@ static void m5208_timer_write(void *opaque, hwaddr offset,
 
         if (s->pcsr & PCSR_EN)
             ptimer_run(s->timer, 0);
+        ptimer_transaction_commit(s->timer);
         break;
     case 2:
+        ptimer_transaction_begin(s->timer);
         s->pmr = value;
         s->pcsr &= ~PCSR_PIF;
         if ((s->pcsr & PCSR_RLD) == 0) {
@@ -104,6 +106,7 @@ static void m5208_timer_write(void *opaque, hwaddr offset,
         } else {
             ptimer_set_limit(s->timer, value, s->pcsr & PCSR_OVW);
         }
+        ptimer_transaction_commit(s->timer);
         break;
     case 4:
         break;
@@ -182,7 +185,6 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
 {
     MemoryRegion *iomem = g_new(MemoryRegion, 1);
     m5208_timer_state *s;
-    QEMUBH *bh;
     int i;
 
     /* SDRAMC.  */
@@ -191,8 +193,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
     /* Timers.  */
     for (i = 0; i < 2; i++) {
         s = g_new0(m5208_timer_state, 1);
-        bh = qemu_bh_new(m5208_timer_trigger, s);
-        s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
+        s->timer = ptimer_init(m5208_timer_trigger, s, PTIMER_POLICY_DEFAULT);
         memory_region_init_io(&s->iomem, NULL, &m5208_timer_ops, s,
                               "m5208-timer", 0x00004000);
         memory_region_add_subregion(address_space, 0xfc080000 + 0x4000 * i,
-- 
2.20.1

Re: [PATCH 8/8] hw/m68k/mcf5208.c: Switch to transaction-based ptimer API

[Richard Henderson]

On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the mcf5208 code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/mcf5208.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 8/8] hw/m68k/mcf5208.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the mcf5208 code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/m68k/mcf5208.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index 34d34eba17c..158c5e4be75 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -9,7 +9,6 @@
>   #include "qemu/osdep.h"
>   #include "qemu/units.h"
>   #include "qemu/error-report.h"
> -#include "qemu/main-loop.h"
>   #include "qapi/error.h"
>   #include "qemu-common.h"
>   #include "cpu.h"
> @@ -79,6 +78,7 @@ static void m5208_timer_write(void *opaque, hwaddr offset,
>               return;
>           }
>   
> +        ptimer_transaction_begin(s->timer);
>           if (s->pcsr & PCSR_EN)
>               ptimer_stop(s->timer);
>   
> @@ -94,8 +94,10 @@ static void m5208_timer_write(void *opaque, hwaddr offset,
>   
>           if (s->pcsr & PCSR_EN)
>               ptimer_run(s->timer, 0);
> +        ptimer_transaction_commit(s->timer);
>           break;
>       case 2:
> +        ptimer_transaction_begin(s->timer);
>           s->pmr = value;
>           s->pcsr &= ~PCSR_PIF;
>           if ((s->pcsr & PCSR_RLD) == 0) {
> @@ -104,6 +106,7 @@ static void m5208_timer_write(void *opaque, hwaddr offset,
>           } else {
>               ptimer_set_limit(s->timer, value, s->pcsr & PCSR_OVW);
>           }
> +        ptimer_transaction_commit(s->timer);
>           break;
>       case 4:
>           break;
> @@ -182,7 +185,6 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
>   {
>       MemoryRegion *iomem = g_new(MemoryRegion, 1);
>       m5208_timer_state *s;
> -    QEMUBH *bh;
>       int i;
>   
>       /* SDRAMC.  */
> @@ -191,8 +193,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
>       /* Timers.  */
>       for (i = 0; i < 2; i++) {
>           s = g_new0(m5208_timer_state, 1);
> -        bh = qemu_bh_new(m5208_timer_trigger, s);
> -        s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
> +        s->timer = ptimer_init(m5208_timer_trigger, s, PTIMER_POLICY_DEFAULT);
>           memory_region_init_io(&s->iomem, NULL, &m5208_timer_ops, s,
>                                 "m5208-timer", 0x00004000);
>           memory_region_add_subregion(address_space, 0xfc080000 + 0x4000 * i,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 8/8] hw/m68k/mcf5208.c: Switch to transaction-based ptimer API

[Thomas Huth]

Am Thu, 17 Oct 2019 14:29:05 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Switch the mcf5208 code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/mcf5208.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

This patch seems to work fine.

Tested-by: Thomas Huth <huth@tuxfamily.org>