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

Message ID 20191017132905.5604-6-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • Convert misc-arch devices to new ptimer API
Related show

Commit Message

Peter Maydell Oct. 17, 2019, 1:29 p.m.
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(-)

-- 
2.20.1

Comments

Richard Henderson Oct. 17, 2019, 3:02 p.m. | #1
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~
Philippe Mathieu-Daudé Oct. 17, 2019, 4:09 p.m. | #2
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));

>   }

>   

>
Peter Maydell Oct. 17, 2019, 4:10 p.m. | #3
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

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));
 }