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

[Simon Glass]

Hi Stephen,

On Tue, Feb 5, 2013 at 12:03 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 02/04/2013 04:48 PM, Tom Warren wrote:
>> tegra_mmc_init() now uses DT info for bus width, WP/CD GPIOs, etc.
>> Tested on Seaboard, fully functional.
>
>> diff --git a/board/compal/paz00/paz00.c b/board/compal/paz00/paz00.c
>> index 1447f47..5cee91a 100644
>> --- a/board/compal/paz00/paz00.c
>> +++ b/board/compal/paz00/paz00.c
>> @@ -55,18 +55,18 @@ static void pin_mux_mmc(void)
>>  /* this is a weak define that we are overriding */
>>  int board_mmc_init(bd_t *bd)
>>  {
>> -     debug("board_mmc_init called\n");
>> +     debug("%s called\n", __func__);
>>
>>       /* Enable muxes, etc. for SDMMC controllers */
>>       pin_mux_mmc();
>>
>> -     debug("board_mmc_init: init eMMC\n");
>> -     /* init dev 0, eMMC chip, with 8-bit bus */
>> -     tegra_mmc_init(0, 8, -1, -1);
>> +     debug("%s: init eMMC\n", __func__);
>> +     /* init dev 0, eMMC chip */
>> +     tegra_mmc_init(0);
>>
>> -     debug("board_mmc_init: init SD slot\n");
>> -     /* init dev 3, SD slot, with 4-bit bus */
>> -     tegra_mmc_init(3, 4, GPIO_PV1, GPIO_PV5);
>> +     debug("%s: init SD slot\n", __func__);
>> +     /* init dev 3, SD slot */
>> +     tegra_mmc_init(3);
>>
>>       return 0;
>>  }
>
> That doesn't look right. The board code still has knowledge of which
> SDHCI controllers are in use by the board. Instead, the board should
> just call tegra_mmc_init() with no parameters at all, and the MMC driver
> should scan the device tree for all present-and-enabled SDHCI nodes, and
> instantiate a U-Boot SDHCI device. Without this, the device tree isn't
> in control of the whole process, so there's little point doing the
> conversion; a new board couldn't be supported /just/ by creating a new
> device tree file.

Agreed.

>
>> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c
>
>> +#ifndef CONFIG_OF_CONTROL
>> +#error "Please enable device tree support to use this driver"
>> +#endif
>
> So CONFIG_OF_CONTROL must be enabled ...
>
>>
>>  static void tegra_get_setup(struct mmc_host *host, int dev_index)
>>  {
>> -     debug("tegra_get_setup: dev_index = %d\n", dev_index);
>> +     debug("%s: dev_index = %d\n", __func__, dev_index);
>> +
>> +#ifdef CONFIG_OF_CONTROL
>
> ... so there's no need for that ifdef
>
>> +     int count, node = 0;
>> +     int node_list[MAX_HOSTS];
>> +
>> +     count = fdtdec_find_aliases_for_id(gd->fdt_blob, "sdmmc",
>> +             COMPAT_NVIDIA_TEGRA20_SDMMC, node_list, MAX_HOSTS);
>> +     debug("%s: count of nodes is %d\n", __func__, count);
>> +
>> +     if (count < dev_index) {
>> +             printf("%s: device index %d exceeds node count (%d)!\n",
>> +                     __func__, dev_index, count);
>> +             return;
>> +     }
>
> This requires that an alias exist in order for the SDHCI node to be
> found/processed. This isn't correct; the SDHCI nodes must be enumerated
> themselves, and then the aliases (if any are present) provide a naming
> hint for them, but even without aliases, the SDHCI nodes must be processed.

I believe this is quite incorrect. Please take a look at the function,
which deals with aliases if they are present, but works in any case.
There is a test in fdtdec_test.c which handles the cases that we
discussed at the time. Quite a bit of effort went into this function
at the time.

So I believe this function should always be used when enumerating
multiple devices. If there is a bug in the function, let's find out
what it is and fix it, and add a new test.

>
>> +     /*
>> +      * NOTE: mmc->bus_width is determined by mmc.c dynamically.
>> +      * Should we override it with this value?
>> +      */
>> +     host->width = fdtdec_get_int(gd->fdt_blob, node, "bus-width", 0);
>> +     if (!host->width)
>> +             debug("%s: no sdmmc width found\n", __func__);
>
> It should be possible to inform the MMC core of the width of the bus in
> terms of wires on the PCB. It should only probe the connected device up
> to that width. If that feature is missing, it can be added later though,
> outside the scope of this patch set.
>
> You didn't Cc the MMC maintainer; they should be Cc'd since this code is
> in drivers/mmc/.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot


Regards,
Simon