From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Fri, 02 Nov 2012 14:58:09 -0600 Subject: [U-Boot] [PATCH 2/2] mmc: tegra: invalidate complete cachelines In-Reply-To: <201211022138.20198.marex@denx.de> References: <1335254024-30115-1-git-send-email-thierry.reding@avionic-design.de> <50942B8A.5080502@wwwdotorg.org> <201211022138.20198.marex@denx.de> Message-ID: <509433E1.2010604@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >>> 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.