Hi On Mon, Jun 8, 2020 at 3: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. >
We will adjust commit message. The idea is that when we invalidate after the DMA transfer, a cache flush happen for the invalidation itself > 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. > Correct > This issue looks like the case described on slide 30 and 31 here: > https://elinux.org/images/6/6d/Rutland2.pdf > Thank you for point out to the slide > > 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. Does it not depend on how invalidation work in the particular CPU? > > 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)? Right, I don't find a better way and yes this is general problem in uboot Michael > > > 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 ... > > Cheers, > Andre. > > > > > Cc: Andre Przywara <andre.przyw...@arm.com> > > Reported-by: Suniel Mahesh <su...@amarulasolutions.com> > > Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com> > > Signed-off-by: Jagan Teki <ja...@amarulasolutions.com> > > Tested-by: Suniel Mahesh <su...@amarulasolutions.com> > > --- > > drivers/nvme/nvme.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > > index 0357aba7f1..fc64d93ab8 100644 > > --- a/drivers/nvme/nvme.c > > +++ b/drivers/nvme/nvme.c > > @@ -466,6 +466,9 @@ int nvme_identify(struct nvme_dev *dev, unsigned nsid, > > > > c.identify.cns = cpu_to_le32(cns); > > > > + invalidate_dcache_range(dma_addr, > > + dma_addr + sizeof(struct nvme_id_ctrl)); > > + > > ret = nvme_submit_admin_cmd(dev, &c, NULL); > > if (!ret) > > invalidate_dcache_range(dma_addr, > > > -- | Michael Nazzareno Trimarchi Amarula Solutions BV | | COO - Founder Cruquiuskade 47 | | +31(0)851119172 Amsterdam 1018 AM NL | | [`as] http://www.amarulasolutions.com |