On 11/05/2012 04:54 PM, Simon Glass wrote:
> Hi Stephen,
>
> On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swar...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
