[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Stefan Theil]

The cache was only flushed before *transmitting* packets, but not
when receiving them, leading to an issue where new packets were
handed to the receive handler with old contents in cache. This
only happens when a lot of packets are received without sending
packages every now and then.

Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
---
 drivers/net/zynq_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
index 9bd79b198a..5825b6dd99 100644
--- a/drivers/net/zynq_gem.c
+++ b/drivers/net/zynq_gem.c
@@ -621,6 +621,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
 
 	*packetp = (uchar *)(uintptr_t)addr;
 
+	flush_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
+	barrier();
+
 	return frame_len;
 }
 
-- 
2.17.1

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Bin Meng]

On Thu, Dec 13, 2018 at 6:18 PM Stefan Theil <stefan.theil@mixed-mode.de> wrote:
>
> The cache was only flushed before *transmitting* packets, but not
> when receiving them, leading to an issue where new packets were
> handed to the receive handler with old contents in cache. This
> only happens when a lot of packets are received without sending
> packages every now and then.
>
> Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
> ---
>  drivers/net/zynq_gem.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
> index 9bd79b198a..5825b6dd99 100644
> --- a/drivers/net/zynq_gem.c
> +++ b/drivers/net/zynq_gem.c
> @@ -621,6 +621,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
>
>         *packetp = (uchar *)(uintptr_t)addr;
>
> +       flush_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));

Shouldn't it be invalidate_dcache_range()?

> +       barrier();
> +
>         return frame_len;
>  }
>

Regards,
Bin

[U-Boot] [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Stefan Theil]

The cache was only flushed before *transmitting* packets, but not
when receiving them, leading to an issue where new packets were
handed to the receive handler with old contents in cache. This
only happens when a lot of packets are received without sending
packages every now and then. Also flushing the receive buffers
in the transmit function makes no sense and can be removed.

Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>

---
Changes for v2:
	- Use invalidate_dcache_range instead of
	  flush_dcache_range
Changes for v3:
	- Remove unnecessary flushing of all RX
	  buffers in zynq_gem_send
Changes for v4:
	- Invalidate receive buffers after allocating
	  them in zynq_gem_probe
---
 drivers/net/zynq_gem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
index 9bd79b198a..79a22fb1ed 100644
--- a/drivers/net/zynq_gem.c
+++ b/drivers/net/zynq_gem.c
@@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev, void *ptr, int len)
 	addr &= ~(ARCH_DMA_MINALIGN - 1);
 	size = roundup(len, ARCH_DMA_MINALIGN);
 	flush_dcache_range(addr, addr + size);
-
-	addr = (ulong)priv->rxbuffers;
-	addr &= ~(ARCH_DMA_MINALIGN - 1);
-	size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
-	flush_dcache_range(addr, addr + size);
 	barrier();
 
 	/* Start transmit */
@@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
 
 	*packetp = (uchar *)(uintptr_t)addr;
 
+	invalidate_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
+	barrier();
+
 	return frame_len;
 }
 
@@ -705,7 +703,9 @@ static int zynq_gem_probe(struct udevice *dev)
 	if (!priv->rxbuffers)
 		return -ENOMEM;
 
-	memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
+	u32 addr = (ulong)priv->rxbuffers;
+	invalidate_dcache_range(addr, addr + roundup(RX_BUF * PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
+	barrier();
 
 	/* Align bd_space to MMU_SECTION_SHIFT */
 	bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
-- 
2.17.1

[U-Boot] [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Bin Meng]

Hi Stefan,

On Mon, Dec 17, 2018 at 3:49 PM Stefan Theil <stefan.theil@mixed-mode.de> wrote:
>
> The cache was only flushed before *transmitting* packets, but not
> when receiving them, leading to an issue where new packets were
> handed to the receive handler with old contents in cache. This
> only happens when a lot of packets are received without sending
> packages every now and then. Also flushing the receive buffers
> in the transmit function makes no sense and can be removed.
>
> Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
>
> ---
> Changes for v2:
>         - Use invalidate_dcache_range instead of
>           flush_dcache_range
> Changes for v3:
>         - Remove unnecessary flushing of all RX
>           buffers in zynq_gem_send
> Changes for v4:
>         - Invalidate receive buffers after allocating
>           them in zynq_gem_probe
> ---
>  drivers/net/zynq_gem.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
> index 9bd79b198a..79a22fb1ed 100644
> --- a/drivers/net/zynq_gem.c
> +++ b/drivers/net/zynq_gem.c
> @@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev, void *ptr, int len)
>         addr &= ~(ARCH_DMA_MINALIGN - 1);
>         size = roundup(len, ARCH_DMA_MINALIGN);
>         flush_dcache_range(addr, addr + size);
> -
> -       addr = (ulong)priv->rxbuffers;
> -       addr &= ~(ARCH_DMA_MINALIGN - 1);
> -       size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> -       flush_dcache_range(addr, addr + size);
>         barrier();
>
>         /* Start transmit */
> @@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
>
>         *packetp = (uchar *)(uintptr_t)addr;
>
> +       invalidate_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> +       barrier();
> +
>         return frame_len;
>  }
>
> @@ -705,7 +703,9 @@ static int zynq_gem_probe(struct udevice *dev)
>         if (!priv->rxbuffers)
>                 return -ENOMEM;
>
> -       memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
> +       u32 addr = (ulong)priv->rxbuffers;
> +       invalidate_dcache_range(addr, addr + roundup(RX_BUF * PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> +       barrier();
>

Does this fix anything? I see no need to update this.

>         /* Align bd_space to MMU_SECTION_SHIFT */
>         bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
> --

Regards,
Bin

[U-Boot] [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Stefan Theil]

> -----Ursprüngliche Nachricht-----
> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> Gesendet: Montag, 17. Dezember 2018 08:52
> An: Stefan Theil
> Cc: U-Boot Mailing List; Michal Simek
> Betreff: Re: [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for
> RX and TX
> 
> Hi Stefan,
> 
> On Mon, Dec 17, 2018 at 3:49 PM Stefan Theil <stefan.theil@mixed-
> mode.de> wrote:
> >
> > The cache was only flushed before *transmitting* packets, but not when
> > receiving them, leading to an issue where new packets were handed to
> > the receive handler with old contents in cache. This only happens when
> > a lot of packets are received without sending packages every now and
> > then. Also flushing the receive buffers in the transmit function makes
> > no sense and can be removed.
> >
> > Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
> >
> > ---
> > Changes for v2:
> >         - Use invalidate_dcache_range instead of
> >           flush_dcache_range
> > Changes for v3:
> >         - Remove unnecessary flushing of all RX
> >           buffers in zynq_gem_send
> > Changes for v4:
> >         - Invalidate receive buffers after allocating
> >           them in zynq_gem_probe
> > ---
> >  drivers/net/zynq_gem.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index
> > 9bd79b198a..79a22fb1ed 100644
> > --- a/drivers/net/zynq_gem.c
> > +++ b/drivers/net/zynq_gem.c
> > @@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev,
> void *ptr, int len)
> >         addr &= ~(ARCH_DMA_MINALIGN - 1);
> >         size = roundup(len, ARCH_DMA_MINALIGN);
> >         flush_dcache_range(addr, addr + size);
> > -
> > -       addr = (ulong)priv->rxbuffers;
> > -       addr &= ~(ARCH_DMA_MINALIGN - 1);
> > -       size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> > -       flush_dcache_range(addr, addr + size);
> >         barrier();
> >
> >         /* Start transmit */
> > @@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev, int
> > flags, uchar **packetp)
> >
> >         *packetp = (uchar *)(uintptr_t)addr;
> >
> > +       invalidate_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN,
> ARCH_DMA_MINALIGN));
> > +       barrier();
> > +
> >         return frame_len;
> >  }
> >
> > @@ -705,7 +703,9 @@ static int zynq_gem_probe(struct udevice *dev)
> >         if (!priv->rxbuffers)
> >                 return -ENOMEM;
> >
> > -       memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
> > +       u32 addr = (ulong)priv->rxbuffers;
> > +       invalidate_dcache_range(addr, addr + roundup(RX_BUF *
> PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> > +       barrier();
> >
> 
> Does this fix anything? I see no need to update this.

I was just following Michal's suggestion (https://lists.denx.de/pipermail/u-boot/2018-December/351969.html):
> Also in probe there should be flush of priv->rxbuffers to make sure that
> it is initialized properly.
> (memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN); - this should be
> useless)

> 
> >         /* Align bd_space to MMU_SECTION_SHIFT */
> >         bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
> > --
> 
> Regards,
> Bin

Regards,
Stefan

[U-Boot] [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Bin Meng]

Hi Stefan,

On Mon, Dec 17, 2018 at 3:57 PM Stefan Theil <Stefan.Theil@mixed-mode.de> wrote:
>
> > -----Ursprüngliche Nachricht-----
> > Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> > Gesendet: Montag, 17. Dezember 2018 08:52
> > An: Stefan Theil
> > Cc: U-Boot Mailing List; Michal Simek
> > Betreff: Re: [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for
> > RX and TX
> >
> > Hi Stefan,
> >
> > On Mon, Dec 17, 2018 at 3:49 PM Stefan Theil <stefan.theil@mixed-
> > mode.de> wrote:
> > >
> > > The cache was only flushed before *transmitting* packets, but not when
> > > receiving them, leading to an issue where new packets were handed to
> > > the receive handler with old contents in cache. This only happens when
> > > a lot of packets are received without sending packages every now and
> > > then. Also flushing the receive buffers in the transmit function makes
> > > no sense and can be removed.
> > >
> > > Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
> > >
> > > ---
> > > Changes for v2:
> > >         - Use invalidate_dcache_range instead of
> > >           flush_dcache_range
> > > Changes for v3:
> > >         - Remove unnecessary flushing of all RX
> > >           buffers in zynq_gem_send
> > > Changes for v4:
> > >         - Invalidate receive buffers after allocating
> > >           them in zynq_gem_probe
> > > ---
> > >  drivers/net/zynq_gem.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index
> > > 9bd79b198a..79a22fb1ed 100644
> > > --- a/drivers/net/zynq_gem.c
> > > +++ b/drivers/net/zynq_gem.c
> > > @@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev,
> > void *ptr, int len)
> > >         addr &= ~(ARCH_DMA_MINALIGN - 1);
> > >         size = roundup(len, ARCH_DMA_MINALIGN);
> > >         flush_dcache_range(addr, addr + size);
> > > -
> > > -       addr = (ulong)priv->rxbuffers;
> > > -       addr &= ~(ARCH_DMA_MINALIGN - 1);
> > > -       size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> > > -       flush_dcache_range(addr, addr + size);
> > >         barrier();
> > >
> > >         /* Start transmit */
> > > @@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev, int
> > > flags, uchar **packetp)
> > >
> > >         *packetp = (uchar *)(uintptr_t)addr;
> > >
> > > +       invalidate_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN,
> > ARCH_DMA_MINALIGN));
> > > +       barrier();
> > > +
> > >         return frame_len;
> > >  }
> > >
> > > @@ -705,7 +703,9 @@ static int zynq_gem_probe(struct udevice *dev)
> > >         if (!priv->rxbuffers)
> > >                 return -ENOMEM;
> > >
> > > -       memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
> > > +       u32 addr = (ulong)priv->rxbuffers;
> > > +       invalidate_dcache_range(addr, addr + roundup(RX_BUF *
> > PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> > > +       barrier();
> > >
> >
> > Does this fix anything? I see no need to update this.
>
> I was just following Michal's suggestion (https://lists.denx.de/pipermail/u-boot/2018-December/351969.html):
> > Also in probe there should be flush of priv->rxbuffers to make sure that
> > it is initialized properly.
> > (memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN); - this should be
> > useless)
>

Michal was suggesting 'flush', not 'invalidate', to make sure
priv->rxbuffers is really initialized to zero by memset().

> >
> > >         /* Align bd_space to MMU_SECTION_SHIFT */
> > >         bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
> > > --
> >

Regards,
Bin

[U-Boot] [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Stefan Theil]

> -----Ursprüngliche Nachricht-----
> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> Gesendet: Montag, 17. Dezember 2018 09:05
> An: Stefan Theil
> Cc: U-Boot Mailing List; Michal Simek
> Betreff: Re: [PATCH v4] zynq-gem: Use appropriate cache flush/invalidate for
> RX and TX
> 
> Hi Stefan,
> 
> On Mon, Dec 17, 2018 at 3:57 PM Stefan Theil <Stefan.Theil@mixed-
> mode.de> wrote:
> >
> > > -----Ursprüngliche Nachricht-----
> > > Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> > > Gesendet: Montag, 17. Dezember 2018 08:52
> > > An: Stefan Theil
> > > Cc: U-Boot Mailing List; Michal Simek
> > > Betreff: Re: [PATCH v4] zynq-gem: Use appropriate cache
> > > flush/invalidate for RX and TX
> > >
> > > Hi Stefan,
> > >
> > > On Mon, Dec 17, 2018 at 3:49 PM Stefan Theil <stefan.theil@mixed-
> > > mode.de> wrote:
> > > >
> > > > The cache was only flushed before *transmitting* packets, but not
> > > > when receiving them, leading to an issue where new packets were
> > > > handed to the receive handler with old contents in cache. This
> > > > only happens when a lot of packets are received without sending
> > > > packages every now and then. Also flushing the receive buffers in
> > > > the transmit function makes no sense and can be removed.
> > > >
> > > > Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
> > > >
> > > > ---
> > > > Changes for v2:
> > > >         - Use invalidate_dcache_range instead of
> > > >           flush_dcache_range
> > > > Changes for v3:
> > > >         - Remove unnecessary flushing of all RX
> > > >           buffers in zynq_gem_send Changes for v4:
> > > >         - Invalidate receive buffers after allocating
> > > >           them in zynq_gem_probe
> > > > ---
> > > >  drivers/net/zynq_gem.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index
> > > > 9bd79b198a..79a22fb1ed 100644
> > > > --- a/drivers/net/zynq_gem.c
> > > > +++ b/drivers/net/zynq_gem.c
> > > > @@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev,
> > > void *ptr, int len)
> > > >         addr &= ~(ARCH_DMA_MINALIGN - 1);
> > > >         size = roundup(len, ARCH_DMA_MINALIGN);
> > > >         flush_dcache_range(addr, addr + size);
> > > > -
> > > > -       addr = (ulong)priv->rxbuffers;
> > > > -       addr &= ~(ARCH_DMA_MINALIGN - 1);
> > > > -       size = roundup((RX_BUF * PKTSIZE_ALIGN),
> ARCH_DMA_MINALIGN);
> > > > -       flush_dcache_range(addr, addr + size);
> > > >         barrier();
> > > >
> > > >         /* Start transmit */
> > > > @@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev,
> > > > int flags, uchar **packetp)
> > > >
> > > >         *packetp = (uchar *)(uintptr_t)addr;
> > > >
> > > > +       invalidate_dcache_range(addr, addr +
> > > > + roundup(PKTSIZE_ALIGN,
> > > ARCH_DMA_MINALIGN));
> > > > +       barrier();
> > > > +
> > > >         return frame_len;
> > > >  }
> > > >
> > > > @@ -705,7 +703,9 @@ static int zynq_gem_probe(struct udevice *dev)
> > > >         if (!priv->rxbuffers)
> > > >                 return -ENOMEM;
> > > >
> > > > -       memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
> > > > +       u32 addr = (ulong)priv->rxbuffers;
> > > > +       invalidate_dcache_range(addr, addr + roundup(RX_BUF *
> > > PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> > > > +       barrier();
> > > >
> > >
> > > Does this fix anything? I see no need to update this.
> >
> > I was just following Michal's suggestion (https://lists.denx.de/pipermail/u-
> boot/2018-December/351969.html):
> > > Also in probe there should be flush of priv->rxbuffers to make sure
> > > that it is initialized properly.
> > > (memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN); - this should
> > > be
> > > useless)
> >
> 
> Michal was suggesting 'flush', not 'invalidate', to make sure
> priv->rxbuffers is really initialized to zero by memset().
> 

Oh you're right. Shouldn't submit patches before my first coffee... I'll fix that.

> > >
> > > >         /* Align bd_space to MMU_SECTION_SHIFT */
> > > >         bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
> > > > --
> > >
> 
> Regards,
> Bin

[U-Boot] [PATCH v5] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Stefan Theil]

The cache was only flushed before *transmitting* packets, but not
when receiving them, leading to an issue where new packets were
handed to the receive handler with old contents in cache. This
only happens when a lot of packets are received without sending
packages every now and then. Also flushing the receive buffers
in the transmit function makes no sense and can be removed.

Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>

---
Changes for v2:
	- Use invalidate_dcache_range instead of
	  flush_dcache_range
Changes for v3:
	- Remove unnecessary flushing of all RX
	  buffers in zynq_gem_send
Changes for v4:
	- Invalidate receive buffers after allocating
	  them in zynq_gem_probe
Changes for v5:
	- Clear and flush receive buffers in
	  zynq_gem_probe instead of invalidating the
	  cache
---
 drivers/net/zynq_gem.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
index 9bd79b198a..3bd0093b7a 100644
--- a/drivers/net/zynq_gem.c
+++ b/drivers/net/zynq_gem.c
@@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev, void *ptr, int len)
 	addr &= ~(ARCH_DMA_MINALIGN - 1);
 	size = roundup(len, ARCH_DMA_MINALIGN);
 	flush_dcache_range(addr, addr + size);
-
-	addr = (ulong)priv->rxbuffers;
-	addr &= ~(ARCH_DMA_MINALIGN - 1);
-	size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
-	flush_dcache_range(addr, addr + size);
 	barrier();
 
 	/* Start transmit */
@@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
 
 	*packetp = (uchar *)(uintptr_t)addr;
 
+	invalidate_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
+	barrier();
+
 	return frame_len;
 }
 
@@ -706,6 +704,9 @@ static int zynq_gem_probe(struct udevice *dev)
 		return -ENOMEM;
 
 	memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
+	u32 addr = (ulong)priv->rxbuffers;
+	flush_dcache_range(addr, addr + roundup(RX_BUF * PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
+	barrier();
 
 	/* Align bd_space to MMU_SECTION_SHIFT */
 	bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
-- 
2.17.1

[U-Boot] [PATCH v5] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Bin Meng]

On Mon, Dec 17, 2018 at 4:12 PM Stefan Theil <stefan.theil@mixed-mode.de> wrote:
>
> The cache was only flushed before *transmitting* packets, but not
> when receiving them, leading to an issue where new packets were
> handed to the receive handler with old contents in cache. This
> only happens when a lot of packets are received without sending
> packages every now and then. Also flushing the receive buffers
> in the transmit function makes no sense and can be removed.
>
> Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
>
> ---
> Changes for v2:
>         - Use invalidate_dcache_range instead of
>           flush_dcache_range
> Changes for v3:
>         - Remove unnecessary flushing of all RX
>           buffers in zynq_gem_send
> Changes for v4:
>         - Invalidate receive buffers after allocating
>           them in zynq_gem_probe
> Changes for v5:
>         - Clear and flush receive buffers in
>           zynq_gem_probe instead of invalidating the
>           cache
> ---
>  drivers/net/zynq_gem.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH v5] zynq-gem: Use appropriate cache flush/invalidate for RX and TX

[Michal Simek]

On 17. 12. 18 9:12, Stefan Theil wrote:
> The cache was only flushed before *transmitting* packets, but not
> when receiving them, leading to an issue where new packets were
> handed to the receive handler with old contents in cache. This
> only happens when a lot of packets are received without sending
> packages every now and then. Also flushing the receive buffers
> in the transmit function makes no sense and can be removed.
> 
> Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
> 
> ---
> Changes for v2:
> 	- Use invalidate_dcache_range instead of
> 	  flush_dcache_range
> Changes for v3:
> 	- Remove unnecessary flushing of all RX
> 	  buffers in zynq_gem_send
> Changes for v4:
> 	- Invalidate receive buffers after allocating
> 	  them in zynq_gem_probe
> Changes for v5:
> 	- Clear and flush receive buffers in
> 	  zynq_gem_probe instead of invalidating the
> 	  cache
> ---
>  drivers/net/zynq_gem.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
> index 9bd79b198a..3bd0093b7a 100644
> --- a/drivers/net/zynq_gem.c
> +++ b/drivers/net/zynq_gem.c
> @@ -570,11 +570,6 @@ static int zynq_gem_send(struct udevice *dev, void *ptr, int len)
>  	addr &= ~(ARCH_DMA_MINALIGN - 1);
>  	size = roundup(len, ARCH_DMA_MINALIGN);
>  	flush_dcache_range(addr, addr + size);
> -
> -	addr = (ulong)priv->rxbuffers;
> -	addr &= ~(ARCH_DMA_MINALIGN - 1);
> -	size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> -	flush_dcache_range(addr, addr + size);
>  	barrier();
>  
>  	/* Start transmit */
> @@ -621,6 +616,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
>  
>  	*packetp = (uchar *)(uintptr_t)addr;
>  
> +	invalidate_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> +	barrier();
> +
>  	return frame_len;
>  }
>  
> @@ -706,6 +704,9 @@ static int zynq_gem_probe(struct udevice *dev)
>  		return -ENOMEM;
>  
>  	memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN);
> +	u32 addr = (ulong)priv->rxbuffers;
> +	flush_dcache_range(addr, addr + roundup(RX_BUF * PKTSIZE_ALIGN, ARCH_DMA_MINALIGN));
> +	barrier();
>  
>  	/* Align bd_space to MMU_SECTION_SHIFT */
>  	bd_space = memalign(1 << MMU_SECTION_SHIFT, BD_SPACE);
> 

Applied.
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181220/2948a503/attachment.sig>

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Michal Simek]

On 13. 12. 18 15:19, Bin Meng wrote:
> On Thu, Dec 13, 2018 at 10:02 PM Stefan Theil
> <Stefan.Theil@mixed-mode.de> wrote:
>>
>>
>>
>>> -----Ursprüngliche Nachricht-----
>>> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
>>> Gesendet: Donnerstag, 13. Dezember 2018 14:59
>>> An: Stefan Theil
>>> Cc: Michal Simek; U-Boot Mailing List
>>> Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
>>> packet to receive handler
>>>
>>> Hi Stefan,
>>>
>>> On Thu, Dec 13, 2018 at 9:51 PM Stefan Theil <Stefan.Theil@mixed-
>>> mode.de> wrote:
>>>>
>>>>> -----Ursprüngliche Nachricht-----
>>>>> Von: Michal Simek [mailto:monstr at monstr.eu]
>>>>> Gesendet: Donnerstag, 13. Dezember 2018 14:48
>>>>> An: Stefan Theil; Bin Meng
>>>>> Cc: U-Boot Mailing List
>>>>> Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing
>>>>> RX packet to receive handler
>>>>>
>>>>> On 13. 12. 18 14:41, Stefan Theil wrote:
>>>>>>
>>>>>>
>>>>>>> -----Ursprüngliche Nachricht-----
>>>>>>> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
>>>>>>> Gesendet: Donnerstag, 13. Dezember 2018 14:37
>>>>>>> An: Stefan Theil
>>>>>>> Cc: U-Boot Mailing List
>>>>>>> Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before
>>>>>>> handing RX packet to receive handler
>>>>>>>
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-
>>>>>>> mode.de> wrote:
>>>>>>>>
>>>>>>>> Hmm good question. I went with flush because that's what's done
>>>>>>>> in the
>>>>>>> transmit function:
>>>>>>>>
>>>>>>>> addr = (ulong) ptr;
>>>>>>>> addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len,
>>>>>>>> ARCH_DMA_MINALIGN); flush_dcache_range(addr,
>>>>>>> addr
>>>>>>>> + size);
>>>>>>>>
>>>>>>>> addr = (ulong)priv->rxbuffers;
>>>>>>>> addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF *
>>>>>>>> PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
>>> flush_dcache_range(addr,
>>>>>>>> addr + size); barrier();
>>>>>>>>
>>>>>>>> But since we actually want the uncached data invalidation seems
>>>>>>>> logical. I
>>>>>>> have to admit though, I don't have much experience with caches.
>>>>>>> This patch completely fixed my problem... Maybe somebody with a
>>>>>>> bit more expertise can add their opinion?
>>>>>>>
>>>>>>> It should be 'invalidate' primitive when it comes to the RX path.
>>>>>>> For TX path, it should be 'flush'.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Bin
>>>>>>
>>>>>> Then why are all receive buffers flushed before sending a packet?
>>>>>> In anySYS_CACHE_SHIFT_6
>>>>> case, I'll try it with invalidate and submit an updated version.
>>>>>
>>>>> When you creating packet, cpu is used and caches are filled with
>>>>> data which you are asking by DMA to send. That's why you need to
>>>>> flush them to DDR for dma to take it. If you don't do that DMA can work
>>> with incorrect data.
>>>>
>>>> I know, but you don't create *receive* packets when you're sending a
>>> packet, are you? Why would you need to flush those before sending a
>>> packet? That's what I'm wondering about.
>>>>
>>>
>>> Why did you mention *receive* packets and sending a packet? The receive
>>> packets and send packets are two different buffers.
>>
>> Take a look at "zynq_gem_send" in "drivers/net/zynq_gem.c" then, where it says:
>>
> 
> I just took a look at the driver, and (see below)
> 
>> addr = (ulong)priv->rxbuffers;
>> addr &= ~(ARCH_DMA_MINALIGN - 1);
>> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
>> flush_dcache_range(addr, addr + size);
>> barrier();
>>
>> I'm fairly certain that "priv->rxbuffers" has nothing to do with sending a packet.
>>
> 
> I strongly suspect the above codes are some leftovers of copy & paste.
> Could you please remove these codes and have a test?

I think it will be good to compare this code with macb driver which is
also in the tree.
Not sure what other drivers are using but zynq_gem is using non cached
area for BDs and cached area for data.

Also in probe there should be flush of priv->rxbuffers to make sure that
it is initialized properly.
(memset(priv->rxbuffers, 0, RX_BUF * PKTSIZE_ALIGN); - this should be
useless)

And then invalidation of data cache after packet is received to make
sure that before cpu reads the packet cache is empty.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181213/bd6ec6cd/attachment.sig>

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Bin Meng]

On Thu, Dec 13, 2018 at 10:02 PM Stefan Theil
<Stefan.Theil@mixed-mode.de> wrote:
>
>
>
> > -----Ursprüngliche Nachricht-----
> > Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> > Gesendet: Donnerstag, 13. Dezember 2018 14:59
> > An: Stefan Theil
> > Cc: Michal Simek; U-Boot Mailing List
> > Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
> > packet to receive handler
> >
> > Hi Stefan,
> >
> > On Thu, Dec 13, 2018 at 9:51 PM Stefan Theil <Stefan.Theil@mixed-
> > mode.de> wrote:
> > >
> > > > -----Ursprüngliche Nachricht-----
> > > > Von: Michal Simek [mailto:monstr at monstr.eu]
> > > > Gesendet: Donnerstag, 13. Dezember 2018 14:48
> > > > An: Stefan Theil; Bin Meng
> > > > Cc: U-Boot Mailing List
> > > > Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing
> > > > RX packet to receive handler
> > > >
> > > > On 13. 12. 18 14:41, Stefan Theil wrote:
> > > > >
> > > > >
> > > > >> -----Ursprüngliche Nachricht-----
> > > > >> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> > > > >> Gesendet: Donnerstag, 13. Dezember 2018 14:37
> > > > >> An: Stefan Theil
> > > > >> Cc: U-Boot Mailing List
> > > > >> Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before
> > > > >> handing RX packet to receive handler
> > > > >>
> > > > >> Hi Stefan,
> > > > >>
> > > > >> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-
> > > > >> mode.de> wrote:
> > > > >>>
> > > > >>> Hmm good question. I went with flush because that's what's done
> > > > >>> in the
> > > > >> transmit function:
> > > > >>>
> > > > >>> addr = (ulong) ptr;
> > > > >>> addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len,
> > > > >>> ARCH_DMA_MINALIGN); flush_dcache_range(addr,
> > > > >> addr
> > > > >>> + size);
> > > > >>>
> > > > >>> addr = (ulong)priv->rxbuffers;
> > > > >>> addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF *
> > > > >>> PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> > flush_dcache_range(addr,
> > > > >>> addr + size); barrier();
> > > > >>>
> > > > >>> But since we actually want the uncached data invalidation seems
> > > > >>> logical. I
> > > > >> have to admit though, I don't have much experience with caches.
> > > > >> This patch completely fixed my problem... Maybe somebody with a
> > > > >> bit more expertise can add their opinion?
> > > > >>
> > > > >> It should be 'invalidate' primitive when it comes to the RX path.
> > > > >> For TX path, it should be 'flush'.
> > > > >>
> > > > >> Regards,
> > > > >> Bin
> > > > >
> > > > > Then why are all receive buffers flushed before sending a packet?
> > > > > In any
> > > > case, I'll try it with invalidate and submit an updated version.
> > > >
> > > > When you creating packet, cpu is used and caches are filled with
> > > > data which you are asking by DMA to send. That's why you need to
> > > > flush them to DDR for dma to take it. If you don't do that DMA can work
> > with incorrect data.
> > >
> > > I know, but you don't create *receive* packets when you're sending a
> > packet, are you? Why would you need to flush those before sending a
> > packet? That's what I'm wondering about.
> > >
> >
> > Why did you mention *receive* packets and sending a packet? The receive
> > packets and send packets are two different buffers.
>
> Take a look at "zynq_gem_send" in "drivers/net/zynq_gem.c" then, where it says:
>

I just took a look at the driver, and (see below)

> addr = (ulong)priv->rxbuffers;
> addr &= ~(ARCH_DMA_MINALIGN - 1);
> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> flush_dcache_range(addr, addr + size);
> barrier();
>
> I'm fairly certain that "priv->rxbuffers" has nothing to do with sending a packet.
>

I strongly suspect the above codes are some leftovers of copy & paste.
Could you please remove these codes and have a test?

> >
> > > > On RX path if cpu touches that memory space before DMA receive data
> > > > to the memory, cpu can work with incorrect data.
> > > > That's why you need to invalidate that range to ensure that data
> > > > which you will read is that one got from DMA.
> > > >
> > > > It is kind of interesting that we didn't observe this issue.
> > >
> > > Probably because most people don't receive enough packets between
> > sending packets, so the receive buffers are flushed often enough.

Regards,
Bin

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Bin Meng]

Hi Stefan,

On Thu, Dec 13, 2018 at 9:51 PM Stefan Theil <Stefan.Theil@mixed-mode.de> wrote:
>
> > -----Ursprüngliche Nachricht-----
> > Von: Michal Simek [mailto:monstr at monstr.eu]
> > Gesendet: Donnerstag, 13. Dezember 2018 14:48
> > An: Stefan Theil; Bin Meng
> > Cc: U-Boot Mailing List
> > Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
> > packet to receive handler
> >
> > On 13. 12. 18 14:41, Stefan Theil wrote:
> > >
> > >
> > >> -----Ursprüngliche Nachricht-----
> > >> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> > >> Gesendet: Donnerstag, 13. Dezember 2018 14:37
> > >> An: Stefan Theil
> > >> Cc: U-Boot Mailing List
> > >> Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before
> > >> handing RX packet to receive handler
> > >>
> > >> Hi Stefan,
> > >>
> > >> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-
> > >> mode.de> wrote:
> > >>>
> > >>> Hmm good question. I went with flush because that's what's done in
> > >>> the
> > >> transmit function:
> > >>>
> > >>> addr = (ulong) ptr;
> > >>> addr &= ~(ARCH_DMA_MINALIGN - 1);
> > >>> size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
> > >> addr
> > >>> + size);
> > >>>
> > >>> addr = (ulong)priv->rxbuffers;
> > >>> addr &= ~(ARCH_DMA_MINALIGN - 1);
> > >>> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> > >>> flush_dcache_range(addr, addr + size); barrier();
> > >>>
> > >>> But since we actually want the uncached data invalidation seems
> > >>> logical. I
> > >> have to admit though, I don't have much experience with caches. This
> > >> patch completely fixed my problem... Maybe somebody with a bit more
> > >> expertise can add their opinion?
> > >>
> > >> It should be 'invalidate' primitive when it comes to the RX path. For
> > >> TX path, it should be 'flush'.
> > >>
> > >> Regards,
> > >> Bin
> > >
> > > Then why are all receive buffers flushed before sending a packet? In any
> > case, I'll try it with invalidate and submit an updated version.
> >
> > When you creating packet, cpu is used and caches are filled with data which
> > you are asking by DMA to send. That's why you need to flush them to DDR
> > for dma to take it. If you don't do that DMA can work with incorrect data.
>
> I know, but you don't create *receive* packets when you're sending a packet, are you? Why would you need to flush those before sending a packet? That's what I'm wondering about.
>

Why did you mention *receive* packets and sending a packet? The
receive packets and send packets are two different buffers.

> > On RX path if cpu touches that memory space before DMA receive data to
> > the memory, cpu can work with incorrect data.
> > That's why you need to invalidate that range to ensure that data which you
> > will read is that one got from DMA.
> >
> > It is kind of interesting that we didn't observe this issue.
>
> Probably because most people don't receive enough packets between sending packets, so the receive buffers are flushed often enough.

Regards,
Bin

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Stefan Theil]

> -----Ursprüngliche Nachricht-----
> Von: Michal Simek [mailto:monstr at monstr.eu]
> Gesendet: Donnerstag, 13. Dezember 2018 14:48
> An: Stefan Theil; Bin Meng
> Cc: U-Boot Mailing List
> Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
> packet to receive handler
> 
> On 13. 12. 18 14:41, Stefan Theil wrote:
> >
> >
> >> -----Ursprüngliche Nachricht-----
> >> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> >> Gesendet: Donnerstag, 13. Dezember 2018 14:37
> >> An: Stefan Theil
> >> Cc: U-Boot Mailing List
> >> Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before
> >> handing RX packet to receive handler
> >>
> >> Hi Stefan,
> >>
> >> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-
> >> mode.de> wrote:
> >>>
> >>> Hmm good question. I went with flush because that's what's done in
> >>> the
> >> transmit function:
> >>>
> >>> addr = (ulong) ptr;
> >>> addr &= ~(ARCH_DMA_MINALIGN - 1);
> >>> size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
> >> addr
> >>> + size);
> >>>
> >>> addr = (ulong)priv->rxbuffers;
> >>> addr &= ~(ARCH_DMA_MINALIGN - 1);
> >>> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> >>> flush_dcache_range(addr, addr + size); barrier();
> >>>
> >>> But since we actually want the uncached data invalidation seems
> >>> logical. I
> >> have to admit though, I don't have much experience with caches. This
> >> patch completely fixed my problem... Maybe somebody with a bit more
> >> expertise can add their opinion?
> >>
> >> It should be 'invalidate' primitive when it comes to the RX path. For
> >> TX path, it should be 'flush'.
> >>
> >> Regards,
> >> Bin
> >
> > Then why are all receive buffers flushed before sending a packet? In any
> case, I'll try it with invalidate and submit an updated version.
> 
> When you creating packet, cpu is used and caches are filled with data which
> you are asking by DMA to send. That's why you need to flush them to DDR
> for dma to take it. If you don't do that DMA can work with incorrect data.

I know, but you don't create *receive* packets when you're sending a packet, are you? Why would you need to flush those before sending a packet? That's what I'm wondering about.

> On RX path if cpu touches that memory space before DMA receive data to
> the memory, cpu can work with incorrect data.
> That's why you need to invalidate that range to ensure that data which you
> will read is that one got from DMA.
> 
> It is kind of interesting that we didn't observe this issue.

Probably because most people don't receive enough packets between sending packets, so the receive buffers are flushed often enough.

> Thanks,
> Michal
> 
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze Maintainer of Linux kernel -
> Xilinx Zynq ARM and ZynqMP ARM64 SoCs U-Boot custodian - Xilinx
> Microblaze/Zynq/ZynqMP/Versal SoCs
> 

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Michal Simek]

On 13. 12. 18 14:41, Stefan Theil wrote:
> 
> 
>> -----Ursprüngliche Nachricht-----
>> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
>> Gesendet: Donnerstag, 13. Dezember 2018 14:37
>> An: Stefan Theil
>> Cc: U-Boot Mailing List
>> Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
>> packet to receive handler
>>
>> Hi Stefan,
>>
>> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-
>> mode.de> wrote:
>>>
>>> Hmm good question. I went with flush because that's what's done in the
>> transmit function:
>>>
>>> addr = (ulong) ptr;
>>> addr &= ~(ARCH_DMA_MINALIGN - 1);
>>> size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
>> addr
>>> + size);
>>>
>>> addr = (ulong)priv->rxbuffers;
>>> addr &= ~(ARCH_DMA_MINALIGN - 1);
>>> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
>>> flush_dcache_range(addr, addr + size); barrier();
>>>
>>> But since we actually want the uncached data invalidation seems logical. I
>> have to admit though, I don't have much experience with caches. This patch
>> completely fixed my problem... Maybe somebody with a bit more expertise
>> can add their opinion?
>>
>> It should be 'invalidate' primitive when it comes to the RX path. For TX path, it
>> should be 'flush'.
>>
>> Regards,
>> Bin
> 
> Then why are all receive buffers flushed before sending a packet? In any case, I'll try it with invalidate and submit an updated version.

When you creating packet, cpu is used and caches are filled with data
which you are asking by DMA to send. That's why you need to flush them
to DDR for dma to take it. If you don't do that DMA can work with
incorrect data.

On RX path if cpu touches that memory space before DMA receive data to
the memory, cpu can work with incorrect data.
That's why you need to invalidate that range to ensure that data which
you will read is that one got from DMA.

It is kind of interesting that we didn't observe this issue.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181213/ac981962/attachment.sig>

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Stefan Theil]

> -----Ursprüngliche Nachricht-----
> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> Gesendet: Donnerstag, 13. Dezember 2018 14:37
> An: Stefan Theil
> Cc: U-Boot Mailing List
> Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
> packet to receive handler
> 
> Hi Stefan,
> 
> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-
> mode.de> wrote:
> >
> > Hmm good question. I went with flush because that's what's done in the
> transmit function:
> >
> > addr = (ulong) ptr;
> > addr &= ~(ARCH_DMA_MINALIGN - 1);
> > size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
> addr
> > + size);
> >
> > addr = (ulong)priv->rxbuffers;
> > addr &= ~(ARCH_DMA_MINALIGN - 1);
> > size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> > flush_dcache_range(addr, addr + size); barrier();
> >
> > But since we actually want the uncached data invalidation seems logical. I
> have to admit though, I don't have much experience with caches. This patch
> completely fixed my problem... Maybe somebody with a bit more expertise
> can add their opinion?
> 
> It should be 'invalidate' primitive when it comes to the RX path. For TX path, it
> should be 'flush'.
> 
> Regards,
> Bin

Then why are all receive buffers flushed before sending a packet? In any case, I'll try it with invalidate and submit an updated version.

Regards,
Stefan

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Michal Simek]

On 13. 12. 18 14:37, Bin Meng wrote:
> Hi Stefan,
> 
> On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-mode.de> wrote:
>>
>> Hmm good question. I went with flush because that's what's done in the transmit function:
>>
>> addr = (ulong) ptr;
>> addr &= ~(ARCH_DMA_MINALIGN - 1);
>> size = roundup(len, ARCH_DMA_MINALIGN);
>> flush_dcache_range(addr, addr + size);
>>
>> addr = (ulong)priv->rxbuffers;
>> addr &= ~(ARCH_DMA_MINALIGN - 1);
>> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
>> flush_dcache_range(addr, addr + size);
>> barrier();
>>
>> But since we actually want the uncached data invalidation seems logical. I have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
> 
> It should be 'invalidate' primitive when it comes to the RX path. For
> TX path, it should be 'flush'.

+1 yes. Please retest with invalidate.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181213/95a5cc71/attachment.sig>

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Bin Meng]

Hi Stefan,

On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed-mode.de> wrote:
>
> Hmm good question. I went with flush because that's what's done in the transmit function:
>
> addr = (ulong) ptr;
> addr &= ~(ARCH_DMA_MINALIGN - 1);
> size = roundup(len, ARCH_DMA_MINALIGN);
> flush_dcache_range(addr, addr + size);
>
> addr = (ulong)priv->rxbuffers;
> addr &= ~(ARCH_DMA_MINALIGN - 1);
> size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
> flush_dcache_range(addr, addr + size);
> barrier();
>
> But since we actually want the uncached data invalidation seems logical. I have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?

It should be 'invalidate' primitive when it comes to the RX path. For
TX path, it should be 'flush'.

Regards,
Bin

[U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler

[Stefan Theil]

Hmm good question. I went with flush because that's what's done in the transmit function:

addr = (ulong) ptr;
addr &= ~(ARCH_DMA_MINALIGN - 1);
size = roundup(len, ARCH_DMA_MINALIGN);
flush_dcache_range(addr, addr + size);

addr = (ulong)priv->rxbuffers;
addr &= ~(ARCH_DMA_MINALIGN - 1);
size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN);
flush_dcache_range(addr, addr + size);
barrier();

But since we actually want the uncached data invalidation seems logical. I have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?

Regards,
Stefan

> -----Ursprüngliche Nachricht-----
> Von: Bin Meng [mailto:bmeng.cn at gmail.com]
> Gesendet: Donnerstag, 13. Dezember 2018 14:13
> An: Stefan Theil
> Cc: U-Boot Mailing List
> Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
> packet to receive handler
> 
> On Thu, Dec 13, 2018 at 6:18 PM Stefan Theil <stefan.theil@mixed-mode.de>
> wrote:
> >
> > The cache was only flushed before *transmitting* packets, but not when
> > receiving them, leading to an issue where new packets were handed to
> > the receive handler with old contents in cache. This only happens when
> > a lot of packets are received without sending packages every now and
> > then.
> >
> > Signed-off-by: Stefan Theil <stefan.theil@mixed-mode.de>
> > ---
> >  drivers/net/zynq_gem.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index
> > 9bd79b198a..5825b6dd99 100644
> > --- a/drivers/net/zynq_gem.c
> > +++ b/drivers/net/zynq_gem.c
> > @@ -621,6 +621,9 @@ static int zynq_gem_recv(struct udevice *dev, int
> > flags, uchar **packetp)
> >
> >         *packetp = (uchar *)(uintptr_t)addr;
> >
> > +       flush_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN,
> > + ARCH_DMA_MINALIGN));
> 
> Shouldn't it be invalidate_dcache_range()?
> 
> > +       barrier();
> > +
> >         return frame_len;
> >  }
> >
> 
> Regards,
> Bin