[U-Boot] [PATCH 2/2] mmc: tegra: invalidate complete cachelines

[Marek Vasut]

Dear Stephen Warren,

> On 11/02/2012 02:38 PM, Marek Vasut wrote:
> > Dear Simon Glass,
> > 
> >> Hi Stephen,
> >> 
> >> On Fri, Nov 2, 2012 at 1:22 PM, Stephen Warren <swarren at wwwdotorg.org> 
wrote:
> >>> On 04/26/2012 11:29 PM, Mike Frysinger wrote:
> >>>> On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:
> >>>>> For reference, see sd_change_freq() in drivers/mmc/mmc.c.
> >>> 
> >>> This is a follow-up to:
> >>> 
> >>> http://lists.denx.de/pipermail/u-boot/2012-April/123080.html
> >>> 
> >>> which was referenced from:
> >>> 
> >>> http://lists.denx.de/pipermail/u-boot/2012-September/133641.html
> >>> 
> >>>> yes, that shows what we're talking about. int sd_change_freq(struct
> >>>> mmc *mmc) { ... stack vars ... int err;
> >>>> ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int
> >>>> timeout; ... stack vars ... ... data.dest = (char *)scr;
> >>>> data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ;
> >>>> err = mmc_send_cmd(mmc, &cmd, &data); ...
> >>>> 
> >>>> static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> >>>> struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if
> >>>> ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
> >>>> unaligned read from %p " "may fail\n", data->dest);
> >>>> invalidate_dcache_range((ulong)data->dest, (ulong)data->dest +
> >>>> data->blocks * data->blocksize); } ...
> >>>> 
> >>>> so what invalidate_dcache_range() will invalidate is from &scr and
> >>>> 8 bytes after that.  the higher layers make sure &scr is aligned
> >>>> properly, but not that it spans a full cache line.  so if the stack
> >>>> layout is: [random stuff] 0x100       [scr[0]] 0x104  [scr[1]] 0x108
> >>>> [err] 0x10a   [timeout] [more stuff]
> >>> 
> >>> That's not the stack layout. scr is allocated (as quoted above) using
> >>> ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for
> >>> scr is always cache-aligned in address and size, even if the code only
> >>> uses 8 bytes of it.
> >>> 
> >>>> this implicitly invalidates [err] and [timeout] and more stuff.  by
> >>>> you forcibly rounding up the length, you no longer get a warning,
> >>>> but you still (silently) blew away those variables from cache
> >>>> possibly losing changes that weren't written back to external
> >>>> memory yet.
> >>> 
> >>> So, this problem won't actually occur here.
> >>> 
> >>> So, Thierry's proposed solution (which I'll re-quote below) would
> >>> 
> >>> actually work just fine:
> >>>> [PATCH 2/2] mmc: tegra: invalidate complete cachelines
> >>>> 
> >>>> The MMC core sometimes reads buffers that are smaller than a
> >>>> complete cacheline, for example when reading the SCR. In order to
> >>>> avoid a warning from the ARM v7 cache handling code, this patch
> >>>> makes sure that complete cachelines are flushed.
> >>>> 
> >>>> Signed-off-by: Thierry Reding <thierry.reding at
> >>>> avionic-design.de> --- drivers/mmc/tegra2_mmc.c |    7 ++++--- 1
> >>>> file changed, 4 insertions(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/mmc/tegra2_mmc.c b/drivers/mmc/tegra2_mmc.c
> >>>> index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++
> >>>> b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int
> >>>> mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask,
> >>>> &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { +
> >>>> ulong end = (ulong)data->dest + +
> >>>> roundup(data->blocks * data->blocksize, +
> >>>> 
> >>>>              ARCH_DMA_MINALIGN); if ((uintptr_t)data->dest &
> >>>> 
> >>>> (ARCH_DMA_MINALIGN - 1)) printf("Warning: unaligned read from %p "
> >>>> "may fail\n", data->dest); -
> >>>> invalidate_dcache_range((ulong)data->dest, -
> >>>> (ulong)data->dest + -                                   data->blocks *
> >>>> data->blocksize); + invalidate_dcache_range((ulong)data->dest, end); }
> >>>> }
> >>>> 
> >>>> --
> >>> 
> >>> ... so long as we require any struct mmc_data data's .dest field to
> >>> point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.
> >>> 
> >>> So, I'd like to propose that we take Thierry's fix, perhaps having
> >>> validated (or implemented some way of enforcing) that
> >>> ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.
> >> 
> >> That sounds ok to me.
> >> 
> >> It is important to avoid silent failure if we can. Are you proposing
> >> to pass a 'size' parameter along with any buffer pointer, or something
> >> else?
> > 
> > Dumb question -- might be unrelated. Does the tegra mmc driver do DMA?
> > And if so, what happens if you do raw read to unaligned address (aka.
> > how come you don't need the bounce buffer)?
> 
> Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why
> the driver is flushing caches!)
> 
> I guess we only support the use of aligned addresses, so e.g. the
> following would work:
> 
> ext2load mmc 0:1 0x00100000 /file
> 
> but the following wouldn't:
> 
> ext2load mmc 0:1 0x00100004 /file
> 
> which while I suppose it is an artificial restriction, hasn't been an
> issue in practice.

Then just enable the bounce buffer and it will work ;-)