From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Fri, 02 Nov 2012 14:22:34 -0600 Subject: [U-Boot] [PATCH 2/2] mmc: tegra: invalidate complete cachelines In-Reply-To: <201204270129.09629.vapier@gentoo.org> References: <1335254024-30115-1-git-send-email-thierry.reding@avionic-design.de> <20120426103443.GA11972@avionic-0098.mockup.avionic-design.de> <201204270129.09629.vapier@gentoo.org> Message-ID: <50942B8A.5080502@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 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.