| Message ID | 1352156642-7975-2-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi Stephen, On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@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@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
On 11/05/2012 04:54 PM, Simon Glass wrote: > Hi Stephen, > > On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> 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. ... > 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. Yes, that's a good idea. It cross my mind before, but I forgot to follow up on it and realize that the cache management APIs are actually common, not CPU-specific. One question here. The MXS MMC driver contains: > if (data->flags & MMC_DATA_WRITE) { > /* Flush data to DRAM so DMA can pick them up */ > 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 + > bbstate.len_aligned)); It the invalidate_dcache_range() really necessary here? I would assume that the flush_dcache_range() call marks the cache non-dirty, so there can no longer be any write-backs to race with the DMA. Certainly, the Tegra MMC driver doesn't include that invalidate call and appears to work fine. I agree with all your comments that I haven't quoted here, and will go implement them. >> -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. I wondered whether we should simply remove the dummy implementations completely; it seems that if any MMC driver needs bouncebuf ever, it always needs it. Hence, there should never be a case where these APIs are called/referenced, yet CONFIG_BOUNCE_BUFFER is not set. However, there is such a case right now; sc_sps_1.h enables the MXS MMC driver but not the bounce buffer. Perhaps I should just send a patch to fix that, and remove these dummy functions. Aside from that, the reason these dummy functions need to set fields in *state right now is that the MXS MMC driver unconditionally accesses those fields, so they need to exist and contain valid data.
On 11/05/2012 04:54 PM, Simon Glass wrote: > Hi Stephen, > > On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@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. >> diff --git a/common/bouncebuf.c b/common/bouncebuf.c >> -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. That doesn't actually work out. The if (data) test in the Tegra driver is checking whether a non-NULL "struct mmc_data *data" is passed to Tegra's mmc_send_cmd(). That value isn't directly passed to bounce_buffer_start(), but rather data->{src,dest}, which doesn't even exist if (!data).
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)) 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); 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); + 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; +}; + /** * 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); /** * 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; 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; }