[U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs
Simon Glass
sjg at chromium.org
Tue Nov 6 01:00:15 CET 2012
Hi Stephen,
On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> From: Stephen Warren <swarren at nvidia.com>
>
> 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.
>
> Note: Ideally, all U-Boot code will always provide address- and size-
> aligned buffers, so a bounce buffer will only ever be needed for user-
> supplied buffers (e.g. load commands). Ensuring this removes the need
> for performance-sucking bounce buffer memory allocations and memcpy()s.
> The one known exception at present is the SCR buffer in sd_change_freq(),
> which is only 8 bytes long. Solving this requires enhancing struct
> mmc_data to know the difference between buffer size and transferred data
> size, or forcing all callers of mmc_send_cmd() to have allocated buffers
> using ALLOC_CACHE_ALIGN_BUFFER(), which will true in this case, is not
> enforced in any way at present, and so cannot be assumed by the core MMC
> code.
>
> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> ---
> drivers/mmc/tegra_mmc.c | 81 +++++++++++++++++++++++------------
> include/configs/tegra-common-post.h | 4 ++
> 2 files changed, 58 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c
> index 1fd5592..e5d55b0 100644
> --- a/drivers/mmc/tegra_mmc.c
> +++ b/drivers/mmc/tegra_mmc.c
> @@ -26,6 +26,7 @@
> #include <asm/arch-tegra/clk_rst.h>
> #include <asm/arch-tegra/tegra_mmc.h>
> #include <mmc.h>
> +#include <bouncebuf.h>
The order seems wrong here - I think bouncebuf and mmc should go above
the asm/ ones, and bouncebuf should be first.
>
> /* support 4 mmc hosts */
> struct mmc mmc_dev[4];
> @@ -66,14 +67,34 @@ static void tegra_get_setup(struct mmc_host *host, int dev_index)
> host->reg = (struct tegra_mmc *)host->base;
> }
>
> -static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data)
> +static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data,
> + struct bounce_buffer_state *bbstate)
> {
> unsigned char ctrl;
> + void *buf;
> + uint8_t bbflags;
> + size_t len;
> +
> + if (data->flags & MMC_DATA_READ) {
> + buf = data->dest;
> + bbflags = GEN_BB_WRITE;
> + } else {
> + buf = (void *)data->src;
> + bbflags = GEN_BB_READ;
> + }
> + len = data->blocks * data->blocksize;
> +
> + bounce_buffer_start(bbstate, buf, len, bbflags);
> +
> + debug("buf: %08X, data->blocks: %u, data->blocksize: %u\n",
> + (u32)bbstate->bounce_buffer, data->blocks, data->blocksize);
>
> - debug("data->dest: %08X, data->blocks: %u, data->blocksize: %u\n",
> - (u32)data->dest, data->blocks, data->blocksize);
> + if (data->flags & MMC_DATA_WRITE)
> + flush_dcache_range((ulong)bbstate->bounce_buffer,
> + (ulong)bbstate->bounce_buffer +
> + bbstate->len_aligned);
>
> - writel((u32)data->dest, &host->reg->sysad);
> + writel((u32)bbstate->bounce_buffer, &host->reg->sysad);
> /*
> * DMASEL[4:3]
> * 00 = Selects SDMA
> @@ -114,14 +135,6 @@ static void mmc_set_transfer_mode(struct mmc_host *host, struct mmc_data *data)
> if (data->flags & MMC_DATA_READ)
> mode |= TEGRA_MMC_TRNMOD_DATA_XFER_DIR_SEL_READ;
>
> - if (data->flags & MMC_DATA_WRITE) {
> - if ((uintptr_t)data->src & (ARCH_DMA_MINALIGN - 1))
> - printf("Warning: unaligned write to %p may fail\n",
> - data->src);
> - flush_dcache_range((ulong)data->src, (ulong)data->src +
> - data->blocks * data->blocksize);
> - }
> -
> writew(mode, &host->reg->trnmod);
> }
>
> @@ -164,6 +177,9 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> int result;
> unsigned int mask = 0;
> unsigned int retry = 0x100000;
> + struct bounce_buffer_state bbstate;
> + int ret;
> +
> debug(" mmc_send_cmd called\n");
>
> result = mmc_wait_inhibit(host, cmd, data, 10 /* ms */);
> @@ -172,7 +188,7 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> return result;
>
> if (data)
> - mmc_prepare_data(host, data);
> + mmc_prepare_data(host, data, &bbstate);
>
> debug("cmd->arg: %08x\n", cmd->cmdarg);
> writel(cmd->cmdarg, &host->reg->argument);
> @@ -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.
>
> /*
> * CMDREG
> @@ -228,19 +246,22 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> if (i == retry) {
> printf("%s: waiting for status update\n", __func__);
> writel(mask, &host->reg->norintsts);
> - return TIMEOUT;
> + ret = TIMEOUT;
> + goto cleanup;
> }
>
> if (mask & TEGRA_MMC_NORINTSTS_CMD_TIMEOUT) {
> /* Timeout Error */
> debug("timeout: %08x cmd %d\n", mask, cmd->cmdidx);
> writel(mask, &host->reg->norintsts);
> - return TIMEOUT;
> + ret = TIMEOUT;
> + goto cleanup;
> } else if (mask & TEGRA_MMC_NORINTSTS_ERR_INTERRUPT) {
> /* Error Interrupt */
> debug("error: %08x cmd %d\n", mask, cmd->cmdidx);
> writel(mask, &host->reg->norintsts);
> - return -1;
> + ret = -1;
> + goto cleanup;
> }
>
> if (cmd->resp_type & MMC_RSP_PRESENT) {
> @@ -269,7 +290,8 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> if (i == retry) {
> printf("%s: card is still busy\n", __func__);
> writel(mask, &host->reg->norintsts);
> - return TIMEOUT;
> + ret = TIMEOUT;
> + goto cleanup;
> }
>
> cmd->response[0] = readl(&host->reg->rspreg0);
> @@ -291,7 +313,8 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> writel(mask, &host->reg->norintsts);
> printf("%s: error during transfer: 0x%08x\n",
> __func__, mask);
> - return -1;
> + ret = -1;
> + goto cleanup;
> } else if (mask & TEGRA_MMC_NORINTSTS_DMA_INTERRUPT) {
> /*
> * DMA Interrupt, restart the transfer where
> @@ -318,22 +341,26 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> readl(&host->reg->norintstsen),
> readl(&host->reg->norintsigen),
> readl(&host->reg->prnsts));
> - return -1;
> + ret = -1;
> + goto cleanup;
> }
> }
> writel(mask, &host->reg->norintsts);
> 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);
> + invalidate_dcache_range((ulong)bbstate.bounce_buffer,
> + (ulong)bbstate.bounce_buffer +
> + bbstate.len_aligned);
> }
> + bounce_buffer_stop(&bbstate);
> }
>
> udelay(1000);
> return 0;
> +
> +cleanup:
> + if (data)
> + bounce_buffer_stop(&bbstate);
> + return ret;
> }
>
> static void mmc_change_clock(struct mmc_host *host, uint clock)
> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
> index ab62e71..2d45933 100644
> --- a/include/configs/tegra-common-post.h
> +++ b/include/configs/tegra-common-post.h
> @@ -251,4 +251,8 @@
>
> #endif /* CONFIG_SPL_BUILD */
>
> +#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.
> +
> #endif /* __TEGRA_COMMON_POST_H */
> --
> 1.7.0.4
>
Regards,
Simon
More information about the U-Boot
mailing list