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

[Jim Shu]

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