[2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API

Message ID 20191017132122.4402-3-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • Convert ppc and microblaze devices to new ptimer API
Related show

Commit Message

Peter Maydell Oct. 17, 2019, 1:21 p.m.
Switch the xilinx_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/xilinx_timer.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.20.1

Comments

Richard Henderson Oct. 17, 2019, 2:14 p.m. | #1
On 10/17/19 6:21 AM, Peter Maydell wrote:
> Switch the xilinx_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/xilinx_timer.c | 13 ++++++++-----

>  1 file changed, 8 insertions(+), 5 deletions(-)


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



r~
Philippe Mathieu-Daudé Oct. 17, 2019, 2:56 p.m. | #2
Hi Peter,

On 10/17/19 3:21 PM, Peter Maydell wrote:
> Switch the xilinx_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/xilinx_timer.c | 13 ++++++++-----

>   1 file changed, 8 insertions(+), 5 deletions(-)

> 

> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c

> index 92dbff304d9..7191ea54f58 100644

> --- a/hw/timer/xilinx_timer.c

> +++ b/hw/timer/xilinx_timer.c

> @@ -28,7 +28,6 @@

>   #include "hw/ptimer.h"

>   #include "hw/qdev-properties.h"

>   #include "qemu/log.h"

> -#include "qemu/main-loop.h"

>   #include "qemu/module.h"

>   

>   #define D(x)

> @@ -52,7 +51,6 @@

>   

>   struct xlx_timer

>   {

> -    QEMUBH *bh;

>       ptimer_state *ptimer;

>       void *parent;

>       int nr; /* for debug.  */

> @@ -134,6 +132,7 @@ timer_read(void *opaque, hwaddr addr, unsigned int size)

>       return r;

>   }

>   

> +/* Must be called inside ptimer transaction block */

>   static void timer_enable(struct xlx_timer *xt)

>   {

>       uint64_t count;

> @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,

>                   value &= ~TCSR_TINT;

>   

>               xt->regs[addr] = value & 0x7ff;

> -            if (value & TCSR_ENT)

> +            if (value & TCSR_ENT) {

> +                ptimer_transaction_begin(xt->ptimer);

>                   timer_enable(xt);

> +                ptimer_transaction_commit(xt->ptimer);


Why not move these inside timer_enable()?

> +            }

>               break;

>    

>           default:

> @@ -220,9 +222,10 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)

>   

>           xt->parent = t;

>           xt->nr = i;

> -        xt->bh = qemu_bh_new(timer_hit, xt);

> -        xt->ptimer = ptimer_init_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);

> +        xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);

> +        ptimer_transaction_begin(xt->ptimer);

>           ptimer_set_freq(xt->ptimer, t->freq_hz);

> +        ptimer_transaction_commit(xt->ptimer);

>       }

>   

>       memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",

>
Peter Maydell Oct. 17, 2019, 3:03 p.m. | #3
On Thu, 17 Oct 2019 at 15:57, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>

> Hi Peter,

>

> On 10/17/19 3:21 PM, Peter Maydell wrote:

> > +/* Must be called inside ptimer transaction block */

> >   static void timer_enable(struct xlx_timer *xt)

> >   {

> >       uint64_t count;

> > @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,

> >                   value &= ~TCSR_TINT;

> >

> >               xt->regs[addr] = value & 0x7ff;

> > -            if (value & TCSR_ENT)

> > +            if (value & TCSR_ENT) {

> > +                ptimer_transaction_begin(xt->ptimer);

> >                   timer_enable(xt);

> > +                ptimer_transaction_commit(xt->ptimer);

>

> Why not move these inside timer_enable()?


Because timer_enable() is called from the callback
function timer_hit(). Since callback functions are called
from within a begin/commit block, if we called begin
again in timer_enable() it would assert().

thanks
-- PMM
Philippe Mathieu-Daudé Oct. 17, 2019, 3:24 p.m. | #4
On 10/17/19 5:03 PM, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 15:57, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

>>

>> Hi Peter,

>>

>> On 10/17/19 3:21 PM, Peter Maydell wrote:

>>> +/* Must be called inside ptimer transaction block */

>>>    static void timer_enable(struct xlx_timer *xt)

>>>    {

>>>        uint64_t count;

>>> @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,

>>>                    value &= ~TCSR_TINT;

>>>

>>>                xt->regs[addr] = value & 0x7ff;

>>> -            if (value & TCSR_ENT)

>>> +            if (value & TCSR_ENT) {

>>> +                ptimer_transaction_begin(xt->ptimer);

>>>                    timer_enable(xt);

>>> +                ptimer_transaction_commit(xt->ptimer);

>>

>> Why not move these inside timer_enable()?

> 

> Because timer_enable() is called from the callback

> function timer_hit(). Since callback functions are called

> from within a begin/commit block, if we called begin

> again in timer_enable() it would assert().


Yes, now I understood the point of this new API ;)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Alistair Francis Oct. 17, 2019, 10 p.m. | #5
On Thu, Oct 17, 2019 at 6:50 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>

> Switch the xilinx_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>


Reviewed-by: Alistair Francis <alistair.francis@wdc.com>


Alistair

> ---

>  hw/timer/xilinx_timer.c | 13 ++++++++-----

>  1 file changed, 8 insertions(+), 5 deletions(-)

>

> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c

> index 92dbff304d9..7191ea54f58 100644

> --- a/hw/timer/xilinx_timer.c

> +++ b/hw/timer/xilinx_timer.c

> @@ -28,7 +28,6 @@

>  #include "hw/ptimer.h"

>  #include "hw/qdev-properties.h"

>  #include "qemu/log.h"

> -#include "qemu/main-loop.h"

>  #include "qemu/module.h"

>

>  #define D(x)

> @@ -52,7 +51,6 @@

>

>  struct xlx_timer

>  {

> -    QEMUBH *bh;

>      ptimer_state *ptimer;

>      void *parent;

>      int nr; /* for debug.  */

> @@ -134,6 +132,7 @@ timer_read(void *opaque, hwaddr addr, unsigned int size)

>      return r;

>  }

>

> +/* Must be called inside ptimer transaction block */

>  static void timer_enable(struct xlx_timer *xt)

>  {

>      uint64_t count;

> @@ -174,8 +173,11 @@ timer_write(void *opaque, hwaddr addr,

>                  value &= ~TCSR_TINT;

>

>              xt->regs[addr] = value & 0x7ff;

> -            if (value & TCSR_ENT)

> +            if (value & TCSR_ENT) {

> +                ptimer_transaction_begin(xt->ptimer);

>                  timer_enable(xt);

> +                ptimer_transaction_commit(xt->ptimer);

> +            }

>              break;

>

>          default:

> @@ -220,9 +222,10 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)

>

>          xt->parent = t;

>          xt->nr = i;

> -        xt->bh = qemu_bh_new(timer_hit, xt);

> -        xt->ptimer = ptimer_init_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);

> +        xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);

> +        ptimer_transaction_begin(xt->ptimer);

>          ptimer_set_freq(xt->ptimer, t->freq_hz);

> +        ptimer_transaction_commit(xt->ptimer);

>      }

>

>      memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",

> --

> 2.20.1

>

>

Patch

diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
index 92dbff304d9..7191ea54f58 100644
--- a/hw/timer/xilinx_timer.c
+++ b/hw/timer/xilinx_timer.c
@@ -28,7 +28,6 @@ 
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 
 #define D(x)
@@ -52,7 +51,6 @@ 
 
 struct xlx_timer
 {
-    QEMUBH *bh;
     ptimer_state *ptimer;
     void *parent;
     int nr; /* for debug.  */
@@ -134,6 +132,7 @@  timer_read(void *opaque, hwaddr addr, unsigned int size)
     return r;
 }
 
+/* Must be called inside ptimer transaction block */
 static void timer_enable(struct xlx_timer *xt)
 {
     uint64_t count;
@@ -174,8 +173,11 @@  timer_write(void *opaque, hwaddr addr,
                 value &= ~TCSR_TINT;
 
             xt->regs[addr] = value & 0x7ff;
-            if (value & TCSR_ENT)
+            if (value & TCSR_ENT) {
+                ptimer_transaction_begin(xt->ptimer);
                 timer_enable(xt);
+                ptimer_transaction_commit(xt->ptimer);
+            }
             break;
  
         default:
@@ -220,9 +222,10 @@  static void xilinx_timer_realize(DeviceState *dev, Error **errp)
 
         xt->parent = t;
         xt->nr = i;
-        xt->bh = qemu_bh_new(timer_hit, xt);
-        xt->ptimer = ptimer_init_with_bh(xt->bh, PTIMER_POLICY_DEFAULT);
+        xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);
+        ptimer_transaction_begin(xt->ptimer);
         ptimer_set_freq(xt->ptimer, t->freq_hz);
+        ptimer_transaction_commit(xt->ptimer);
     }
 
     memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",