-----Ursprüngliche Nachricht----- Von: Michal Simek [mailto:monstr@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@mixed-mode.de wrote:
-----Ursprüngliche Nachricht----- Von: Bin Meng [mailto:bmeng.cn@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@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@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.
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