[PATCH 0/2] mmc: sdhci: Defer probe if regulators are not registered

[PATCH 0/2] mmc: sdhci: Defer probe if regulators are not registered

[Pavan Kunapuli]

Regulator driver registration may sometimes be done after
mmc driver registration. In such cases, defer mmc probe
to obtain vmmc and vmmcq regulators.
Added vmmc and vmmcq supplies to tegra dt files.

Pavan Kunapuli (2):
  ARM: dt: tegra: Add sdhci regulators
  mmc: sdhci: Defer probe if regulator_get fails

 arch/arm/boot/dts/tegra20-harmony.dts    |   27 ++++++++++++++++++++-
 arch/arm/boot/dts/tegra20-paz00.dts      |   24 +++++++++++++++++++-
 arch/arm/boot/dts/tegra20-seaboard.dts   |   35 ++++++++++++++++++++++++++++-
 arch/arm/boot/dts/tegra20-tamonten.dtsi  |   10 ++++++++
 arch/arm/boot/dts/tegra20-ventana.dts    |   36 ++++++++++++++++++++++++++++-
 arch/arm/boot/dts/tegra20-whistler.dts   |    9 +++++-
 arch/arm/boot/dts/tegra30-cardhu-a02.dts |    2 +
 arch/arm/boot/dts/tegra30-cardhu-a04.dts |    2 +
 arch/arm/boot/dts/tegra30-cardhu.dtsi    |   19 ++++++++++++++-
 drivers/mmc/host/sdhci.c                 |   25 ++++++++++++++++++--
 10 files changed, 176 insertions(+), 13 deletions(-)

[PATCH 1/2] ARM: dt: tegra: Add sdhci regulators

[Pavan Kunapuli]

Adding vmmc and vmmcq supplies for sdhci nodes
in tegra dt files.

Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com>
---
 arch/arm/boot/dts/tegra20-harmony.dts    |   27 ++++++++++++++++++++-
 arch/arm/boot/dts/tegra20-paz00.dts      |   24 +++++++++++++++++++-
 arch/arm/boot/dts/tegra20-seaboard.dts   |   35 ++++++++++++++++++++++++++++-
 arch/arm/boot/dts/tegra20-tamonten.dtsi  |   10 ++++++++
 arch/arm/boot/dts/tegra20-ventana.dts    |   36 ++++++++++++++++++++++++++++-
 arch/arm/boot/dts/tegra20-whistler.dts   |    9 +++++-
 arch/arm/boot/dts/tegra30-cardhu-a02.dts |    2 +
 arch/arm/boot/dts/tegra30-cardhu-a04.dts |    2 +
 arch/arm/boot/dts/tegra30-cardhu.dtsi    |   19 ++++++++++++++-
 9 files changed, 154 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-harmony.dts b/arch/arm/boot/dts/tegra20-harmony.dts
index 74b8a47..5b02afe 100644
--- a/arch/arm/boot/dts/tegra20-harmony.dts
+++ b/arch/arm/boot/dts/tegra20-harmony.dts
@@ -419,7 +419,8 @@
 		status = "okay";
 		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
 		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
-		power-gpios = <&gpio 155 0>; /* gpio PT3 */
+		vmmc-supply = <&vddio_sd_reg>;
+		vqmmc-supply = <&vddio_sd_reg>;
 		bus-width = <4>;
 	};
 
@@ -427,7 +428,8 @@
 		status = "okay";
 		cd-gpios = <&gpio 58 0>; /* gpio PH2 */
 		wp-gpios = <&gpio 59 0>; /* gpio PH3 */
-		power-gpios = <&gpio 70 0>; /* gpio PI6 */
+		vmmc-supply = <&vddio_sdmmc_reg>;
+		vqmmc-supply = <&vddio_sdmmc_reg>;
 		bus-width = <8>;
 	};
 
@@ -495,6 +497,27 @@
 			gpio = <&gpio 176 0>; /* gpio PW0 */
 			enable-active-high;
 		};
+
+		vddio_sdmmc_reg: regulator at 6 {
+			compatible = "regulator-fixed";
+			reg = <6>;
+			regulator-name = "vddio_sdmmc";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio 70 0>; /* gpio PW0 */
+			enable-active-high;
+		};
+
+		vddio_sd_reg: regulator at 7 {
+			compatible = "regulator-fixed";
+			reg = <7>;
+			regulator-name = "vddio_sd";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio 155 0>; /* gpio PW0 */
+			enable-active-high;
+		};
+
 	};
 
 	sound {
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index 6a93d14..e161b65 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -422,13 +422,17 @@
 		status = "okay";
 		cd-gpios = <&gpio 173 0>; /* gpio PV5 */
 		wp-gpios = <&gpio 57 0>;  /* gpio PH1 */
-		power-gpios = <&gpio 169 0>; /* gpio PV1 */
 		bus-width = <4>;
+		vmmc-supply = <&vddio_sd_reg>;
+		vqmmc-supply = <&vddio_sd_reg>;
+
 	};
 
 	sdhci at c8000600 {
 		status = "okay";
 		bus-width = <8>;
+		vmmc-supply = <&vddio_sdmmc_reg>;
+		vqmmc-supply = <&vddio_sdmmc_reg>;
 	};
 
 	gpio-keys {
@@ -465,6 +469,24 @@
 			regulator-max-microvolt = <5000000>;
 			regulator-always-on;
 		};
+
+		vddio_sd_reg: regulator at 1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "vddio_sd";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			enable-active-high;
+			gpio = <&gpio 169 0>; /* gpio PV1 */
+		};
+
+		vddio_sdmmc_reg: regulator at 2 {
+			compatible = "regulator-fixed";
+			reg = <2>;
+			regulator-name  = "vddio_sdmmc";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
 	};
 
 	sound {
diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
index eafeca6..1b7d692 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -450,7 +450,7 @@
 					regulator-always-on;
 				};
 
-				ldo5 {
+				ldo5_reg: ldo5 {
 					regulator-name = "vdd_ldo5,vcore_mmc";
 					regulator-min-microvolt = <2850000>;
 					regulator-max-microvolt = <2850000>;
@@ -565,6 +565,9 @@
 		status = "okay";
 		power-gpios = <&gpio 86 0>; /* gpio PK6 */
 		bus-width = <4>;
+		vmmc-supply = <&vddio_sdmmc_reg>;
+		vqmmc-supply = <&vddio_sdmmc_reg>;
+
 	};
 
 	sdhci at c8000400 {
@@ -573,11 +576,15 @@
 		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
 		power-gpios = <&gpio 70 0>; /* gpio PI6 */
 		bus-width = <4>;
+		vmmc-supply = <&vddio_sd_reg>;
+		vqmmc-supply = <&vddio_sd_reg>;
 	};
 
 	sdhci at c8000600 {
 		status = "okay";
 		bus-width = <8>;
+		vmmc-supply = <&ldo5_reg>;
+		vqmmc-supply = <&vddio_nand_reg>;
 	};
 
 	gpio-keys {
@@ -632,6 +639,32 @@
 			gpio = <&pmic 1 0>;
 			enable-active-high;
 		};
+
+		vddio_sd_reg: regulator at 3 {
+			compatible = "regulator-fixed";
+			reg = <3>;
+			regulator-name = "vddio_sd";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio 176 0>; /* gpio PW0 */
+			enable-active-high;
+		};
+
+		vddio_sdmmc_reg: regulator at 4 {
+			compatible = "regulator-fixed";
+			reg = <4>;
+			regulator-name = "vddio_sdmmc";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		vddio_nand_reg: regulator at 5 {
+			compatible = "regulator-fixed";
+			reg = <5>;
+			regulator-name = "vddio_nand";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+		};
 	};
 
 	sound {
diff --git a/arch/arm/boot/dts/tegra20-tamonten.dtsi b/arch/arm/boot/dts/tegra20-tamonten.dtsi
index 5b3d8b1..a60e828 100644
--- a/arch/arm/boot/dts/tegra20-tamonten.dtsi
+++ b/arch/arm/boot/dts/tegra20-tamonten.dtsi
@@ -396,6 +396,8 @@
 		wp-gpios = <&gpio 59 0>; /* gpio PH3 */
 		bus-width = <4>;
 		status = "okay";
+		vmmc-supply = <&vddio_sdmmc_reg>;
+		vqmmc-supply = <&vddio_sdmmc_reg>;
 	};
 
 	regulators {
@@ -412,5 +414,13 @@
 			regulator-max-microvolt = <5000000>;
 			regulator-always-on;
 		};
+
+		vddio_sdmmc_reg: regulator at 1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "vddio_sdmmc";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index bec8bb2..0210670 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -376,7 +376,7 @@
 					regulator-always-on;
 				};
 
-				ldo5 {
+				ldo5_reg: ldo5 {
 					regulator-name = "vdd_ldo5,vcore_mmc";
 					regulator-min-microvolt = <2850000>;
 					regulator-max-microvolt = <2850000>;
@@ -439,19 +439,24 @@
 		status = "okay";
 		power-gpios = <&gpio 86 0>; /* gpio PK6 */
 		bus-width = <4>;
+		vmmc-supply = <&vddio_sdmmc_reg>;
+		vqmmc-supply = <&vddio_sdmmc_reg>;
 	};
 
 	sdhci at c8000400 {
 		status = "okay";
 		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
 		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
-		power-gpios = <&gpio 70 0>; /* gpio PI6 */
 		bus-width = <4>;
+		vmmc-supply = <&vddio_sd_reg>;
+		vqmmc-supply = <&vddio_sd_reg>;
 	};
 
 	sdhci at c8000600 {
 		status = "okay";
 		bus-width = <8>;
+		vmmc-supply = <&ldo5_reg>;
+		vqmmc-supply = <&vddio_nand_reg>;
 	};
 
 	regulators {
@@ -506,6 +511,33 @@
 			gpio = <&gpio 176 0>; /* gpio PW0 */
 			enable-active-high;
 		};
+
+		vddio_sd_reg: regulator at 5 {
+			compatible = "regulator-fixed";
+			reg = <5>;
+			regulator-name = "vddio_sd";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio 70 0>; /* gpio PW0 */
+			enable-active-high;
+		};
+
+		vddio_sdmmc_reg: regulator at 6 {
+			compatible = "regulator-fixed";
+			reg = <6>;
+			regulator-name = "vddio_sdmmc";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		vddio_nand_reg: regulator at 7 {
+			compatible = "regulator-fixed";
+			reg = <7>;
+			regulator-name = "vddio_nand";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+		};
+
 	};
 
 	sound {
diff --git a/arch/arm/boot/dts/tegra20-whistler.dts b/arch/arm/boot/dts/tegra20-whistler.dts
index 94a71c9..bdc4918 100644
--- a/arch/arm/boot/dts/tegra20-whistler.dts
+++ b/arch/arm/boot/dts/tegra20-whistler.dts
@@ -349,7 +349,7 @@
 					regulator-always-on;
 				};
 
-				ldo5 {
+				ldo5_reg: ldo5 {
 					regulator-name = "nvvdd_ldo5,vcore_mmc,avdd_lcd1,vddio_1wire";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
@@ -394,7 +394,7 @@
 					regulator-max-microvolt = <3300000>;
 				};
 
-				ldo12 {
+				ldo12_reg: ldo12 {
 					regulator-name = "nvvdd_ldo12,vddio_sdio";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
@@ -496,11 +496,16 @@
 		status = "okay";
 		wp-gpios = <&gpio 173 0>; /* gpio PV5 */
 		bus-width = <8>;
+		vmmc-supply = <&ldo12_reg>;
+		vqmmc-supply = <&ldo12_reg>;
 	};
 
 	sdhci at c8000600 {
 		status = "okay";
 		bus-width = <8>;
+		vmmc-supply = <&ldo5_reg>;
+		vqmmc-supply = <&ldo5_reg>;
+
 	};
 
 	regulators {
diff --git a/arch/arm/boot/dts/tegra30-cardhu-a02.dts b/arch/arm/boot/dts/tegra30-cardhu-a02.dts
index adc88aa..697effa 100644
--- a/arch/arm/boot/dts/tegra30-cardhu-a02.dts
+++ b/arch/arm/boot/dts/tegra30-cardhu-a02.dts
@@ -88,6 +88,8 @@
 		status = "okay";
 		power-gpios = <&gpio 28 0>; /* gpio PD4 */
 		bus-width = <4>;
+		vmmc-supply = <&vio_reg>;
+		vqmmc-supply = <&vio_reg>;
 	};
 };
 
diff --git a/arch/arm/boot/dts/tegra30-cardhu-a04.dts b/arch/arm/boot/dts/tegra30-cardhu-a04.dts
index 08163e1..ee4820a 100644
--- a/arch/arm/boot/dts/tegra30-cardhu-a04.dts
+++ b/arch/arm/boot/dts/tegra30-cardhu-a04.dts
@@ -100,5 +100,7 @@
 		status = "okay";
 		power-gpios = <&gpio 27 0>; /* gpio PD3 */
 		bus-width = <4>;
+		vmmc-supply = <&vio_reg>;
+		vqmmc-supply = <&vio_reg>;
 	};
 };
diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index b245e6c..fbf0fae 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -247,7 +247,7 @@
 
 				ldo5_reg: ldo5 {
 					regulator-name = "vddio_sdmmc,avdd_vdac";
-					regulator-min-microvolt = <3300000>;
+					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <3300000>;
 					regulator-always-on;
 				};
@@ -290,13 +290,16 @@
 		status = "okay";
 		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
 		wp-gpios = <&gpio 155 0>; /* gpio PT3 */
-		power-gpios = <&gpio 31 0>; /* gpio PD7 */
 		bus-width = <4>;
+		vmmc-supply = <&sdcard_3v3_reg>;
+		vqmmc-supply = <&ldo5_reg>;
 	};
 
 	sdhci at 78000600 {
 		status = "okay";
 		bus-width = <8>;
+		vmmc-supply = <&emmc_3v3_reg>;
+		vqmmc-supply = <&vio_reg>;
 	};
 
 	regulators {
@@ -451,6 +454,18 @@
 			gpio-open-drain;
 			vin-supply = <&vdd_5v0_reg>;
 		};
+
+		sdcard_3v3_reg: regulator at 13 {
+			compatible = "regulator-fixed";
+			reg = <13>;
+			regulator-name = "sdcard_3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			enable-active-high;
+			gpio = <&gpio 31 0>; /* GPIO PD7 */
+			vin-supply = <&sys_3v3_reg>;
+		};
+
 	};
 
 	sound {
-- 
1.7.1.1

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Pavan Kunapuli]

vmmc and vqmmc regulators control the voltage to
the host and device. Defer the probe if either of
them is not registered.

Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com>
---
 drivers/mmc/host/sdhci.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7922adb..925c403 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2844,11 +2844,17 @@ int sdhci_add_host(struct sdhci_host *host)
 	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
-	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
+	/*
+	 * If vqmmc regulator and no 1.8V signalling, then there's no UHS.
+	 * vqmmc regulator should be present. If it's not present,
+	 * assume the regulator driver registration is not yet done and
+	 * defer the probe.
+	 */
 	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
 	if (IS_ERR(host->vqmmc)) {
-		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
+		pr_err("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
 		host->vqmmc = NULL;
+		return -EPROBE_DEFER;
 	}
 	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
 		regulator_enable(host->vqmmc);
@@ -2903,10 +2909,17 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	ocr_avail = 0;
 
+	/*
+	 * vmmc regulator should be present. If it's not present,
+	 * assume the regulator driver registration is not yet done
+	 * and defer the probe.
+	 */
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
 	if (IS_ERR(host->vmmc)) {
-		pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
+		pr_err("%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
+		ret = -EPROBE_DEFER;
+		goto vmmc_err;
 	} else
 		regulator_enable(host->vmmc);
 
@@ -3121,7 +3134,13 @@ reset:
 untasklet:
 	tasklet_kill(&host->card_tasklet);
 	tasklet_kill(&host->finish_tasklet);
+vmmc_err:
+	if (host->vqmmc) {
+		if (regulator_is_enabled(host->vqmmc))
+			regulator_disable(host->vqmmc);
 
+		regulator_put(host->vqmmc);
+	}
 	return ret;
 }
 
-- 
1.7.1.1

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Lucas Stach]

Am Dienstag, den 23.10.2012, 12:49 +0530 schrieb Pavan Kunapuli:
> vmmc and vqmmc regulators control the voltage to
> the host and device. Defer the probe if either of
> them is not registered.
> 
Does this work with boards where we don't have any MMC supplies? Or are
we just deferring the probe indefinitely there?

For boards that power MMC unconditionally, are we supposed to add dummy
regulators to make them work with this patchset?

> Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com>
> ---
>  drivers/mmc/host/sdhci.c |   25 ++++++++++++++++++++++---
>  1 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7922adb..925c403 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2844,11 +2844,17 @@ int sdhci_add_host(struct sdhci_host *host)
>  	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>  
> -	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
> +	/*
> +	 * If vqmmc regulator and no 1.8V signalling, then there's no UHS.
> +	 * vqmmc regulator should be present. If it's not present,
> +	 * assume the regulator driver registration is not yet done and
> +	 * defer the probe.
> +	 */
>  	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
>  	if (IS_ERR(host->vqmmc)) {
> -		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> +		pr_err("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
>  		host->vqmmc = NULL;
> +		return -EPROBE_DEFER;
>  	}
>  	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
>  		regulator_enable(host->vqmmc);
> @@ -2903,10 +2909,17 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	ocr_avail = 0;
>  
> +	/*
> +	 * vmmc regulator should be present. If it's not present,
> +	 * assume the regulator driver registration is not yet done
> +	 * and defer the probe.
> +	 */
>  	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
>  	if (IS_ERR(host->vmmc)) {
> -		pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
> +		pr_err("%s: no vmmc regulator found\n", mmc_hostname(mmc));
>  		host->vmmc = NULL;
> +		ret = -EPROBE_DEFER;
> +		goto vmmc_err;
>  	} else
>  		regulator_enable(host->vmmc);
>  
> @@ -3121,7 +3134,13 @@ reset:
>  untasklet:
>  	tasklet_kill(&host->card_tasklet);
>  	tasklet_kill(&host->finish_tasklet);
> +vmmc_err:
> +	if (host->vqmmc) {
> +		if (regulator_is_enabled(host->vqmmc))
> +			regulator_disable(host->vqmmc);
>  
> +		regulator_put(host->vqmmc);
> +	}
>  	return ret;
>  }
>  

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Pavan Kunapuli]

>> Does this work with boards where we don't have any MMC supplies? Or are we just deferring the probe indefinitely there?
The probe will be deferred indefinitely.

>> For boards that power MMC unconditionally, are we supposed to add dummy regulators to make them work with this patchset?
Yes. We need to add dummy regulators for boards that power MMC unconditionally. I am not sure if there is any other solution other than using deferred probe that would guarantee regulators availability.

-----Original Message-----
From: Lucas Stach [mailto:dev at lynxeye.de] 
Sent: 23 October 2012 13:27
To: Pavan Kunapuli
Cc: swarren at wwwdotorg.org; linux at arm.linux.org.uk; cjb at laptop.org; linux-tegra at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; linux-mmc at vger.kernel.org
Subject: Re: [PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

Am Dienstag, den 23.10.2012, 12:49 +0530 schrieb Pavan Kunapuli:
> vmmc and vqmmc regulators control the voltage to the host and device. 
> Defer the probe if either of them is not registered.
> 
Does this work with boards where we don't have any MMC supplies? Or are we just deferring the probe indefinitely there?

For boards that power MMC unconditionally, are we supposed to add dummy regulators to make them work with this patchset?

> Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com>
> ---
>  drivers/mmc/host/sdhci.c |   25 ++++++++++++++++++++++---
>  1 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 
> 7922adb..925c403 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2844,11 +2844,17 @@ int sdhci_add_host(struct sdhci_host *host)
>  	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>  
> -	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
> +	/*
> +	 * If vqmmc regulator and no 1.8V signalling, then there's no UHS.
> +	 * vqmmc regulator should be present. If it's not present,
> +	 * assume the regulator driver registration is not yet done and
> +	 * defer the probe.
> +	 */
>  	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
>  	if (IS_ERR(host->vqmmc)) {
> -		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> +		pr_err("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
>  		host->vqmmc = NULL;
> +		return -EPROBE_DEFER;
>  	}
>  	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
>  		regulator_enable(host->vqmmc);
> @@ -2903,10 +2909,17 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	ocr_avail = 0;
>  
> +	/*
> +	 * vmmc regulator should be present. If it's not present,
> +	 * assume the regulator driver registration is not yet done
> +	 * and defer the probe.
> +	 */
>  	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
>  	if (IS_ERR(host->vmmc)) {
> -		pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
> +		pr_err("%s: no vmmc regulator found\n", mmc_hostname(mmc));
>  		host->vmmc = NULL;
> +		ret = -EPROBE_DEFER;
> +		goto vmmc_err;
>  	} else
>  		regulator_enable(host->vmmc);
>  
> @@ -3121,7 +3134,13 @@ reset:
>  untasklet:
>  	tasklet_kill(&host->card_tasklet);
>  	tasklet_kill(&host->finish_tasklet);
> +vmmc_err:
> +	if (host->vqmmc) {
> +		if (regulator_is_enabled(host->vqmmc))
> +			regulator_disable(host->vqmmc);
>  
> +		regulator_put(host->vqmmc);
> +	}
>  	return ret;
>  }
>  

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Philip Rakity]

On 23 Oct 2012, at 09:19, Pavan Kunapuli <pkunapuli@nvidia.com> wrote:

> vmmc and vqmmc regulators control the voltage to
> the host and device. Defer the probe if either of
> them is not registered.
> 
> Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com>
> ---
> drivers/mmc/host/sdhci.c |   25 ++++++++++++++++++++++---
> 1 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7922adb..925c403 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2844,11 +2844,17 @@ int sdhci_add_host(struct sdhci_host *host)
> 	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
> 		mmc->caps |= MMC_CAP_NEEDS_POLL;
> 
> -	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
> +	/*
> +	 * If vqmmc regulator and no 1.8V signalling, then there's no UHS.
> +	 * vqmmc regulator should be present. If it's not present,
> +	 * assume the regulator driver registration is not yet done and
> +	 * defer the probe.
> +	 */
> 	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
> 	if (IS_ERR(host->vqmmc)) {
> -		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> +		pr_err("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> 		host->vqmmc = NULL;
> +		return -EPROBE_DEFER;

Some systems exist where vmmc regulator exists and vqmmc regulator does not.  The vmmc regular is fixed at 3.3v.  
Deferring the probe will cause issues.

> 	}
> 	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
> 		regulator_enable(host->vqmmc);
> @@ -2903,10 +2909,17 @@ int sdhci_add_host(struct sdhci_host *host)
> 
> 	ocr_avail = 0;
> 
> +	/*
> +	 * vmmc regulator should be present. If it's not present,
> +	 * assume the regulator driver registration is not yet done
> +	 * and defer the probe.
> +	 */
> 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
> 	if (IS_ERR(host->vmmc)) {
> -		pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
> +		pr_err("%s: no vmmc regulator found\n", mmc_hostname(mmc));
> 		host->vmmc = NULL;
> +		ret = -EPROBE_DEFER;
> +		goto vmmc_err;
> 	} else
> 		regulator_enable(host->vmmc);
> 
> @@ -3121,7 +3134,13 @@ reset:
> untasklet:
> 	tasklet_kill(&host->card_tasklet);
> 	tasklet_kill(&host->finish_tasklet);
> +vmmc_err:
> +	if (host->vqmmc) {
> +		if (regulator_is_enabled(host->vqmmc))
> +			regulator_disable(host->vqmmc);
> 
> +		regulator_put(host->vqmmc);
> +	}
> 	return ret;
> }
> 
> -- 
> 1.7.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Russell King - ARM Linux]

On Tue, Oct 23, 2012 at 03:21:58PM +0200, Philip Rakity wrote:
> On 23 Oct 2012, at 09:19, Pavan Kunapuli <pkunapuli@nvidia.com> wrote:
> > -	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
> > +	/*
> > +	 * If vqmmc regulator and no 1.8V signalling, then there's no UHS.
> > +	 * vqmmc regulator should be present. If it's not present,
> > +	 * assume the regulator driver registration is not yet done and
> > +	 * defer the probe.
> > +	 */
> > 	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
> > 	if (IS_ERR(host->vqmmc)) {
> > -		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> > +		pr_err("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> > 		host->vqmmc = NULL;
> > +		return -EPROBE_DEFER;
> 
> Some systems exist where vmmc regulator exists and vqmmc regulator does not.  The vmmc regular is fixed at 3.3v.  
> Deferring the probe will cause issues.

That's one of the issues of deferred probing: you don't know if one of the
_get() functions failed because it just isn't present, or whether it's
failed because it hasn't been registered yet.

The two conditions are indistinguishable from the driver point of view.

The solution to it is to ensure that those drivers where hardware resource
X is optional provide a dummy resource to the driver when the hardware
resource is not available.  That means, as far as the driver is concerned,
that it will always expect to get a full set of resources whether or not
the hardware has them.

If drivers care about such stuff, then we should have xxx_is_dummy()
functions (eg, clk_is_dummy(clk)) which return TRUE when the hardware
resource is not available.  (which we could use NULL to indicate and
would be in keeping with the specified IS_ERR() usage of these APIs.)

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Stephen Warren]

On 10/23/2012 01:57 AM, Lucas Stach wrote:
> Am Dienstag, den 23.10.2012, 12:49 +0530 schrieb Pavan Kunapuli:
>> vmmc and vqmmc regulators control the voltage to
>> the host and device. Defer the probe if either of
>> them is not registered.
>
> Does this work with boards where we don't have any MMC supplies? Or are
> we just deferring the probe indefinitely there?
> 
> For boards that power MMC unconditionally, are we supposed to add dummy
> regulators to make them work with this patchset?

I believe that dummy (fixed) regulators are supposed to be provided in
all cases where the platform doesn't actually have one. The fact that
everything worked OK without them before this patch was most likely a
mistake/accident.

[PATCH 2/2] mmc: sdhci: Defer probe if regulator_get fails

[Mark Brown]

On Tue, Oct 23, 2012 at 10:29:41AM -0600, Stephen Warren wrote:
> On 10/23/2012 01:57 AM, Lucas Stach wrote:

> > Does this work with boards where we don't have any MMC supplies? Or are
> > we just deferring the probe indefinitely there?

> > For boards that power MMC unconditionally, are we supposed to add dummy
> > regulators to make them work with this patchset?

> I believe that dummy (fixed) regulators are supposed to be provided in
> all cases where the platform doesn't actually have one. The fact that
> everything worked OK without them before this patch was most likely a
> mistake/accident.

That's the general idea; clearly there *are* supplies here, they're just
not software controlled supplies.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121023/e239279e/attachment.sig>

[PATCH 1/2] ARM: dt: tegra: Add sdhci regulators

[Marc Dietrich]

Pavan,

On Tuesday 23 October 2012 12:48:59 Pavan Kunapuli wrote:
> Adding vmmc and vmmcq supplies for sdhci nodes
> in tegra dt files.

<...>

> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
> b/arch/arm/boot/dts/tegra20-paz00.dts index 6a93d14..e161b65 100644
> --- a/arch/arm/boot/dts/tegra20-paz00.dts
> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
> @@ -422,13 +422,17 @@
>  		status = "okay";
>  		cd-gpios = <&gpio 173 0>; /* gpio PV5 */
>  		wp-gpios = <&gpio 57 0>;  /* gpio PH1 */
> -		power-gpios = <&gpio 169 0>; /* gpio PV1 */
>  		bus-width = <4>;
> +		vmmc-supply = <&vddio_sd_reg>;
> +		vqmmc-supply = <&vddio_sd_reg>;
>  	};
> 
>  	sdhci at c8000600 {
>  		status = "okay";
>  		bus-width = <8>;
> +		vmmc-supply = <&vddio_sdmmc_reg>;
> +		vqmmc-supply = <&vddio_sdmmc_reg>;

to make it better, this should be:

vmmc-supply = <&vcore_mmc_reg>; and
vqmmc-supply = <&vddio_nand_reg>;

with vcore_mmc_reg is ldo5 and vddio_nand_reg is the fixed regulator below.

By doing so, I get "sdhci-tegra sdhci-tegra.3: could not set regulator OCR 
(-1)". MMC subsys wants to set ldo5 to 3.3V but it is fixed. I guess that's 
harmless for now. 

>  	};
> 
>  	gpio-keys {
> @@ -465,6 +469,24 @@
>  			regulator-max-microvolt = <5000000>;
>  			regulator-always-on;
>  		};
> +
> +		vddio_sd_reg: regulator at 1 {
> +			compatible = "regulator-fixed";
> +			reg = <1>;
> +			regulator-name = "vddio_sd";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +			enable-active-high;
> +			gpio = <&gpio 169 0>; /* gpio PV1 */
> +		};
> +
> +		vddio_sdmmc_reg: regulator at 2 {

       ^^ vddio_nand_reg acording to schematics

> +			compatible = "regulator-fixed";
> +			reg = <2>;
> +			regulator-name  = "vddio_sdmmc";

also here

> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +		};
>  	};
> 
>  	sound {

[PATCH 1/2] ARM: dt: tegra: Add sdhci regulators

[Stephen Warren]

On 10/23/2012 12:07 PM, Marc Dietrich wrote:
> Pavan,
> 
> On Tuesday 23 October 2012 12:48:59 Pavan Kunapuli wrote:
>> Adding vmmc and vmmcq supplies for sdhci nodes
>> in tegra dt files.
> 
> <...>
> 
>> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
>> b/arch/arm/boot/dts/tegra20-paz00.dts index 6a93d14..e161b65 100644
>> --- a/arch/arm/boot/dts/tegra20-paz00.dts
>> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
>> @@ -422,13 +422,17 @@
>>  		status = "okay";
>>  		cd-gpios = <&gpio 173 0>; /* gpio PV5 */
>>  		wp-gpios = <&gpio 57 0>;  /* gpio PH1 */
>> -		power-gpios = <&gpio 169 0>; /* gpio PV1 */
>>  		bus-width = <4>;
>> +		vmmc-supply = <&vddio_sd_reg>;
>> +		vqmmc-supply = <&vddio_sd_reg>;
>>  	};
>>
>>  	sdhci at c8000600 {
>>  		status = "okay";
>>  		bus-width = <8>;
>> +		vmmc-supply = <&vddio_sdmmc_reg>;
>> +		vqmmc-supply = <&vddio_sdmmc_reg>;
> 
> to make it better, this should be:
> 
> vmmc-supply = <&vcore_mmc_reg>; and
> vqmmc-supply = <&vddio_nand_reg>;
> 
> with vcore_mmc_reg is ldo5 and vddio_nand_reg is the fixed regulator below.

I think for the eMMC (not SD card), the core supply is actually +3vs_s3,
and hence fixed; it looks like LDO5 used to be used, but they changed
it. See the note for the VCC connections on the eMMC chip.

> By doing so, I get "sdhci-tegra sdhci-tegra.3: could not set regulator OCR 
> (-1)". MMC subsys wants to set ldo5 to 3.3V but it is fixed. I guess that's 
> harmless for now. 

What I mention above might end up fixing that?

[PATCH 1/2] ARM: dt: tegra: Add sdhci regulators

[Marc Dietrich]

On Tuesday 23 October 2012 12:42:11 Stephen Warren wrote:
> On 10/23/2012 12:07 PM, Marc Dietrich wrote:
> > Pavan,
> > 
> > On Tuesday 23 October 2012 12:48:59 Pavan Kunapuli wrote:
> >> Adding vmmc and vmmcq supplies for sdhci nodes
> >> in tegra dt files.
> > 
> > <...>
> > 
> >> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
> >> b/arch/arm/boot/dts/tegra20-paz00.dts index 6a93d14..e161b65 100644
> >> --- a/arch/arm/boot/dts/tegra20-paz00.dts
> >> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
> >> @@ -422,13 +422,17 @@
> >> 
> >>  		status = "okay";
> >>  		cd-gpios = <&gpio 173 0>; /* gpio PV5 */
> >>  		wp-gpios = <&gpio 57 0>;  /* gpio PH1 */
> >> 
> >> -		power-gpios = <&gpio 169 0>; /* gpio PV1 */
> >> 
> >>  		bus-width = <4>;
> >> 
> >> +		vmmc-supply = <&vddio_sd_reg>;
> >> +		vqmmc-supply = <&vddio_sd_reg>;
> >> 
> >>  	};
> >>  	
> >>  	sdhci at c8000600 {
> >>  	
> >>  		status = "okay";
> >>  		bus-width = <8>;
> >> 
> >> +		vmmc-supply = <&vddio_sdmmc_reg>;
> >> +		vqmmc-supply = <&vddio_sdmmc_reg>;
> > 
> > to make it better, this should be:
> > 
> > vmmc-supply = <&vcore_mmc_reg>; and
> > vqmmc-supply = <&vddio_nand_reg>;
> > 
> > with vcore_mmc_reg is ldo5 and vddio_nand_reg is the fixed regulator
> > below.
> 
> I think for the eMMC (not SD card), the core supply is actually +3vs_s3,
> and hence fixed; it looks like LDO5 used to be used, but they changed
> it. See the note for the VCC connections on the eMMC chip.

yes, you are right. The schematic had a comment that this was changed.

> > By doing so, I get "sdhci-tegra sdhci-tegra.3: could not set regulator OCR
> > (-1)". MMC subsys wants to set ldo5 to 3.3V but it is fixed. I guess
> > that's
> > harmless for now.
> 
> What I mention above might end up fixing that?

yes, it does.

So the original patch is just fine. Sorry for the noise.

Marc