Hi Stephen, On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <[email protected]> wrote: > From: Stephen Warren <[email protected]> > > 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 <[email protected]> > --- > 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
