[U-Boot] [PATCH 2/3] common: rework bouncebuf implementation
Simon Glass
sjg at chromium.org
Tue Nov 6 00:54:48 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>
>
> The current bouncebuf API requires all parameters to be passed to both
> bounce_buffer_start() and bounce_buffer_stop(). This works fine when
> both functions are called from the same place. However, Tegra's MMC
> driver splits the data setup and post-processing steps between two
> functions, and passing all that state around separately would be painful.
> Modify the bouncebuf API to accept a state structure as a parameter, so
> that only a single struct needs to be passed to both APIs.
>
> Also, don't modify the data pointer, but rather store the temporary
> buffer in this state struct. This also avoids passing the buffer pointer
> all over the place. The bouncebuf code ensures that client code can
> always use a single buffer pointer in the state structure, irrespective
> of whether a bounce buffer actually had to be allocated.
>
> Finally, store the aligned/rounded length in the state structure too, so
> that client code doesn't need to duplicate the size alignment/rounding
> code, and hence have to guess at the value it was aligned/rounded to.
>
> Update the MXS MMC driver for this change.
I missed this API when it came through. I think your changes improve it a lot.
>
> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> ---
> common/bouncebuf.c | 41 ++++++++++++++++-------------------------
> drivers/mmc/mxsmmc.c | 25 ++++++++++++++-----------
> include/bouncebuf.h | 36 +++++++++++++++++++++++-------------
> 3 files changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index ffd3c90..210d70d 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
> @@ -50,44 +50,35 @@ static int addr_aligned(void *data, size_t len)
> return 1;
> }
>
> -int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags)
> +int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
> + size_t len, uint8_t flags)
> {
> - void *tmp;
> - size_t alen;
> + state->user_buffer = data;
> + state->bounce_buffer = data;
> + state->len = len;
> + state->len_aligned = roundup(len, ARCH_DMA_MINALIGN);
> + state->flags = flags;
>
> - if (addr_aligned(*data, len)) {
> - *backup = NULL;
> + if (addr_aligned(data, len))
Maybe consider checking for data == NULL here, and return 0. This
would allow you to remove your 'if (data)' checks in the tegra driver.
Would need to update function description in the header file though.
> return 0;
> - }
> -
> - alen = roundup(len, ARCH_DMA_MINALIGN);
> - tmp = memalign(ARCH_DMA_MINALIGN, alen);
>
> - if (!tmp)
> + state->bounce_buffer = memalign(ARCH_DMA_MINALIGN, state->len_aligned);
> + if (!state->bounce_buffer)
> return -ENOMEM;
>
> - if (flags & GEN_BB_READ)
> - memcpy(tmp, *data, len);
> -
> - *backup = *data;
> - *data = tmp;
> + if (state->flags & GEN_BB_READ)
> + memcpy(state->bounce_buffer, state->user_buffer, state->len);
I wonder if the dcache handling could be done here (and in the
memcpy() below), perhaps under the control of a new flag. After the
cache code in both drivers seems very similar.
>
> return 0;
> }
>
> -int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags)
> +int bounce_buffer_stop(struct bounce_buffer_state *state)
> {
> - void *tmp = *data;
> -
> - /* The buffer was already aligned, since "backup" is NULL. */
> - if (!*backup)
> + if (state->bounce_buffer == state->user_buffer)
> return 0;
>
> - if (flags & GEN_BB_WRITE)
> - memcpy(*backup, *data, len);
> -
> - *data = *backup;
> - free(tmp);
Don't you need to keep the free()?
> + if (state->flags & GEN_BB_WRITE)
> + memcpy(state->user_buffer, state->bounce_buffer, state->len);
>
> return 0;
> }
> diff --git a/drivers/mmc/mxsmmc.c b/drivers/mmc/mxsmmc.c
> index 109acbf..d314d7d 100644
> --- a/drivers/mmc/mxsmmc.c
> +++ b/drivers/mmc/mxsmmc.c
> @@ -96,11 +96,11 @@ static int mxsmmc_send_cmd_pio(struct mxsmmc_priv *priv, struct mmc_data *data)
> static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
> {
> uint32_t data_count = data->blocksize * data->blocks;
> - uint32_t cache_data_count = roundup(data_count, ARCH_DMA_MINALIGN);
> int dmach;
> struct mxs_dma_desc *desc = priv->desc;
> - void *addr, *backup;
> + void *addr;
> uint8_t flags;
> + struct bounce_buffer_state bbstate;
>
> memset(desc, 0, sizeof(struct mxs_dma_desc));
> desc->address = (dma_addr_t)desc;
> @@ -115,19 +115,21 @@ static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
> flags = GEN_BB_READ;
> }
>
> - bounce_buffer_start(&addr, data_count, &backup, flags);
> + bounce_buffer_start(&bbstate, addr, data_count, flags);
>
> - priv->desc->cmd.address = (dma_addr_t)addr;
> + priv->desc->cmd.address = (dma_addr_t)bbstate.bounce_buffer;
>
> if (data->flags & MMC_DATA_WRITE) {
> /* Flush data to DRAM so DMA can pick them up */
> - flush_dcache_range((uint32_t)addr,
> - (uint32_t)(addr) + cache_data_count);
> + flush_dcache_range((uint32_t)bbstate.bounce_buffer,
> + (uint32_t)(bbstate.bounce_buffer) +
> + bbstate.len_aligned);
> }
>
> /* Invalidate the area, so no writeback into the RAM races with DMA */
> invalidate_dcache_range((uint32_t)priv->desc->cmd.address,
> - (uint32_t)(priv->desc->cmd.address + cache_data_count));
> + (uint32_t)(priv->desc->cmd.address +
> + bbstate.len_aligned));
>
> priv->desc->cmd.data |= MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM |
> (data_count << MXS_DMA_DESC_BYTES_OFFSET);
> @@ -135,17 +137,18 @@ static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
> dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + priv->id;
> mxs_dma_desc_append(dmach, priv->desc);
> if (mxs_dma_go(dmach)) {
> - bounce_buffer_stop(&addr, data_count, &backup, flags);
> + bounce_buffer_stop(&bbstate);
> return COMM_ERR;
> }
>
> /* The data arrived into DRAM, invalidate cache over them */
> if (data->flags & MMC_DATA_READ) {
> - invalidate_dcache_range((uint32_t)addr,
> - (uint32_t)(addr) + cache_data_count);
> + invalidate_dcache_range((uint32_t)bbstate.bounce_buffer,
> + (uint32_t)(bbstate.bounce_buffer) +
> + bbstate.len_aligned);
> }
>
> - bounce_buffer_stop(&addr, data_count, &backup, flags);
> + bounce_buffer_stop(&bbstate);
>
> return 0;
> }
> diff --git a/include/bouncebuf.h b/include/bouncebuf.h
> index 31021c5..205a1ed 100644
> --- a/include/bouncebuf.h
> +++ b/include/bouncebuf.h
> @@ -52,33 +52,43 @@
> #define GEN_BB_RW (GEN_BB_READ | GEN_BB_WRITE)
>
> #ifdef CONFIG_BOUNCE_BUFFER
> +struct bounce_buffer_state {
> + void *user_buffer;
> + void *bounce_buffer;
> + size_t len;
> + size_t len_aligned;
> + uint8_t flags;
Would struct bounce_buffer be better?
Perhaps add member comments?
> +};
> +
> /**
> * bounce_buffer_start() -- Start the bounce buffer session
> + * state: stores state passed between bounce_buffer_{start,stop}
> * data: pointer to buffer to be aligned
> * len: length of the buffer
> - * backup: pointer to backup buffer (the original value is stored here if
> - * needed
> * flags: flags describing the transaction, see above.
> */
> -int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags);
> +int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
> + size_t len, uint8_t flags);
I would argue that a uint8_t parameter doesn't make a lot of sense.
Why not just unsigned int?
> /**
> * bounce_buffer_stop() -- Finish the bounce buffer session
> - * data: pointer to buffer that was aligned
> - * len: length of the buffer
> - * backup: pointer to backup buffer (the original value is stored here if
> - * needed
> - * flags: flags describing the transaction, see above.
> + * state: stores state passed between bounce_buffer_{start,stop}
> */
> -int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags);
> +int bounce_buffer_stop(struct bounce_buffer_state *state);
> #else
> -static inline int bounce_buffer_start(void **data, size_t len, void **backup,
> - uint8_t flags)
> +struct bounce_buffer_state {
> + void *bounce_buffer;
> + size_t len_aligned;
> +};
> +
> +static inline int bounce_buffer_start(struct bounce_buffer_state *state,
> + void *data, size_t len, uint8_t flags)
> {
> + state->bounce_buffer = data;
> + state->len_aligned = len;
Why do you need to do this if it is just a null implementation?
Perhaps add a comment.
> return 0;
> }
>
> -static inline int bounce_buffer_stop(void **data, size_t len, void **backup,
> - uint8_t flags)
> +static inline int bounce_buffer_stop(struct bounce_buffer_state *state)
> {
> return 0;
> }
> --
> 1.7.0.4
>
Regards,
Simon
More information about the U-Boot
mailing list