| Message ID | 20240726055933.817-5-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: > > When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA, > which may let AXI DMA to send RX full IRQs. It is aligned with real AXI > DMA/ethernet IP behavior in the FPGA. > > Also, now ethernet RX blocks the RX packets when it is disabled. It > should send and clear the remaining RX packets when enabling it. > > Signed-off-by: Jim Shu <jim.shu@sifive.com> > --- > hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++----------------- > 1 file changed, 42 insertions(+), 29 deletions(-) > > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c > index 05d41bd548..8428f10946 100644 > --- a/hw/net/xilinx_axienet.c > +++ b/hw/net/xilinx_axienet.c > @@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, unsigned size) > return r; > } > > +static void axienet_eth_rx_notify(void *opaque) > +{ > + XilinxAXIEnet *s = XILINX_AXI_ENET(opaque); > + > + /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ */ > + if (!axienet_rx_enabled(s)) { > + return; > + } This checks s->rcw[1] & RCW1_RX, and does nothing if it's not set... > static void enet_write(void *opaque, hwaddr addr, > uint64_t value, unsigned size) > { > @@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr, > } else { > qemu_flush_queued_packets(qemu_get_queue(s->nic)); > } > + > + /* > + * When RX is enabled, check if any remaining data in rxmem > + * and send them. > + */ > + if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) { > + axienet_eth_rx_notify(s); > + } ...but at this callsite we open-code a check on RCW1_RX and skip the call if it's not set. We don't need to check twice. > break; > > case R_TC: thanks -- PMM
Hi Peter, Thanks for the suggestion. axienet_eth_rx_notify() is also called by axidma_write() as a notify() callback, so we need to check RCW1_RX in the function. I think I could remove RCW1_RX checking in the enet_write() to avoid double checking. I will fix it in the v2 patchset. Thanks, JIm Shu. On Mon, Jul 29, 2024 at 11:31 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote: > > > > When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA, > > which may let AXI DMA to send RX full IRQs. It is aligned with real AXI > > DMA/ethernet IP behavior in the FPGA. > > > > Also, now ethernet RX blocks the RX packets when it is disabled. It > > should send and clear the remaining RX packets when enabling it. > > > > Signed-off-by: Jim Shu <jim.shu@sifive.com> > > --- > > hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++----------------- > > 1 file changed, 42 insertions(+), 29 deletions(-) > > > > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c > > index 05d41bd548..8428f10946 100644 > > --- a/hw/net/xilinx_axienet.c > > +++ b/hw/net/xilinx_axienet.c > > @@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, unsigned size) > > return r; > > } > > > > +static void axienet_eth_rx_notify(void *opaque) > > +{ > > + XilinxAXIEnet *s = XILINX_AXI_ENET(opaque); > > + > > + /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ */ > > + if (!axienet_rx_enabled(s)) { > > + return; > > + } > > This checks s->rcw[1] & RCW1_RX, and does nothing if it's not set... > > > static void enet_write(void *opaque, hwaddr addr, > > uint64_t value, unsigned size) > > { > > @@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr, > > } else { > > qemu_flush_queued_packets(qemu_get_queue(s->nic)); > > } > > + > > + /* > > + * When RX is enabled, check if any remaining data in rxmem > > + * and send them. > > + */ > > + if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) { > > + axienet_eth_rx_notify(s); > > + } > > ...but at this callsite we open-code a check on RCW1_RX and > skip the call if it's not set. We don't need to check twice. > > > break; > > > > case R_TC: > > thanks > -- PMM
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c index 05d41bd548..8428f10946 100644 --- a/hw/net/xilinx_axienet.c +++ b/hw/net/xilinx_axienet.c @@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, unsigned size) return r; } +static void axienet_eth_rx_notify(void *opaque) +{ + XilinxAXIEnet *s = XILINX_AXI_ENET(opaque); + + /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ */ + if (!axienet_rx_enabled(s)) { + return; + } + + while (s->rxappsize && stream_can_push(s->tx_control_dev, + axienet_eth_rx_notify, s)) { + size_t ret = stream_push(s->tx_control_dev, + (void *)s->rxapp + CONTROL_PAYLOAD_SIZE + - s->rxappsize, s->rxappsize, true); + s->rxappsize -= ret; + } + + while (s->rxsize && stream_can_push(s->tx_data_dev, + axienet_eth_rx_notify, s)) { + size_t ret = stream_push(s->tx_data_dev, (void *)s->rxmem + s->rxpos, + s->rxsize, true); + s->rxsize -= ret; + s->rxpos += ret; + if (!s->rxsize) { + s->regs[R_IS] |= IS_RX_COMPLETE; + if (s->need_flush) { + s->need_flush = false; + qemu_flush_queued_packets(qemu_get_queue(s->nic)); + } + } + } + enet_update_irq(s); +} + static void enet_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { @@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr, } else { qemu_flush_queued_packets(qemu_get_queue(s->nic)); } + + /* + * When RX is enabled, check if any remaining data in rxmem + * and send them. + */ + if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) { + axienet_eth_rx_notify(s); + } break; case R_TC: @@ -666,35 +708,6 @@ static int enet_match_addr(const uint8_t *buf, uint32_t f0, uint32_t f1) return match; } -static void axienet_eth_rx_notify(void *opaque) -{ - XilinxAXIEnet *s = XILINX_AXI_ENET(opaque); - - while (s->rxappsize && stream_can_push(s->tx_control_dev, - axienet_eth_rx_notify, s)) { - size_t ret = stream_push(s->tx_control_dev, - (void *)s->rxapp + CONTROL_PAYLOAD_SIZE - - s->rxappsize, s->rxappsize, true); - s->rxappsize -= ret; - } - - while (s->rxsize && stream_can_push(s->tx_data_dev, - axienet_eth_rx_notify, s)) { - size_t ret = stream_push(s->tx_data_dev, (void *)s->rxmem + s->rxpos, - s->rxsize, true); - s->rxsize -= ret; - s->rxpos += ret; - if (!s->rxsize) { - s->regs[R_IS] |= IS_RX_COMPLETE; - if (s->need_flush) { - s->need_flush = false; - qemu_flush_queued_packets(qemu_get_queue(s->nic)); - } - } - } - enet_update_irq(s); -} - static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size) { XilinxAXIEnet *s = qemu_get_nic_opaque(nc);
When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA, which may let AXI DMA to send RX full IRQs. It is aligned with real AXI DMA/ethernet IP behavior in the FPGA. Also, now ethernet RX blocks the RX packets when it is disabled. It should send and clear the remaining RX packets when enabling it. Signed-off-by: Jim Shu <jim.shu@sifive.com> --- hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 29 deletions(-)