Hi Tom, On Tue, Feb 12, 2013 at 11:05 AM, Tom Warren <twarren.nvi...@gmail.com> wrote: > Simon, > > On Tue, Feb 12, 2013 at 11:07 AM, Simon Glass <s...@chromium.org> wrote: >> Hi, >> >> On Tue, Feb 5, 2013 at 1:02 PM, Tom Warren <twarren.nvi...@gmail.com> wrote: >>> Stephen, >>> >>> On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swar...@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@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >> >> Regards, >> Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot