[U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs

[Stephen Warren <swarren@wwwdotorg.org>]

On 11/05/2012 05:00 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>
>>
>> Tegra's MMC driver does DMA, and hence needs cache-aligned buffers. In
>> some cases (e.g. user load commands) this cannot be guaranteed by callers
>> of the MMC APIs. To solve this, modify the Tegra MMC driver to use the
>> new bounce_buffer_*() APIs.

>> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c

>>  #include <asm/arch-tegra/clk_rst.h>
>>  #include <asm/arch-tegra/tegra_mmc.h>
>>  #include <mmc.h>
>> +#include <bouncebuf.h>
> 
> The order seems wrong here - I think bouncebuf and mmc should go above
> the asm/ ones, and bouncebuf should be first.

Is there a defined order for header files? I suppose I should try and
read and remember more documentation!

>> @@ -180,8 +196,10 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>         if (data)
>>                 mmc_set_transfer_mode(host, data);
>>
>> -       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
>> -               return -1;
>> +       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) {
>> +               ret = -1;
>> +               goto cleanup;
>> +       }
> 
> You might consider putting this body in a function so you don't need
> these two lines everywhere below.

I'm not quite sure how a function would work here; a function can't
really goto. Do you mean a macro? I'd tend to this a macro would
obfuscate the pretty simple code.

Oh, perhaps you mean having a new top-level function that does:

bounce_buffer_start();
calls a function to do all the work
bounce_buffer_stop();

That would certainly simplify the patch.

>> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h

>> +#ifdef CONFIG_TEGRA_MMC
>> +#define CONFIG_BOUNCE_BUFFER
>> +#endif
> 
> Is there really any harm in just defining this always (say in the
> tegra20-common.h)? The functions should be dropped if not used.

I suppose it'd be fine to always enable this since the linker should
drop the functions when not referenced. Of course, that relies on
bouncebuf.o not having any global side-effects (e.g. registering things
via custom linker segments that are always pulled in). The code above
represents the actual dependency too; hopefully one day U-Boot will
sprout Kconfig, and that logic can be replaced by:

config TEGRA_MMC
    select BOUNCE_BUFFER