1. Patchew
  2. QEMU
  3. [PATCH 0/3] Convert ppc and microblaze devices to new ptimer API
  4. View patch

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

Peter Maydell posted 3 patches 6 years ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>, David Gibson <david@gibson.dropbear.id.au>, Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Jason Wang <jasowang@redhat.com>
[PATCH 2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
Posted by Peter Maydell 6 years ago 
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);
+            }
             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
Re: [PATCH 2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
Posted by Richard Henderson 6 years ago 
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~
Re: [PATCH 2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
Posted by Alistair Francis 6 years ago 
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
>
>
Re: [PATCH 2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
Posted by Philippe Mathieu-Daudé 6 years ago 
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",
>
Re: [PATCH 2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
Posted by Peter Maydell 6 years ago 
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
Re: [PATCH 2/3] hw/timer/xilinx_timer.c: Switch to transaction-based ptimer API
Posted by Philippe Mathieu-Daudé 6 years ago 
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>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.