Hi André, On Mon, Jun 8, 2020 at 9:52 PM André Przywara <andre.przyw...@arm.com> wrote: > > On 07/06/2020 12:22, Jagan Teki wrote: > > Hi, > > (CC: ing Mark) > > Without looking to deep, I think invalidating the cache might be the > right thing to do, but the rationale or at least the wording of it seems > somehow flawed: > > > Some architecture like ARM Cortex A53, A72 would need > > Please don't mix the term "architecture" with actual implementations. > And the problem is more general, I would say. This *might* be a case > here where this is due to speculation, and then I would expect it to > only show on an A72, for instance. I guess this is about NVMe on RK3399? > Does U-Boot run on an A53 or an A72 core? > > > to invalidate dcache to sync the cache with the memory > > contents before flushing the cache to memory. > > That is somewhat confusing. What does "sync" and "flush" mean here? > AFAIK only "clean" and "invalidate" are clearly defined terms. > The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is > purely about the buffer for the *return* data, in which case I would > expect this being a pure invalidation problem. > > This issue looks like the case described on slide 30 and 31 here: > https://elinux.org/images/6/6d/Rutland2.pdf
Thanks for the slides. This is really helpful. > > > The NVME here submitting the admin command using dma_addr > > to the memory without prior cache invalidation. This causing > > dma_addr is pointing to an invalid location in the memory > > This does not sound right: dma_addr is always pointing to the right > location in memory. > The actual reason this fixes something might be that (some) cache lines > of the DMA buffer were dirty and were evicted *before* the existing > invalidation, but *after* the DMA transfer. That sounds unlikely in an > U-Boot context, though, but would match the case described in Mark's slides. > > So there might be more to this. Are we missing a barrier here? Should we > use coherent buffers (memory mapped as un-cached) for the return DMA in > the first place (but I don't think U-Boot provides those)? > > > and found the sane nvme_ctrl result. > > > > Below example shows the nvme disk scan result improper result > > > > => nvme scan > > nvme_get_info_from_identify: nn = 544502629, vwc = 100, > > sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105 > > > > So, invalidating the cache before submitting the admin command > > makes the dma_addr points to a valid location in the memory. > > If this shows up already, I think we should address all the comments in > the driver where it says we should do cache maintenance. And it makes me > wonder how this actually works today ... > I believe the driver was only validated on x86 and NXP's PowerPC series where cache coherence is ensured between the DMA master and the system memory. I suspect it was never validated on ARM hence this patch. I agree we should address all the comments in the driver about cache maintenance. Regards, Bin