Hi Stephen, On Fri, Nov 2, 2012 at 1:22 PM, Stephen Warren <swar...@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? Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
