| Message ID | 1360021735-13286-3-git-send-email-twarren@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
Hi Tom, Am Montag, 4. Februar 2013, 16:48:55 schrieb Tom Warren: > tegra_mmc_init() now uses DT info for bus width, WP/CD GPIOs, etc. > Tested on Seaboard, fully functional. > > Signed-off-by: Tom Warren <twarren@nvidia.com> > --- > arch/arm/include/asm/arch-tegra/mmc.h | 2 +- > arch/arm/include/asm/arch-tegra/tegra_mmc.h | 12 +- > board/avionic-design/common/tamonten.c | 4 +- > board/compal/paz00/paz00.c | 14 +- > board/compulab/trimslice/trimslice.c | 10 +- > board/nvidia/harmony/harmony.c | 12 +- > board/nvidia/seaboard/seaboard.c | 14 +- > board/nvidia/whistler/whistler.c | 4 +- > board/toradex/colibri_t20_iris/colibri_t20_iris.c | 2 +- > drivers/mmc/tegra_mmc.c | 186 > ++++++++++++++------- include/fdtdec.h | > 1 + > lib/fdtdec.c | 1 + > 12 files changed, 165 insertions(+), 97 deletions(-) > > [...] > > 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); This looks wrong because the sd is on sdmmc0 > > - 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); and the emmc on sdmmc3. The DTS is correct. Not your fault as it seems to be wrong in the original code already. I guess it didn't made large difference but may in the future. I wonder how to test this though. Marc
Marc, On Tue, Feb 5, 2013 at 2:28 AM, Marc Dietrich <marvin24@gmx.de> wrote: > Hi Tom, > > Am Montag, 4. Februar 2013, 16:48:55 schrieb Tom Warren: >> tegra_mmc_init() now uses DT info for bus width, WP/CD GPIOs, etc. >> Tested on Seaboard, fully functional. >> >> Signed-off-by: Tom Warren <twarren@nvidia.com> >> --- >> arch/arm/include/asm/arch-tegra/mmc.h | 2 +- >> arch/arm/include/asm/arch-tegra/tegra_mmc.h | 12 +- >> board/avionic-design/common/tamonten.c | 4 +- >> board/compal/paz00/paz00.c | 14 +- >> board/compulab/trimslice/trimslice.c | 10 +- >> board/nvidia/harmony/harmony.c | 12 +- >> board/nvidia/seaboard/seaboard.c | 14 +- >> board/nvidia/whistler/whistler.c | 4 +- >> board/toradex/colibri_t20_iris/colibri_t20_iris.c | 2 +- >> drivers/mmc/tegra_mmc.c | 186 >> ++++++++++++++------- include/fdtdec.h | >> 1 + >> lib/fdtdec.c | 1 + >> 12 files changed, 165 insertions(+), 97 deletions(-) >> >> [...] >> >> 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); > > This looks wrong because the sd is on sdmmc0 > >> >> - 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); > > and the emmc on sdmmc3. The DTS is correct. > > Not your fault as it seems to be wrong in the original code already. > I guess it didn't made large difference but may in the future. I wonder how to > test this though. > > Marc > > OK, so just the comments are wrong in paz00.c - I can fix that if I have to do a V2 patchset, or when I apply the patches to u-boot-tegra. As to testing, just stop at the command prompt and select each device (mmc dev 0, etc.) and run mmcinfo. You should be able to tell from the data displayed whether you are on an SD-card or eMMC chip. You can also eject the SD-card and you should get a warning about card presence due to the CD GPIO. Tom
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 > + 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. > + /* > + * 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/.
On Tuesday 05 February 2013 08:31:03 Tom Warren wrote: > Marc, > > On Tue, Feb 5, 2013 at 2:28 AM, Marc Dietrich <marvin24@gmx.de> wrote: > >> > >> [...] > >> > >> 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); > > > > This looks wrong because the sd is on sdmmc0 > > > >> - 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); > > > > and the emmc on sdmmc3. The DTS is correct. > > > > Not your fault as it seems to be wrong in the original code already. > > I guess it didn't made large difference but may in the future. I wonder > > how to test this though. > > > > Marc > > OK, so just the comments are wrong in paz00.c - I can fix that if I > have to do a V2 patchset, or when I apply the patches to u-boot-tegra. ah no, this is weird! index 3 maps to sdmmc1 index 2 maps to sdmmc2 index 1 maps to sdmmc3 index 0 maps to sdmmc4 so all is fine, nearly ... > As to testing, just stop at the command prompt and select each device > (mmc dev 0, etc.) and run mmcinfo. You should be able to tell from the > data displayed whether you are on an SD-card or eMMC chip. You can > also eject the SD-card and you should get a warning about card > presence due to the CD GPIO. the sd card is not detected because: TEGRA20 Board: Compal Paz00 DRAM: 512 MiB MMC: tegra_get_setup: dev_index = 0 tegra_get_setup: count of nodes is 2 tegra_get_setup: found controller at c8000600, width = 8, periph_id = 15 tegra_mmc_init: index 0, bus width 8 pwr_gpio -1 cd_gpio -1 tegra_mmc_init: bus width = 8 tegra_get_setup: dev_index = 3 tegra_get_setup: count of nodes is 2 tegra_get_setup: device index 3 exceeds node count (2)! If I understand correctly, you are counting the aliases only, not the controllers..., so index 3 (the sdcard) is not initialized at all. Arrr, debugging stole all of my time, but I guess this needs fixing. Marc
Marc, On Tue, Feb 5, 2013 at 1:06 PM, Marc Dietrich <marvin24@gmx.de> wrote: > On Tuesday 05 February 2013 08:31:03 Tom Warren wrote: >> Marc, >> >> On Tue, Feb 5, 2013 at 2:28 AM, Marc Dietrich <marvin24@gmx.de> wrote: >> >> >> >> [...] >> >> >> >> 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); >> > >> > This looks wrong because the sd is on sdmmc0 >> > >> >> - 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); >> > >> > and the emmc on sdmmc3. The DTS is correct. >> > >> > Not your fault as it seems to be wrong in the original code already. >> > I guess it didn't made large difference but may in the future. I wonder >> > how to test this though. >> > >> > Marc >> >> OK, so just the comments are wrong in paz00.c - I can fix that if I >> have to do a V2 patchset, or when I apply the patches to u-boot-tegra. > > ah no, this is weird! > > index 3 maps to sdmmc1 > index 2 maps to sdmmc2 > index 1 maps to sdmmc3 > index 0 maps to sdmmc4 > > so all is fine, nearly ... > >> As to testing, just stop at the command prompt and select each device >> (mmc dev 0, etc.) and run mmcinfo. You should be able to tell from the >> data displayed whether you are on an SD-card or eMMC chip. You can >> also eject the SD-card and you should get a warning about card >> presence due to the CD GPIO. > > the sd card is not detected because: > > TEGRA20 > Board: Compal Paz00 > DRAM: 512 MiB > MMC: tegra_get_setup: dev_index = 0 > tegra_get_setup: count of nodes is 2 > tegra_get_setup: found controller at c8000600, width = 8, periph_id = 15 > tegra_mmc_init: index 0, bus width 8 pwr_gpio -1 cd_gpio -1 > tegra_mmc_init: bus width = 8 > tegra_get_setup: dev_index = 3 > tegra_get_setup: count of nodes is 2 > tegra_get_setup: device index 3 exceeds node count (2)! > > If I understand correctly, you are counting the aliases only, not the > controllers..., so index 3 (the sdcard) is not initialized at all. Arrr, > debugging stole all of my time, but I guess this needs fixing. Yep, I am checking the aliases to get a node count (just like the Tegra SPI, SLINK, and I2C drivers, and the Exynos SPI and S3C I2C drivers). I left the Paz00 tegra_mmc_init(3) call the same as originally (w/o the width and GPIO params, of course). The device numbering is kind of arbitrary - if there are only 2 MMC devices present, I'd expect dev 0 to be eMMC and dev 1 to be SD (again, like my T20 reference board, Seaboard). I don't see that Paz uses mmc anywhere in its config files for a boot script - does it have to have mmc dev 3 be SD? or would dev 1 work? Note that the tegra20-common-post.h file that all T20 boards inherit uses dev 0 and dev 1. Let me look into it - wish I had a Paz00 board here to debug with. I'll try to simulate this on my Seaboard, maybe with all 4 MMC device addresses in the alias. Thanks for the help, Tom > > Marc > > >
On 02/05/2013 01:41 PM, Tom Warren wrote: ... > I left the Paz00 tegra_mmc_init(3) call the same as originally (w/o > the width and GPIO params, of course). The device numbering is kind of > arbitrary - if there are only 2 MMC devices present, I'd expect dev 0 > to be eMMC and dev 1 to be SD (again, like my T20 reference board, > Seaboard). I don't see that Paz uses mmc anywhere in its config files > for a boot script - does it have to have mmc dev 3 be SD? or would dev > 1 work? Note that the tegra20-common-post.h file that all T20 boards > inherit uses dev 0 and dev 1. This board currently uses U-Boot MMC device ID 0 for the eMMC and U-Boot MMC device ID 1 for the SD card slot. The U-Boot device ID's aren't directly related to the Tegra HW instance IDs.
Tom, On Tuesday 05 February 2013 13:41:21 Tom Warren wrote: > Marc, > > On Tue, Feb 5, 2013 at 1:06 PM, Marc Dietrich <marvin24@gmx.de> wrote: > > On Tuesday 05 February 2013 08:31:03 Tom Warren wrote: > >> Marc, > >> > >> On Tue, Feb 5, 2013 at 2:28 AM, Marc Dietrich <marvin24@gmx.de> wrote: > >> >> [...] > >> >> > >> >> 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); > >> > > >> > This looks wrong because the sd is on sdmmc0 > >> > > >> >> - 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); > >> > > >> > and the emmc on sdmmc3. The DTS is correct. > >> > > >> > Not your fault as it seems to be wrong in the original code already. > >> > I guess it didn't made large difference but may in the future. I wonder > >> > how to test this though. > >> > > >> > Marc > >> > >> OK, so just the comments are wrong in paz00.c - I can fix that if I > >> have to do a V2 patchset, or when I apply the patches to u-boot-tegra. > > > > ah no, this is weird! > > > > index 3 maps to sdmmc1 > > index 2 maps to sdmmc2 > > index 1 maps to sdmmc3 > > index 0 maps to sdmmc4 > > > > so all is fine, nearly ... > > > >> As to testing, just stop at the command prompt and select each device > >> (mmc dev 0, etc.) and run mmcinfo. You should be able to tell from the > >> data displayed whether you are on an SD-card or eMMC chip. You can > >> also eject the SD-card and you should get a warning about card > >> presence due to the CD GPIO. > > > > the sd card is not detected because: > > > > TEGRA20 > > Board: Compal Paz00 > > DRAM: 512 MiB > > MMC: tegra_get_setup: dev_index = 0 > > tegra_get_setup: count of nodes is 2 > > tegra_get_setup: found controller at c8000600, width = 8, periph_id = 15 > > tegra_mmc_init: index 0, bus width 8 pwr_gpio -1 cd_gpio -1 > > tegra_mmc_init: bus width = 8 > > tegra_get_setup: dev_index = 3 > > tegra_get_setup: count of nodes is 2 > > tegra_get_setup: device index 3 exceeds node count (2)! > > > > If I understand correctly, you are counting the aliases only, not the > > controllers..., so index 3 (the sdcard) is not initialized at all. Arrr, > > debugging stole all of my time, but I guess this needs fixing. > > Yep, I am checking the aliases to get a node count (just like the > Tegra SPI, SLINK, and I2C drivers, and the Exynos SPI and S3C I2C > drivers). > > I left the Paz00 tegra_mmc_init(3) call the same as originally (w/o > the width and GPIO params, of course). The device numbering is kind of > arbitrary - if there are only 2 MMC devices present, I'd expect dev 0 > to be eMMC and dev 1 to be SD (again, like my T20 reference board, > Seaboard). I don't see that Paz uses mmc anywhere in its config files > for a boot script - does it have to have mmc dev 3 be SD? or would dev > 1 work? Note that the tegra20-common-post.h file that all T20 boards > inherit uses dev 0 and dev 1. U-boot scans all devices during boot, so no need to specify a specific one. I think what Stephen is suggesting is the right way. Forget all this dev ids and let the device tree control everything. The aliases can be used for enumbering if someone really needs it. > Let me look into it - wish I had a Paz00 board here to debug with. > I'll try to simulate this on my Seaboard, maybe with all 4 MMC device > addresses in the alias. He, if you ever come to old Europe, there are still some shops selling them ;-) Marc
Stephen, On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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. > >> + /* >> + * 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
Marc, On Tue, Feb 5, 2013 at 1:54 PM, Marc Dietrich <marvin24@gmx.de> wrote: > Tom, > > On Tuesday 05 February 2013 13:41:21 Tom Warren wrote: >> Marc, >> >> On Tue, Feb 5, 2013 at 1:06 PM, Marc Dietrich <marvin24@gmx.de> wrote: >> > On Tuesday 05 February 2013 08:31:03 Tom Warren wrote: >> >> Marc, >> >> >> >> On Tue, Feb 5, 2013 at 2:28 AM, Marc Dietrich <marvin24@gmx.de> wrote: >> >> >> [...] >> >> >> >> >> >> 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); >> >> > >> >> > This looks wrong because the sd is on sdmmc0 >> >> > >> >> >> - 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); >> >> > >> >> > and the emmc on sdmmc3. The DTS is correct. >> >> > >> >> > Not your fault as it seems to be wrong in the original code already. >> >> > I guess it didn't made large difference but may in the future. I wonder >> >> > how to test this though. >> >> > >> >> > Marc >> >> >> >> OK, so just the comments are wrong in paz00.c - I can fix that if I >> >> have to do a V2 patchset, or when I apply the patches to u-boot-tegra. >> > >> > ah no, this is weird! >> > >> > index 3 maps to sdmmc1 >> > index 2 maps to sdmmc2 >> > index 1 maps to sdmmc3 >> > index 0 maps to sdmmc4 >> > >> > so all is fine, nearly ... >> > >> >> As to testing, just stop at the command prompt and select each device >> >> (mmc dev 0, etc.) and run mmcinfo. You should be able to tell from the >> >> data displayed whether you are on an SD-card or eMMC chip. You can >> >> also eject the SD-card and you should get a warning about card >> >> presence due to the CD GPIO. >> > >> > the sd card is not detected because: >> > >> > TEGRA20 >> > Board: Compal Paz00 >> > DRAM: 512 MiB >> > MMC: tegra_get_setup: dev_index = 0 >> > tegra_get_setup: count of nodes is 2 >> > tegra_get_setup: found controller at c8000600, width = 8, periph_id = 15 >> > tegra_mmc_init: index 0, bus width 8 pwr_gpio -1 cd_gpio -1 >> > tegra_mmc_init: bus width = 8 >> > tegra_get_setup: dev_index = 3 >> > tegra_get_setup: count of nodes is 2 >> > tegra_get_setup: device index 3 exceeds node count (2)! >> > >> > If I understand correctly, you are counting the aliases only, not the >> > controllers..., so index 3 (the sdcard) is not initialized at all. Arrr, >> > debugging stole all of my time, but I guess this needs fixing. >> >> Yep, I am checking the aliases to get a node count (just like the >> Tegra SPI, SLINK, and I2C drivers, and the Exynos SPI and S3C I2C >> drivers). >> >> I left the Paz00 tegra_mmc_init(3) call the same as originally (w/o >> the width and GPIO params, of course). The device numbering is kind of >> arbitrary - if there are only 2 MMC devices present, I'd expect dev 0 >> to be eMMC and dev 1 to be SD (again, like my T20 reference board, >> Seaboard). I don't see that Paz uses mmc anywhere in its config files >> for a boot script - does it have to have mmc dev 3 be SD? or would dev >> 1 work? Note that the tegra20-common-post.h file that all T20 boards >> inherit uses dev 0 and dev 1. > > U-boot scans all devices during boot, so no need to specify a specific one. I > think what Stephen is suggesting is the right way. Forget all this dev ids and > let the device tree control everything. The aliases can be used for enumbering > if someone really needs it. Yeah, I just now realized the the tegra_mmc driver used this code to assign base addresses to dev IDs: switch (dev_index) { case 1: host->base = TEGRA_SDMMC3_BASE; host->mmc_id = PERIPH_ID_SDMMC3; break; case 2: host->base = TEGRA_SDMMC2_BASE; host->mmc_id = PERIPH_ID_SDMMC2; break; case 3: host->base = TEGRA_SDMMC1_BASE; host->mmc_id = PERIPH_ID_SDMMC1; break; case 0: default: host->base = TEGRA_SDMMC4_BASE; host->mmc_id = PERIPH_ID_SDMMC4; break; } which puts SDMMC4 at dev 0 (eMMC, I knew that) and the other 3 (usually SDIO) in reverse order, SDMMC1 = dev 3, etc. I didn't know that. As you say, letting the DT control it all, with only a single 'mmc_init' call from the board file is the way to go. I'll redo and submit a V2 later in the week. Thanks again! > >> Let me look into it - wish I had a Paz00 board here to debug with. >> I'll try to simulate this on my Seaboard, maybe with all 4 MMC device >> addresses in the alias. > > He, if you ever come to old Europe, there are still some shops selling > them ;-) Unfortunately, no plans to travel to Europe in the near future. :( > > Marc > >
On 02/05/2013 02:02 PM, Tom Warren wrote: > Stephen, > > On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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 ... >>> @@ -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("%s: init eMMC\n", __func__); >>> + /* init dev 0, eMMC chip */ >>> + tegra_mmc_init(0); ... >>> + debug("%s: init SD slot\n", __func__); >>> + /* init dev 3, SD slot */ >>> + tegra_mmc_init(3); >> >> 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. ... >>> static void tegra_get_setup(struct mmc_host *host, int dev_index) ... >>> + 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. Tegra's I2C driver has just a single i2c_init_board() function, which calls fdtdec_find_aliases_for_id() to find all the DT nodes for a given compatible value (and associated aliases if there are any, I believe), then calls process_nodes() to loop over them all, and initialize/register them.
Hi Stephen, On Tue, Feb 5, 2013 at 12:03 PM, Stephen Warren <swarren@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
Hi, On Tue, Feb 5, 2013 at 1:02 PM, Tom Warren <twarren.nvidia@gmail.com> wrote: > Stephen, > > On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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. >> >>> + /* >>> + * 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
Simon, On Tue, Feb 12, 2013 at 11:07 AM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Tue, Feb 5, 2013 at 1:02 PM, Tom Warren <twarren.nvidia@gmail.com> wrote: >> Stephen, >> >> On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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. > >>> >>>> + /* >>>> + * 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
Hi Tom, On Tue, Feb 12, 2013 at 11:05 AM, Tom Warren <twarren.nvidia@gmail.com> wrote: > Simon, > > On Tue, Feb 12, 2013 at 11:07 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi, >> >> On Tue, Feb 5, 2013 at 1:02 PM, Tom Warren <twarren.nvidia@gmail.com> wrote: >>> Stephen, >>> >>> On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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
On 02/12/2013 11:07 AM, Simon Glass wrote: > Hi, > > On Tue, Feb 5, 2013 at 1:02 PM, Tom Warren <twarren.nvidia@gmail.com> wrote: >> Stephen, >> >> On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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.
Hi Stephen, On Tue, Feb 12, 2013 at 12:13 PM, Stephen Warren <swarren@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@gmail.com> wrote: >>> Stephen, >>> >>> On Tue, Feb 5, 2013 at 1:03 PM, Stephen Warren <swarren@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
diff --git a/arch/arm/include/asm/arch-tegra/mmc.h b/arch/arm/include/asm/arch-tegra/mmc.h index 5c95047..344cdfb 100644 --- a/arch/arm/include/asm/arch-tegra/mmc.h +++ b/arch/arm/include/asm/arch-tegra/mmc.h @@ -22,6 +22,6 @@ #ifndef _TEGRA_MMC_H_ #define _TEGRA_MMC_H_ -int tegra_mmc_init(int dev_index, int bus_width, int pwr_gpio, int cd_gpio); +int tegra_mmc_init(int dev_index); #endif /* _TEGRA_MMC_H_ */ diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index dd746ca..c229e55 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -27,6 +27,8 @@ #define TEGRA_SDMMC3_BASE 0xC8000400 #define TEGRA_SDMMC4_BASE 0xC8000600 +#define MAX_HOSTS 4 /* Max number of 'hosts'/controllers */ + #ifndef __ASSEMBLY__ struct tegra_mmc { unsigned int sysad; /* _SYSTEM_ADDRESS_0 */ @@ -119,12 +121,16 @@ struct tegra_mmc { struct mmc_host { struct tegra_mmc *reg; + int enabled; /* 1 to enable, 0 to disable */ + int width; /* Bus Width, 1, 4 or 8 */ + int removable; /* Removable media (0 = eMMC, 1 = SD-card) */ + enum periph_id mmc_id; /* Peripheral ID: PERIPH_ID_... */ + struct fdt_gpio_state cd_gpio; /* Change Detect GPIO */ + struct fdt_gpio_state pwr_gpio; /* Power GPIO */ + struct fdt_gpio_state wp_gpio; /* Write Protect GPIO */ unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ unsigned int base; /* Base address, SDMMC1/2/3/4 */ - enum periph_id mmc_id; /* Peripheral ID: PERIPH_ID_... */ - int pwr_gpio; /* Power GPIO */ - int cd_gpio; /* Change Detect GPIO */ }; #endif /* __ASSEMBLY__ */ diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c index e6a932e..5e4bd43 100644 --- a/board/avionic-design/common/tamonten.c +++ b/board/avionic-design/common/tamonten.c @@ -69,8 +69,8 @@ int board_mmc_init(bd_t *bd) /* Enable muxes, etc. for SDMMC controllers */ pin_mux_mmc(); - /* init dev 0, SD slot, with 4-bit bus */ - tegra_mmc_init(0, 4, GPIO_PI6, GPIO_PH2); + /* init dev 0, SD slot */ + tegra_mmc_init(0); return 0; } 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; } diff --git a/board/compulab/trimslice/trimslice.c b/board/compulab/trimslice/trimslice.c index 8f4dd09..a1ec5e4 100644 --- a/board/compulab/trimslice/trimslice.c +++ b/board/compulab/trimslice/trimslice.c @@ -64,16 +64,16 @@ 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(); - /* init dev 0 (SDMMC4), (micro-SD slot) with 4-bit bus */ - tegra_mmc_init(0, 4, -1, GPIO_PP1); + /* init dev 0 (SDMMC4), (micro-SD slot) */ + tegra_mmc_init(0); - /* init dev 3 (SDMMC1), (SD slot) with 4-bit bus */ - tegra_mmc_init(3, 4, -1, -1); + /* init dev 3 (SDMMC1), (SD slot) */ + tegra_mmc_init(3); return 0; } diff --git a/board/nvidia/harmony/harmony.c b/board/nvidia/harmony/harmony.c index 93430ed..cec85a3 100644 --- a/board/nvidia/harmony/harmony.c +++ b/board/nvidia/harmony/harmony.c @@ -58,18 +58,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 SD slot J26\n"); + debug("%s: init SD slot J26\n", __func__); /* init dev 0, SD slot J26, with 8-bit bus */ - tegra_mmc_init(0, 8, GPIO_PI6, GPIO_PH2); + tegra_mmc_init(0); - debug("board_mmc_init: init SD slot J5\n"); - /* init dev 2, SD slot J5, with 4-bit bus */ - tegra_mmc_init(2, 4, GPIO_PT3, GPIO_PI5); + debug("%s: init SD slot J5\n", __func__); + /* init dev 2 */ + tegra_mmc_init(2); return 0; } diff --git a/board/nvidia/seaboard/seaboard.c b/board/nvidia/seaboard/seaboard.c index 3e33da0..f0dad8e 100644 --- a/board/nvidia/seaboard/seaboard.c +++ b/board/nvidia/seaboard/seaboard.c @@ -65,18 +65,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 1, SD slot, with 4-bit bus */ - tegra_mmc_init(1, 4, GPIO_PI6, GPIO_PI5); + debug("%s: init SD-card slot\n", __func__); + /* init dev 1, SD-card slot */ + tegra_mmc_init(1); return 0; } diff --git a/board/nvidia/whistler/whistler.c b/board/nvidia/whistler/whistler.c index 592cd6b..194dc1b 100644 --- a/board/nvidia/whistler/whistler.c +++ b/board/nvidia/whistler/whistler.c @@ -74,10 +74,10 @@ int board_mmc_init(bd_t *bd) pin_mux_mmc(); /* init dev 0 (SDMMC4), (J29 "HSMMC") with 8-bit bus */ - tegra_mmc_init(0, 8, -1, -1); + tegra_mmc_init(0); /* init dev 1 (SDMMC3), (J40 "SDIO3") with 8-bit bus */ - tegra_mmc_init(1, 8, -1, -1); + tegra_mmc_init(1); return 0; } diff --git a/board/toradex/colibri_t20_iris/colibri_t20_iris.c b/board/toradex/colibri_t20_iris/colibri_t20_iris.c index e40a986..456f201 100644 --- a/board/toradex/colibri_t20_iris/colibri_t20_iris.c +++ b/board/toradex/colibri_t20_iris/colibri_t20_iris.c @@ -39,7 +39,7 @@ int board_mmc_init(bd_t *bd) funcmux_select(PERIPH_ID_SDMMC4, FUNCMUX_SDMMC4_ATB_GMA_4_BIT); pinmux_tristate_disable(PINGRP_GMB); - tegra_mmc_init(0, 4, -1, GPIO_PC7); + tegra_mmc_init(0); return 0; } diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index d749ab0..24af636 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -21,6 +21,7 @@ #include <bouncebuf.h> #include <common.h> +#include <fdtdec.h> #include <asm/gpio.h> #include <asm/io.h> #include <asm/arch/clock.h> @@ -28,10 +29,14 @@ #include <asm/arch-tegra/tegra_mmc.h> #include <mmc.h> -/* support 4 mmc hosts */ -struct mmc mmc_dev[4]; -struct mmc_host mmc_host[4]; +DECLARE_GLOBAL_DATA_PTR; +struct mmc mmc_dev[MAX_HOSTS]; +struct mmc_host mmc_host[MAX_HOSTS]; + +#ifndef CONFIG_OF_CONTROL +#error "Please enable device tree support to use this driver" +#endif /** * Get the host address and peripheral ID for a device. Devices are numbered @@ -42,8 +47,60 @@ struct mmc_host mmc_host[4]; */ 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 + 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; + } + + node = node_list[dev_index]; + + host->enabled = fdtdec_get_is_enabled(gd->fdt_blob, node); + + host->reg = (struct tegra_mmc *)fdtdec_get_addr(gd->fdt_blob, + node, "reg"); + if ((fdt_addr_t)host->reg == FDT_ADDR_T_NONE) { + debug("%s: no sdmmc base reg info found\n", __func__); + return; + } + + host->mmc_id = clock_decode_periph_id(gd->fdt_blob, node); + if (host->mmc_id == PERIPH_ID_NONE) { + debug("%s: could not decode periph id\n", __func__); + return; + } + + /* + * 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__); + /* This flag isn't used anywhere in this driver or mmc.c */ + host->removable = fdtdec_get_int(gd->fdt_blob, node, "removable", 0); + + /* These GPIOs are optional */ + fdtdec_decode_gpio(gd->fdt_blob, node, "cd-gpios", &host->cd_gpio); + fdtdec_decode_gpio(gd->fdt_blob, node, "wp-gpios", &host->wp_gpio); + fdtdec_decode_gpio(gd->fdt_blob, node, "power-gpios", &host->pwr_gpio); + + host->base = (unsigned)host->reg; + + debug("%s: found controller at %p, width = %d, periph_id = %d\n", + __func__, host->reg, host->width, host->mmc_id); +#else switch (dev_index) { case 1: host->base = TEGRA_SDMMC3_BASE; @@ -63,8 +120,8 @@ static void tegra_get_setup(struct mmc_host *host, int dev_index) host->mmc_id = PERIPH_ID_SDMMC4; break; } - host->reg = (struct tegra_mmc *)host->base; +#endif /* !CONFIG_OF_CONTROL */ } static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data, @@ -72,10 +129,9 @@ static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data, { unsigned char ctrl; - - debug("buf: %p (%p), data->blocks: %u, data->blocksize: %u\n", - bbstate->bounce_buffer, bbstate->user_buffer, data->blocks, - data->blocksize); + debug("%s: buf: %p (%p), data->blocks: %u, data->blocksize: %u\n", + __func__, bbstate->bounce_buffer, bbstate->user_buffer, + data->blocks, data->blocksize); writel((u32)bbstate->bounce_buffer, &host->reg->sysad); /* @@ -98,7 +154,7 @@ static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data, static void mmc_set_transfer_mode(struct mmc_host *host, struct mmc_data *data) { unsigned short mode; - debug(" mmc_set_transfer_mode called\n"); + debug("%s called\n", __func__); /* * TRNMOD * MUL1SIN0[5] : Multi/Single Block Select @@ -121,10 +177,8 @@ static void mmc_set_transfer_mode(struct mmc_host *host, struct mmc_data *data) writew(mode, &host->reg->trnmod); } -static int mmc_wait_inhibit(struct mmc_host *host, - struct mmc_cmd *cmd, - struct mmc_data *data, - unsigned int timeout) +static int mmc_wait_inhibit(struct mmc_host *host, struct mmc_cmd *cmd, + struct mmc_data *data, unsigned int timeout) { /* * PRNSTS @@ -148,19 +202,18 @@ static int mmc_wait_inhibit(struct mmc_host *host, timeout--; udelay(1000); } - return 0; } static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, - struct mmc_data *data, struct bounce_buffer *bbstate) + struct mmc_data *data, struct bounce_buffer *bbstate) { struct mmc_host *host = (struct mmc_host *)mmc->priv; int flags, i; int result; unsigned int mask = 0; unsigned int retry = 0x100000; - debug(" mmc_send_cmd called\n"); + debug("%s called\n", __func__); result = mmc_wait_inhibit(host, cmd, data, 10 /* ms */); @@ -170,7 +223,7 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, if (data) mmc_prepare_data(host, data, bbstate); - debug("cmd->arg: %08x\n", cmd->cmdarg); + debug("%s: cmd->arg: %08x\n", __func__, cmd->cmdarg); writel(cmd->cmdarg, &host->reg->argument); if (data) @@ -207,7 +260,7 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, if (data) flags |= TEGRA_MMC_TRNMOD_DATA_PRESENT_SELECT_DATA_TRANSFER; - debug("cmd: %d\n", cmd->cmdidx); + debug("%s: cmd: %d\n", __func__, cmd->cmdidx); writew((cmd->cmdidx << 8) | flags, &host->reg->cmdreg); @@ -229,12 +282,14 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, if (mask & TEGRA_MMC_NORINTSTS_CMD_TIMEOUT) { /* Timeout Error */ - debug("timeout: %08x cmd %d\n", mask, cmd->cmdidx); + debug("%s: timeout: %08x cmd %d\n", __func__, mask, + cmd->cmdidx); writel(mask, &host->reg->norintsts); return TIMEOUT; } else if (mask & TEGRA_MMC_NORINTSTS_ERR_INTERRUPT) { /* Error Interrupt */ - debug("error: %08x cmd %d\n", mask, cmd->cmdidx); + debug("%s: error: %08x cmd %d\n", __func__, mask, + cmd->cmdidx); writel(mask, &host->reg->norintsts); return -1; } @@ -251,8 +306,8 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, cmd->response[i] |= readb(offset - 1); } - debug("cmd->resp[%d]: %08x\n", - i, cmd->response[i]); + debug("%s: cmd->resp[%d]: %08x\n", + __func__, i, cmd->response[i]); } } else if (cmd->resp_type & MMC_RSP_BUSY) { for (i = 0; i < retry; i++) { @@ -269,10 +324,12 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, } cmd->response[0] = readl(&host->reg->rspreg0); - debug("cmd->resp[0]: %08x\n", cmd->response[0]); + debug("%s: cmd->resp[0]: %08x\n", + __func__, cmd->response[0]); } else { cmd->response[0] = readl(&host->reg->rspreg0); - debug("cmd->resp[0]: %08x\n", cmd->response[0]); + debug("%s: cmd->resp[0]: %08x\n", + __func__, cmd->response[0]); } } @@ -295,13 +352,13 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, */ unsigned int address = readl(&host->reg->sysad); - debug("DMA end\n"); + debug("%s: DMA end\n", __func__); writel(TEGRA_MMC_NORINTSTS_DMA_INTERRUPT, - &host->reg->norintsts); + &host->reg->norintsts); writel(address, &host->reg->sysad); } else if (mask & TEGRA_MMC_NORINTSTS_XFER_COMPLETE) { /* Transfer Complete */ - debug("r/w is done\n"); + debug("%s: r/w is done\n", __func__); break; } else if (get_timer(start) > 2000UL) { writel(mask, &host->reg->norintsts); @@ -325,13 +382,14 @@ static int mmc_send_cmd_bounced(struct mmc *mmc, struct mmc_cmd *cmd, } static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, - struct mmc_data *data) + struct mmc_data *data) { void *buf; unsigned int bbflags; size_t len; struct bounce_buffer bbstate; int ret; + debug("%s: called\n", __func__); if (data) { if (data->flags & MMC_DATA_READ) { @@ -359,8 +417,7 @@ static void mmc_change_clock(struct mmc_host *host, uint clock) int div; unsigned short clk; unsigned long timeout; - - debug(" mmc_change_clock called\n"); + debug("%s called\n", __func__); /* * Change Tegra SDMMCx clock divisor here. Source is 216MHz, @@ -368,9 +425,8 @@ static void mmc_change_clock(struct mmc_host *host, uint clock) */ if (clock == 0) goto out; - clock_adjust_periph_pll_div(host->mmc_id, CLOCK_ID_PERIPH, clock, - &div); - debug("div = %d\n", div); + clock_adjust_periph_pll_div(host->mmc_id, CLOCK_ID_PERIPH, clock, &div); + debug("%s: div = %d\n", __func__, div); writew(0, &host->reg->clkcon); @@ -383,7 +439,7 @@ static void mmc_change_clock(struct mmc_host *host, uint clock) */ div >>= 1; clk = ((div << TEGRA_MMC_CLKCON_SDCLK_FREQ_SEL_SHIFT) | - TEGRA_MMC_CLKCON_INTERNAL_CLOCK_ENABLE); + TEGRA_MMC_CLKCON_INTERNAL_CLOCK_ENABLE); writew(clk, &host->reg->clkcon); /* Wait max 10 ms */ @@ -401,7 +457,7 @@ static void mmc_change_clock(struct mmc_host *host, uint clock) clk |= TEGRA_MMC_CLKCON_SD_CLOCK_ENABLE; writew(clk, &host->reg->clkcon); - debug("mmc_change_clock: clkcon = %08X\n", clk); + debug("%s: clkcon = %08X\n", __func__, clk); out: host->clock = clock; @@ -411,9 +467,8 @@ static void mmc_set_ios(struct mmc *mmc) { struct mmc_host *host = mmc->priv; unsigned char ctrl; - debug(" mmc_set_ios called\n"); - - debug("bus_width: %x, clock: %d\n", mmc->bus_width, mmc->clock); + debug("%s: bus_width: %x, clock: %d\n", __func__, mmc->bus_width, + mmc->clock); /* Change clock first */ mmc_change_clock(host, mmc->clock); @@ -436,13 +491,13 @@ static void mmc_set_ios(struct mmc *mmc) ctrl &= ~(1 << 1); writeb(ctrl, &host->reg->hostctl); - debug("mmc_set_ios: hostctl = %08X\n", ctrl); + debug("%s: hostctl = %08X\n", __func__, ctrl); } static void mmc_reset(struct mmc_host *host) { unsigned int timeout; - debug(" mmc_reset called\n"); + debug("%s called\n", __func__); /* * RSTALL[0] : Software reset for all @@ -471,12 +526,11 @@ static int mmc_core_init(struct mmc *mmc) { struct mmc_host *host = (struct mmc_host *)mmc->priv; unsigned int mask; - debug(" mmc_core_init called\n"); mmc_reset(host); host->version = readw(&host->reg->hcver); - debug("host version = %x\n", host->version); + debug("%s: host version = %x\n", __func__, host->version); /* mask all */ writel(0xffffffff, &host->reg->norintstsen); @@ -515,44 +569,49 @@ static int mmc_core_init(struct mmc *mmc) int tegra_mmc_getcd(struct mmc *mmc) { struct mmc_host *host = (struct mmc_host *)mmc->priv; + debug("%s called, host->cd_gpio = 0x%08X\n", __func__, + (unsigned)&host->cd_gpio); - debug("tegra_mmc_getcd called\n"); - - if (host->cd_gpio >= 0) - return !gpio_get_value(host->cd_gpio); + if (fdt_gpio_isvalid(&host->cd_gpio)) + return !fdtdec_get_gpio(&host->cd_gpio); return 1; } -int tegra_mmc_init(int dev_index, int bus_width, int pwr_gpio, int cd_gpio) +int tegra_mmc_init(int dev_index) { struct mmc_host *host; char gpusage[12]; /* "SD/MMCn PWR" or "SD/MMCn CD" */ struct mmc *mmc; - - debug(" tegra_mmc_init: index %d, bus width %d " - "pwr_gpio %d cd_gpio %d\n", - dev_index, bus_width, pwr_gpio, cd_gpio); + int card_det = 0; host = &mmc_host[dev_index]; - host->clock = 0; - host->pwr_gpio = pwr_gpio; - host->cd_gpio = cd_gpio; + /* Read DT and get config */ tegra_get_setup(host, dev_index); + if (!host->enabled) + return -1; + + debug("%s: index %d, bus width %d pwr_gpio %d cd_gpio %d\n", + __func__, dev_index, host->width, + host->pwr_gpio.gpio, host->cd_gpio.gpio); + host->clock = 0; clock_start_periph_pll(host->mmc_id, CLOCK_ID_PERIPH, 20000000); - if (host->pwr_gpio >= 0) { + if (fdt_gpio_isvalid(&host->pwr_gpio)) { sprintf(gpusage, "SD/MMC%d PWR", dev_index); - gpio_request(host->pwr_gpio, gpusage); - gpio_direction_output(host->pwr_gpio, 1); + gpio_request(host->pwr_gpio.gpio, gpusage); + fdtdec_set_gpio(&host->pwr_gpio, 1); + debug(" Power GPIO name = %s\n", host->pwr_gpio.name); } - if (host->cd_gpio >= 0) { + if (fdt_gpio_isvalid(&host->cd_gpio)) { sprintf(gpusage, "SD/MMC%d CD", dev_index); - gpio_request(host->cd_gpio, gpusage); - gpio_direction_input(host->cd_gpio); + gpio_request(host->cd_gpio.gpio, gpusage); + card_det = fdtdec_get_gpio(&host->cd_gpio); + debug(" CD GPIO name = %s\n", host->cd_gpio.name); + debug("%s: CD state = %d\n", __func__, card_det); } mmc = &mmc_dev[dev_index]; @@ -566,9 +625,10 @@ int tegra_mmc_init(int dev_index, int bus_width, int pwr_gpio, int cd_gpio) mmc->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195; mmc->host_caps = 0; - if (bus_width == 8) + debug("%s: bus width = %d\n", __func__, host->width); + if (host->width == 8) mmc->host_caps |= MMC_MODE_8BIT; - if (bus_width >= 4) + if (host->width >= 4) mmc->host_caps |= MMC_MODE_4BIT; mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS | MMC_MODE_HC; diff --git a/include/fdtdec.h b/include/fdtdec.h index f77d195..e16d6a5 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -61,6 +61,7 @@ struct fdt_memory { */ enum fdt_compat_id { COMPAT_UNKNOWN, + COMPAT_NVIDIA_TEGRA20_SDMMC, /* Tegra SDMMC controller */ COMPAT_NVIDIA_TEGRA20_USB, /* Tegra20 USB port */ COMPAT_NVIDIA_TEGRA20_I2C, /* Tegra20 i2c */ COMPAT_NVIDIA_TEGRA20_DVC, /* Tegra20 dvc (really just i2c) */ diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 16921e1..4cd18a8 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -36,6 +36,7 @@ DECLARE_GLOBAL_DATA_PTR; #define COMPAT(id, name) name static const char * const compat_names[COMPAT_COUNT] = { COMPAT(UNKNOWN, "<none>"), + COMPAT(NVIDIA_TEGRA20_SDMMC, "nvidia,tegra20-sdhci"), COMPAT(NVIDIA_TEGRA20_USB, "nvidia,tegra20-ehci"), COMPAT(NVIDIA_TEGRA20_I2C, "nvidia,tegra20-i2c"), COMPAT(NVIDIA_TEGRA20_DVC, "nvidia,tegra20-i2c-dvc"),
tegra_mmc_init() now uses DT info for bus width, WP/CD GPIOs, etc. Tested on Seaboard, fully functional. Signed-off-by: Tom Warren <twarren@nvidia.com> --- arch/arm/include/asm/arch-tegra/mmc.h | 2 +- arch/arm/include/asm/arch-tegra/tegra_mmc.h | 12 +- board/avionic-design/common/tamonten.c | 4 +- board/compal/paz00/paz00.c | 14 +- board/compulab/trimslice/trimslice.c | 10 +- board/nvidia/harmony/harmony.c | 12 +- board/nvidia/seaboard/seaboard.c | 14 +- board/nvidia/whistler/whistler.c | 4 +- board/toradex/colibri_t20_iris/colibri_t20_iris.c | 2 +- drivers/mmc/tegra_mmc.c | 186 ++++++++++++++------- include/fdtdec.h | 1 + lib/fdtdec.c | 1 + 12 files changed, 165 insertions(+), 97 deletions(-)