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