On Mon, 20 Jul 2020 09:29:31 +0200 (CEST) [email protected] (Etienne Dublé) wrote:
> Hi Jason, > Thanks for testing and for the additional fix. > In my case (with dhcp followed by 3 TFTP transfers), the board booted fine 20 > times in a row with only my fix applied. Previously it was booting once in 10 > trials or so. > The driver resets tx_index in bcmgenet_gmac_eth_send() if it is higher than > 256, and the same occurs for rx_index in bcmgenet_gmac_free_pkt(), so I > suppose the issue you faced only occurs when the boundary case is reached > between two commands? > Anyway, thanks for the update. FWIW I also have an update. My Raspberry Pi 4 was consistently failing to boot from the network. Network packet capture was full of truncated, duplicate and otherwise abnormal packets. Most interestingly, many truncated packets had the same length as a packet transmitted exactly 256 frames before. Matthias has rebuilt U-Boot with these two patches added: - https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/ - https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/ With these changes, the boot process is rock-solid, and I'm not seeing any abnormal packets in the capture any longer. Therefore let me add my Tested-by: Petr Tesarik <[email protected]> Cheers, Petr T > Cheers > Etienne > > > > De: "Jason Wessel" <jason.wessel at windriver.com> > ?: "Etienne DUBLE" <etienne.duble at gmail.com>, "joe hershberger" > <joe.hershberger at ni.com> > Cc: u-boot at lists.denx.de, "ETIENNE DUBLE" <etienne.duble at imag.fr> > Envoy?: Jeudi 16 Juillet 2020 18:02:11 > Objet: Re: [PATCH] bcmgenet: fix DMA buffer management > > On 7/16/20 7:02 AM, Jason Wessel wrote: > > On 7/9/20 3:11 AM, etienne.duble at gmail.com wrote: > >> From: Etienne Dubl? <etienne.duble at imag.fr> > >> > >> This commit fixes a serious issue occuring when several network > >> commands are run on a raspberry pi 4 board: for instance a "dhcp" > >> command and then one or several "tftp" commands. In this case, > >> packet recv callbacks were called several times on the same packets, > >> and send function was failing most of the time. > >> > >> note: if the boot procedure is made of a single network > >> command, the issue is not visible. > >> > >> The issue is related to management of the packet ring buffers > >> (producer / consumer) and DMA. > >> Each time a packet is received, the ethernet device stores it > >> in the buffer and increments an index called RDMA_PROD_INDEX. > >> Each time the driver outputs a received packet, it increments > >> another index called RDMA_CONS_INDEX. > >> > >> Between each pair of network commands, as part of the driver > >> 'start' function, previous code tried to reset both RDMA_CONS_INDEX > >> and RDMA_PROD_INDEX to 0. But RDMA_PROD_INDEX cannot be written from > >> driver side, thus its value was actually not updated, and only > >> RDMA_CONS_INDEX was reset to 0. This was resulting in a major > >> synchronization issue between the driver and the device. Most > >> visible bahavior was that the driver seemed to receive again the > >> packets from the previous commands (e.g. DHCP response packets > >> "received" again when performing the first TFTP command). > >> > >> This fix consists in setting RDMA_CONS_INDEX to the same > >> value as RDMA_PROD_INDEX, when resetting the driver. > >> > >> The same kind of fix was needed on the TX side, and a few variables > >> had to be reset accordingly (c_index, tx_index, rx_index). > > > > > > While there is some kind of problem with the driver, because I too > > have observed a problem with multiple requests timing out or failing, > > this patch makes the problem much worse. I was only able to complete > > a single tftp request. > > > > In my case I am using a static IP address and serverip. > > > > Also your patch was missing the sign-off line. Please consider > > running your patches through scripts/checkpatch.pl. > > > > Cheers, > > Jason. > > > >> --- > >> drivers/net/bcmgenet.c | 15 +++++++-------- > >> 1 file changed, 7 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c > >> index 11b6148ab6..a4facfd63f 100644 > >> --- a/drivers/net/bcmgenet.c > >> +++ b/drivers/net/bcmgenet.c > >> @@ -378,8 +378,6 @@ static void rx_descs_init(struct bcmgenet_eth_priv > >> *priv) > >> u32 len_stat, i; > >> void *desc_base = priv->rx_desc_base; > >> > >> - priv->c_index = 0; > >> - > >> len_stat = (RX_BUF_LENGTH << DMA_BUFLENGTH_SHIFT) | DMA_OWN; > >> > >> for (i = 0; i < RX_DESCS; i++) { > >> @@ -403,8 +401,10 @@ static void rx_ring_init(struct bcmgenet_eth_priv > >> *priv) > >> writel(RX_DESCS * DMA_DESC_SIZE / 4 - 1, > >> priv->mac_reg + RDMA_RING_REG_BASE + DMA_END_ADDR); > >> > >> - writel(0x0, priv->mac_reg + RDMA_PROD_INDEX); > >> - writel(0x0, priv->mac_reg + RDMA_CONS_INDEX); > >> + /* cannot init RDMA_PROD_INDEX to 0, so align RDMA_CONS_INDEX on it > >> instead */ > >> + priv->c_index = readl(priv->mac_reg + RDMA_PROD_INDEX); > >> + writel(priv->c_index, priv->mac_reg + RDMA_CONS_INDEX); > >> + priv->rx_index = priv->c_index; > > > printf("before RX_IDX: 0x%x\n", priv->rx_index); > > I added a printf() like above for the RX and TX to see what is going on when > I try and transfer a kernel Image file the second time. > > > U-Boot> tftp ${loadaddr} bootfs/Image > before RX_IDX: 0x0 > before TX_IDX: 0x0 > Using ethernet at 7d580000 device > Filename 'bootfs/Image'. > Load address: 0x80000 > Loading: ## Warning: gatewayip needed but not set > ################################################## 16.8 MiB > 6.1 MiB/s > done > Bytes transferred = 17615360 (10cca00 hex) > U-Boot> tftp ${loadaddr} bootfs/Image > before RX_IDX: 0xe4 > before TX_IDX: 0x2ee3 > Using ethernet at 7d580000 device > Filename 'bootfs/Image'. > Load address: 0x80000 > Loading: ## Warning: gatewayip needed but not set > > > > The TX_IDX is now 0x2ee3 which is definitely not going to work. > > According to the driver file there are only 256 (0xFF) slots, > which is why it hangs, with your change. > > Jason. > > >> writel((RX_DESCS << DMA_RING_SIZE_SHIFT) | RX_BUF_LENGTH, > >> priv->mac_reg + RDMA_RING_REG_BASE + DMA_RING_BUF_SIZE); > >> writel(DMA_FC_THRESH_VALUE, priv->mac_reg + RDMA_XON_XOFF_THRESH); > >> @@ -421,8 +421,9 @@ static void tx_ring_init(struct bcmgenet_eth_priv > >> *priv) > >> writel(0x0, priv->mac_reg + TDMA_WRITE_PTR); > >> writel(TX_DESCS * DMA_DESC_SIZE / 4 - 1, > >> priv->mac_reg + TDMA_RING_REG_BASE + DMA_END_ADDR); > >> - writel(0x0, priv->mac_reg + TDMA_PROD_INDEX); > >> - writel(0x0, priv->mac_reg + TDMA_CONS_INDEX); > >> + /* cannot init TDMA_CONS_INDEX to 0, so align TDMA_PROD_INDEX on it > >> instead */ > >> + priv->tx_index = readl(priv->mac_reg + TDMA_CONS_INDEX); > >> + writel(priv->tx_index, priv->mac_reg + TDMA_PROD_INDEX); > >> writel(0x1, priv->mac_reg + TDMA_RING_REG_BASE + DMA_MBUF_DONE_THRESH); > >> writel(0x0, priv->mac_reg + TDMA_FLOW_PERIOD); > >> writel((TX_DESCS << DMA_RING_SIZE_SHIFT) | RX_BUF_LENGTH, > >> @@ -469,8 +470,6 @@ static int bcmgenet_gmac_eth_start(struct udevice > >> *dev) > >> > >> priv->tx_desc_base = priv->mac_reg + GENET_TX_OFF; > >> priv->rx_desc_base = priv->mac_reg + GENET_RX_OFF; > >> - priv->tx_index = 0x0; > >> - priv->rx_index = 0x0; > >> > >> bcmgenet_umac_reset(priv); > >> > >>
pgpi_DLrmGwTx.pgp
Description: Digitální podpis OpenPGP
