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

[Simon Glass]

Hi Tom,

On Tue, Feb 12, 2013 at 11:05 AM, Tom Warren <twarren.nvidia at gmail.com> wrote:
> Simon,
>
> On Tue, Feb 12, 2013 at 11:07 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi,
>>
>> On Tue, Feb 5, 2013 at 1:02 PM, Tom Warren <twarren.nvidia at gmail.com> wrote:
>>> Stephen,
>>>
>>> On Tue, Feb 5, 2013 at 1: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.
>>>>
>>>>> 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
>>>
>>> I took Allen's recent SPI/SLINK driver(s) as an example, but as you
>>> point out, if CONFIG_OF_CONTROL isn't enabled, you get build errors
>>> anyway.
>>>
>>>>
>>>>> +     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.
>>> Again, I used Allen's SLINK driver for as a template here. In fact, it
>>> looks like our I2C and SPI/SLINK drivers do this, as well as the
>>> Exynos SPI and S3C24x0 I2C driver all do this.  Can you point out a
>>> U-Boot driver that does it the right way (preferably with more than 1
>>> node, like MMC)? I can take a look at that code to use as an example
>>> of what you're proposing above.
>>
>> You have it correct already. Stephen please take another look and let
>> me know what problem you see with this approach. I'm very sorry that I
>> am so late to this discussion.
>
> PTAL at the current patchset (v2) - I changed it to look for 'sdhci',
> which names the node and the aliases, and seem to work fine. I also
> have one function that parses all the nodes at once, so only one call
> to tegra_mmc_init() is needed from the board level.

Great. Yes I think that was the version I looked at (above, in this thread).

Regards,
Simon

>
>>
>>>>
>>>>> +     /*
>>>>> +      * 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/.
>>>
>>> Thanks, added Andy Fleming to CC.
>>>
>>> Tom
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>> Regards,
>> Simon