diff mbox series

[3/4] hw/dma: xilinx_axidma: Reset qemu_irq when DMA/Stream is reset

Message ID 20240726055933.817-4-jim.shu@sifive.com
State New
Headers show
Series [1/4] hw/dma: xilinx_axidma: Correct the txlen value in the descriptor | expand

Commit Message

Jim Shu July 26, 2024, 5:59 a.m. UTC 
Current DMA/Stream reset will clear interrupt pending bit of DMA device.
The qemu_irq of device should be updated at the same time.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/dma/xilinx_axidma.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Peter Maydell July 29, 2024, 3:23 p.m. UTC | #1 
On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
>
> Current DMA/Stream reset will clear interrupt pending bit of DMA device.
> The qemu_irq of device should be updated at the same time.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>  hw/dma/xilinx_axidma.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 728246f925..beb3fbf1d5 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -177,11 +177,24 @@ static inline int stream_halted(struct Stream *s)
>      return !!(s->regs[R_DMASR] & DMASR_HALTED);
>  }
>
> +static void stream_update_irq(struct Stream *s)
> +{
> +    unsigned int pending, mask, irq;
> +
> +    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
> +    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
> +
> +    irq = pending & mask;
> +
> +    qemu_set_irq(s->irq, !!irq);
> +}
> +
>  static void stream_reset(struct Stream *s)
>  {
>      s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
>      s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
>      s->sof = true;
> +    stream_update_irq(s); /* Clear interrupt */
>  }

The general rule of thumb in QEMU is not to call
qemu_set_irq() from a DeviceState::reset function,
and xilinx_axidma_reset calls stream_reset. I think
probably the best thing is to separate out whether
this is a DeviceState::reset or a software-triggered
reset, and only call qemu_set_irq() in the latter case.

thanks
-- PMM
Jim Shu Aug. 1, 2024, 1:55 p.m. UTC | #2 
Hi Peter,

Except DeviceState::reset(), stream_reset() is only used in
axidma_write() and axidma_write() has qemu_set_irq() at the end of
function.
I think this commit could be dropped. I will remove it in the v2 patchset.


Thanks,
Jim Shu



On Mon, Jul 29, 2024 at 11:23 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
> >
> > Current DMA/Stream reset will clear interrupt pending bit of DMA device.
> > The qemu_irq of device should be updated at the same time.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  hw/dma/xilinx_axidma.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index 728246f925..beb3fbf1d5 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -177,11 +177,24 @@ static inline int stream_halted(struct Stream *s)
> >      return !!(s->regs[R_DMASR] & DMASR_HALTED);
> >  }
> >
> > +static void stream_update_irq(struct Stream *s)
> > +{
> > +    unsigned int pending, mask, irq;
> > +
> > +    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
> > +    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
> > +
> > +    irq = pending & mask;
> > +
> > +    qemu_set_irq(s->irq, !!irq);
> > +}
> > +
> >  static void stream_reset(struct Stream *s)
> >  {
> >      s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
> >      s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
> >      s->sof = true;
> > +    stream_update_irq(s); /* Clear interrupt */
> >  }
>
> The general rule of thumb in QEMU is not to call
> qemu_set_irq() from a DeviceState::reset function,
> and xilinx_axidma_reset calls stream_reset. I think
> probably the best thing is to separate out whether
> this is a DeviceState::reset or a software-triggered
> reset, and only call qemu_set_irq() in the latter case.
>
> thanks
> -- PMM
diff mbox series

Patch

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 728246f925..beb3fbf1d5 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -177,11 +177,24 @@  static inline int stream_halted(struct Stream *s)
     return !!(s->regs[R_DMASR] & DMASR_HALTED);
 }
 
+static void stream_update_irq(struct Stream *s)
+{
+    unsigned int pending, mask, irq;
+
+    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
+    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
+
+    irq = pending & mask;
+
+    qemu_set_irq(s->irq, !!irq);
+}
+
 static void stream_reset(struct Stream *s)
 {
     s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
     s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
     s->sof = true;
+    stream_update_irq(s); /* Clear interrupt */
 }
 
 /* Map an offset addr into a channel index.  */
@@ -249,18 +262,6 @@  static MemTxResult stream_desc_store(struct Stream *s, hwaddr addr)
     return result;
 }
 
-static void stream_update_irq(struct Stream *s)
-{
-    unsigned int pending, mask, irq;
-
-    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
-    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
-
-    irq = pending & mask;
-
-    qemu_set_irq(s->irq, !!irq);
-}
-
 static void stream_reload_complete_cnt(struct Stream *s)
 {
     unsigned int comp_th;