[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