| Message ID | 20240726055933.817-4-jim.shu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Several fixes of AXI-ethernet/DMA | expand |
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
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 --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;
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(-)