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

[Stefan Theil <Stefan.Theil@mixed-mode.de>]

> -----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