Message ID | 20191017132122.4402-4-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_axidma 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/dma/xilinx_axidma.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 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_axidma 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/dma/xilinx_axidma.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c > index e035d1f7504..fb3a978e282 100644 > --- a/hw/dma/xilinx_axidma.c > +++ b/hw/dma/xilinx_axidma.c > @@ -31,7 +31,6 @@ > #include "hw/ptimer.h" > #include "hw/qdev-properties.h" > #include "qemu/log.h" > -#include "qemu/main-loop.h" > #include "qemu/module.h" > > #include "hw/stream.h" > @@ -104,7 +103,6 @@ enum { > }; > > struct Stream { > - QEMUBH *bh; > ptimer_state *ptimer; > qemu_irq irq; > > @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s) > unsigned int comp_delay; > > /* Start the delayed timer. */ > + ptimer_transaction_begin(s->ptimer); > comp_delay = s->regs[R_DMACR] >> 24; > if (comp_delay) { > ptimer_stop(s->ptimer); > @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s) > s->regs[R_DMASR] |= DMASR_IOC_IRQ; > stream_reload_complete_cnt(s); > } > + ptimer_transaction_commit(s->ptimer); I'd restrict the transaction here within the if() statement: -- >8 -- @@ -244,9 +244,11 @@ static void stream_complete(struct Stream *s) /* Start the delayed timer. */ comp_delay = s->regs[R_DMACR] >> 24; if (comp_delay) { + ptimer_transaction_begin(s->ptimer); ptimer_stop(s->ptimer); ptimer_set_count(s->ptimer, comp_delay); ptimer_run(s->ptimer, 1); + ptimer_transaction_commit(s->ptimer); } s->complete_cnt--; --- > } > > static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev, > @@ -551,9 +551,10 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp) > struct Stream *st = &s->streams[i]; > > st->nr = i; > - st->bh = qemu_bh_new(timer_hit, st); > - st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT); > + st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT); > + ptimer_transaction_begin(st->ptimer); > ptimer_set_freq(st->ptimer, s->freqhz); > + ptimer_transaction_commit(st->ptimer); > } > return; > >
On Thu, 17 Oct 2019 at 16:01, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Hi Peter, > > On 10/17/19 3:21 PM, Peter Maydell wrote: > > Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c > > index e035d1f7504..fb3a978e282 100644 > > --- a/hw/dma/xilinx_axidma.c > > +++ b/hw/dma/xilinx_axidma.c > > @@ -31,7 +31,6 @@ > > #include "hw/ptimer.h" > > #include "hw/qdev-properties.h" > > #include "qemu/log.h" > > -#include "qemu/main-loop.h" > > #include "qemu/module.h" > > > > #include "hw/stream.h" > > @@ -104,7 +103,6 @@ enum { > > }; > > > > struct Stream { > > - QEMUBH *bh; > > ptimer_state *ptimer; > > qemu_irq irq; > > > > @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s) > > unsigned int comp_delay; > > > > /* Start the delayed timer. */ > > + ptimer_transaction_begin(s->ptimer); > > comp_delay = s->regs[R_DMACR] >> 24; > > if (comp_delay) { > > ptimer_stop(s->ptimer); > > @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s) > > s->regs[R_DMASR] |= DMASR_IOC_IRQ; > > stream_reload_complete_cnt(s); > > } > > + ptimer_transaction_commit(s->ptimer); > > I'd restrict the transaction here within the if() statement: > > -- >8 -- > @@ -244,9 +244,11 @@ static void stream_complete(struct Stream *s) > /* Start the delayed timer. */ > comp_delay = s->regs[R_DMACR] >> 24; > if (comp_delay) { > + ptimer_transaction_begin(s->ptimer); > ptimer_stop(s->ptimer); > ptimer_set_count(s->ptimer, comp_delay); > ptimer_run(s->ptimer, 1); > + ptimer_transaction_commit(s->ptimer); > } > > s->complete_cnt--; The timer_hit callback function itself writes to s->complete_cnt, so we don't want to allow it to be called (via the commit()) before stream_complete() is done with changing that state. thanks -- PMM
On 10/17/19 5:06 PM, Peter Maydell wrote: > On Thu, 17 Oct 2019 at 16:01, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> Hi Peter, >> >> On 10/17/19 3:21 PM, Peter Maydell wrote: >>> Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c >>> index e035d1f7504..fb3a978e282 100644 >>> --- a/hw/dma/xilinx_axidma.c >>> +++ b/hw/dma/xilinx_axidma.c >>> @@ -31,7 +31,6 @@ >>> #include "hw/ptimer.h" >>> #include "hw/qdev-properties.h" >>> #include "qemu/log.h" >>> -#include "qemu/main-loop.h" >>> #include "qemu/module.h" >>> >>> #include "hw/stream.h" >>> @@ -104,7 +103,6 @@ enum { >>> }; >>> >>> struct Stream { >>> - QEMUBH *bh; >>> ptimer_state *ptimer; >>> qemu_irq irq; >>> >>> @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s) >>> unsigned int comp_delay; >>> >>> /* Start the delayed timer. */ >>> + ptimer_transaction_begin(s->ptimer); >>> comp_delay = s->regs[R_DMACR] >> 24; >>> if (comp_delay) { >>> ptimer_stop(s->ptimer); >>> @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s) >>> s->regs[R_DMASR] |= DMASR_IOC_IRQ; >>> stream_reload_complete_cnt(s); >>> } >>> + ptimer_transaction_commit(s->ptimer); >> >> I'd restrict the transaction here within the if() statement: >> >> -- >8 -- >> @@ -244,9 +244,11 @@ static void stream_complete(struct Stream *s) >> /* Start the delayed timer. */ >> comp_delay = s->regs[R_DMACR] >> 24; >> if (comp_delay) { >> + ptimer_transaction_begin(s->ptimer); >> ptimer_stop(s->ptimer); >> ptimer_set_count(s->ptimer, comp_delay); >> ptimer_run(s->ptimer, 1); >> + ptimer_transaction_commit(s->ptimer); >> } >> >> s->complete_cnt--; > > The timer_hit callback function itself writes to > s->complete_cnt, so we don't want to allow it to > be called (via the commit()) before stream_complete() > is done with changing that state. Indeed we have timer_hit() -> stream_reload_complete_cnt() which uses s->complete_cnt. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Thu, Oct 17, 2019 at 6:53 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c > index e035d1f7504..fb3a978e282 100644 > --- a/hw/dma/xilinx_axidma.c > +++ b/hw/dma/xilinx_axidma.c > @@ -31,7 +31,6 @@ > #include "hw/ptimer.h" > #include "hw/qdev-properties.h" > #include "qemu/log.h" > -#include "qemu/main-loop.h" > #include "qemu/module.h" > > #include "hw/stream.h" > @@ -104,7 +103,6 @@ enum { > }; > > struct Stream { > - QEMUBH *bh; > ptimer_state *ptimer; > qemu_irq irq; > > @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s) > unsigned int comp_delay; > > /* Start the delayed timer. */ > + ptimer_transaction_begin(s->ptimer); > comp_delay = s->regs[R_DMACR] >> 24; > if (comp_delay) { > ptimer_stop(s->ptimer); > @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s) > s->regs[R_DMASR] |= DMASR_IOC_IRQ; > stream_reload_complete_cnt(s); > } > + ptimer_transaction_commit(s->ptimer); > } > > static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev, > @@ -551,9 +551,10 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp) > struct Stream *st = &s->streams[i]; > > st->nr = i; > - st->bh = qemu_bh_new(timer_hit, st); > - st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT); > + st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT); > + ptimer_transaction_begin(st->ptimer); > ptimer_set_freq(st->ptimer, s->freqhz); > + ptimer_transaction_commit(st->ptimer); > } > return; > > -- > 2.20.1 > >
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c index e035d1f7504..fb3a978e282 100644 --- a/hw/dma/xilinx_axidma.c +++ b/hw/dma/xilinx_axidma.c @@ -31,7 +31,6 @@ #include "hw/ptimer.h" #include "hw/qdev-properties.h" #include "qemu/log.h" -#include "qemu/main-loop.h" #include "qemu/module.h" #include "hw/stream.h" @@ -104,7 +103,6 @@ enum { }; struct Stream { - QEMUBH *bh; ptimer_state *ptimer; qemu_irq irq; @@ -242,6 +240,7 @@ static void stream_complete(struct Stream *s) unsigned int comp_delay; /* Start the delayed timer. */ + ptimer_transaction_begin(s->ptimer); comp_delay = s->regs[R_DMACR] >> 24; if (comp_delay) { ptimer_stop(s->ptimer); @@ -255,6 +254,7 @@ static void stream_complete(struct Stream *s) s->regs[R_DMASR] |= DMASR_IOC_IRQ; stream_reload_complete_cnt(s); } + ptimer_transaction_commit(s->ptimer); } static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev, @@ -551,9 +551,10 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp) struct Stream *st = &s->streams[i]; st->nr = i; - st->bh = qemu_bh_new(timer_hit, st); - st->ptimer = ptimer_init_with_bh(st->bh, PTIMER_POLICY_DEFAULT); + st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT); + ptimer_transaction_begin(st->ptimer); ptimer_set_freq(st->ptimer, s->freqhz); + ptimer_transaction_commit(st->ptimer); } return;
Switch the xilinx_axidma 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/dma/xilinx_axidma.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) -- 2.20.1