Re: [PATCH 3/3] hw/dma/xilinx_axidma.c: Switch to transaction-based ptimer API

[Peter Maydell <peter.maydell@linaro.org>]

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