| Message ID | 20191017132905.5604-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert misc-arch devices to new ptimer API | expand |
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~
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)); > } > >
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
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)); }
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