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

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

[Tom Warren]

This patchset adds device-tree support to the Tegra MMC driver.
All device config is done via properties in the DT files instead
of hard-coded config options/function arguments.

I've tested this on my Seaboard and everything works fine,
including card detect. For the other T20 boards, I've used
the Linux kernel DTS files for the sdhci nodes where there
wasn't one already, or expanded the info that was already
there. Everything builds fine, but I haven't tested anything
but Seaboard.

Tom Warren (2):
  Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files
  Tegra: MMC: Add DT support to MMC driver for all T20 boards

 arch/arm/dts/tegra20.dtsi                         |   12 ++
 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/avionic-design/dts/tegra20-medcom-wide.dts  |    9 +
 board/avionic-design/dts/tegra20-plutux.dts       |    9 +
 board/avionic-design/dts/tegra20-tec.dts          |    9 +
 board/compal/dts/tegra20-paz00.dts                |   22 +++-
 board/compal/paz00/paz00.c                        |   14 +-
 board/compulab/dts/tegra20-trimslice.dts          |   17 ++
 board/compulab/trimslice/trimslice.c              |   10 +-
 board/nvidia/dts/tegra20-harmony.dts              |   21 +++
 board/nvidia/dts/tegra20-seaboard.dts             |   19 ++-
 board/nvidia/dts/tegra20-ventana.dts              |   23 +++
 board/nvidia/dts/tegra20-whistler.dts             |   16 ++
 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 +-
 board/toradex/dts/tegra20-colibri_t20_iris.dts    |    8 +
 drivers/mmc/tegra_mmc.c                           |  186 ++++++++++++++-------
 include/fdtdec.h                                  |    1 +
 lib/fdtdec.c                                      |    1 +
 23 files changed, 322 insertions(+), 105 deletions(-)

[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Tom Warren]

Linux dts files were used for those boards that didn't already
have sdhci info populated. If a cd-gpio was present, I set
"removable = <1>", else removable = <0>.

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
 arch/arm/dts/tegra20.dtsi                        |   12 +++++++++++
 board/avionic-design/dts/tegra20-medcom-wide.dts |    9 ++++++++
 board/avionic-design/dts/tegra20-plutux.dts      |    9 ++++++++
 board/avionic-design/dts/tegra20-tec.dts         |    9 ++++++++
 board/compal/dts/tegra20-paz00.dts               |   22 +++++++++++++++++++-
 board/compulab/dts/tegra20-trimslice.dts         |   17 ++++++++++++++++
 board/nvidia/dts/tegra20-harmony.dts             |   21 ++++++++++++++++++++
 board/nvidia/dts/tegra20-seaboard.dts            |   19 ++++++++++++-----
 board/nvidia/dts/tegra20-ventana.dts             |   23 ++++++++++++++++++++++
 board/nvidia/dts/tegra20-whistler.dts            |   16 +++++++++++++++
 board/toradex/dts/tegra20-colibri_t20_iris.dts   |    8 +++++++
 11 files changed, 157 insertions(+), 8 deletions(-)

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index 12049fd..2603277 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -307,23 +307,35 @@
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000000 0x200>;
 		interrupts = < 46 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC1, PLLP_OUT? */
+		clocks = <&tegra_car 14>;
 	};
 
 	sdhci at c8000200 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000200 0x200>;
 		interrupts = < 47 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC2, PLLP_OUT? */
+		clocks = <&tegra_car 9>;
 	};
 
 	sdhci at c8000400 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000400 0x200>;
 		interrupts = < 51 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC3, PLLP_OUT? */
+		clocks = <&tegra_car 69>;
 	};
 
 	sdhci at c8000600 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000600 0x200>;
 		interrupts = < 63 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC4, PLLP_OUT? */
+		clocks = <&tegra_car 15>;
 	};
 };
diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
index e46afbe..33d174d 100644
--- a/board/avionic-design/dts/tegra20-medcom-wide.dts
+++ b/board/avionic-design/dts/tegra20-medcom-wide.dts
@@ -55,6 +55,15 @@
 		status = "disabled";
 	};
 
+	sdhci at c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
 	lcd_panel: panel {
 		clock = <61715000>;
 		xres = <1366>;
diff --git a/board/avionic-design/dts/tegra20-plutux.dts b/board/avionic-design/dts/tegra20-plutux.dts
index 3e6cce0..3a0147f 100644
--- a/board/avionic-design/dts/tegra20-plutux.dts
+++ b/board/avionic-design/dts/tegra20-plutux.dts
@@ -41,4 +41,13 @@
 	usb at c5004000 {
 		status = "disabled";
 	};
+
+	sdhci at c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		bus-width = <4>;
+		removable = <1>;
+	};
 };
diff --git a/board/avionic-design/dts/tegra20-tec.dts b/board/avionic-design/dts/tegra20-tec.dts
index bf3ff1d..521a255 100644
--- a/board/avionic-design/dts/tegra20-tec.dts
+++ b/board/avionic-design/dts/tegra20-tec.dts
@@ -66,6 +66,15 @@
 		status = "disabled";
 	};
 
+	sdhci at c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
 	lcd_panel: panel {
 		clock = <33260000>;
 		xres = <800>;
diff --git a/board/compal/dts/tegra20-paz00.dts b/board/compal/dts/tegra20-paz00.dts
index 31b064d..ed709c8 100644
--- a/board/compal/dts/tegra20-paz00.dts
+++ b/board/compal/dts/tegra20-paz00.dts
@@ -3,11 +3,13 @@
 /include/ ARCH_CPU_DTS
 
 / {
-        model = "Toshiba AC100 / Dynabook AZ";
-        compatible = "compal,paz00", "nvidia,tegra20";
+	model = "Toshiba AC100 / Dynabook AZ";
+	compatible = "compal,paz00", "nvidia,tegra20";
 
 	aliases {
 		usb0 = "/usb at c5008000";
+		sdmmc0 = "/sdhci at c8000600";
+		sdmmc1 = "/sdhci at c8000000";
 	};
 
 	memory {
@@ -53,6 +55,22 @@
 		status = "disabled";
 	};
 
+	sdhci at c8000000 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 173 0>;	/* gpio PV5 */
+		wp-gpios = <&gpio 57 0>;	/* gpio PH1 */
+		power-gpios = <&gpio 169 0>;	/* gpio PV1 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
+	sdhci at c8000600 {
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
+	};
+
 	lcd_panel: panel {
 		/* PAZ00 has 1024x600 */
 		clock = <54030000>;
diff --git a/board/compulab/dts/tegra20-trimslice.dts b/board/compulab/dts/tegra20-trimslice.dts
index c8a4dd4..0ab1c1d 100644
--- a/board/compulab/dts/tegra20-trimslice.dts
+++ b/board/compulab/dts/tegra20-trimslice.dts
@@ -9,6 +9,8 @@
 	aliases {
 		usb0 = "/usb at c5008000";
 		usb1 = "/usb at c5000000";
+		sdmmc0 = "/sdhci at c8000600";
+		sdmmc1 = "/sdhci at c8000000";
 	};
 
 	memory {
@@ -42,4 +44,19 @@
 	usb at c5004000 {
 		status = "disabled";
 	};
+
+	sdhci at c8000000 {
+		status = "okay";
+		bus-width = <4>;
+		removable = <0>;
+	};
+
+	sdhci at c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 121 0>;	/* gpio PP1 */
+		wp-gpios = <&gpio 122 0>;	/* gpio PP2 */
+		bus-width = <4>;
+		removable = <1>;
+	};
 };
diff --git a/board/nvidia/dts/tegra20-harmony.dts b/board/nvidia/dts/tegra20-harmony.dts
index aeda3a1..de7ebdc 100644
--- a/board/nvidia/dts/tegra20-harmony.dts
+++ b/board/nvidia/dts/tegra20-harmony.dts
@@ -9,6 +9,8 @@
 	aliases {
 		usb0 = "/usb at c5008000";
 		usb1 = "/usb at c5004000";
+		sdmmc0 = "/sdhci at c8000600";
+		sdmmc1 = "/sdhci at c8000200";
 	};
 
 	memory {
@@ -52,4 +54,23 @@
 	usb at c5004000 {
 		nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
 	};
+
+	sdhci at c8000200 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 69 0>;	/* gpio PI5 */
+		wp-gpios = <&gpio 57 0>;	/* gpio PH1 */
+		power-gpios = <&gpio 155 0>;	/* gpio PT3 */
+		bus-width = <4>;
+		removeable = <1>;
+	};
+
+	sdhci at c8000600 {
+		status = "okay";
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		power-gpios = <&gpio 70 0>;	/* gpio PI6 */
+		bus-width = <8>;
+		removeable = <1>;
+	};
 };
diff --git a/board/nvidia/dts/tegra20-seaboard.dts b/board/nvidia/dts/tegra20-seaboard.dts
index 527a296..1b1f563 100644
--- a/board/nvidia/dts/tegra20-seaboard.dts
+++ b/board/nvidia/dts/tegra20-seaboard.dts
@@ -12,14 +12,15 @@
 	};
 
 	aliases {
-		/* This defines the order of our USB ports */
+		/* This defines the order of our ports */
 		usb0 = "/usb at c5008000";
 		usb1 = "/usb at c5000000";
-
 		i2c0 = "/i2c at 7000d000";
 		i2c1 = "/i2c at 7000c000";
 		i2c2 = "/i2c at 7000c400";
 		i2c3 = "/i2c at 7000c500";
+		sdmmc0 = "/sdhci at c8000600";
+		sdmmc1 = "/sdhci at c8000400";
 	};
 
 	memory {
@@ -156,13 +157,19 @@
 	};
 
 	sdhci at c8000400 {
-		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
-		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
-		power-gpios = <&gpio 70 0>; /* gpio PI6 */
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 69 0>;	/* card detect, gpio PI5 */
+		wp-gpios = <&gpio 57 0>;	/* write protect, gpio PH1 */
+		power-gpios = <&gpio 70 3>;	/* power enable, gpio PI6 */
+		bus-width = <4>;
+		removable = <1>;
 	};
 
 	sdhci at c8000600 {
-		support-8bit;
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
 	};
 
 	lcd_panel: panel {
diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
index 3e5e39d..fed27ab 100644
--- a/board/nvidia/dts/tegra20-ventana.dts
+++ b/board/nvidia/dts/tegra20-ventana.dts
@@ -41,4 +41,27 @@
 	usb at c5004000 {
 		status = "disabled";
 	};
+
+	sdhci at c8000000 {
+		status = "okay";
+		power-gpios = <&gpio 86 0>;	/* gpio PK6 */
+		bus-width = <4>;
+		removable = <0>;
+	};
+
+	sdhci at c8000400 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 69 0>;	/* gpio PI5 */
+		wp-gpios = <&gpio 57 0>;	/* gpio PH1 */
+		power-gpios = <&gpio 70 0>;	/* gpio PI6 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
+	sdhci at c8000600 {
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
+	};
 };
diff --git a/board/nvidia/dts/tegra20-whistler.dts b/board/nvidia/dts/tegra20-whistler.dts
index 4579557..ae198bd 100644
--- a/board/nvidia/dts/tegra20-whistler.dts
+++ b/board/nvidia/dts/tegra20-whistler.dts
@@ -9,6 +9,8 @@
 	aliases {
 		i2c0 = "/i2c at 7000d000";
 		usb0 = "/usb at c5008000";
+		sdmmc0 = "/sdhci at c8000600";
+		sdmmc1 = "/sdhci at c8000400";
 	};
 
 	memory {
@@ -57,4 +59,18 @@
 	usb at c5004000 {
 		status = "disabled";
 	};
+
+	sdhci at c8000400 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		wp-gpios = <&gpio 173 0>;	/* gpio PV5 */
+		bus-width = <8>;
+		removable = <1>;
+	};
+
+	sdhci at c8000600 {
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
+	};
 };
diff --git a/board/toradex/dts/tegra20-colibri_t20_iris.dts b/board/toradex/dts/tegra20-colibri_t20_iris.dts
index c29b43a..1689e1a 100644
--- a/board/toradex/dts/tegra20-colibri_t20_iris.dts
+++ b/board/toradex/dts/tegra20-colibri_t20_iris.dts
@@ -35,4 +35,12 @@
 			compatible = "nand-flash";
 		};
 	};
+
+	sdhci at c8000400 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cp-gpios = <&gpio 39 0>;	/* gpio PC7 */
+		bus-width = <4>;
+		removable = <1>;
+	};
 };
-- 
1.7.0.4

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

[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/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"),
-- 
1.7.0.4

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

[Tom Warren]

Folks with Tegra20-based boards (Compulab, Avionics Design, etc.) -
please check my DT node settings for address, GPIOs (CD, WP, and
power, if utilized), and width. I tried to keep everything the same as
the tegra_mmc_init() function args that were removed, plus integrating
what was in the Linux kernel DTS files for your board(s). Please also
do a quick functional test to ensure everything still works as before
with the MMC devices on your board(s).

Stephen, please check the boards you are familiar with (Whistler,
PAZ00, Trimslice, Harmony) that I may not have here in AZ.

Thanks,

Tom

On Mon, Feb 4, 2013 at 4:48 PM, Tom Warren <twarren.nvidia@gmail.com> wrote:
> This patchset adds device-tree support to the Tegra MMC driver.
> All device config is done via properties in the DT files instead
> of hard-coded config options/function arguments.
>
> I've tested this on my Seaboard and everything works fine,
> including card detect. For the other T20 boards, I've used
> the Linux kernel DTS files for the sdhci nodes where there
> wasn't one already, or expanded the info that was already
> there. Everything builds fine, but I haven't tested anything
> but Seaboard.
>
> Tom Warren (2):
>   Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files
>   Tegra: MMC: Add DT support to MMC driver for all T20 boards
>
>  arch/arm/dts/tegra20.dtsi                         |   12 ++
>  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/avionic-design/dts/tegra20-medcom-wide.dts  |    9 +
>  board/avionic-design/dts/tegra20-plutux.dts       |    9 +
>  board/avionic-design/dts/tegra20-tec.dts          |    9 +
>  board/compal/dts/tegra20-paz00.dts                |   22 +++-
>  board/compal/paz00/paz00.c                        |   14 +-
>  board/compulab/dts/tegra20-trimslice.dts          |   17 ++
>  board/compulab/trimslice/trimslice.c              |   10 +-
>  board/nvidia/dts/tegra20-harmony.dts              |   21 +++
>  board/nvidia/dts/tegra20-seaboard.dts             |   19 ++-
>  board/nvidia/dts/tegra20-ventana.dts              |   23 +++
>  board/nvidia/dts/tegra20-whistler.dts             |   16 ++
>  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 +-
>  board/toradex/dts/tegra20-colibri_t20_iris.dts    |    8 +
>  drivers/mmc/tegra_mmc.c                           |  186 ++++++++++++++-------
>  include/fdtdec.h                                  |    1 +
>  lib/fdtdec.c                                      |    1 +
>  23 files changed, 322 insertions(+), 105 deletions(-)
>

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

[Marc Dietrich]

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

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

[Thierry Reding]

On Mon, Feb 04, 2013 at 04:48:53PM -0700, Tom Warren wrote:
> This patchset adds device-tree support to the Tegra MMC driver.
> All device config is done via properties in the DT files instead
> of hard-coded config options/function arguments.
> 
> I've tested this on my Seaboard and everything works fine,
> including card detect. For the other T20 boards, I've used
> the Linux kernel DTS files for the sdhci nodes where there
> wasn't one already, or expanded the info that was already
> there. Everything builds fine, but I haven't tested anything
> but Seaboard.
> 
> Tom Warren (2):
>   Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files
>   Tegra: MMC: Add DT support to MMC driver for all T20 boards

I've tested on TEC only, but since Medcom-Wide and Plutux are also based
on Tamonten they should be good as well, so:

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130205/48a8cfd0/attachment.pgp>

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

[Tom Warren]

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

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

[Tom Warren]

Thierry,

On Tue, Feb 5, 2013 at 3:21 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Mon, Feb 04, 2013 at 04:48:53PM -0700, Tom Warren wrote:
>> This patchset adds device-tree support to the Tegra MMC driver.
>> All device config is done via properties in the DT files instead
>> of hard-coded config options/function arguments.
>>
>> I've tested this on my Seaboard and everything works fine,
>> including card detect. For the other T20 boards, I've used
>> the Linux kernel DTS files for the sdhci nodes where there
>> wasn't one already, or expanded the info that was already
>> there. Everything builds fine, but I haven't tested anything
>> but Seaboard.
>>
>> Tom Warren (2):
>>   Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files
>>   Tegra: MMC: Add DT support to MMC driver for all T20 boards
>
> I've tested on TEC only, but since Medcom-Wide and Plutux are also based
> on Tamonten they should be good as well, so:
>
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

Great! Thanks

Tom

[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Stephen Warren]

On 02/04/2013 04:48 PM, Tom Warren wrote:
> Linux dts files were used for those boards that didn't already
> have sdhci info populated. If a cd-gpio was present, I set
> "removable = <1>", else removable = <0>.

> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi

>  	sdhci at c8000200 {
>  		compatible = "nvidia,tegra20-sdhci";
>  		reg = <0xc8000200 0x200>;
>  		interrupts = < 47 >;
> +		status = "disabled";
> +		/* PERIPH_ID_SDMMC2, PLLP_OUT? */
> +		clocks = <&tegra_car 9>;
>  	};

What does "PLLP_OUT?" mean?

I'm not entirely convinced it's a good idea to add this comment, since
it creates a diff between the .dts files in the kernel and U-Boot.

Similarly, the status and clocks properties are in the other order in
the kernel .dts files. It'd be good to be consistent to allow minimal
diffs between them.

> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts

I suppose that there are no aliases in this file because only one SDHCI
controller is enabled. I wonder if we should add aliases to all .dts
files just to be explicit? Perhaps it's not necessary because this board
really will never ever get another SDHCI controller added (I assume that
any SDHCI controllers the board has are already enabled, although I
wonder about SDIO...), so there doesn't need to be a "hint" that there
should be an alias added too?

> +	sdhci at c8000600 {

In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
its parameters are defined by the carrier board. I think U-Boot .dts
files should match.


The following 3 comments apply to all the .dts files (or at least the
1st and 3rd; not sure about the 2nd):

> +		status = "okay";
> +		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */

I don't think that comment is particularly useful. While it's true,
duplicating it every single place a GPIO is used seems wasteful. And the
comment is more re: the GPIO binding that anything to do with SDHCI.
Plus, it makes another diff relative to the kernel.

> +		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
> +		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */

The kernel appears to have a space before the comment not a TAB, so this
makes another diff..

> +		bus-width = <4>;
> +		removable = <1>;

What is "removable"? That's not in the binding documentation. There is a
"non-removable" property defined in the kernel's
Documentation/devicetree/bindings/mmc/mmc.txt though...

> +	};

> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts

This file doesn't have any aliases added.

> +	sdhci at c8000000 {
> +		status = "okay";
> +		power-gpios = <&gpio 86 0>;	/* gpio PK6 */
> +		bus-width = <4>;
> +		removable = <0>;
> +	};

I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.

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

[Stephen Warren]

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/.

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

[Marc Dietrich]

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

[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Tom Warren]

Stephen,

On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/04/2013 04:48 PM, Tom Warren wrote:
>> Linux dts files were used for those boards that didn't already
>> have sdhci info populated. If a cd-gpio was present, I set
>> "removable = <1>", else removable = <0>.
>
>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>
>>       sdhci at c8000200 {
>>               compatible = "nvidia,tegra20-sdhci";
>>               reg = <0xc8000200 0x200>;
>>               interrupts = < 47 >;
>> +             status = "disabled";
>> +             /* PERIPH_ID_SDMMC2, PLLP_OUT? */
>> +             clocks = <&tegra_car 9>;
>>       };
>
> What does "PLLP_OUT?" mean?

The clock source used for this periph. I removed it in the I2C DT
files - I'll remove it here, too, because it's up to the driver to
choose that based on the freq.

>
> I'm not entirely convinced it's a good idea to add this comment, since
> it creates a diff between the .dts files in the kernel and U-Boot.
>
> Similarly, the status and clocks properties are in the other order in
> the kernel .dts files. It'd be good to be consistent to allow minimal
> diffs between them.

I used the kernel DTS files you pointed to in our internal 'Initialize
SDHCI from device tree' bug:
repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
branch for-next

The order matches arch/arm/boot/dts/tegra20.dtsi. I don't see a clocks
property in that kernel file, either. I do see that the interrupts
(that were already in tegra20.dtsi before I edited it) don't match the
kernel dtsi for most peripherals (i2c, i2s, sdhci, gpio). I'll fix the
SDHCI ones in the next patchset, though.

>
>> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
>
> I suppose that there are no aliases in this file because only one SDHCI
> controller is enabled. I wonder if we should add aliases to all .dts
> files just to be explicit? Perhaps it's not necessary because this board
> really will never ever get another SDHCI controller added (I assume that
> any SDHCI controllers the board has are already enabled, although I
> wonder about SDIO...), so there doesn't need to be a "hint" that there
> should be an alias added too?

If there was already an alias property in the DT file, then I tried to
be consistent and add one for mmc. But adding aliases to
other-than-NVIDIA boards should be up to the board
maintainer/manufacturer, IMO.

>
>> +     sdhci at c8000600 {
>
> In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
> its parameters are defined by the carrier board. I think U-Boot .dts
> files should match.

Saw that, but I didn't replicate it here because, well, U-Boot's Not
Linux, and IMO each board file (dts) should have its periph nodes
called out explicitly. If they all happen to be exactly the same for
each board, then I think the manufacturer/board maintainer can do that
'optimization' if they wish - they know better than I if they're
coming out with a new board that may differ in its SDHCI properties
(GPIOs, for instance).

>
>
> The following 3 comments apply to all the .dts files (or at least the
> 1st and 3rd; not sure about the 2nd):
>
>> +             status = "okay";
>> +             /* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
>
> I don't think that comment is particularly useful. While it's true,
> duplicating it every single place a GPIO is used seems wasteful. And the
> comment is more re: the GPIO binding that anything to do with SDHCI.
> Plus, it makes another diff relative to the kernel.

Diffing the DTS files against the kernel isn't really that big a deal
with a decent diff program (kompare, etc.). That GPIO comment is
useful if you need to understand the 3rd param for the pwr-gpios, for
instance (the cd and wp gpios almost always are input/low). And it
only appears once in each DTS file, not "in every single place a GPIO
is used".
>
>> +             cd-gpios = <&gpio 58 0>;        /* gpio PH2 */
>> +             wp-gpios = <&gpio 59 0>;        /* gpio PH3 */
>
> The kernel appears to have a space before the comment not a TAB, so this
> makes another diff..
Really? That seems a little nit-picky. :/
My whitespace is consistent through-out the DTS file, and with how I
always space comments on the end of a line of 'code'.

>
>> +             bus-width = <4>;
>> +             removable = <1>;
>
> What is "removable"? That's not in the binding documentation. There is a
> "non-removable" property defined in the kernel's
> Documentation/devicetree/bindings/mmc/mmc.txt though...

These are left-over from Seaboard's original DTS file (with which I
did my original development). It isn't used anywhere in U-Boot that I
can see. I can remove it.

>
>> +     };
>
>> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
>
> This file doesn't have any aliases added.
>
>> +     sdhci at c8000000 {
>> +             status = "okay";
>> +             power-gpios = <&gpio 86 0>;     /* gpio PK6 */
>> +             bus-width = <4>;
>> +             removable = <0>;
>> +     };
>
> I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
I'll remove it.

Tom

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

[Tom Warren]

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
>
>
>

[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Stephen Warren]

On 02/05/2013 01:29 PM, Tom Warren wrote:
> Stephen,
> 
> On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/04/2013 04:48 PM, Tom Warren wrote:
>>> Linux dts files were used for those boards that didn't already
>>> have sdhci info populated. If a cd-gpio was present, I set
>>> "removable = <1>", else removable = <0>.
>>
>>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>>
>>>       sdhci at c8000200 {
>>>               compatible = "nvidia,tegra20-sdhci";
>>>               reg = <0xc8000200 0x200>;
>>>               interrupts = < 47 >;
>>> +             status = "disabled";
>>> +             /* PERIPH_ID_SDMMC2, PLLP_OUT? */
>>> +             clocks = <&tegra_car 9>;
>>>       };
>>
>> What does "PLLP_OUT?" mean?
> 
> The clock source used for this periph. I removed it in the I2C DT
> files - I'll remove it here, too, because it's up to the driver to
> choose that based on the freq.
> 
>>
>> I'm not entirely convinced it's a good idea to add this comment, since
>> it creates a diff between the .dts files in the kernel and U-Boot.
>>
>> Similarly, the status and clocks properties are in the other order in
>> the kernel .dts files. It'd be good to be consistent to allow minimal
>> diffs between them.
> 
> I used the kernel DTS files you pointed to in our internal 'Initialize
> SDHCI from device tree' bug:
> repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
> branch for-next

linux-next or the Tegra git tree have the latest additions. arm-soc
hopefully will have them merged in the next day or two.

git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(whatever the latest tag is there)

git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git
(for-next)

>>> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
>>
>> I suppose that there are no aliases in this file because only one SDHCI
>> controller is enabled. I wonder if we should add aliases to all .dts
>> files just to be explicit? Perhaps it's not necessary because this board
>> really will never ever get another SDHCI controller added (I assume that
>> any SDHCI controllers the board has are already enabled, although I
>> wonder about SDIO...), so there doesn't need to be a "hint" that there
>> should be an alias added too?
> 
> If there was already an alias property in the DT file, then I tried to
> be consistent and add one for mmc. But adding aliases to
> other-than-NVIDIA boards should be up to the board
> maintainer/manufacturer, IMO.

I don't think so; at least with the driver as-is, the code appears not
to work without aliases, so not adding them causes a regression. Even
ignoring that, I don't see why the code->DT conversion patch shouldn't
do this for all boards.

>>> +     sdhci at c8000600 {
>>
>> In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
>> its parameters are defined by the carrier board. I think U-Boot .dts
>> files should match.
> 
> Saw that, but I didn't replicate it here because, well, U-Boot's Not
> Linux, and IMO each board file (dts) should have its periph nodes
> called out explicitly. If they all happen to be exactly the same for
> each board, then I think the manufacturer/board maintainer can do that
> 'optimization' if they wish - they know better than I if they're
> coming out with a new board that may differ in its SDHCI properties
> (GPIOs, for instance).

No, this has nothing to do with U-Boot vs. Linux. The device tree files
are (should eventually be) standard between the two, and indeed hosted
outside U-Boot. Unrelated, common code is common and should be
represented at a common location. In this case, the vendor for this
particular file has already correctly chosen to put the SDHCI nodes in
the common file, so this needs to be maintained.

>> The following 3 comments apply to all the .dts files (or at least the
>> 1st and 3rd; not sure about the 2nd):
>>
>>> +             status = "okay";
>>> +             /* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
>>
>> I don't think that comment is particularly useful. While it's true,
>> duplicating it every single place a GPIO is used seems wasteful. And the
>> comment is more re: the GPIO binding that anything to do with SDHCI.
>> Plus, it makes another diff relative to the kernel.
> 
> Diffing the DTS files against the kernel isn't really that big a deal
> with a decent diff program (kompare, etc.). That GPIO comment is
> useful if you need to understand the 3rd param for the pwr-gpios, for
> instance (the cd and wp gpios almost always are input/low). And it
> only appears once in each DTS file, not "in every single place a GPIO
> is used".

If there is no diff at all, it's even easier.

The third parameter is already documented in the binding documentation.

>>> +             cd-gpios = <&gpio 58 0>;        /* gpio PH2 */
>>> +             wp-gpios = <&gpio 59 0>;        /* gpio PH3 */
>>
>> The kernel appears to have a space before the comment not a TAB, so this
>> makes another diff..
>
> Really? That seems a little nit-picky. :/
> My whitespace is consistent through-out the DTS file, and with how I
> always space comments on the end of a line of 'code'.

Yes, really. Why would I bother to make this review comment otherwise.
As I have repeatedly specified, the idea is to reduce the diff between
the kernel and U-Boot .dts files so the diff for a node is zero. There's
a big difference between zero (no manual checking required at all) vs.
even something trivial (always requires manual checking every time a
comparison is made).

Note that at some point, all these .dts files are probably going to be
removed from the kernel and U-Boot anyway, and hosted separately, so
unifying them can only help there.

Sometimes I wonder why I even both reviewing things.

>>> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
>>
>> This file doesn't have any aliases added.
>>
>>> +     sdhci at c8000000 {
>>> +             status = "okay";
>>> +             power-gpios = <&gpio 86 0>;     /* gpio PK6 */
>>> +             bus-width = <4>;
>>> +             removable = <0>;
>>> +     };
>>
>> I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
>
> It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
> I'll remove it.

Perhaps, but as I said before, whole nodes present/missing is a lot
easier to deal with than diffs within nodes.

Right now, I believe your/Simon's policy on DT is to only include in the
U-Boot .dts files what's actually needed for U-Boot. I've asked that
this be done on a per-node basis rather than per-property basis in order
to reduce diffs. If you want to change that, and include nodes that
U-Boot doesn't need, that'd be great and assist unification, but then
I'd recommend simply importing the current kernel .dts files as-is
without any changes, rather than adding things piece-meal.

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

[Stephen Warren]

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.

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

[Marc Dietrich]

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

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

[Tom Warren]

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

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

[Tom Warren]

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
>
>

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

[Stephen Warren]

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.

[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Simon Glass]

Hi,

On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/05/2013 01:29 PM, Tom Warren wrote:
>> Stephen,
>>
>> On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 02/04/2013 04:48 PM, Tom Warren wrote:
>>>> Linux dts files were used for those boards that didn't already
>>>> have sdhci info populated. If a cd-gpio was present, I set
>>>> "removable = <1>", else removable = <0>.
>>>
>>>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>>>
>>>>       sdhci at c8000200 {
>>>>               compatible = "nvidia,tegra20-sdhci";
>>>>               reg = <0xc8000200 0x200>;
>>>>               interrupts = < 47 >;
>>>> +             status = "disabled";
>>>> +             /* PERIPH_ID_SDMMC2, PLLP_OUT? */
>>>> +             clocks = <&tegra_car 9>;
>>>>       };
>>>
>>> What does "PLLP_OUT?" mean?
>>
>> The clock source used for this periph. I removed it in the I2C DT
>> files - I'll remove it here, too, because it's up to the driver to
>> choose that based on the freq.
>>
>>>
>>> I'm not entirely convinced it's a good idea to add this comment, since
>>> it creates a diff between the .dts files in the kernel and U-Boot.
>>>
>>> Similarly, the status and clocks properties are in the other order in
>>> the kernel .dts files. It'd be good to be consistent to allow minimal
>>> diffs between them.
>>
>> I used the kernel DTS files you pointed to in our internal 'Initialize
>> SDHCI from device tree' bug:
>> repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
>> branch for-next
>
> linux-next or the Tegra git tree have the latest additions. arm-soc
> hopefully will have them merged in the next day or two.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> (whatever the latest tag is there)
>
> git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git
> (for-next)
>
>>>> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
>>>
>>> I suppose that there are no aliases in this file because only one SDHCI
>>> controller is enabled. I wonder if we should add aliases to all .dts
>>> files just to be explicit? Perhaps it's not necessary because this board
>>> really will never ever get another SDHCI controller added (I assume that
>>> any SDHCI controllers the board has are already enabled, although I
>>> wonder about SDIO...), so there doesn't need to be a "hint" that there
>>> should be an alias added too?
>>
>> If there was already an alias property in the DT file, then I tried to
>> be consistent and add one for mmc. But adding aliases to
>> other-than-NVIDIA boards should be up to the board
>> maintainer/manufacturer, IMO.
>
> I don't think so; at least with the driver as-is, the code appears not
> to work without aliases, so not adding them causes a regression. Even
> ignoring that, I don't see why the code->DT conversion patch shouldn't
> do this for all boards.
>
>>>> +     sdhci at c8000600 {
>>>
>>> In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
>>> its parameters are defined by the carrier board. I think U-Boot .dts
>>> files should match.
>>
>> Saw that, but I didn't replicate it here because, well, U-Boot's Not
>> Linux, and IMO each board file (dts) should have its periph nodes
>> called out explicitly. If they all happen to be exactly the same for
>> each board, then I think the manufacturer/board maintainer can do that
>> 'optimization' if they wish - they know better than I if they're
>> coming out with a new board that may differ in its SDHCI properties
>> (GPIOs, for instance).
>
> No, this has nothing to do with U-Boot vs. Linux. The device tree files
> are (should eventually be) standard between the two, and indeed hosted
> outside U-Boot. Unrelated, common code is common and should be
> represented at a common location. In this case, the vendor for this
> particular file has already correctly chosen to put the SDHCI nodes in
> the common file, so this needs to be maintained.
>
>>> The following 3 comments apply to all the .dts files (or at least the
>>> 1st and 3rd; not sure about the 2nd):
>>>
>>>> +             status = "okay";
>>>> +             /* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
>>>
>>> I don't think that comment is particularly useful. While it's true,
>>> duplicating it every single place a GPIO is used seems wasteful. And the
>>> comment is more re: the GPIO binding that anything to do with SDHCI.
>>> Plus, it makes another diff relative to the kernel.
>>
>> Diffing the DTS files against the kernel isn't really that big a deal
>> with a decent diff program (kompare, etc.). That GPIO comment is
>> useful if you need to understand the 3rd param for the pwr-gpios, for
>> instance (the cd and wp gpios almost always are input/low). And it
>> only appears once in each DTS file, not "in every single place a GPIO
>> is used".
>
> If there is no diff at all, it's even easier.
>
> The third parameter is already documented in the binding documentation.
>
>>>> +             cd-gpios = <&gpio 58 0>;        /* gpio PH2 */
>>>> +             wp-gpios = <&gpio 59 0>;        /* gpio PH3 */
>>>
>>> The kernel appears to have a space before the comment not a TAB, so this
>>> makes another diff..
>>
>> Really? That seems a little nit-picky. :/
>> My whitespace is consistent through-out the DTS file, and with how I
>> always space comments on the end of a line of 'code'.
>
> Yes, really. Why would I bother to make this review comment otherwise.
> As I have repeatedly specified, the idea is to reduce the diff between
> the kernel and U-Boot .dts files so the diff for a node is zero. There's
> a big difference between zero (no manual checking required at all) vs.
> even something trivial (always requires manual checking every time a
> comparison is made).
>
> Note that at some point, all these .dts files are probably going to be
> removed from the kernel and U-Boot anyway, and hosted separately, so
> unifying them can only help there.
>
> Sometimes I wonder why I even both reviewing things.
>
>>>> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
>>>
>>> This file doesn't have any aliases added.
>>>
>>>> +     sdhci at c8000000 {
>>>> +             status = "okay";
>>>> +             power-gpios = <&gpio 86 0>;     /* gpio PK6 */
>>>> +             bus-width = <4>;
>>>> +             removable = <0>;
>>>> +     };
>>>
>>> I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
>>
>> It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
>> I'll remove it.
>
> Perhaps, but as I said before, whole nodes present/missing is a lot
> easier to deal with than diffs within nodes.
>
> Right now, I believe your/Simon's policy on DT is to only include in the
> U-Boot .dts files what's actually needed for U-Boot. I've asked that
> this be done on a per-node basis rather than per-property basis in order
> to reduce diffs. If you want to change that, and include nodes that
> U-Boot doesn't need, that'd be great and assist unification, but then
> I'd recommend simply importing the current kernel .dts files as-is
> without any changes, rather than adding things piece-meal.

I have to say that within reason I like the idea of bring in the DT
from the kernel as is, limited perhaps to the nodes that U-Boot
actually uses.

A separate repo for the DT files seems like something that should
happen, but I have seen little progress on that front. Still, when it
happens, it would be nice it we could drop U-Boot's files and just use
the kernel's. That will be a lot easier if we head in that direction
now.

Regards,
Simon

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Scott Wood]

On 02/05/2013 10:56:59 PM, Simon Glass wrote:
> Hi,
> 
> On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren  
> <swarren@wwwdotorg.org> wrote:
> > Right now, I believe your/Simon's policy on DT is to only include  
> in the
> > U-Boot .dts files what's actually needed for U-Boot. I've asked that
> > this be done on a per-node basis rather than per-property basis in  
> order
> > to reduce diffs. If you want to change that, and include nodes that
> > U-Boot doesn't need, that'd be great and assist unification, but  
> then
> > I'd recommend simply importing the current kernel .dts files as-is
> > without any changes, rather than adding things piece-meal.
> 
> I have to say that within reason I like the idea of bring in the DT
> from the kernel as is, limited perhaps to the nodes that U-Boot
> actually uses.
> 
> A separate repo for the DT files seems like something that should
> happen, but I have seen little progress on that front. Still, when it
> happens, it would be nice it we could drop U-Boot's files and just use
> the kernel's. That will be a lot easier if we head in that direction
> now.

I think any device tree that makes assumptions about what U-Boot will  
be fixing up, or even what addresses U-Boot will configure devices at,  
belongs in the U-Boot tree.  Keeping such trees in Linux has been  
awkward so far, especially when a change gets made to such an  
assumption, or when U-Boot isn't the only supported firmware.

-Scott

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

[Simon Glass]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot


Regards,
Simon

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

[Simon Glass]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon

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

[Tom Warren]

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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon

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

[Simon Glass]

Hi Tom,

On Tue, Feb 12, 2013 at 11:05 AM, Tom Warren <twarren.nvidia@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 at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>> Regards,
>> Simon

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

[Stephen Warren]

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.

[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@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