[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Marek Vasut]

The RX descriptor field 3 should contain only OWN and BUF1V bits before
being used for receiving data by the DMA engine. However, right now, if
the descriptor was already used for receiving data and is being cleared,
the field 3 is only modified and the aforementioned two bits are ORRed
into the field. This could lead to a residual dirty bits being left in
the field 3 from previous transfer, and it generally does. Fully set the
field 3 instead to clear those residual dirty bits.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/dwc_eth_qos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 0564bebf76..4bce6d4290 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1242,7 +1242,7 @@ static int eqos_start(struct udevice *dev)
 		struct eqos_desc *rx_desc = &(eqos->rx_descs[i]);
 		rx_desc->des0 = (u32)(ulong)(eqos->rx_dma_buf +
 					     (i * EQOS_MAX_PACKET_SIZE));
-		rx_desc->des3 |= EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
+		rx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
 	}
 	eqos->config->ops->eqos_flush_desc(eqos->descs);
 
@@ -1436,7 +1436,7 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
 	 * writes to the rest of the descriptor too.
 	 */
 	mb();
-	rx_desc->des3 |= EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
+	rx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
 	eqos->config->ops->eqos_flush_desc(rx_desc);
 
 	writel((ulong)rx_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
-- 
2.25.1

[PATCH 2/6] net: dwc_eth_qos: Correctly wrap around TX descriptor tail pointer

[Marek Vasut]

This code programs the next descriptor in the TX descriptor ring into
the hardware as the last valid TX descriptor. The problem is that if
the currenty descriptor is the last one in the array, the code will
not wrap around correctly and use TX descriptor 0 again, but instead
will use TX descriptor at address right past the TX descriptor ring,
which is the first descriptor in the RX ring.

Fix this by adding the necessary wrap-around.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/dwc_eth_qos.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 4bce6d4290..c4f665bda9 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1373,7 +1373,8 @@ static int eqos_send(struct udevice *dev, void *packet, int length)
 	tx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_FD | EQOS_DESC3_LD | length;
 	eqos->config->ops->eqos_flush_desc(tx_desc);
 
-	writel((ulong)(tx_desc + 1), &eqos->dma_regs->ch0_txdesc_tail_pointer);
+	writel((ulong)(&(eqos->tx_descs[eqos->tx_desc_idx])),
+		&eqos->dma_regs->ch0_txdesc_tail_pointer);
 
 	for (i = 0; i < 1000000; i++) {
 		eqos->config->ops->eqos_inval_desc(tx_desc);
-- 
2.25.1

[PATCH 3/6] net: dwc_eth_qos: Flush the RX descriptors on init

[Marek Vasut]

Currently the code only flushes the first RX descriptor, not every entry
in the RX descriptor ring. Fix this, to make sure the DMA engine can pick
the RX descriptors correctly.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/dwc_eth_qos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index c4f665bda9..66cc301c8c 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1243,8 +1243,8 @@ static int eqos_start(struct udevice *dev)
 		rx_desc->des0 = (u32)(ulong)(eqos->rx_dma_buf +
 					     (i * EQOS_MAX_PACKET_SIZE));
 		rx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
+		eqos->config->ops->eqos_flush_desc(rx_desc);
 	}
-	eqos->config->ops->eqos_flush_desc(eqos->descs);
 
 	writel(0, &eqos->dma_regs->ch0_txdesc_list_haddress);
 	writel((ulong)eqos->tx_descs, &eqos->dma_regs->ch0_txdesc_list_address);
-- 
2.25.1

[PATCH 4/6] net: dwc_eth_qos: Invalidate RX descriptor before reading

[Marek Vasut]

The current code polls the RX desciptor ring for new packets by reading
the RX descriptor status. This works by accident, as the RX descriptors
are often in non-cacheable memory. However, the driver does support use
of RX descriptors in cacheable memory.

This patch adds a missing RX descriptor invalidation, which assures the
CPU will read a fresh copy of the RX descriptor instead of a cached one.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/dwc_eth_qos.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 66cc301c8c..e40c461278 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1397,6 +1397,7 @@ static int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
 	debug("%s(dev=%p, flags=%x):\n", __func__, dev, flags);
 
 	rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
+	eqos->config->ops->eqos_inval_desc(rx_desc);
 	if (rx_desc->des3 & EQOS_DESC3_OWN) {
 		debug("%s: RX packet not available\n", __func__);
 		return -EAGAIN;
-- 
2.25.1

[PATCH 5/6] net: dwc_eth_qos: Invalidate RX packet DMA buffer

[Marek Vasut]

This patch prevents an issue where the RX packet might have been
accessed by the CPU, which now has cached data from the packet in
the caches and possibly various write buffers, and these data may
be evicted from the caches into the DRAM while the buffer is also
written by the DMA.

By invalidating the buffer after the CPU accessed it and before the
DMA populates the buffer, it is assured that the buffer will not be
corrupted.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/dwc_eth_qos.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index e40c461278..7dadb10fe7 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1430,6 +1430,9 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
 	}
 
 	rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
+
+	eqos->config->ops->eqos_inval_buffer(packet, length);
+
 	rx_desc->des0 = (u32)(ulong)packet;
 	rx_desc->des1 = 0;
 	rx_desc->des2 = 0;
@@ -1492,6 +1495,9 @@ static int eqos_probe_resources_core(struct udevice *dev)
 	}
 	debug("%s: rx_pkt=%p\n", __func__, eqos->rx_pkt);
 
+	eqos->config->ops->eqos_inval_buffer(eqos->rx_dma_buf,
+			EQOS_MAX_PACKET_SIZE * EQOS_DESCRIPTORS_RX);
+
 	debug("%s: OK\n", __func__);
 	return 0;
 
-- 
2.25.1

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Marek Vasut]

The DMA may attempt to write a DMA descriptor in the ring while it is
being updated. By writing the DMA descriptor buffer address to 0, it
is assured the DMA will not use such a buffer and the buffer can be
updated without any interference.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/dwc_eth_qos.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 7dadb10fe7..c86b9d59a5 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
 
 	rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
 
+	rx_desc->des0 = 0;
+	mb();
+	eqos->config->ops->eqos_flush_desc(rx_desc);
 	eqos->config->ops->eqos_inval_buffer(packet, length);
-
 	rx_desc->des0 = (u32)(ulong)packet;
 	rx_desc->des1 = 0;
 	rx_desc->des2 = 0;
-- 
2.25.1

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> The RX descriptor field 3 should contain only OWN and BUF1V bits before
> being used for receiving data by the DMA engine. However, right now, if
> the descriptor was already used for receiving data and is being cleared,
> the field 3 is only modified and the aforementioned two bits are ORRed
> into the field. This could lead to a residual dirty bits being left in
> the field 3 from previous transfer, and it generally does. Fully set the
> field 3 instead to clear those residual dirty bits.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 0564bebf76..4bce6d4290 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1242,7 +1242,7 @@ static int eqos_start(struct udevice *dev)
>                 struct eqos_desc *rx_desc = &(eqos->rx_descs[i]);
>                 rx_desc->des0 = (u32)(ulong)(eqos->rx_dma_buf +
>                                              (i * EQOS_MAX_PACKET_SIZE));
> -               rx_desc->des3 |= EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
> +               rx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
>         }
>         eqos->config->ops->eqos_flush_desc(eqos->descs);
>
> @@ -1436,7 +1436,7 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>          * writes to the rest of the descriptor too.
>          */
>         mb();
> -       rx_desc->des3 |= EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
> +       rx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
>         eqos->config->ops->eqos_flush_desc(rx_desc);
>
>         writel((ulong)rx_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 2/6] net: dwc_eth_qos: Correctly wrap around TX descriptor tail pointer

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> This code programs the next descriptor in the TX descriptor ring into
> the hardware as the last valid TX descriptor. The problem is that if
> the currenty descriptor is the last one in the array, the code will
> not wrap around correctly and use TX descriptor 0 again, but instead
> will use TX descriptor at address right past the TX descriptor ring,
> which is the first descriptor in the RX ring.
>
> Fix this by adding the necessary wrap-around.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 4bce6d4290..c4f665bda9 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1373,7 +1373,8 @@ static int eqos_send(struct udevice *dev, void *packet, int length)
>         tx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_FD | EQOS_DESC3_LD | length;
>         eqos->config->ops->eqos_flush_desc(tx_desc);
>
> -       writel((ulong)(tx_desc + 1), &eqos->dma_regs->ch0_txdesc_tail_pointer);
> +       writel((ulong)(&(eqos->tx_descs[eqos->tx_desc_idx])),
> +               &eqos->dma_regs->ch0_txdesc_tail_pointer);
>
>         for (i = 0; i < 1000000; i++) {
>                 eqos->config->ops->eqos_inval_desc(tx_desc);
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 3/6] net: dwc_eth_qos: Flush the RX descriptors on init

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> Currently the code only flushes the first RX descriptor, not every entry
> in the RX descriptor ring. Fix this, to make sure the DMA engine can pick
> the RX descriptors correctly.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index c4f665bda9..66cc301c8c 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1243,8 +1243,8 @@ static int eqos_start(struct udevice *dev)
>                 rx_desc->des0 = (u32)(ulong)(eqos->rx_dma_buf +
>                                              (i * EQOS_MAX_PACKET_SIZE));
>                 rx_desc->des3 = EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
> +               eqos->config->ops->eqos_flush_desc(rx_desc);
>         }
> -       eqos->config->ops->eqos_flush_desc(eqos->descs);
>
>         writel(0, &eqos->dma_regs->ch0_txdesc_list_haddress);
>         writel((ulong)eqos->tx_descs, &eqos->dma_regs->ch0_txdesc_list_address);
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 5/6] net: dwc_eth_qos: Invalidate RX packet DMA buffer

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> This patch prevents an issue where the RX packet might have been
> accessed by the CPU, which now has cached data from the packet in
> the caches and possibly various write buffers, and these data may
> be evicted from the caches into the DRAM while the buffer is also
> written by the DMA.
>
> By invalidating the buffer after the CPU accessed it and before the
> DMA populates the buffer, it is assured that the buffer will not be
> corrupted.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index e40c461278..7dadb10fe7 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1430,6 +1430,9 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>         }
>
>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
> +
> +       eqos->config->ops->eqos_inval_buffer(packet, length);
> +
>         rx_desc->des0 = (u32)(ulong)packet;
>         rx_desc->des1 = 0;
>         rx_desc->des2 = 0;
> @@ -1492,6 +1495,9 @@ static int eqos_probe_resources_core(struct udevice *dev)
>         }
>         debug("%s: rx_pkt=%p\n", __func__, eqos->rx_pkt);
>
> +       eqos->config->ops->eqos_inval_buffer(eqos->rx_dma_buf,
> +                       EQOS_MAX_PACKET_SIZE * EQOS_DESCRIPTORS_RX);
> +
>         debug("%s: OK\n", __func__);
>         return 0;
>
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 4/6] net: dwc_eth_qos: Invalidate RX descriptor before reading

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> The current code polls the RX desciptor ring for new packets by reading
> the RX descriptor status. This works by accident, as the RX descriptors
> are often in non-cacheable memory. However, the driver does support use
> of RX descriptors in cacheable memory.
>
> This patch adds a missing RX descriptor invalidation, which assures the
> CPU will read a fresh copy of the RX descriptor instead of a cached one.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 66cc301c8c..e40c461278 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1397,6 +1397,7 @@ static int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
>         debug("%s(dev=%p, flags=%x):\n", __func__, dev, flags);
>
>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
> +       eqos->config->ops->eqos_inval_desc(rx_desc);
>         if (rx_desc->des3 & EQOS_DESC3_OWN) {
>                 debug("%s: RX packet not available\n", __func__);
>                 return -EAGAIN;
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> The DMA may attempt to write a DMA descriptor in the ring while it is
> being updated. By writing the DMA descriptor buffer address to 0, it
> is assured the DMA will not use such a buffer and the buffer can be
> updated without any interference.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 7dadb10fe7..c86b9d59a5 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>
>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
>
> +       rx_desc->des0 = 0;
> +       mb();
Better use wmb() for better understanding your goal here ?
> +       eqos->config->ops->eqos_flush_desc(rx_desc);
>         eqos->config->ops->eqos_inval_buffer(packet, length);
> -
>         rx_desc->des0 = (u32)(ulong)packet;
>         rx_desc->des1 = 0;
>         rx_desc->des2 = 0;
> --
> 2.25.1
>

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Marek Vasut]

On 3/23/20 8:09 AM, Ramon Fried wrote:
> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>>
>> The DMA may attempt to write a DMA descriptor in the ring while it is
>> being updated. By writing the DMA descriptor buffer address to 0, it
>> is assured the DMA will not use such a buffer and the buffer can be
>> updated without any interference.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Patrice Chotard <patrice.chotard@st.com>
>> Cc: Patrick Delaunay <patrick.delaunay@st.com>
>> Cc: Ramon Fried <rfried.dev@gmail.com>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> ---
>>  drivers/net/dwc_eth_qos.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>> index 7dadb10fe7..c86b9d59a5 100644
>> --- a/drivers/net/dwc_eth_qos.c
>> +++ b/drivers/net/dwc_eth_qos.c
>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>>
>>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
>>
>> +       rx_desc->des0 = 0;
>> +       mb();
> Better use wmb() for better understanding your goal here ?

The driver uses mb() all over the place, so I'm just keeping it
consistent here.

But I wonder whether we even need all those barriers in this driver at all.

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Ramon Fried]

On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex@denx.de> wrote:
>
> On 3/23/20 8:09 AM, Ramon Fried wrote:
> > On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> The DMA may attempt to write a DMA descriptor in the ring while it is
> >> being updated. By writing the DMA descriptor buffer address to 0, it
> >> is assured the DMA will not use such a buffer and the buffer can be
> >> updated without any interference.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Joe Hershberger <joe.hershberger@ni.com>
> >> Cc: Patrice Chotard <patrice.chotard@st.com>
> >> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> >> Cc: Ramon Fried <rfried.dev@gmail.com>
> >> Cc: Stephen Warren <swarren@nvidia.com>
> >> ---
> >>  drivers/net/dwc_eth_qos.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> >> index 7dadb10fe7..c86b9d59a5 100644
> >> --- a/drivers/net/dwc_eth_qos.c
> >> +++ b/drivers/net/dwc_eth_qos.c
> >> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
> >>
> >>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
> >>
> >> +       rx_desc->des0 = 0;
> >> +       mb();
> > Better use wmb() for better understanding your goal here ?
>
> The driver uses mb() all over the place, so I'm just keeping it
> consistent here.
>
> But I wonder whether we even need all those barriers in this driver at all.
In that case that's fine :)

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Ramon Fried]

On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> The DMA may attempt to write a DMA descriptor in the ring while it is
> being updated. By writing the DMA descriptor buffer address to 0, it
> is assured the DMA will not use such a buffer and the buffer can be
> updated without any interference.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 7dadb10fe7..c86b9d59a5 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>
>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
>
> +       rx_desc->des0 = 0;
> +       mb();
> +       eqos->config->ops->eqos_flush_desc(rx_desc);
>         eqos->config->ops->eqos_inval_buffer(packet, length);
> -
>         rx_desc->des0 = (u32)(ulong)packet;
>         rx_desc->des1 = 0;
>         rx_desc->des2 = 0;
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Marek Vasut]

On 3/23/20 1:19 PM, Ramon Fried wrote:
> On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/23/20 8:09 AM, Ramon Fried wrote:
>>> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The DMA may attempt to write a DMA descriptor in the ring while it is
>>>> being updated. By writing the DMA descriptor buffer address to 0, it
>>>> is assured the DMA will not use such a buffer and the buffer can be
>>>> updated without any interference.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>> Cc: Patrice Chotard <patrice.chotard@st.com>
>>>> Cc: Patrick Delaunay <patrick.delaunay@st.com>
>>>> Cc: Ramon Fried <rfried.dev@gmail.com>
>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>> ---
>>>>  drivers/net/dwc_eth_qos.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>>> index 7dadb10fe7..c86b9d59a5 100644
>>>> --- a/drivers/net/dwc_eth_qos.c
>>>> +++ b/drivers/net/dwc_eth_qos.c
>>>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>>>>
>>>>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
>>>>
>>>> +       rx_desc->des0 = 0;
>>>> +       mb();
>>> Better use wmb() for better understanding your goal here ?
>>
>> The driver uses mb() all over the place, so I'm just keeping it
>> consistent here.
>>
>> But I wonder whether we even need all those barriers in this driver at all.
> In that case that's fine :)

I mean, I am asking about the need for those barriers here, that's my
question here.

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Ramon Fried]

On Mon, Mar 23, 2020 at 2:22 PM Marek Vasut <marex@denx.de> wrote:
>
> On 3/23/20 1:19 PM, Ramon Fried wrote:
> > On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/23/20 8:09 AM, Ramon Fried wrote:
> >>> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> The DMA may attempt to write a DMA descriptor in the ring while it is
> >>>> being updated. By writing the DMA descriptor buffer address to 0, it
> >>>> is assured the DMA will not use such a buffer and the buffer can be
> >>>> updated without any interference.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
> >>>> Cc: Patrice Chotard <patrice.chotard@st.com>
> >>>> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> >>>> Cc: Ramon Fried <rfried.dev@gmail.com>
> >>>> Cc: Stephen Warren <swarren@nvidia.com>
> >>>> ---
> >>>>  drivers/net/dwc_eth_qos.c | 4 +++-
> >>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> >>>> index 7dadb10fe7..c86b9d59a5 100644
> >>>> --- a/drivers/net/dwc_eth_qos.c
> >>>> +++ b/drivers/net/dwc_eth_qos.c
> >>>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
> >>>>
> >>>>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
> >>>>
> >>>> +       rx_desc->des0 = 0;
> >>>> +       mb();
> >>> Better use wmb() for better understanding your goal here ?
> >>
> >> The driver uses mb() all over the place, so I'm just keeping it
> >> consistent here.
> >>
> >> But I wonder whether we even need all those barriers in this driver at all.
> > In that case that's fine :)
>
> I mean, I am asking about the need for those barriers here, that's my
> question here.
Better safe than sorry. unless you have the means to test it to see if
it doesn't break.

Thanks,
Ramon.

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Marek Vasut]

On 3/23/20 1:50 PM, Ramon Fried wrote:
> On Mon, Mar 23, 2020 at 2:22 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/23/20 1:19 PM, Ramon Fried wrote:
>>> On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/23/20 8:09 AM, Ramon Fried wrote:
>>>>> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> The DMA may attempt to write a DMA descriptor in the ring while it is
>>>>>> being updated. By writing the DMA descriptor buffer address to 0, it
>>>>>> is assured the DMA will not use such a buffer and the buffer can be
>>>>>> updated without any interference.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>>>> Cc: Patrice Chotard <patrice.chotard@st.com>
>>>>>> Cc: Patrick Delaunay <patrick.delaunay@st.com>
>>>>>> Cc: Ramon Fried <rfried.dev@gmail.com>
>>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>>> ---
>>>>>>  drivers/net/dwc_eth_qos.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>>>>> index 7dadb10fe7..c86b9d59a5 100644
>>>>>> --- a/drivers/net/dwc_eth_qos.c
>>>>>> +++ b/drivers/net/dwc_eth_qos.c
>>>>>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
>>>>>>
>>>>>>         rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]);
>>>>>>
>>>>>> +       rx_desc->des0 = 0;
>>>>>> +       mb();
>>>>> Better use wmb() for better understanding your goal here ?
>>>>
>>>> The driver uses mb() all over the place, so I'm just keeping it
>>>> consistent here.
>>>>
>>>> But I wonder whether we even need all those barriers in this driver at all.
>>> In that case that's fine :)
>>
>> I mean, I am asking about the need for those barriers here, that's my
>> question here.
> Better safe than sorry. unless you have the means to test it to see if
> it doesn't break.

On STM32MP1, yes. Not on the Tegra platform.

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Stephen Warren]

On 3/22/20 7:45 PM, Marek Vasut wrote:
> The RX descriptor field 3 should contain only OWN and BUF1V bits before
> being used for receiving data by the DMA engine. However, right now, if
> the descriptor was already used for receiving data and is being cleared,
> the field 3 is only modified and the aforementioned two bits are ORRed
> into the field. This could lead to a residual dirty bits being left in
> the field 3 from previous transfer, and it generally does. Fully set the
> field 3 instead to clear those residual dirty bits.

The series,
Tested-by: Stephen Warren <swarren@nvidia.com>
(On Jetson, using test/py tests)

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> 
> The RX descriptor field 3 should contain only OWN and BUF1V bits before being
> used for receiving data by the DMA engine. However, right now, if the descriptor
> was already used for receiving data and is being cleared, the field 3 is only
> modified and the aforementioned two bits are ORRed into the field. This could lead
> to a residual dirty bits being left in the field 3 from previous transfer, and it
> generally does. Fully set the field 3 instead to clear those residual dirty bits.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/dwc_eth_qos.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index
> 0564bebf76..4bce6d4290 100644
> --- a/drivers/net/dwc_eth_qos.c

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>

Thanks

Patrick

[PATCH 2/6] net: dwc_eth_qos: Correctly wrap around TX descriptor tail pointer

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> 
> This code programs the next descriptor in the TX descriptor ring into the hardware
> as the last valid TX descriptor. The problem is that if the currenty descriptor is the
> last one in the array, the code will not wrap around correctly and use TX
> descriptor 0 again, but instead will use TX descriptor at address right past the TX
> descriptor ring, which is the first descriptor in the RX ring.
> 
> Fix this by adding the necessary wrap-around.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>

Thanks

Patrick

[PATCH 3/6] net: dwc_eth_qos: Flush the RX descriptors on init

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> To: u-boot at lists.denx.de
> Cc: Marek Vasut <marex@denx.de>; Joe Hershberger
> <joe.hershberger@ni.com>; Patrice CHOTARD <patrice.chotard@st.com>;
> Patrick DELAUNAY <patrick.delaunay@st.com>; Ramon Fried
> <rfried.dev@gmail.com>; Stephen Warren <swarren@nvidia.com>
> Subject: [PATCH 3/6] net: dwc_eth_qos: Flush the RX descriptors on init
> Importance: High
> 
> Currently the code only flushes the first RX descriptor, not every entry in the RX
> descriptor ring. Fix this, to make sure the DMA engine can pick the RX
> descriptors correctly.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>

Thanks

Patrick

[PATCH 4/6] net: dwc_eth_qos: Invalidate RX descriptor before reading

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> 
> The current code polls the RX desciptor ring for new packets by reading the RX
> descriptor status. This works by accident, as the RX descriptors are often in non-
> cacheable memory. However, the driver does support use of RX descriptors in
> cacheable memory.
> 
> This patch adds a missing RX descriptor invalidation, which assures the CPU will
> read a fresh copy of the RX descriptor instead of a cached one.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>

Patrick

[PATCH 5/6] net: dwc_eth_qos: Invalidate RX packet DMA buffer

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> 
> This patch prevents an issue where the RX packet might have been accessed by
> the CPU, which now has cached data from the packet in the caches and possibly
> various write buffers, and these data may be evicted from the caches into the
> DRAM while the buffer is also written by the DMA.
> 
> By invalidating the buffer after the CPU accessed it and before the DMA populates
> the buffer, it is assured that the buffer will not be corrupted.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>

Patrick

[PATCH 6/6] net: dwc_eth_qos: Prevent DMA from writing updated RX DMA descriptor

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> 
> The DMA may attempt to write a DMA descriptor in the ring while it is being
> updated. By writing the DMA descriptor buffer address to 0, it is assured the DMA
> will not use such a buffer and the buffer can be updated without any interference.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---

Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 23 mars 2020 02:45
> 
> The RX descriptor field 3 should contain only OWN and BUF1V bits before being
> used for receiving data by the DMA engine. However, right now, if the descriptor
> was already used for receiving data and is being cleared, the field 3 is only
> modified and the aforementioned two bits are ORRed into the field. This could lead
> to a residual dirty bits being left in the field 3 from previous transfer, and it
> generally does. Fully set the field 3 instead to clear those residual dirty bits.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---

For the series,
Tested-by: Patrick Delaunay <patrick.delaunay@st.com> (on STM32MP15C-DK2, manual test: dhcp and run bootcmd_pxe)

Regards

Patrick

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Patrick DELAUNAY]

Dear Marek,

> From: Patrick DELAUNAY
> Sent: lundi 6 avril 2020 17:11
> 
> Dear Marek,
> 
> > From: Marek Vasut <marex@denx.de>
> > Sent: lundi 23 mars 2020 02:45
> >
> > The RX descriptor field 3 should contain only OWN and BUF1V bits
> > before being used for receiving data by the DMA engine. However, right
> > now, if the descriptor was already used for receiving data and is
> > being cleared, the field 3 is only modified and the aforementioned two
> > bits are ORRed into the field. This could lead to a residual dirty
> > bits being left in the field 3 from previous transfer, and it generally does. Fully
> set the field 3 instead to clear those residual dirty bits.
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Joe Hershberger <joe.hershberger@ni.com>
> > Cc: Patrice Chotard <patrice.chotard@st.com>
> > Cc: Patrick Delaunay <patrick.delaunay@st.com>
> > Cc: Ramon Fried <rfried.dev@gmail.com>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > ---
> 
> For the series,
> Tested-by: Patrick Delaunay <patrick.delaunay@st.com> (on STM32MP15C-DK2,
> manual test: dhcp and run bootcmd_pxe)

To complete my test and to check the cache management in the driver,

I test the sequence (CONFIG_SYS_NONCACHED_MEMORY is activated):

1) ping with dcache ON: Always OK

STM32MP> dhcp
STM32MP> ping 91.162.57.93

2) ping with dcache OFF : Always OK

STM32MP> dcache off 
STM32MP> ping 91.162.57.93 

3) ping with dcache ON : Failed 

STM32MP> dcache on 
STM32MP> ping 91.162.57.93

Failed with "eqos_send: TX timeout"
and after "eqos_recv: RX packet not available"

4) ping with dcache OFF : Always OK

STM32MP> dcache on 
STM32MP> ping 91.162.57.93


I don't sure this sequence is really valid 
or if this issue show a problem in cache management.

As I don't see any obvious issue in eqos_send,
do you idea on reason of this issue ?

> Regards
> 
> Patrick

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Marek Vasut]

On 4/7/20 11:49 AM, Patrick DELAUNAY wrote:
> Dear Marek,

Hi,

> To complete my test and to check the cache management in the driver,
> 
> I test the sequence (CONFIG_SYS_NONCACHED_MEMORY is activated):
> 
> 1) ping with dcache ON: Always OK
> 
> STM32MP> dhcp
> STM32MP> ping 91.162.57.93
> 
> 2) ping with dcache OFF : Always OK
> 
> STM32MP> dcache off 
> STM32MP> ping 91.162.57.93 
> 
> 3) ping with dcache ON : Failed 
> 
> STM32MP> dcache on 
> STM32MP> ping 91.162.57.93
> 
> Failed with "eqos_send: TX timeout"
> and after "eqos_recv: RX packet not available"
> 
> 4) ping with dcache OFF : Always OK
> 
> STM32MP> dcache on 
> STM32MP> ping 91.162.57.93
> 
> 
> I don't sure this sequence is really valid 
> or if this issue show a problem in cache management.
> 
> As I don't see any obvious issue in eqos_send,
> do you idea on reason of this issue ?

So the test is basically:

dhcp
while true ; do
 ping 192.168.1.300 ;
 dcache off ;
 ping 192.168.1.300 ;
 dcache on ;
done

right ?

I can tell you what you're likely gonna see if you debug it further.

Use the board on an isolated network segment (get some USB ethernet or
whatever), disable dhcp server, avahi and all other such stuff. Then run
wireshark on that interface.

Then, patch the dwmac driver to dump every packet it sends and receives,
the first 16 or so bytes are enough, so you'd see the header. Then run
your test.

After a while, you will see that the DWMAC transmits some valid packet,
but receives a packet with the exact same source MAC it just transmitted
, the source MAC of the stm32 board. But if I recall it correctly, you
won't see that packet on the line in wireshark, it just somehow loops
back in.

You can even accelerate that process by using arping from the host PC,
just arping the stm32 and the error will happen faster.

I was hoping I managed to fix all of these problems in this series, but
apparently there is more :-( Do you feel like debugging this further?

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Patrick DELAUNAY]

Dear,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 7 avril 2020 12:04
> 
> On 4/7/20 11:49 AM, Patrick DELAUNAY wrote:
> > Dear Marek,
> 
> Hi,
> 
> > To complete my test and to check the cache management in the driver,
> >
> > I test the sequence (CONFIG_SYS_NONCACHED_MEMORY is activated):
> >
> > 1) ping with dcache ON: Always OK
> >
> > STM32MP> dhcp
> > STM32MP> ping 91.162.57.93
> >
> > 2) ping with dcache OFF : Always OK
> >
> > STM32MP> dcache off
> > STM32MP> ping 91.162.57.93
> >
> > 3) ping with dcache ON : Failed
> >
> > STM32MP> dcache on
> > STM32MP> ping 91.162.57.93
> >
> > Failed with "eqos_send: TX timeout"
> > and after "eqos_recv: RX packet not available"
> >
> > 4) ping with dcache OFF : Always OK
> >
> > STM32MP> dcache on
> > STM32MP> ping 91.162.57.93
> >
> >
> > I don't sure this sequence is really valid or if this issue show a
> > problem in cache management.
> >
> > As I don't see any obvious issue in eqos_send, do you idea on reason
> > of this issue ?
> 
> So the test is basically:
> 
> dhcp
> while true ; do
>  ping 192.168.1.300 ;
>  dcache off ;
>  ping 192.168.1.300 ;
>  dcache on ;
> done
> 
> right ?

Yes, exactly and it is more clear

> 
> I can tell you what you're likely gonna see if you debug it further.
> 
> Use the board on an isolated network segment (get some USB ethernet or
> whatever), disable dhcp server, avahi and all other such stuff. Then run wireshark
> on that interface.
> 
> Then, patch the dwmac driver to dump every packet it sends and receives, the
> first 16 or so bytes are enough, so you'd see the header. Then run your test.
> 
> After a while, you will see that the DWMAC transmits some valid packet, but
> receives a packet with the exact same source MAC it just transmitted , the source
> MAC of the stm32 board. But if I recall it correctly, you won't see that packet on
> the line in wireshark, it just somehow loops back in.
> 
> You can even accelerate that process by using arping from the host PC, just
> arping the stm32 and the error will happen faster.
> 
> I was hoping I managed to fix all of these problems in this series, but apparently
> there is more :-( Do you feel like debugging this further?

Yes it is my fear.

Thanks for advices...
I will try to debug it
but my bandwith is limited for the next week, so I can't give a guarantee.

Regards

Patrick

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Marek Vasut]

On 3/23/20 2:45 AM, Marek Vasut wrote:
> The RX descriptor field 3 should contain only OWN and BUF1V bits before
> being used for receiving data by the DMA engine. However, right now, if
> the descriptor was already used for receiving data and is being cleared,
> the field 3 is only modified and the aforementioned two bits are ORRed
> into the field. This could lead to a residual dirty bits being left in
> the field 3 from previous transfer, and it generally does. Fully set the
> field 3 instead to clear those residual dirty bits.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>

Joe, this was posted for a month, can this go in ?

[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

[Marek Vasut]

On 4/7/20 3:19 PM, Patrick DELAUNAY wrote:
> Dear,
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: mardi 7 avril 2020 12:04
>>
>> On 4/7/20 11:49 AM, Patrick DELAUNAY wrote:
>>> Dear Marek,
>>
>> Hi,
>>
>>> To complete my test and to check the cache management in the driver,
>>>
>>> I test the sequence (CONFIG_SYS_NONCACHED_MEMORY is activated):
>>>
>>> 1) ping with dcache ON: Always OK
>>>
>>> STM32MP> dhcp
>>> STM32MP> ping 91.162.57.93
>>>
>>> 2) ping with dcache OFF : Always OK
>>>
>>> STM32MP> dcache off
>>> STM32MP> ping 91.162.57.93
>>>
>>> 3) ping with dcache ON : Failed
>>>
>>> STM32MP> dcache on
>>> STM32MP> ping 91.162.57.93
>>>
>>> Failed with "eqos_send: TX timeout"
>>> and after "eqos_recv: RX packet not available"
>>>
>>> 4) ping with dcache OFF : Always OK
>>>
>>> STM32MP> dcache on
>>> STM32MP> ping 91.162.57.93
>>>
>>>
>>> I don't sure this sequence is really valid or if this issue show a
>>> problem in cache management.
>>>
>>> As I don't see any obvious issue in eqos_send, do you idea on reason
>>> of this issue ?
>>
>> So the test is basically:
>>
>> dhcp
>> while true ; do
>>  ping 192.168.1.300 ;
>>  dcache off ;
>>  ping 192.168.1.300 ;
>>  dcache on ;
>> done
>>
>> right ?
> 
> Yes, exactly and it is more clear
> 
>>
>> I can tell you what you're likely gonna see if you debug it further.
>>
>> Use the board on an isolated network segment (get some USB ethernet or
>> whatever), disable dhcp server, avahi and all other such stuff. Then run wireshark
>> on that interface.
>>
>> Then, patch the dwmac driver to dump every packet it sends and receives, the
>> first 16 or so bytes are enough, so you'd see the header. Then run your test.
>>
>> After a while, you will see that the DWMAC transmits some valid packet, but
>> receives a packet with the exact same source MAC it just transmitted , the source
>> MAC of the stm32 board. But if I recall it correctly, you won't see that packet on
>> the line in wireshark, it just somehow loops back in.
>>
>> You can even accelerate that process by using arping from the host PC, just
>> arping the stm32 and the error will happen faster.
>>
>> I was hoping I managed to fix all of these problems in this series, but apparently
>> there is more :-( Do you feel like debugging this further?
> 
> Yes it is my fear.
> 
> Thanks for advices...
> I will try to debug it
> but my bandwith is limited for the next week, so I can't give a guarantee.

I sent a patch to pad the descriptors to cacheline size just now,
however I still didn't get Joe to review/apply any of the networking
patches. I hope he's just busy with other stuff ...