| Message ID | 20191017132122.4402-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert ppc and microblaze devices to new ptimer API | expand |
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~
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", >
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
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>
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 > >
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",
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