Hi Stephen, On Tue, Feb 5, 2013 at 12: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.
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@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