From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Tue, 06 Nov 2012 11:50:22 -0700 Subject: [U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs In-Reply-To: References: <1352156642-7975-1-git-send-email-swarren@wwwdotorg.org> <1352156642-7975-3-git-send-email-swarren@wwwdotorg.org> Message-ID: <50995BEE.7020108@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/05/2012 05:00 PM, Simon Glass wrote: > Hi Stephen, > > On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren wrote: >> From: Stephen Warren >> >> Tegra's MMC driver does DMA, and hence needs cache-aligned buffers. In >> some cases (e.g. user load commands) this cannot be guaranteed by callers >> of the MMC APIs. To solve this, modify the Tegra MMC driver to use the >> new bounce_buffer_*() APIs. >> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c >> #include >> #include >> #include >> +#include > > The order seems wrong here - I think bouncebuf and mmc should go above > the asm/ ones, and bouncebuf should be first. Is there a defined order for header files? I suppose I should try and read and remember more documentation! >> @@ -180,8 +196,10 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, >> if (data) >> mmc_set_transfer_mode(host, data); >> >> - if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) >> - return -1; >> + if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) { >> + ret = -1; >> + goto cleanup; >> + } > > You might consider putting this body in a function so you don't need > these two lines everywhere below. I'm not quite sure how a function would work here; a function can't really goto. Do you mean a macro? I'd tend to this a macro would obfuscate the pretty simple code. Oh, perhaps you mean having a new top-level function that does: bounce_buffer_start(); calls a function to do all the work bounce_buffer_stop(); That would certainly simplify the patch. >> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h >> +#ifdef CONFIG_TEGRA_MMC >> +#define CONFIG_BOUNCE_BUFFER >> +#endif > > Is there really any harm in just defining this always (say in the > tegra20-common.h)? The functions should be dropped if not used. I suppose it'd be fine to always enable this since the linker should drop the functions when not referenced. Of course, that relies on bouncebuf.o not having any global side-effects (e.g. registering things via custom linker segments that are always pulled in). The code above represents the actual dependency too; hopefully one day U-Boot will sprout Kconfig, and that logic can be replaced by: config TEGRA_MMC select BOUNCE_BUFFER