[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 15:58
> An: Bin Meng; Stefan Theil; U-Boot Mailing List
> Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX
> packet to receive handler
> 
> On 13. 12. 18 15:19, Bin Meng wrote:
> > 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 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.

That actually should be ok, as the official Technical Reference Manual for the Zynq-7000 uses that scheme as well. It tells you to flush the TX buffer before sending a packet, and recommends to put the BDs in a non-cached area (UG585, p.518). What it doesn't mention is to invalidate the RX buffer before reading it. Nobody's perfect I guess.

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

Do you want me to include that in my patch as well?

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

Regards,
Stefan