linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc()
@ 2014-12-15 23:25 Doug Anderson
  2014-12-15 23:25 ` [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Doug Anderson @ 2014-12-15 23:25 UTC (permalink / raw)
  To: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
	Andrew Bresticker, linux-rockchip, linux-arm-kernel,
	Doug Anderson, chris, johan.rudholm, tim.kryger, andrew_gabbasov,
	adrian.hunter, linux-mmc, linux-kernel

This adds logic to the MMC core to set VQMMC.  This is expected to be
called by MMC drivers like dw_mmc as part of (or instead of) their
start_signal_voltage_switch() callback.

A few notes:

* When setting the signal voltage to 3.3V we do our best to make VQMMC
  and VMMC match.  It's been reported that this makes some old cards
  happy since they were tested back in the day before UHS when VQMMC
  and VMMC were provided by the same regulator.  A nice side effect of
  this is that we don't end up on the hairy edge of VQMMC (2.7V),
  which some EEs claim is a little too close to the minimum for
  comfort.

* When setting the signal voltage to 1.8V or 1.2V we aim for that
  specific voltage instead of picking the lowest one in the range.

* We very purposely don't print errors in mmc_regulator_set_vqmmc().
  There are cases where the MMC core will try several different
  voltages and we don't want to pollute the logs.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Now use existing regulator_set_voltage_tol() function.

 drivers/mmc/core/core.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |  7 +++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9584bff..670dc65 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1333,6 +1333,57 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
 }
 EXPORT_SYMBOL_GPL(mmc_regulator_set_ocr);
 
+static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
+						  int new_uV, int tol_uV)
+{
+	/*
+	 * Check if supported first to avoid errors since we may try several
+	 * signal levels during power up and don't want to show errors.
+	 */
+	if (!regulator_is_supported_voltage_tol(regulator, new_uV, tol_uV))
+		return -EINVAL;
+
+	return regulator_set_voltage_tol(regulator, new_uV, tol_uV);
+}
+
+/**
+ * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
+ *
+ * For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible.
+ * That will match the behavior of old boards where VQMMC and VMMC were supplied
+ * by the same supply.  The Bus Operating conditions for 3.3V signaling in the
+ * SD card spec also define VQMMC in terms of VMMC.
+ *
+ * For 1.2V and 1.8V signaling we'll try to get as close as possible to the
+ * requested voltage.  This is definitely a good idea for UHS where there's a
+ * separate regulator on the card that's trying to make 1.8V and it's best if
+ * we match.
+ *
+ * This function is expected to be used by a controller's
+ * start_signal_voltage_switch() function.
+ */
+int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	/* If no vqmmc supply then we can't change the voltage */
+	if (IS_ERR(mmc->supply.vqmmc))
+		return -EINVAL;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_120:
+		return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+			1200000, 100000);
+	case MMC_SIGNAL_VOLTAGE_180:
+		return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+			1800000, 100000);
+	case MMC_SIGNAL_VOLTAGE_330:
+		return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+			regulator_get_voltage(mmc->supply.vmmc), 300000);
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(mmc_regulator_set_vqmmc);
+
 #endif /* CONFIG_REGULATOR */
 
 int mmc_regulator_get_supply(struct mmc_host *mmc)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 9f32270..524d6fc 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -416,6 +416,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);
 int mmc_regulator_set_ocr(struct mmc_host *mmc,
 			struct regulator *supply,
 			unsigned short vdd_bit);
+int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios);
 #else
 static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
 {
@@ -428,6 +429,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
 {
 	return 0;
 }
+
+static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc,
+					  struct mmc_ios *ios)
+{
+	return -EINVAL;
+}
 #endif
 
 int mmc_regulator_get_supply(struct mmc_host *mmc);
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
  2014-12-15 23:25 [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
@ 2014-12-15 23:25 ` Doug Anderson
  2014-12-15 23:44   ` Andrew Bresticker
  2014-12-15 23:25 ` [PATCH v2 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
  2014-12-15 23:42 ` [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Andrew Bresticker
  2 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2014-12-15 23:25 UTC (permalink / raw)
  To: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
	Andrew Bresticker, linux-rockchip, linux-arm-kernel,
	Doug Anderson, chris, linux-mmc, linux-kernel

We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
helper has some advantages:

1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
   VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
   compatibility.

2. We get rid of a few more warnings when probing unsupported
   voltages.

3. We get rid of some non-dw_mmc specific code in dw_mmc.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2: None

 drivers/mmc/host/dw_mmc.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67c0451..4ae800c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1160,8 +1160,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct dw_mci *host = slot->host;
 	u32 uhs;
 	u32 v18 = SDMMC_UHS_18V << slot->id;
-	int min_uv, max_uv;
-	int ret;
 
 	/*
 	 * Program the voltage.  Note that some instances of dw_mmc may use
@@ -1170,24 +1168,11 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	 */
 	uhs = mci_readl(host, UHS_REG);
 	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
-		min_uv = 2700000;
-		max_uv = 3600000;
 		uhs &= ~v18;
 	} else {
-		min_uv = 1700000;
-		max_uv = 1950000;
 		uhs |= v18;
 	}
-	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
-
-		if (ret) {
-			dev_dbg(&mmc->class_dev,
-					 "Regulator set error %d: %d - %d\n",
-					 ret, min_uv, max_uv);
-			return ret;
-		}
-	}
+	mmc_regulator_set_vqmmc(mmc, ios);
 	mci_writel(host, UHS_REG, uhs);
 
 	return 0;
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb
  2014-12-15 23:25 [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
  2014-12-15 23:25 ` [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
@ 2014-12-15 23:25 ` Doug Anderson
  2014-12-15 23:42 ` [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Andrew Bresticker
  2 siblings, 0 replies; 6+ messages in thread
From: Doug Anderson @ 2014-12-15 23:25 UTC (permalink / raw)
  To: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
	Andrew Bresticker, linux-rockchip, linux-arm-kernel,
	Doug Anderson, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
	galak, linux, devicetree, linux-kernel

Specifying these rails should eventually let us do UHS.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Fix subject line

 arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 3e067dd..d660fe6 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -114,6 +114,8 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
 	status = "okay";
+	vmmc-supply = <&vcc_io>;
+	vqmmc-supply = <&vccio_sd>;
 };
 
 &i2c0 {
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc()
  2014-12-15 23:25 [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
  2014-12-15 23:25 ` [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
  2014-12-15 23:25 ` [PATCH v2 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
@ 2014-12-15 23:42 ` Andrew Bresticker
  2 siblings, 0 replies; 6+ messages in thread
From: Andrew Bresticker @ 2014-12-15 23:42 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
	Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Chris Ball, johan.rudholm, tim.kryger,
	andrew_gabbasov, Adrian Hunter, linux-mmc, linux-kernel

Hi Doug,

On Mon, Dec 15, 2014 at 3:25 PM, Doug Anderson <dianders@chromium.org> wrote:
> This adds logic to the MMC core to set VQMMC.  This is expected to be
> called by MMC drivers like dw_mmc as part of (or instead of) their
> start_signal_voltage_switch() callback.
>
> A few notes:
>
> * When setting the signal voltage to 3.3V we do our best to make VQMMC
>   and VMMC match.  It's been reported that this makes some old cards
>   happy since they were tested back in the day before UHS when VQMMC
>   and VMMC were provided by the same regulator.  A nice side effect of
>   this is that we don't end up on the hairy edge of VQMMC (2.7V),
>   which some EEs claim is a little too close to the minimum for
>   comfort.
>
> * When setting the signal voltage to 1.8V or 1.2V we aim for that
>   specific voltage instead of picking the lowest one in the range.
>
> * We very purposely don't print errors in mmc_regulator_set_vqmmc().
>   There are cases where the MMC core will try several different
>   voltages and we don't want to pollute the logs.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Looks good to me.

Reviewed-by: Andrew Bresticker <abrestic@chromium.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
  2014-12-15 23:25 ` [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
@ 2014-12-15 23:44   ` Andrew Bresticker
  2014-12-16  0:09     ` Doug Anderson
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Bresticker @ 2014-12-15 23:44 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
	Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Chris Ball, linux-mmc, linux-kernel

Hi Doug,

On Mon, Dec 15, 2014 at 3:25 PM, Doug Anderson <dianders@chromium.org> wrote:
> We've introduced a new helper in the MMC core:
> mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
> helper has some advantages:
>
> 1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
>    VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
>    compatibility.
>
> 2. We get rid of a few more warnings when probing unsupported
>    voltages.
>
> 3. We get rid of some non-dw_mmc specific code in dw_mmc.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

One small comment below...

> @@ -1170,24 +1168,11 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>          */
>         uhs = mci_readl(host, UHS_REG);
>         if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> -               min_uv = 2700000;
> -               max_uv = 3600000;
>                 uhs &= ~v18;
>         } else {
> -               min_uv = 1700000;
> -               max_uv = 1950000;
>                 uhs |= v18;
>         }
> -       if (!IS_ERR(mmc->supply.vqmmc)) {
> -               ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
> -
> -               if (ret) {
> -                       dev_dbg(&mmc->class_dev,
> -                                        "Regulator set error %d: %d - %d\n",
> -                                        ret, min_uv, max_uv);
> -                       return ret;
> -               }
> -       }
> +       mmc_regulator_set_vqmmc(mmc, ios);

Shouldn't we check the return value here and bail out of the voltage
switch procedure if it fails?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
  2014-12-15 23:44   ` Andrew Bresticker
@ 2014-12-16  0:09     ` Doug Anderson
  0 siblings, 0 replies; 6+ messages in thread
From: Doug Anderson @ 2014-12-16  0:09 UTC (permalink / raw)
  To: Andrew Bresticker
  Cc: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
	Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Chris Ball, linux-mmc, linux-kernel

Andrew,

On Mon, Dec 15, 2014 at 3:44 PM, Andrew Bresticker
<abrestic@chromium.org> wrote:
>> -       if (!IS_ERR(mmc->supply.vqmmc)) {
>> -               ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
>> -
>> -               if (ret) {
>> -                       dev_dbg(&mmc->class_dev,
>> -                                        "Regulator set error %d: %d - %d\n",
>> -                                        ret, min_uv, max_uv);
>> -                       return ret;
>> -               }
>> -       }
>> +       mmc_regulator_set_vqmmc(mmc, ios);
>
> Shouldn't we check the return value here and bail out of the voltage
> switch procedure if it fails?

My thought was to purposely _not_ check the return value.  The comment
right above this says:

* Program the voltage.  Note that some instances of dw_mmc may use
* the UHS_REG for this.  For other instances (like exynos) the UHS_REG
* does no harm but you need to set the regulator directly.  Try both.

...I'm not 100% sure that I understand how the UHS_REG is supposed to
be implemented since I've never seen a system that uses it.  I was
assuming that if you have UHS_REG you don't need the regulator...

...but probably a better solution is to return the error if "
!IS_ERR(mmc->supply.vqmmc)".  Let me fix that up and repost.


-Doug

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-12-16  0:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-15 23:25 [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
2014-12-15 23:25 ` [PATCH v2 2/3] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
2014-12-15 23:44   ` Andrew Bresticker
2014-12-16  0:09     ` Doug Anderson
2014-12-15 23:25 ` [PATCH v2 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
2014-12-15 23:42 ` [PATCH v2 1/3] mmc: core: Add mmc_regulator_set_vqmmc() Andrew Bresticker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).