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

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>