[U-Boot] [PATCH v4 4/4] Tegra: MMC: Add DT support to MMC driver for all T20 boards

[Tom Warren]

Andy,

On Mon, Feb 18, 2013 at 4:10 PM, Andy Fleming <afleming at gmail.com> wrote:
>
>
>
> On Thu, Feb 14, 2013 at 3:04 PM, Tom Warren <twarren.nvidia at gmail.com>
> wrote:
>>
>> tegra_mmc_init() now parses the DT info for bus width, WP/CD GPIOs, etc.
>> Tested on Seaboard, fully functional.
>>
>> Tamonten boards (medcom-wide, plutux, and tec) use a different/new
>> dtsi file w/common settings.
>>
>> Signed-off-by: Tom Warren <twarren at nvidia.com>
>> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
>> ---
>> v2:
>> - all boards now call tegra_mmc_init once, w/no params
>> - count MMC controllers, not aliases
>> - AD boards (medcom/plutux/tec) use common tegra20-tamonten.dtsi
>> v3:
>> - move any power init inside board's pin_mux_mmc function, and/or
>>  create pin_mux_mmc function if necessary.
>> - move board_mmc_init out of each board file and into ../common/board.c
>> v4:
>> - remove #ifdef CONFIG_TEGRA_MMC from trimslice.c
>> - fix minor whitespace issue in board/nvidia/common/board.c
>> - remove marking of used node_list entries in MMC driver, not needed
>>
>
>
>>
>> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c
>> index d749ab0..918a98d 100644
>> --- a/drivers/mmc/tegra_mmc.c
>> +++ b/drivers/mmc/tegra_mmc.c
>> @@ -21,6 +21,7 @@
>>
>>  #include <bouncebuf.h>
>>  #include <common.h>
>> +#include <fdtdec.h>
>>  #include <asm/gpio.h>
>>  #include <asm/io.h>
>>  #include <asm/arch/clock.h>
>> @@ -28,54 +29,23 @@
>>  #include <asm/arch-tegra/tegra_mmc.h>
>>  #include <mmc.h>
>>
>> -/* support 4 mmc hosts */
>> -struct mmc mmc_dev[4];
>> -struct mmc_host mmc_host[4];
>> +DECLARE_GLOBAL_DATA_PTR;
>>
>> +struct mmc mmc_dev[MAX_HOSTS];
>> +struct mmc_host mmc_host[MAX_HOSTS];
>>
>> -/**
>> - * Get the host address and peripheral ID for a device. Devices are
>> numbered
>> - * from 0 to 3.
>> - *
>> - * @param host         Structure to fill in (base, reg, mmc_id)
>> - * @param dev_index    Device index (0-3)
>> - */
>> -static void tegra_get_setup(struct mmc_host *host, int dev_index)
>> -{
>> -       debug("tegra_get_setup: dev_index = %d\n", dev_index);
>> -
>> -       switch (dev_index) {
>> -       case 1:
>> -               host->base = TEGRA_SDMMC3_BASE;
>> -               host->mmc_id = PERIPH_ID_SDMMC3;
>> -               break;
>> -       case 2:
>> -               host->base = TEGRA_SDMMC2_BASE;
>> -               host->mmc_id = PERIPH_ID_SDMMC2;
>> -               break;
>> -       case 3:
>> -               host->base = TEGRA_SDMMC1_BASE;
>> -               host->mmc_id = PERIPH_ID_SDMMC1;
>> -               break;
>> -       case 0:
>> -       default:
>> -               host->base = TEGRA_SDMMC4_BASE;
>> -               host->mmc_id = PERIPH_ID_SDMMC4;
>> -               break;
>> -       }
>> -
>> -       host->reg = (struct tegra_mmc *)host->base;
>> -}
>> +#ifndef CONFIG_OF_CONTROL
>> +#error "Please enable device tree support to use this driver"
>> +#endif
>>
>>  static void mmc_prepare_data(struct mmc_host *host, struct mmc_data
>> *data,
>>                                 struct bounce_buffer *bbstate)
>>  {
>>         unsigned char ctrl;
>>
>> -
>> -       debug("buf: %p (%p), data->blocks: %u, data->blocksize: %u\n",
>> -               bbstate->bounce_buffer, bbstate->user_buffer,
>> data->blocks,
>> -               data->blocksize);
>> +       debug("%s: buf: %p (%p), data->blocks: %u, data->blocksize: %u\n",
>> +               __func__, bbstate->bounce_buffer, bbstate->user_buffer,
>> +               data->blocks, data->blocksize);
>
>
>
> This patch is FULL of these changes. It makes it almost impossible to
> identify the substantive changes to this code. In the future, please put
> changes to debug output in a separate patch, unless it directly applies to
> the relevant change. Also, I note that you didn't mention the fact that you
> reworked all the debug() calls to prefix with the function name in your
> patch description.
I can move the debug changes to a separate patch, no problem. I added
the function name prefix because, in some cases, the hard-coded string
didn't identify the function correctly (someone moved or renamed the
function during a feature add/rewrite (bounce buffer, etc.) and didn't
update the debug printf string). So I thought it best to add the
function name to each debug printf, making it easier to identify where
the info was coming from. I'll add a comment about that to the new
patch, too. Thanks.

>
>>
>> +/*
>> + * Process a list of nodes, adding them to our list of SDMMC ports.
>> + *
>> + * @param blob          fdt blob
>> + * @param node_list     list of nodes to process (any <=0 are ignored)
>> + * @param count         number of nodes to process
>> + * @return 0 if ok, -1 on error
>> + */
>> +static int process_nodes(const void *blob, int node_list[], int count)
>> +{
>> +       struct mmc_host *host;
>> +       int i, node;
>> +
>> +       debug("%s: count = %d\n", __func__, count);
>> +
>> +       /* build mmc_host[] for each controller */
>> +       for (i = 0; i < count; i++) {
>> +               node = node_list[i];
>> +               if (node <= 0)
>> +                       continue;
>> +
>> +               host = &mmc_host[i];
>> +               host->id = i;
>> +
>> +               if (mmc_get_config(blob, node, host)) {
>> +                       printf("%s: failed to decode dev %d\n", __func__,
>> i);
>> +                       return -1;
>> +               }
>> +               do_mmc_init(i);
>> +       }
>> +       return 0;
>> +}
>> +
>> +void tegra_mmc_init(void)
>> +{
>> +       int node_list[MAX_HOSTS], count;
>> +       const void *blob = gd->fdt_blob;
>> +       debug("%s entry\n", __func__);
>> +
>> +       count = fdtdec_find_aliases_for_id(blob, "sdhci",
>> +               COMPAT_NVIDIA_TEGRA20_SDMMC, node_list, MAX_HOSTS);
>> +       debug("%s: count of sdhci nodes is %d\n", __func__, count);
>> +
>> +       if (process_nodes(blob, node_list, count)) {
>> +               printf("%s: Error processing mmc node(s)!\n", __func__);
>> +               return;
>> +       }
>> +}
>
>
>
> Hmmm.... what does fdtdec_find_aliases_for_id() do? It looks like you are
> attempting to go through all of the sdhci nodes, and I can't help but wonder
> why you wouldn't use the approach the kernel uses --
> for_each_compatible_node()? I'm a little worried that "aliases" are being
> overused, here.
I used the same basic code that's in every other DT-supporting driver
in U-Boot right now. As Stephen points out in his response, the code
iterates over the compatible nodes, and then takes into account the
aliases, allowing you to number the devices as you see fit according
to how people use (or have used) the devices on your board, i.e. MMC 0
is the eMMC chip and MMC 1 is the SD-Card slot, even though (on Tegra)
the SDIO register space(s) come before the eMMC reg base.

And again, not to harp on this, but I don't usually look to the kernel
for examples of how to do things in U-Boot - I look for pre-existing
U-Boot code that does what I need, and if it works for my application,
I don't over-analyze it or try to dissect it too deeply. But if a
feature is too new to have much extant U-Boot code (like DT files),
then I'll go looking for kernel examples or ask Stephen or another
kernel guru for help. But that's the exception, and not the rule.

Thanks,

Tom
>
> Andy