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

[Simon Glass]

Hi Stephen,

On Tue, Feb 12, 2013 at 12:13 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 02/12/2013 11:07 AM, Simon Glass 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.
>
> Could you explain how this works then? The code I looked at (a) was
> driven by board files not DT enumerationk (b) errored out if no alias ID
> was present.
>
> But given there's a V2, I'll go look at that and check again.

I'm really talking about the function fdtdec_add_aliases_for_id(). It
is designed to do exactly what you wanted, from memory, except that it
only supports a single compatible ID. If there is no alias node, then
it should still find all the compatible nodes. The alias node is only
used to order them.

In terms of being driver by board files, it is best if the drivers do
this, so that the enumeration is independent of any board file.

Regards,
Simon