[U-Boot] [PATCH 2/2] mmc: tegra: invalidate complete cachelines
Stephen Warren
swarren at wwwdotorg.org
Fri Nov 2 21:22:34 CET 2012
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.
More information about the U-Boot
mailing list