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

[Simon Glass]

Hi Stephen,

On Tue, Nov 6, 2012 at 10:50 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 11/05/2012 05:00 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>> From: Stephen Warren <swarren at 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!

According to Mike, "the order should generally be:
        - stuff in include/
        - stuff in sys/
        - stuff in linux/
        - stuff in asm/
each sub-region can be sorted, but i don't think we should go against the
subdir rule"

>
>>> @@ -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.

Yes that's what I meant.

>
>>> 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

Yes, maybe it isn't that simple in general, but it seems that it might
be safe with Tegra at least.

Regards,
Simon