[U-Boot] [PATCH] nvme: add more cache flushes

[Patrick Wildt]

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 <patrick at blueri.se> 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 <patrick at blueri.se> 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 <patrick at blueri.se>
> > > > ---
> > > >  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.

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