On Thu, Oct 17, 2019 at 8:44 AM Patrick Wildt <[email protected]> wrote: > > On Thu, Oct 17, 2019 at 10:55:11AM +0800, Bin Meng wrote: > > Hi Patrick, > > > > On Wed, Oct 16, 2019 at 11:35 PM Patrick Wildt <[email protected]> wrote: > > > > > > On Wed, Oct 16, 2019 at 06:11:23PM +0800, Bin Meng wrote: > > > > On Mon, Oct 14, 2019 at 7:11 PM Patrick Wildt <[email protected]> wrote: > > > > > > > > > > On an i.MX8MQ our nvme driver doesn't completely work since we are > > > > > missing a few cache flushes. One is the prp list, which is an extra > > > > > buffer that we need to flush before handing it to the hardware. Also > > > > > the block read/write operations needs more cache flushes on this SoC. > > > > > > > > > > Signed-off-by: Patrick Wildt <[email protected]> > > > > > --- > > > > > drivers/nvme/nvme.c | 15 +++++++++------ > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > > > > > index 2444e0270f..69d5e3eedc 100644 > > > > > --- a/drivers/nvme/nvme.c > > > > > +++ b/drivers/nvme/nvme.c > > > > > @@ -123,6 +123,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, > > > > > u64 *prp2, > > > > > } > > > > > *prp2 = (ulong)dev->prp_pool; > > > > > > > > > > + flush_dcache_range((ulong)dev->prp_pool, (ulong)dev->prp_pool > > > > > + > > > > > + dev->prp_entry_num * sizeof(u64)); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -717,9 +720,10 @@ static ulong nvme_blk_rw(struct udevice *udev, > > > > > lbaint_t blknr, > > > > > u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); > > > > > u64 total_lbas = blkcnt; > > > > > > > > > > - if (!read) > > > > > - flush_dcache_range((unsigned long)buffer, > > > > > - (unsigned long)buffer + total_len); > > > > > + flush_dcache_range((unsigned long)buffer, > > > > > + (unsigned long)buffer + total_len); > > > > > > > > Why we need this for read? > > > > > > I'm no processor engineer, but I have read (and "seen") the following > > > on ARMs. If I'm wrong. please correct me. > > > > > > Since the buffer is usually allocated cache-aligned on the stack, > > > it is very possible that this buffer was previously still used > > > as it's supposed to be used: as stack. Thus, the caches can still > > > be filled, and are not yet evicted/flushed to memory. Now it is > > > possible that between the DMA access from the device and our cache > > > invalidation, the CPU cache writes back its contents to memory, > > > overwriting whatever the NVMe just gave us. > > > > OK, this makes sense. So if we allocate the buffer from the heap, we > > should only care about flush on write, right? Can you test this? > > If you're talking about having a bounce buffer: You'd need to flush > it once upon allocation, because that part of the heap could have also > be used earlier by someone else.
And this is exactly what common/bouncebuf.c does ;-) Regards, Simon > > Best regards, > Patrick > > > > > > > > > + invalidate_dcache_range((unsigned long)buffer, > > > > > + (unsigned long)buffer + total_len); > > > > > > > > > > c.rw.opcode = read ? nvme_cmd_read : nvme_cmd_write; > > > > > c.rw.flags = 0; > > > > > @@ -755,9 +759,8 @@ static ulong nvme_blk_rw(struct udevice *udev, > > > > > lbaint_t blknr, > > > > > buffer += lbas << ns->lba_shift; > > > > > } > > > > > > > > > > - if (read) > > > > > - invalidate_dcache_range((unsigned long)buffer, > > > > > - (unsigned long)buffer + > > > > > total_len); > > > > > + invalidate_dcache_range((unsigned long)buffer, > > > > > + (unsigned long)buffer + total_len); > > > > > > > > Why we need this for write? > > > > > > That's a good point. After the transaction, if it was a read then > > > we need to invalidate it, as we might have speculatively read it. > > > On a write, we don't care about its contents. I will test it w/o > > > this chunk and report back. > > > > > > > Regards, > > Bin > _______________________________________________ > U-Boot mailing list > [email protected] > https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
