[U-Boot] [PATCH 2/3] common: rework bouncebuf implementation

[Stephen Warren <swarren@wwwdotorg.org>]

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.