From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wessel Date: Thu, 16 Jul 2020 21:45:12 -0500 Subject: [PATCH] bcmgenet: fix DMA buffer management In-Reply-To: References: <20200709081142.30861-1-etienne.duble@gmail.com> <91e35f25-e73d-ee8a-fd06-ac5b7c17b1f3@windriver.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 7/16/20 11:02 AM, Jason Wessel wrote: > > > 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? >>> >>> 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. > diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c index a4facfd63f..1b7e7ba2bf 100644 --- a/drivers/net/bcmgenet.c +++ b/drivers/net/bcmgenet.c @@ -405,6 +405,7 @@ static void rx_ring_init(struct bcmgenet_eth_priv *priv) 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; + priv->rx_index &= 0xFF; 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); @@ -424,6 +425,7 @@ static void tx_ring_init(struct bcmgenet_eth_priv *priv) /* 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); + priv->tx_index &= 0xFF; 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, After some testing it turns the lower 8 bits of the indexes will always match up with the DMA buffer index. If you also apply the patch above the ethernet becomes reliable. 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); >>> >>>