[PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[Marek Vasut]

Patch all drivers and core code which uses mmc_set_signal_voltage()
and prepare it for the fact that mmc_set_signal_voltage() can return
a value > 0, which would happen if the signal voltage switch did NOT
happen, because the voltage was already set correctly.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Ludovic Barre <ludovic.barre@st.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-mmc@vger.kernel.org
---
 drivers/mmc/core/core.c              | 10 +++++-----
 drivers/mmc/core/mmc.c               | 16 ++++++++--------
 drivers/mmc/host/dw_mmc-k3.c         |  2 +-
 drivers/mmc/host/dw_mmc.c            |  3 +--
 drivers/mmc/host/mtk-sd.c            |  2 +-
 drivers/mmc/host/renesas_sdhi_core.c |  2 +-
 drivers/mmc/host/sdhci-sprd.c        |  2 +-
 drivers/mmc/host/sdhci.c             |  6 +++---
 8 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 4c5de6d37ac7..98a3552205cb 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1142,7 +1142,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
 	if (host->ops->start_signal_voltage_switch)
 		err = host->ops->start_signal_voltage_switch(host, &host->ios);
 
-	if (err)
+	if (err < 0)
 		host->ios.signal_voltage = old_signal_voltage;
 
 	return err;
@@ -1152,11 +1152,11 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
 void mmc_set_initial_signal_voltage(struct mmc_host *host)
 {
 	/* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
-	if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330))
+	if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) >= 0)
 		dev_dbg(mmc_dev(host), "Initial signal voltage of 3.3v\n");
-	else if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
+	else if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) >= 0)
 		dev_dbg(mmc_dev(host), "Initial signal voltage of 1.8v\n");
-	else if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120))
+	else if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) >= 0)
 		dev_dbg(mmc_dev(host), "Initial signal voltage of 1.2v\n");
 }
 
@@ -1172,7 +1172,7 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host)
 	host->ios.clock = 0;
 	mmc_set_ios(host);
 
-	if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
+	if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) < 0)
 		return -EAGAIN;
 
 	/* Keep clock gated for at least 10 ms, though spec only says 5 ms */
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index de94fbe629bd..9f5aae051f6d 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1121,7 +1121,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 	 */
 	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_1_2V) {
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
-		if (!err)
+		if (err >= 0)
 			return 0;
 	}
 
@@ -1130,7 +1130,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
 
 	/* make sure vccq is 3.3v after switching disaster */
-	if (err)
+	if (err < 0)
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330);
 
 	return err;
@@ -1339,11 +1339,11 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_2V)
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
 
-	if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
+	if (err < 0 && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
 
 	/* If fails try again during next card power cycle */
-	if (err)
+	if (err < 0)
 		goto out_err;
 
 	err = mmc_select_bus_width(card);
@@ -1437,11 +1437,11 @@ static int mmc_select_hs200(struct mmc_card *card)
 	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
 
-	if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
+	if (err < 0 && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
 
 	/* If fails try again during next card power cycle */
-	if (err)
+	if (err < 0)
 		return err;
 
 	mmc_select_driver_type(card);
@@ -1480,7 +1480,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 err:
 	if (err) {
 		/* fall back to the old signal voltage, if fails report error */
-		if (mmc_set_signal_voltage(host, old_signal_voltage))
+		if (mmc_set_signal_voltage(host, old_signal_voltage) < 0)
 			err = -EIO;
 
 		pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
@@ -1769,7 +1769,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 		err = mmc_select_bus_width(card);
 		if (err > 0 && mmc_card_hs(card)) {
 			err = mmc_select_hs_ddr(card);
-			if (err)
+			if (err < 0)
 				goto free_card;
 		}
 	}
diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 23b6f65b3785..50977ff18074 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -424,7 +424,7 @@ static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 		ret = mmc_regulator_set_vqmmc(mmc, ios);
-		if (ret) {
+		if (ret < 0) {
 			dev_err(host->dev, "Regulator set error %d\n", ret);
 			return ret;
 		}
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index bc5278ab5707..5d1f8a3ec3a5 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1546,8 +1546,7 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 		ret = mmc_regulator_set_vqmmc(mmc, ios);
-
-		if (ret) {
+		if (ret < 0) {
 			dev_dbg(&mmc->class_dev,
 					 "Regulator set error %d - %s V\n",
 					 ret, uhs & v18 ? "1.8" : "3.3");
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index b221c02cc71f..dc6710f9f36f 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -1379,7 +1379,7 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
 		}
 
 		ret = mmc_regulator_set_vqmmc(mmc, ios);
-		if (ret) {
+		if (ret < 0) {
 			dev_dbg(host->dev, "Regulator set error %d (%d)\n",
 				ret, ios->signal_voltage);
 		} else {
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index df826661366f..08952b6681f1 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -237,7 +237,7 @@ static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 			MMC_SIGNAL_VOLTAGE_330 ? 0 : -EINVAL;
 
 	ret = mmc_regulator_set_vqmmc(host->mmc, ios);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	return pinctrl_select_state(priv->pinctrl, pin_state);
diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
index 2ab42c59e4f8..f6c7666e8fa4 100644
--- a/drivers/mmc/host/sdhci-sprd.c
+++ b/drivers/mmc/host/sdhci-sprd.c
@@ -434,7 +434,7 @@ static int sdhci_sprd_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 		ret = mmc_regulator_set_vqmmc(mmc, ios);
-		if (ret) {
+		if (ret < 0) {
 			pr_err("%s: Switching signalling voltage failed\n",
 			       mmc_hostname(mmc));
 			return ret;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3f716466fcfd..5f9d4c8d24f8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2411,7 +2411,7 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
 			ret = mmc_regulator_set_vqmmc(mmc, ios);
-			if (ret) {
+			if (ret < 0) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));
 				return -EIO;
@@ -2434,7 +2434,7 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 			return -EINVAL;
 		if (!IS_ERR(mmc->supply.vqmmc)) {
 			ret = mmc_regulator_set_vqmmc(mmc, ios);
-			if (ret) {
+			if (ret < 0) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));
 				return -EIO;
@@ -2466,7 +2466,7 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 			return -EINVAL;
 		if (!IS_ERR(mmc->supply.vqmmc)) {
 			ret = mmc_regulator_set_vqmmc(mmc, ios);
-			if (ret) {
+			if (ret < 0) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));
 				return -EIO;
-- 
2.25.1

[PATCH 2/3] mmc: Return 1 from mmc_set_signal_voltage() if switch skipped

[Marek Vasut]

Adjust mmc_set_signal_voltage() to return 1 if the voltage switch was
skipped because the regulator voltage was already correct. This allows
drivers to detect such condition and possibly skip various voltage
switching extras.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Ludovic Barre <ludovic.barre@st.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-mmc@vger.kernel.org
---
 drivers/mmc/core/regulator.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c
index b6febbcf8978..2805ea8a070e 100644
--- a/drivers/mmc/core/regulator.c
+++ b/drivers/mmc/core/regulator.c
@@ -136,6 +136,8 @@ static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
 						  int min_uV, int target_uV,
 						  int max_uV)
 {
+	int curr_voltage;
+
 	/*
 	 * Check if supported first to avoid errors since we may try several
 	 * signal levels during power up and don't want to show errors.
@@ -143,6 +145,14 @@ static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
 	if (!regulator_is_supported_voltage(regulator, min_uV, max_uV))
 		return -EINVAL;
 
+	/*
+	 * The voltage is already set, no need to switch.
+	 * Return 1 to indicate that no switch happened.
+	 */
+	curr_voltage = regulator_get_voltage(regulator);
+	if (curr_voltage == target_uV)
+		return 1;
+
 	return regulator_set_voltage_triplet(regulator, min_uV, target_uV,
 					     max_uV);
 }
-- 
2.25.1

[PATCH 3/3] mmc: mmci: Switch to mmc_set_signal_voltage()

[Marek Vasut]

Instead of reimplementing the logic in mmc_set_signal_voltage(),
use the mmc code function directly.

This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
for the voltage switch to complete, even though no voltage switch really
happened. But since mmc_set_signal_voltage() would return 0, then the
condition for calling .post_sig_volt_switch() is not satisfied if the
switch did not happen.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Ludovic Barre <ludovic.barre@st.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-mmc@vger.kernel.org
---
 drivers/mmc/host/mmci.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 647567def612..b8c8f0e623de 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1861,31 +1861,15 @@ static int mmci_get_cd(struct mmc_host *mmc)
 static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct mmci_host *host = mmc_priv(mmc);
-	int ret = 0;
-
-	if (!IS_ERR(mmc->supply.vqmmc)) {
+	int ret;
 
-		switch (ios->signal_voltage) {
-		case MMC_SIGNAL_VOLTAGE_330:
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-						2700000, 3600000);
-			break;
-		case MMC_SIGNAL_VOLTAGE_180:
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-						1700000, 1950000);
-			break;
-		case MMC_SIGNAL_VOLTAGE_120:
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-						1100000, 1300000);
-			break;
-		}
+	ret = mmc_regulator_set_vqmmc(mmc, ios);
 
-		if (!ret && host->ops && host->ops->post_sig_volt_switch)
-			ret = host->ops->post_sig_volt_switch(host, ios);
+	if (!ret && host->ops && host->ops->post_sig_volt_switch)
+		ret = host->ops->post_sig_volt_switch(host, ios);
 
-		if (ret)
-			dev_warn(mmc_dev(mmc), "Voltage switch failed\n");
-	}
+	if (ret < 0)
+		dev_warn(mmc_dev(mmc), "Voltage switch failed\n");
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH 2/3] mmc: Return 1 from mmc_set_signal_voltage() if switch skipped

[Ulf Hansson]

On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
>
> Adjust mmc_set_signal_voltage() to return 1 if the voltage switch was
> skipped because the regulator voltage was already correct. This allows
> drivers to detect such condition and possibly skip various voltage
> switching extras.

This change to the code isn't about mmc_set_signal_voltage(), but
about mmc_regulator_set_voltage_if_supported(). Please update the
changelog to reflect this.

Moreover, as a part of $subject patch, you also need to adopt
mmc_regulator_set_vqmmc() to cope with the new behaviour of
mmc_regulator_set_voltage_if_supported().

>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Ludovic Barre <ludovic.barre@st.com>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-mmc@vger.kernel.org

If possible, please drop all these, as I don't think we need them as
references part of the patch. Of course, it's easier for you to keep
them, I can also drop it while applying.

> ---
>  drivers/mmc/core/regulator.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c
> index b6febbcf8978..2805ea8a070e 100644
> --- a/drivers/mmc/core/regulator.c
> +++ b/drivers/mmc/core/regulator.c
> @@ -136,6 +136,8 @@ static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
>                                                   int min_uV, int target_uV,
>                                                   int max_uV)
>  {
> +       int curr_voltage;

Nitpick: To be consistent with other variable names, maybe current_uV
is a better name.

> +
>         /*
>          * Check if supported first to avoid errors since we may try several
>          * signal levels during power up and don't want to show errors.
> @@ -143,6 +145,14 @@ static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
>         if (!regulator_is_supported_voltage(regulator, min_uV, max_uV))
>                 return -EINVAL;
>
> +       /*
> +        * The voltage is already set, no need to switch.
> +        * Return 1 to indicate that no switch happened.
> +        */
> +       curr_voltage = regulator_get_voltage(regulator);
> +       if (curr_voltage == target_uV)
> +               return 1;
> +
>         return regulator_set_voltage_triplet(regulator, min_uV, target_uV,
>                                              max_uV);
>  }
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[Ulf Hansson]

On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
>
> Patch all drivers and core code which uses mmc_set_signal_voltage()
> and prepare it for the fact that mmc_set_signal_voltage() can return
> a value > 0, which would happen if the signal voltage switch did NOT
> happen, because the voltage was already set correctly.

I am not sure why you want to change mmc_set_signal_voltage(), can you
elaborate on that?

I thought we discussed changing mmc_regulator_set_vqmmc(), what am I missing?

>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Ludovic Barre <ludovic.barre@st.com>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-mmc@vger.kernel.org
> ---
>  drivers/mmc/core/core.c              | 10 +++++-----
>  drivers/mmc/core/mmc.c               | 16 ++++++++--------
>  drivers/mmc/host/dw_mmc-k3.c         |  2 +-
>  drivers/mmc/host/dw_mmc.c            |  3 +--
>  drivers/mmc/host/mtk-sd.c            |  2 +-
>  drivers/mmc/host/renesas_sdhi_core.c |  2 +-
>  drivers/mmc/host/sdhci-sprd.c        |  2 +-
>  drivers/mmc/host/sdhci.c             |  6 +++---
>  8 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 4c5de6d37ac7..98a3552205cb 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1142,7 +1142,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
>         if (host->ops->start_signal_voltage_switch)
>                 err = host->ops->start_signal_voltage_switch(host, &host->ios);
>
> -       if (err)
> +       if (err < 0)
>                 host->ios.signal_voltage = old_signal_voltage;
>
>         return err;
> @@ -1152,11 +1152,11 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
>  void mmc_set_initial_signal_voltage(struct mmc_host *host)
>  {
>         /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
> -       if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330))
> +       if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) >= 0)
>                 dev_dbg(mmc_dev(host), "Initial signal voltage of 3.3v\n");
> -       else if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
> +       else if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) >= 0)
>                 dev_dbg(mmc_dev(host), "Initial signal voltage of 1.8v\n");
> -       else if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120))
> +       else if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) >= 0)
>                 dev_dbg(mmc_dev(host), "Initial signal voltage of 1.2v\n");
>  }
>
> @@ -1172,7 +1172,7 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host)
>         host->ios.clock = 0;
>         mmc_set_ios(host);
>
> -       if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
> +       if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) < 0)
>                 return -EAGAIN;
>
>         /* Keep clock gated for at least 10 ms, though spec only says 5 ms */
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index de94fbe629bd..9f5aae051f6d 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1121,7 +1121,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
>          */
>         if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_1_2V) {
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
> -               if (!err)
> +               if (err >= 0)
>                         return 0;
>         }
>
> @@ -1130,7 +1130,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
>
>         /* make sure vccq is 3.3v after switching disaster */
> -       if (err)
> +       if (err < 0)
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330);
>
>         return err;
> @@ -1339,11 +1339,11 @@ static int mmc_select_hs400es(struct mmc_card *card)
>         if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_2V)
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
>
> -       if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
> +       if (err < 0 && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
>
>         /* If fails try again during next card power cycle */
> -       if (err)
> +       if (err < 0)
>                 goto out_err;
>
>         err = mmc_select_bus_width(card);
> @@ -1437,11 +1437,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>         if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
>
> -       if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
> +       if (err < 0 && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
>
>         /* If fails try again during next card power cycle */
> -       if (err)
> +       if (err < 0)
>                 return err;
>
>         mmc_select_driver_type(card);
> @@ -1480,7 +1480,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>  err:
>         if (err) {
>                 /* fall back to the old signal voltage, if fails report error */
> -               if (mmc_set_signal_voltage(host, old_signal_voltage))
> +               if (mmc_set_signal_voltage(host, old_signal_voltage) < 0)
>                         err = -EIO;
>
>                 pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
> @@ -1769,7 +1769,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>                 err = mmc_select_bus_width(card);
>                 if (err > 0 && mmc_card_hs(card)) {
>                         err = mmc_select_hs_ddr(card);
> -                       if (err)
> +                       if (err < 0)
>                                 goto free_card;
>                 }
>         }
> diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
> index 23b6f65b3785..50977ff18074 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -424,7 +424,7 @@ static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
>
>         if (!IS_ERR(mmc->supply.vqmmc)) {
>                 ret = mmc_regulator_set_vqmmc(mmc, ios);
> -               if (ret) {
> +               if (ret < 0) {

This change makes sense to me, however it's also a bit confusing, as
the changelog refers to changes for mmc_set_signal_voltage().

As I understand it, we want mmc_regulator_set_vqmmc() to return 1, in
case the current voltage level is the same as the requested "new"
target".

>                         dev_err(host->dev, "Regulator set error %d\n", ret);
>                         return ret;
>                 }

[...]

So, to conclude, it seems like $subject patch needs to be reworked a
bit - just keep the changes you have made to the host drivers, then
throw away the other parts in core.

Kind regards
Uffe

Re: [PATCH 3/3] mmc: mmci: Switch to mmc_set_signal_voltage()

[Ulf Hansson]

On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
>
> Instead of reimplementing the logic in mmc_set_signal_voltage(),
> use the mmc code function directly.

Again, this isn't about mmc_set_signal_voltage() but about
mmc_regulator_set_vqmmc(). Please update the changelog to reflect
that.

>
> This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
> VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
> for the voltage switch to complete, even though no voltage switch really
> happened. But since mmc_set_signal_voltage() would return 0, then the
> condition for calling .post_sig_volt_switch() is not satisfied if the
> switch did not happen.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Ludovic Barre <ludovic.barre@st.com>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-mmc@vger.kernel.org
> ---
>  drivers/mmc/host/mmci.c | 28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 647567def612..b8c8f0e623de 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1861,31 +1861,15 @@ static int mmci_get_cd(struct mmc_host *mmc)
>  static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct mmci_host *host = mmc_priv(mmc);
> -       int ret = 0;
> -
> -       if (!IS_ERR(mmc->supply.vqmmc)) {
> +       int ret;
>
> -               switch (ios->signal_voltage) {
> -               case MMC_SIGNAL_VOLTAGE_330:
> -                       ret = regulator_set_voltage(mmc->supply.vqmmc,
> -                                               2700000, 3600000);
> -                       break;
> -               case MMC_SIGNAL_VOLTAGE_180:
> -                       ret = regulator_set_voltage(mmc->supply.vqmmc,
> -                                               1700000, 1950000);
> -                       break;
> -               case MMC_SIGNAL_VOLTAGE_120:
> -                       ret = regulator_set_voltage(mmc->supply.vqmmc,
> -                                               1100000, 1300000);
> -                       break;
> -               }
> +       ret = mmc_regulator_set_vqmmc(mmc, ios);
>
> -               if (!ret && host->ops && host->ops->post_sig_volt_switch)
> -                       ret = host->ops->post_sig_volt_switch(host, ios);
> +       if (!ret && host->ops && host->ops->post_sig_volt_switch)

I would suggest to add a comment here somewhere, that you explicitly
want to avoid calling the ->post_sig_volt_switch() unless the voltage
really changed. Just to make this clear.

> +               ret = host->ops->post_sig_volt_switch(host, ios);
>
> -               if (ret)
> -                       dev_warn(mmc_dev(mmc), "Voltage switch failed\n");
> -       }
> +       if (ret < 0)
> +               dev_warn(mmc_dev(mmc), "Voltage switch failed\n");
>
>         return ret;
>  }
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[Marek Vasut]

On 4/15/20 10:40 AM, Ulf Hansson wrote:
> On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
>>
>> Patch all drivers and core code which uses mmc_set_signal_voltage()
>> and prepare it for the fact that mmc_set_signal_voltage() can return
>> a value > 0, which would happen if the signal voltage switch did NOT
>> happen, because the voltage was already set correctly.
> 
> I am not sure why you want to change mmc_set_signal_voltage(), can you
> elaborate on that?
> 
> I thought we discussed changing mmc_regulator_set_vqmmc(), what am I missing?

Because mmc_set_signal_voltage() optionally calls
host->ops_>start_signal_voltage_switch() , which can now return value >
0 , so the rest of the core needs to be patched to cater for that.

[...]

>> diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
>> index 23b6f65b3785..50977ff18074 100644
>> --- a/drivers/mmc/host/dw_mmc-k3.c
>> +++ b/drivers/mmc/host/dw_mmc-k3.c
>> @@ -424,7 +424,7 @@ static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
>>
>>         if (!IS_ERR(mmc->supply.vqmmc)) {
>>                 ret = mmc_regulator_set_vqmmc(mmc, ios);
>> -               if (ret) {
>> +               if (ret < 0) {
> 
> This change makes sense to me, however it's also a bit confusing, as
> the changelog refers to changes for mmc_set_signal_voltage().
> 
> As I understand it, we want mmc_regulator_set_vqmmc() to return 1, in
> case the current voltage level is the same as the requested "new"
> target".

Yes. So a failure is now only a return value < 0.

>>                         dev_err(host->dev, "Regulator set error %d\n", ret);
>>                         return ret;
>>                 }
> 
> [...]
> 
> So, to conclude, it seems like $subject patch needs to be reworked a
> bit - just keep the changes you have made to the host drivers, then
> throw away the other parts in core.

I think the core parts are necessary as well though , see above.

Re: [PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[Ulf Hansson]

On Thu, 16 Apr 2020 at 12:29, Marek Vasut <marex@denx.de> wrote:
>
> On 4/15/20 10:40 AM, Ulf Hansson wrote:
> > On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
> >>
> >> Patch all drivers and core code which uses mmc_set_signal_voltage()
> >> and prepare it for the fact that mmc_set_signal_voltage() can return
> >> a value > 0, which would happen if the signal voltage switch did NOT
> >> happen, because the voltage was already set correctly.
> >
> > I am not sure why you want to change mmc_set_signal_voltage(), can you
> > elaborate on that?
> >
> > I thought we discussed changing mmc_regulator_set_vqmmc(), what am I missing?
>
> Because mmc_set_signal_voltage() optionally calls
> host->ops_>start_signal_voltage_switch() , which can now return value >
> 0 , so the rest of the core needs to be patched to cater for that.

The issue that you wanted to solve (at least by looking at the
original patch) was to understand whether the vqmmc regulator changes
voltage level and then take different actions based on that in the
mmci host driver.

You don't need to change anything related to mmc_set_signal_voltage()
to accomplish that, do you? Moreover, I am worried that it may affect
the host driver's expectations from when
->start_signal_voltage_switch() may be called.

[...]

Kind regards
Uffe

Re: [PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[Marek Vasut]

On 4/16/20 2:07 PM, Ulf Hansson wrote:
> On Thu, 16 Apr 2020 at 12:29, Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/15/20 10:40 AM, Ulf Hansson wrote:
>>> On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> Patch all drivers and core code which uses mmc_set_signal_voltage()
>>>> and prepare it for the fact that mmc_set_signal_voltage() can return
>>>> a value > 0, which would happen if the signal voltage switch did NOT
>>>> happen, because the voltage was already set correctly.
>>>
>>> I am not sure why you want to change mmc_set_signal_voltage(), can you
>>> elaborate on that?
>>>
>>> I thought we discussed changing mmc_regulator_set_vqmmc(), what am I missing?
>>
>> Because mmc_set_signal_voltage() optionally calls
>> host->ops_>start_signal_voltage_switch() , which can now return value >
>> 0 , so the rest of the core needs to be patched to cater for that.
> 
> The issue that you wanted to solve (at least by looking at the
> original patch) was to understand whether the vqmmc regulator changes
> voltage level and then take different actions based on that in the
> mmci host driver.
> 
> You don't need to change anything related to mmc_set_signal_voltage()
> to accomplish that, do you? Moreover, I am worried that it may affect
> the host driver's expectations from when
> ->start_signal_voltage_switch() may be called.

So, shall I just patch all the sites which use mmc_regulator_set_vqmmc()
with something like

return mmc_regulator_set_vqmmc();
becomes
ret = mmc_regulator_set_vqmmc();
if (ret > 0)
	ret = 0;
return ret;

?

Re: [PATCH 1/3] mmc: Prepare all code for mmc_set_signal_voltage() returning > 0

[Ulf Hansson]

On Thu, 16 Apr 2020 at 16:45, Marek Vasut <marex@denx.de> wrote:
>
> On 4/16/20 2:07 PM, Ulf Hansson wrote:
> > On Thu, 16 Apr 2020 at 12:29, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 4/15/20 10:40 AM, Ulf Hansson wrote:
> >>> On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> Patch all drivers and core code which uses mmc_set_signal_voltage()
> >>>> and prepare it for the fact that mmc_set_signal_voltage() can return
> >>>> a value > 0, which would happen if the signal voltage switch did NOT
> >>>> happen, because the voltage was already set correctly.
> >>>
> >>> I am not sure why you want to change mmc_set_signal_voltage(), can you
> >>> elaborate on that?
> >>>
> >>> I thought we discussed changing mmc_regulator_set_vqmmc(), what am I missing?
> >>
> >> Because mmc_set_signal_voltage() optionally calls
> >> host->ops_>start_signal_voltage_switch() , which can now return value >
> >> 0 , so the rest of the core needs to be patched to cater for that.
> >
> > The issue that you wanted to solve (at least by looking at the
> > original patch) was to understand whether the vqmmc regulator changes
> > voltage level and then take different actions based on that in the
> > mmci host driver.
> >
> > You don't need to change anything related to mmc_set_signal_voltage()
> > to accomplish that, do you? Moreover, I am worried that it may affect
> > the host driver's expectations from when
> > ->start_signal_voltage_switch() may be called.
>
> So, shall I just patch all the sites which use mmc_regulator_set_vqmmc()
> with something like
>
> return mmc_regulator_set_vqmmc();
> becomes
> ret = mmc_regulator_set_vqmmc();
> if (ret > 0)
>         ret = 0;
> return ret;

Yes, something along the lines of that. Or if you think it's better to
make mmc_regulator_set_vqmmc() to pass an out parameter. Whatever you
think looks best, I am fine with whatever.

Kind regards
Uffe