a few sdhci/imx clean up and fix patches

After commit d6463f170cf0 ("mmc: sdhci: Remove redundant runtime PM calls"), some of original sdhci_do_xx() function wrappers becomes meaningless, so remove them.

Signed-off-by Dong Aisheng <[email protected]>

Quoted 5 lines · Dong Aisheng

After commit d6463f170cf0 ("mmc: sdhci: Remove redundant runtime PM calls"), some of original sdhci_do_xx() function wrappers becomes meaningless, so remove them.

Signed-off-by Dong Aisheng <[email protected]>

Didn't apply cleanly, otherwise:

Acked-by Adrian Hunter <[email protected]>

After commit d6463f170cf0 ("mmc: sdhci: Remove redundant runtime PM calls"), some of original sdhci_do_xx() function wrappers becomes meaningless, so remove them.

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 60 ++++++++++++------------------------------------
 1 file changed, 15 insertions(+), 45 deletions(-)

drivers/mmc/host/sdhci.c +15 -46

index 8e74e75..be52a3a 100644
@@ -53,7 +53,7 @@ static void sdhci_finish_data(struct sdhci_host *);
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
-static int sdhci_do_get_cd(struct sdhci_host *host);
+static int sdhci_get_cd(struct mmc_host *mmc);
 
 #ifdef CONFIG_PM
 static void sdhci_runtime_pm_bus_on(struct sdhci_host *host);

@@ -194,7 +194,7 @@ EXPORT_SYMBOL_GPL(sdhci_reset);
 static void sdhci_do_reset(struct sdhci_host *host, u8 mask)
 {
        if (host->quirks & SDHCI_QUIRK_NO_CARD_NO_RESET) {
-               if (!sdhci_do_get_cd(host))
+               if (!sdhci_get_cd(host->mmc))
                        return;
        }
 

@@ -1393,11 +1393,11 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 }
 EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
 
-static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
+static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
+       struct sdhci_host *host = mmc_priv(mmc);
        unsigned long flags;
        u8 ctrl;
-       struct mmc_host *mmc = host->mmc;
 
        spin_lock_irqsave(&host->lock, flags);
 

@@ -1551,16 +1551,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
        spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+static int sdhci_get_cd(struct mmc_host *mmc)
 {
        struct sdhci_host *host = mmc_priv(mmc);
-
-       sdhci_do_set_ios(host, ios);
-}
-
-static int sdhci_do_get_cd(struct sdhci_host *host)
-{
-       int gpio_cd = mmc_gpio_get_cd(host->mmc);
+       int gpio_cd = mmc_gpio_get_cd(mmc);
 
        if (host->flags & SDHCI_DEVICE_DEAD)
                return 0;

@@ -1584,13 +1578,6 @@ static int sdhci_do_get_cd(struct sdhci_host *host)
        return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
 }
 
-static int sdhci_get_cd(struct mmc_host *mmc)
-{
-       struct sdhci_host *host = mmc_priv(mmc);
-
-       return sdhci_do_get_cd(host);
-}
-
 static int sdhci_check_ro(struct sdhci_host *host)
 {
        unsigned long flags;

@@ -1615,8 +1602,9 @@ static int sdhci_check_ro(struct sdhci_host *host)
 
 #define SAMPLE_COUNT   5
 
-static int sdhci_do_get_ro(struct sdhci_host *host)
+static int sdhci_get_ro(struct mmc_host *mmc)
 {
+       struct sdhci_host *host = mmc_priv(mmc);
        int i, ro_count;
 
        if (!(host->quirks & SDHCI_QUIRK_UNSTABLE_RO_DETECT))

@@ -1641,13 +1629,6 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
                host->ops->hw_reset(host);
 }
 
-static int sdhci_get_ro(struct mmc_host *mmc)
-{
-       struct sdhci_host *host = mmc_priv(mmc);
-
-       return sdhci_do_get_ro(host);
-}
-
 static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
 {
        if (!(host->flags & SDHCI_DEVICE_DEAD)) {

@@ -1677,10 +1658,10 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
        spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
-                                               struct mmc_ios *ios)
+static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
+                                            struct mmc_ios *ios)
 {
-       struct mmc_host *mmc = host->mmc;
+       struct sdhci_host *host = mmc_priv(mmc);
        u16 ctrl;
        int ret;
 

@@ -1768,17 +1749,6 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
        }
 }
 
-static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
-       struct mmc_ios *ios)
-{
-       struct sdhci_host *host = mmc_priv(mmc);
-
-       if (host->version < SDHCI_SPEC_300)
-               return 0;
-
-       return sdhci_do_start_signal_voltage_switch(host, ios);
-}
-
 static int sdhci_card_busy(struct mmc_host *mmc)
 {
        struct sdhci_host *host = mmc_priv(mmc);

@@ -2070,7 +2040,7 @@ static void sdhci_card_event(struct mmc_host *mmc)
        if (host->ops->card_event)
                host->ops->card_event(host);
 
-       present = sdhci_do_get_cd(host);
+       present = sdhci_get_cd(host->mmc);
 
        spin_lock_irqsave(&host->lock, flags);
 

@@ -2643,7 +2613,7 @@ int sdhci_resume_host(struct sdhci_host *host)
                sdhci_init(host, 0);
                host->pwr = 0;
                host->clock = 0;
-               sdhci_do_set_ios(host, &host->mmc->ios);
+               sdhci_set_ios(host->mmc, &host->mmc->ios);
        } else {
                sdhci_init(host, (host->mmc->pm_flags & MMC_PM_KEEP_POWER));
                mmiowb();

@@ -2721,8 +2691,8 @@ int sdhci_runtime_resume_host(struct sdhci_host *host)
        /* Force clock and power re-program */
        host->pwr = 0;
        host->clock = 0;
-       sdhci_do_start_signal_voltage_switch(host, &host->mmc->ios);
-       sdhci_do_set_ios(host, &host->mmc->ios);
+       sdhci_start_signal_voltage_switch(host->mmc, &host->mmc->ios);
+       sdhci_set_ios(host->mmc, &host->mmc->ios);
 
        if ((host_flags & SDHCI_PV_ENABLED) &&
                !(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN)) {
--

1.9.1

Move sdhci_get_cd() to avoid needing to declare this function before use.

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 55 ++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

drivers/mmc/host/sdhci.c +27 -28

index be52a3a..839aa4c 100644
@@ -53,7 +53,6 @@ static void sdhci_finish_data(struct sdhci_host *);
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
-static int sdhci_get_cd(struct mmc_host *mmc);
 
 #ifdef CONFIG_PM
 static void sdhci_runtime_pm_bus_on(struct sdhci_host *host);

@@ -161,6 +160,33 @@ static void sdhci_disable_card_detection(struct sdhci_host *host)
 	sdhci_set_card_detection(host, false);
 }
 
+static int sdhci_get_cd(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	int gpio_cd = mmc_gpio_get_cd(mmc);
+
+	if (host->flags & SDHCI_DEVICE_DEAD)
+		return 0;
+
+	/* If nonremovable, assume that the card is always present. */
+	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+		return 1;
+
+	/*
+	 * Try slot gpio detect, if defined it take precedence
+	 * over build in controller functionality
+	 */
+	if (!IS_ERR_VALUE(gpio_cd))
+		return !!gpio_cd;
+
+	/* If polling, assume that the card is always present. */
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		return 1;
+
+	/* Host native card detect */
+	return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+}
+
 void sdhci_reset(struct sdhci_host *host, u8 mask)
 {
 	unsigned long timeout;

@@ -1551,33 +1577,6 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_get_cd(struct mmc_host *mmc)
-{
-	struct sdhci_host *host = mmc_priv(mmc);
-	int gpio_cd = mmc_gpio_get_cd(mmc);
-
-	if (host->flags & SDHCI_DEVICE_DEAD)
-		return 0;
-
-	/* If nonremovable, assume that the card is always present. */
-	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
-		return 1;
-
-	/*
-	 * Try slot gpio detect, if defined it take precedence
-	 * over build in controller functionality
-	 */
-	if (!IS_ERR_VALUE(gpio_cd))
-		return !!gpio_cd;
-
-	/* If polling, assume that the card is always present. */
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		return 1;
-
-	/* Host native card detect */
-	return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
-}
-
 static int sdhci_check_ro(struct sdhci_host *host)
 {
 	unsigned long flags;

Quoted 2 lines · Dong Aisheng

Move sdhci_get_cd() to avoid needing to declare this function before use.

In fact, we shouldn't be calling host functions directly. i.e. the call should be host->mmc->ops->get_cd().

However the caller sdhci_do_reset() is called under spinlock so it should not be calling ->get_cd at all.

What that means is that we really need to get rid of SDHCI_QUIRK_NO_CARD_NO_RESET, after which this patch wouldn't be needed, so let's leave it for now.

Quoted 87 lines

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 55 ++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

drivers/mmc/host/sdhci.c +27 -28

index be52a3a..839aa4c 100644
@@ -53,7 +53,6 @@ static void sdhci_finish_data(struct sdhci_host *);
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
-static int sdhci_get_cd(struct mmc_host *mmc);
 
 #ifdef CONFIG_PM
 static void sdhci_runtime_pm_bus_on(struct sdhci_host *host);

@@ -161,6 +160,33 @@ static void sdhci_disable_card_detection(struct sdhci_host *host)
      sdhci_set_card_detection(host, false);
 }
 
+static int sdhci_get_cd(struct mmc_host *mmc)
+{
+     struct sdhci_host *host = mmc_priv(mmc);
+     int gpio_cd = mmc_gpio_get_cd(mmc);
+
+     if (host->flags & SDHCI_DEVICE_DEAD)
+             return 0;
+
+     /* If nonremovable, assume that the card is always present. */
+     if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+             return 1;
+
+     /*
+      * Try slot gpio detect, if defined it take precedence
+      * over build in controller functionality
+      */
+     if (!IS_ERR_VALUE(gpio_cd))
+             return !!gpio_cd;
+
+     /* If polling, assume that the card is always present. */
+     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+             return 1;
+
+     /* Host native card detect */
+     return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+}
+
 void sdhci_reset(struct sdhci_host *host, u8 mask)
 {
      unsigned long timeout;

@@ -1551,33 +1577,6 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
      spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_get_cd(struct mmc_host *mmc)
-{
-     struct sdhci_host *host = mmc_priv(mmc);
-     int gpio_cd = mmc_gpio_get_cd(mmc);
-
-     if (host->flags & SDHCI_DEVICE_DEAD)
-             return 0;
-
-     /* If nonremovable, assume that the card is always present. */
-     if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
-             return 1;
-
-     /*
-      * Try slot gpio detect, if defined it take precedence
-      * over build in controller functionality
-      */
-     if (!IS_ERR_VALUE(gpio_cd))
-             return !!gpio_cd;
-
-     /* If polling, assume that the card is always present. */
-     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-             return 1;
-
-     /* Host native card detect */
-     return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
-}
-
 static int sdhci_check_ro(struct sdhci_host *host)
 {
      unsigned long flags;

Quoted 10 lines · Adrian Hunter

Quoted 2 lines · Dong Aisheng

Move sdhci_get_cd() to avoid needing to declare this function before use.

In fact, we shouldn't be calling host functions directly. i.e. the call should be host->mmc->ops->get_cd().

However the caller sdhci_do_reset() is called under spinlock so it should not be calling ->get_cd at all.

Quoted 4 lines

What that means is that we really need to get rid of SDHCI_QUIRK_NO_CARD_NO_RESET, after which this patch wouldn't be needed, so let's leave it for now.

Yes, i agree we need get rid of SDHCI_QUIRK_NO_CARD_NO_RESET. However, after that, we still need to remove the sdhci_get_cd pre-declaration, right?

Regards
Dong Aisheng

Quoted 88 lines

Quoted 87 lines

Signed-off-by Dong Aisheng <[email protected]>

---
  drivers/mmc/host/sdhci.c | 55 ++++++++++++++++++++++++------------------------
  1 file changed, 27 insertions(+), 28 deletions(-)

drivers/mmc/host/sdhci.c +27 -28

index be52a3a..839aa4c 100644
@@ -53,7 +53,6 @@ static void sdhci_finish_data(struct sdhci_host *);
  static void sdhci_finish_command(struct sdhci_host *);
  static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
  static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
-static int sdhci_get_cd(struct mmc_host *mmc);
 
  #ifdef CONFIG_PM
  static void sdhci_runtime_pm_bus_on(struct sdhci_host *host);

@@ -161,6 +160,33 @@ static void sdhci_disable_card_detection(struct sdhci_host *host)
       sdhci_set_card_detection(host, false);
  }
 
+static int sdhci_get_cd(struct mmc_host *mmc)
+{
+     struct sdhci_host *host = mmc_priv(mmc);
+     int gpio_cd = mmc_gpio_get_cd(mmc);
+
+     if (host->flags & SDHCI_DEVICE_DEAD)
+             return 0;
+
+     /* If nonremovable, assume that the card is always present. */
+     if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+             return 1;
+
+     /*
+      * Try slot gpio detect, if defined it take precedence
+      * over build in controller functionality
+      */
+     if (!IS_ERR_VALUE(gpio_cd))
+             return !!gpio_cd;
+
+     /* If polling, assume that the card is always present. */
+     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+             return 1;
+
+     /* Host native card detect */
+     return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+}
+
  void sdhci_reset(struct sdhci_host *host, u8 mask)
  {
       unsigned long timeout;

@@ -1551,33 +1577,6 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
       spin_unlock_irqrestore(&host->lock, flags);
  }
 
-static int sdhci_get_cd(struct mmc_host *mmc)
-{
-     struct sdhci_host *host = mmc_priv(mmc);
-     int gpio_cd = mmc_gpio_get_cd(mmc);
-
-     if (host->flags & SDHCI_DEVICE_DEAD)
-             return 0;
-
-     /* If nonremovable, assume that the card is always present. */
-     if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
-             return 1;
-
-     /*
-      * Try slot gpio detect, if defined it take precedence
-      * over build in controller functionality
-      */
-     if (!IS_ERR_VALUE(gpio_cd))
-             return !!gpio_cd;
-
-     /* If polling, assume that the card is always present. */
-     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-             return 1;
-
-     /* Host native card detect */
-     return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
-}
-
  static int sdhci_check_ro(struct sdhci_host *host)
  {
       unsigned long flags;

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/core/mmc_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/core/mmc_ops.c +1 -1

index 62355bd..d220bbd 100644
@@ -553,7 +553,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 
 		/*
 		 * We are not allowed to issue a status command and the host
-		 * does'nt support MMC_CAP_WAIT_WHILE_BUSY, then we can only
+		 * doesn't support MMC_CAP_WAIT_WHILE_BUSY, then we can only
 		 * rely on waiting for the stated timeout to be sufficient.
 		 */
 		if (!send_status) {

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 97 ++++++++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 40 deletions(-)

drivers/mmc/host/sdhci.c +57 -40

index 839aa4c..7f63f5d 100644
@@ -1657,19 +1657,10 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
-					     struct mmc_ios *ios)
+static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
+					  struct mmc_ios *ios)
 {
-	struct sdhci_host *host = mmc_priv(mmc);
 	u16 ctrl;
-	int ret;
-
-	/*
-	 * Signal Voltage Switching is only applicable for Host Controllers
-	 * v3.00 and above.
-	 */
-	if (host->version < SDHCI_SPEC_300)
-		return 0;
 
 	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 

@@ -1679,15 +1670,6 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 		ctrl &= ~SDHCI_CTRL_VDD_180;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
-		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
-			if (ret) {
-				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
-					mmc_hostname(mmc));
-				return -EIO;
-			}
-		}
 		/* Wait for 5ms */
 		usleep_range(5000, 5500);
 

@@ -1697,20 +1679,10 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 			return 0;
 
 		pr_warn("%s: 3.3V regulator output did not became stable\n",
-			mmc_hostname(mmc));
+			mmc_hostname(host->mmc));
 
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
-		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
-			if (ret) {
-				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
-					mmc_hostname(mmc));
-				return -EIO;
-			}
-		}
-
 		/*
 		 * Enable 1.8V Signal Enable in the Host Control2
 		 * register

@@ -1728,18 +1700,63 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 			return 0;
 
 		pr_warn("%s: 1.8V regulator output did not became stable\n",
-			mmc_hostname(mmc));
+			mmc_hostname(host->mmc));
 
 		return -EAGAIN;
+	default:
+		/* No signal voltage switch required */
+		return 0;
+	}
+}
+
+static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
+					     struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	int ret;
+
+	/*
+	 * Signal Voltage Switching is only applicable for Host Controllers
+	 * v3.00 and above.
+	 */
+	if (host->version < SDHCI_SPEC_300)
+		return 0;
+
+	ret = sdhci_do_signal_voltage_switch(host, ios);
+	if (ret)
+		return ret;
+
+	if (IS_ERR(mmc->supply.vqmmc))
+		return 0;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_330:
+		ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
+					    3600000);
+		if (ret) {
+			pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
+				mmc_hostname(mmc));
+			return -EIO;
+		}
+
+		return 0;
+	case MMC_SIGNAL_VOLTAGE_180:
+		ret = regulator_set_voltage(mmc->supply.vqmmc,
+				1700000, 1950000);
+		if (ret) {
+			pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
+				mmc_hostname(mmc));
+			return -EIO;
+		}
+
+		return 0;
 	case MMC_SIGNAL_VOLTAGE_120:
-		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
-			if (ret) {
-				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
-					mmc_hostname(mmc));
-				return -EIO;
-			}
+		ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
+					    1300000);
+		if (ret) {
+			pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
+				mmc_hostname(mmc));
+			return -EIO;
 		}
 		return 0;
 	default:

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Quoted 144 lines

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 97 ++++++++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 40 deletions(-)

drivers/mmc/host/sdhci.c +57 -40

index 839aa4c..7f63f5d 100644
@@ -1657,19 +1657,10 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
      spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
-                                          struct mmc_ios *ios)
+static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
+                                       struct mmc_ios *ios)
 {
-     struct sdhci_host *host = mmc_priv(mmc);
      u16 ctrl;
-     int ret;
-
-     /*
-      * Signal Voltage Switching is only applicable for Host Controllers
-      * v3.00 and above.
-      */
-     if (host->version < SDHCI_SPEC_300)
-             return 0;
 
      ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 

@@ -1679,15 +1670,6 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
              ctrl &= ~SDHCI_CTRL_VDD_180;
              sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
-             if (!IS_ERR(mmc->supply.vqmmc)) {
-                     ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-                                                 3600000);
-                     if (ret) {
-                             pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
-                                     mmc_hostname(mmc));
-                             return -EIO;
-                     }
-             }
              /* Wait for 5ms */
              usleep_range(5000, 5500);
 

@@ -1697,20 +1679,10 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
                      return 0;
 
              pr_warn("%s: 3.3V regulator output did not became stable\n",
-                     mmc_hostname(mmc));
+                     mmc_hostname(host->mmc));
 
              return -EAGAIN;
      case MMC_SIGNAL_VOLTAGE_180:
-             if (!IS_ERR(mmc->supply.vqmmc)) {
-                     ret = regulator_set_voltage(mmc->supply.vqmmc,
-                                     1700000, 1950000);
-                     if (ret) {
-                             pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
-                                     mmc_hostname(mmc));
-                             return -EIO;
-                     }
-             }
-
              /*
               * Enable 1.8V Signal Enable in the Host Control2
               * register

@@ -1728,18 +1700,63 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
                      return 0;
 
              pr_warn("%s: 1.8V regulator output did not became stable\n",
-                     mmc_hostname(mmc));
+                     mmc_hostname(host->mmc));
 
              return -EAGAIN;
+     default:
+             /* No signal voltage switch required */
+             return 0;
+     }
+}
+
+static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
+                                          struct mmc_ios *ios)
+{
+     struct sdhci_host *host = mmc_priv(mmc);
+     int ret;
+
+     /*
+      * Signal Voltage Switching is only applicable for Host Controllers
+      * v3.00 and above.
+      */
+     if (host->version < SDHCI_SPEC_300)
+             return 0;
+
+     ret = sdhci_do_signal_voltage_switch(host, ios);
+     if (ret)
+             return ret;
+
+     if (IS_ERR(mmc->supply.vqmmc))
+             return 0;
+
+     switch (ios->signal_voltage) {
+     case MMC_SIGNAL_VOLTAGE_330:
+             ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
+                                         3600000);
+             if (ret) {
+                     pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
+                             mmc_hostname(mmc));
+                     return -EIO;
+             }
+
+             return 0;
+     case MMC_SIGNAL_VOLTAGE_180:
+             ret = regulator_set_voltage(mmc->supply.vqmmc,
+                             1700000, 1950000);
+             if (ret) {
+                     pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
+                             mmc_hostname(mmc));
+                     return -EIO;
+             }
+
+             return 0;
      case MMC_SIGNAL_VOLTAGE_120:
-             if (!IS_ERR(mmc->supply.vqmmc)) {
-                     ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-                                                 1300000);
-                     if (ret) {
-                             pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
-                                     mmc_hostname(mmc));
-                             return -EIO;
-                     }
+             ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
+                                         1300000);
+             if (ret) {
+                     pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
+                             mmc_hostname(mmc));
+                     return -EIO;
              }
              return 0;
      default:

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

Quoted 60 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

Quoted 3 lines

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

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

Quoted 63 lines · Adrian Hunter

Quoted 62 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

Quoted 5 lines

Quoted 1 line

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

Quoted 3 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

Quoted 64 lines · Dong Aisheng

Quoted 63 lines · Adrian Hunter

Quoted 62 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

We have to move forward *and* avoid potential breakage.

In this case it seems me that the risk of breakage outweighs the value of prettier code.

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

drivers/mmc/host/sdhci.c +3 -7

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));
 
 
 
--

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

Hi Adrian,

Quoted 102 lines · Adrian Hunter

Quoted 64 lines · Dong Aisheng

Quoted 63 lines · Adrian Hunter

Quoted 62 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

We have to move forward *and* avoid potential breakage.

In this case it seems me that the risk of breakage outweighs the value of prettier code.

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 4 lines

Quoted 3 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Quoted 4 lines

Quoted 2 lines

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

maybe.. this question should not be related with this discussion..
But i have one question..sdhci_do_start_signal_voltage_switch() returned 0 or EAGAIN, when IS_ERR(mmc->supply.vqmmc) is ture. It there any problem?

I'm also checking on core side. but just wondering this. (Because i'm fixing dwmmc controller for this.)

Best Regards,
Jaehoon Chung

Quoted 44 lines

drivers/mmc/host/sdhci.c +3 -7

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));
 
 
 
--

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

Quoted 105 lines · Jaehoon Chung

Hi Adrian,

Quoted 102 lines · Adrian Hunter

Quoted 64 lines · Dong Aisheng

Quoted 63 lines · Adrian Hunter

Quoted 62 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

We have to move forward *and* avoid potential breakage.

In this case it seems me that the risk of breakage outweighs the value of prettier code.

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 4 lines

Quoted 3 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

maybe.. this question should not be related with this discussion..
But i have one question..sdhci_do_start_signal_voltage_switch() returned 0 or EAGAIN, when IS_ERR(mmc->supply.vqmmc) is ture. It there any problem?

Not that I am aware of.

Quoted 3 lines

I'm also checking on core side. but just wondering this. (Because i'm fixing dwmmc controller for this.)

What is the problem?

Quoted 43 lines

Best Regards,
Jaehoon Chung

Quoted 38 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 8 lines

Quoted 6 lines

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

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

Quoted 115 lines · Adrian Hunter

Quoted 105 lines · Jaehoon Chung

Hi Adrian,

Quoted 102 lines · Adrian Hunter

Quoted 64 lines · Dong Aisheng

Quoted 63 lines · Adrian Hunter

Quoted 62 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

We have to move forward *and* avoid potential breakage.

In this case it seems me that the risk of breakage outweighs the value of prettier code.

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 4 lines

Quoted 3 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

maybe.. this question should not be related with this discussion..
But i have one question..sdhci_do_start_signal_voltage_switch() returned 0 or EAGAIN, when IS_ERR(mmc->supply.vqmmc) is ture. It there any problem?

Quoted 7 lines

Not that I am aware of.

Quoted 3 lines

I'm also checking on core side. but just wondering this. (Because i'm fixing dwmmc controller for this.)

What is the problem?

It should be difference with dwmmc controller.
if mmc->supply.vqmmc is not assigned, it didn't change the voltage..
(I'm not sure that HOST_CONTROL2 register can internally change the voltage..because i didn't have SDHC 3.0 spec.)

__mmc_set_signal_voltage()
-> host->ops->start_signal_voltage_switch()
-> host controller just set the bits for v1.8 or v3.3..(if vqmmc didn't use.)
if return -EAGAIN or 0 , the bits that was set for v1.8 or v3.3 should be maintained.

And do mmc_power_cycle() -> mmc_power_off and mmc_set_initial_state()

But host controller didn't initialize.

That is dwmmc controller's side..

As my understanding, if voltage switch(UHS-I mode) is failed, it should be set to HS mode. but dwmmc controller didn't work this case..so i will fix.
(Some parts are code bugs in dwmmc controller.)

I don't know sdhci is working fine or not..just wondering. :)

I'm going to analyze the sequence and other thing..so i may miss something.

Best Regards,
Jaehoon Chung

Quoted 58 lines

Quoted 43 lines

Best Regards,
Jaehoon Chung

Quoted 38 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 8 lines

Quoted 6 lines

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

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

Quoted 139 lines · Jaehoon Chung

Quoted 115 lines · Adrian Hunter

Quoted 105 lines · Jaehoon Chung

Hi Adrian,

Quoted 102 lines · Adrian Hunter

Quoted 64 lines · Dong Aisheng

Quoted 63 lines · Adrian Hunter

Quoted 62 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

We have to move forward *and* avoid potential breakage.

In this case it seems me that the risk of breakage outweighs the value of prettier code.

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 4 lines

Quoted 3 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

maybe.. this question should not be related with this discussion..
But i have one question..sdhci_do_start_signal_voltage_switch() returned 0 or EAGAIN, when IS_ERR(mmc->supply.vqmmc) is ture. It there any problem?

Quoted 7 lines

Not that I am aware of.

Quoted 3 lines

I'm also checking on core side. but just wondering this. (Because i'm fixing dwmmc controller for this.)

What is the problem?

It should be difference with dwmmc controller.
if mmc->supply.vqmmc is not assigned, it didn't change the voltage..
(I'm not sure that HOST_CONTROL2 register can internally change the voltage..because i didn't have SDHC 3.0 spec.)

__mmc_set_signal_voltage()
-> host->ops->start_signal_voltage_switch()
-> host controller just set the bits for v1.8 or v3.3..(if vqmmc didn't use.)
if return -EAGAIN or 0 , the bits that was set for v1.8 or v3.3 should be maintained.

And do mmc_power_cycle() -> mmc_power_off and mmc_set_initial_state()

But host controller didn't initialize.

That is dwmmc controller's side..

Quoted 4 lines

As my understanding, if voltage switch(UHS-I mode) is failed, it should be set to HS mode. but dwmmc controller didn't work this case..so i will fix.
(Some parts are code bugs in dwmmc controller.)

MMC core will retry HS mode initialization if voltage switch reach 10 times failure.
See: mmc_sd_get_cid()

Quoted 3 lines

I don't know sdhci is working fine or not..just wondering. :)

I tested sdhci is working fine.

Quoted 2 lines

I'm going to analyze the sequence and other thing..so i may miss something.

Probably you may need check if the IO voltage is correctly back up after failure.

Regards
Dong Aisheng

Quoted 62 lines

Best Regards,
Jaehoon Chung

Quoted 58 lines

Quoted 43 lines

Best Regards,
Jaehoon Chung

Quoted 38 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 8 lines

Quoted 6 lines

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

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

Quoted 70 lines · Dong Aisheng

Quoted 68 lines · Adrian Hunter

Quoted 60 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

Quoted 3 lines

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Quoted 2 lines

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

We have to move forward *and* avoid potential breakage.

In this case it seems me that the risk of breakage outweighs the value of prettier code.

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 71 lines · Adrian Hunter

Quoted 70 lines · Dong Aisheng

Quoted 68 lines · Adrian Hunter

Quoted 60 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

Quoted 3 lines

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Quoted 9 lines

Quoted 2 lines

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Yes, now i understand your concern.

Quoted 7 lines

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

Quoted 3 lines

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

The "correct way" i mean here is .start_signal_voltage_switch() shouldn't be order dependant, would you agree?

Quoted 3 lines

We have to move forward *and* avoid potential breakage.

If really break happens, fix platform driver, not common SDHCI. That's the same thing you done for sdhci_set_power().

Quoted 4 lines

In this case it seems me that the risk of breakage outweighs the value of prettier code.

Actually my main purpose is patch 6: using mmc_regulator_set_vqmmc() which is worth and it does improve the stability and eliminate the potential signal issue.
However it's not the same way as you proposed.
See below.

Quoted 4 lines

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

Sorry, i did not get that you want to change like below. However, it looks that it does not make too much sense to call mmc_regulator_set_vqmmc() for each VOLTAGE type like 3.3v/1.8v/1.2v which introduces ugliness because mmc_regulator_set_vqmmc() already handles it internally, right?
Only because we want to keep an "ASSUMED" order as before?

Regards
Dong Aisheng

Quoted 37 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 95 lines · Dong Aisheng

Quoted 71 lines · Adrian Hunter

Quoted 70 lines · Dong Aisheng

Quoted 68 lines · Adrian Hunter

Quoted 60 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

Quoted 3 lines

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Quoted 9 lines

Quoted 2 lines

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Yes, now i understand your concern.

Quoted 10 lines

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

Quoted 2 lines

The "correct way" i mean here is .start_signal_voltage_switch() shouldn't be order dependant, would you agree?

No. There is no way to know if the regulator must be switched before or after the host controller register is changed.

Quoted 6 lines

Quoted 3 lines

We have to move forward *and* avoid potential breakage.

If really break happens, fix platform driver, not common SDHCI. That's the same thing you done for sdhci_set_power().

In that case the original behaviour was kept in the common SDHCI code and the driver had to provide its own way.

Quoted 24 lines

Quoted 4 lines

In this case it seems me that the risk of breakage outweighs the value of prettier code.

Actually my main purpose is patch 6: using mmc_regulator_set_vqmmc() which is worth and it does improve the stability and eliminate the potential signal issue.
However it's not the same way as you proposed.
See below.

Quoted 4 lines

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

Quoted 6 lines

Sorry, i did not get that you want to change like below. However, it looks that it does not make too much sense to call mmc_regulator_set_vqmmc() for each VOLTAGE type like 3.3v/1.8v/1.2v which introduces ugliness because mmc_regulator_set_vqmmc() already handles it internally, right?
Only because we want to keep an "ASSUMED" order as before?

Yes

Quoted 42 lines

Regards
Dong Aisheng

Quoted 37 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 99 lines · Adrian Hunter

Quoted 97 lines · Dong Aisheng

Quoted 71 lines · Adrian Hunter

Quoted 70 lines · Dong Aisheng

Quoted 68 lines · Adrian Hunter

Quoted 60 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

Quoted 3 lines

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Quoted 9 lines

Quoted 2 lines

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Yes, now i understand your concern.

Quoted 10 lines

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

The "correct way" i mean here is .start_signal_voltage_switch() shouldn't be order dependant, would you agree?

Quoted 3 lines

No. There is no way to know if the regulator must be switched before or after the host controller register is changed.

Hmm... If there is no way to know the correct order, how can we assume the exist order is correct?

And i already pointed out, the exist order is also confused that it switch controller first then vqmmc for 3.3v and switch vqmmc first, then controller for 1.8v

If we can't sure the exist order is correct, why do we block the changing to correctly use mmc_regulator_set_vqmmc() to improve the driver stability?

And actually the change is not made arbitrarily, i already list all possibilities based on my knowledge. People can raise more if any.

The target is correct that we make start_signal_voltage_switch() order independant.
It's worth a try even there's a potential very low possibility break IMHO.

Or else if we can find a better way to switch to
mmc_regulator_set_vqmmc i would also love to try.

Maybe we need more people's thought on it!

Ulf,
Would you give some inputs?

Regards
Dong Aisheng

Quoted 86 lines

Quoted 6 lines

Quoted 3 lines

We have to move forward *and* avoid potential breakage.

If really break happens, fix platform driver, not common SDHCI. That's the same thing you done for sdhci_set_power().

In that case the original behaviour was kept in the common SDHCI code and the driver had to provide its own way.

Quoted 24 lines

Quoted 4 lines

In this case it seems me that the risk of breakage outweighs the value of prettier code.

Actually my main purpose is patch 6: using mmc_regulator_set_vqmmc() which is worth and it does improve the stability and eliminate the potential signal issue.
However it's not the same way as you proposed.
See below.

Quoted 4 lines

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

Quoted 6 lines

Sorry, i did not get that you want to change like below. However, it looks that it does not make too much sense to call mmc_regulator_set_vqmmc() for each VOLTAGE type like 3.3v/1.8v/1.2v which introduces ugliness because mmc_regulator_set_vqmmc() already handles it internally, right?
Only because we want to keep an "ASSUMED" order as before?

Yes

Quoted 42 lines

Regards
Dong Aisheng

Quoted 37 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Quoted 100 lines · Dong Aisheng

Quoted 99 lines · Adrian Hunter

Quoted 97 lines · Dong Aisheng

Quoted 71 lines · Adrian Hunter

Quoted 70 lines · Dong Aisheng

Quoted 68 lines · Adrian Hunter

Quoted 60 lines · Dong Aisheng

Hi Adrian,

Thanks for the review first.

Quoted 8 lines · Adrian Hunter

Quoted 5 lines · Dong Aisheng

Handle host and regulator signal voltage switch separately. Move host signal voltage switch code into a separated function sdhci_do_signal_voltage_switch() first, the following patches will remove the regulator voltage switch code and use the common mmc_regulator_set_vqmmc() instead.

You have changed the order that things are done.

Yes, the oder changes a bit that we always do controller voltage switch first. I suppose the order is irrelevant here since i don't recall any requirement from card.

Actually the original order is also a bit mass.
e.g.
For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc. But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller. It looks to us the original one also order irrelevant.

Quoted 4 lines

There is no way to know
what that will break, so let's not do that. What about just changing regulator_set_voltage() to mmc_regulator_set_vqmmc()?

Currently what i can think out VIO switch using are three cases: (Pls help add if any)
1) Both host IO and card IO use external vqmmc to do switch (e.g eMMC 1.8V DDR/HS200/HS400 mode)

eMMC has no IO voltage switch protocol and requirement, so usually board designed
using fixed 1.8V for eMMC and host IO.
Event it's switchable, it should be done in the first mmc_power_up(). Dynamical switch later may cause eMMC unable to work properly. (We have been confirmed about this issue by many eMMC vendors like Micron and Sandisk. I'm not sure if any exceptions in the community still doing VIO dynamical switch for eMMC, if yes, please help share the experience!).

Event some people still do dynamical IO switch for eMMC, since eMMC spec has no requirement, so the order should also not care.

2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)

SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal regulator to do card IO voltage switch. It does not use external vqmmc regulator.
So order irrelevant too.

3) Host using controller IO switch while card using external vqmmc (special SDIO3.0 or eMMC)
I have met some special SDIO3.0 card like Broadcom WiFi which does not follow the spec and using external regulator for card IO voltage. Usually it's required to fix to 1.8v and also not order irrelevant.

Quoted 3 lines

For eMMC, refer to case 1), it should be fixed to 1.8v at power up.

So it looks all cases seems are not order required.

I don't agree that there is any way to know that other host controllers are not affected. I don't want a repeat of sdhci_set_power().

Quoted 9 lines

Quoted 2 lines

Can you share some more info about sdhci_set_power() issue? I'd like to see if we are same the issue.

Not the same issue, but the same concept. People changing the code under the impression that their way was correct, and then breaking other people's drivers. Check the git history and mailing list.

	http://marc.info/?l=linux-mmc&m=145880454106474&w=2

Yes, now i understand your concern.

Quoted 10 lines

Quoted 6 lines

BTW, IMHO i don't think we should stop keep moving only afraid of potential break if it's correct way. Because .start_signal_voltage_switch() interface seems shouldn't be order dependant.
If it is, then it should be fixed and handled in high layer like MMC core rather than in host driver. Right?

The SDHCI spec. does not define how to use external regulators, so there is no "correct way".

The "correct way" i mean here is .start_signal_voltage_switch() shouldn't be order dependant, would you agree?

Quoted 6 lines

Quoted 3 lines

No. There is no way to know if the regulator must be switched before or after the host controller register is changed.

Hmm... If there is no way to know the correct order, how can we assume the exist order is correct?

There is no correct order. This is outside the SDHCI spec. and so belongs to individual drivers.

If it mattered we could push the ugly code down onto the drivers, and then driver maintainers could opt to use the new pretty code. However at the moment there is a lot more important work, so I would want to avoid that code churn.

Quoted 7 lines

And i already pointed out, the exist order is also confused that it switch controller first then vqmmc for 3.3v and switch vqmmc first, then controller for 1.8v

If we can't sure the exist order is correct, why do we block the changing to correctly use mmc_regulator_set_vqmmc() to improve the driver stability?

Not sure what you mean here. I have already showed how we can use mmc_regulator_set_vqmmc().

Quoted 1 line

Quoted 3 lines

And actually the change is not made arbitrarily, i already list all possibilities based on my knowledge. People can raise more if any.

You are assuming every driver has a maintainer and every maintainer is following this thread, and understands how it might affect all the different versions of their hardware. That is extremely unlikely.

Quoted 4 lines

The target is correct that we make start_signal_voltage_switch() order independant.
It's worth a try even there's a potential very low possibility break IMHO.

And so we disagree.

Aren't your needs met by changing to use mmc_regulator_set_vqmmc() the way I suggested?

Quoted 99 lines

Or else if we can find a better way to switch to
mmc_regulator_set_vqmmc i would also love to try.

Maybe we need more people's thought on it!

Ulf,
Would you give some inputs?

Regards
Dong Aisheng

Quoted 86 lines

Quoted 6 lines

Quoted 3 lines

We have to move forward *and* avoid potential breakage.

If really break happens, fix platform driver, not common SDHCI. That's the same thing you done for sdhci_set_power().

In that case the original behaviour was kept in the common SDHCI code and the driver had to provide its own way.

Quoted 24 lines

Quoted 4 lines

In this case it seems me that the risk of breakage outweighs the value of prettier code.

Actually my main purpose is patch 6: using mmc_regulator_set_vqmmc() which is worth and it does improve the stability and eliminate the potential signal issue.
However it's not the same way as you proposed.
See below.

Quoted 4 lines

By the way, there are ways to get rid of the ugliness - such as pushing it down into individual drivers.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Please instead send a patch for just using mmc_regulator_set_vqmmc() in place of regulator_set_voltage().

Just using mmc_regulator_set_vqmmc() also changes the order which is the same situation.

How so? It looks like a drop-in replacement to me:

Quoted 6 lines

Sorry, i did not get that you want to change like below. However, it looks that it does not make too much sense to call mmc_regulator_set_vqmmc() for each VOLTAGE type like 3.3v/1.8v/1.2v which introduces ugliness because mmc_regulator_set_vqmmc() already handles it internally, right?
Only because we want to keep an "ASSUMED" order as before?

Yes

Quoted 42 lines

Regards
Dong Aisheng

Quoted 37 lines

drivers/mmc/host/sdhci.c +3 -6

index 94cffa77490a..69b4d48aff87 100644
@@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-						    3600000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc,
-					1700000, 1950000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
 					mmc_hostname(mmc));

@@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_120:
 		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-						    1300000);
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
 			if (ret) {
 				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
 					mmc_hostname(mmc));

Instead of using private VCCQ regulator signal voltage switch code, we switch to use the more robust common function mmc_regulator_set_vqmmc() in MMC core which set the target voltage as close as possible to target voltage.

Signed-off-by Dong Aisheng <[email protected]>

---
Don't have a board to test mmc_regulator_set_vqmmc() switch way, need others to help verify.

---
 drivers/mmc/host/sdhci.c | 39 ++-------------------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

drivers/mmc/host/sdhci.c +2 -37

index 7f63f5d..2338aab 100644
@@ -1726,43 +1726,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 	if (ret)
 		return ret;
 
-	if (IS_ERR(mmc->supply.vqmmc))
-		return 0;
-
-	switch (ios->signal_voltage) {
-	case MMC_SIGNAL_VOLTAGE_330:
-		ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-					    3600000);
-		if (ret) {
-			pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
-				mmc_hostname(mmc));
-			return -EIO;
-		}
-
-		return 0;
-	case MMC_SIGNAL_VOLTAGE_180:
-		ret = regulator_set_voltage(mmc->supply.vqmmc,
-				1700000, 1950000);
-		if (ret) {
-			pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
-				mmc_hostname(mmc));
-			return -EIO;
-		}
-
-		return 0;
-	case MMC_SIGNAL_VOLTAGE_120:
-		ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-					    1300000);
-		if (ret) {
-			pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
-				mmc_hostname(mmc));
-			return -EIO;
-		}
-		return 0;
-	default:
-		/* No signal voltage switch required */
-		return 0;
-	}
+	/* do regulator signal voltage switch if exist */
+	return mmc_regulator_set_vqmmc(mmc, ios);
 }
 
 static int sdhci_card_busy(struct mmc_host *mmc)

Quoted 5 lines · Dong Aisheng

Instead of using private VCCQ regulator signal voltage switch code, we switch to use the more robust common function mmc_regulator_set_vqmmc() in MMC core which set the target voltage as close as possible to target voltage.

Quoted 4 lines

Signed-off-by Dong Aisheng <[email protected]>

---
Don't have a board to test mmc_regulator_set_vqmmc() switch way, need others to help verify.

So was there a reason you wanted to change it?

I agree mmc_regulator_set_vqmmc() looks superior, but we really need feedback from people using this code..

Quoted 55 lines

---
 drivers/mmc/host/sdhci.c | 39 ++-------------------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

drivers/mmc/host/sdhci.c +2 -37

index 7f63f5d..2338aab 100644
@@ -1726,43 +1726,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
      if (ret)
              return ret;
 
-     if (IS_ERR(mmc->supply.vqmmc))
-             return 0;
-
-     switch (ios->signal_voltage) {
-     case MMC_SIGNAL_VOLTAGE_330:
-             ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
-                                         3600000);
-             if (ret) {
-                     pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
-                             mmc_hostname(mmc));
-                     return -EIO;
-             }
-
-             return 0;
-     case MMC_SIGNAL_VOLTAGE_180:
-             ret = regulator_set_voltage(mmc->supply.vqmmc,
-                             1700000, 1950000);
-             if (ret) {
-                     pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
-                             mmc_hostname(mmc));
-                     return -EIO;
-             }
-
-             return 0;
-     case MMC_SIGNAL_VOLTAGE_120:
-             ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
-                                         1300000);
-             if (ret) {
-                     pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
-                             mmc_hostname(mmc));
-                     return -EIO;
-             }
-             return 0;
-     default:
-             /* No signal voltage switch required */
-             return 0;
-     }
+     /* do regulator signal voltage switch if exist */
+     return mmc_regulator_set_vqmmc(mmc, ios);
 }
 
 static int sdhci_card_busy(struct mmc_host *mmc)

Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V which means it can work on DDR mode with either 3.3v IO or 1.8v IO voltage. MMC core will first try 1.8v then 3.3v if host claims MMC_CAP_1_8V_DDR support.
However the host driver voltage switch code does not check NO_1_8_V quirk which may set a wrong 1.8v and causes the card fixed to 3.3v VIO un-work.

Checking 1.8V quirk before setting it to avoid such issue.

CC stable <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mmc/host/sdhci.c +2 -0

index 2338aab..96ccb15 100644
@@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
 
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
+		if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
+			return -EINVAL;
 		/*
 		 * Enable 1.8V Signal Enable in the Host Control2
 		 * register

Quoted 8 lines · Dong Aisheng

Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V which means it can work on DDR mode with either 3.3v IO or 1.8v IO voltage. MMC core will first try 1.8v then 3.3v if host claims MMC_CAP_1_8V_DDR support.
However the host driver voltage switch code does not check NO_1_8_V quirk which may set a wrong 1.8v and causes the card fixed to 3.3v VIO un-work.

Quoted 1 line

Checking 1.8V quirk before setting it to avoid such issue.

We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.

There are two possibilities:

1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of MMC_CAP_1_8V_DDR. You'll need Ulf's feedback on that.

2. Replace ->start_signal_voltage_switch() i.e.

host->mmc_host_ops.start_signal_voltage_switch =
esdhci_start_signal_voltage_switch;

You will also need to change all calls to sdhci_start_signal_voltage_switch() with host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.

Quoted 21 lines

CC stable <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mmc/host/sdhci.c +2 -0

index 2338aab..96ccb15 100644
@@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
 
              return -EAGAIN;
      case MMC_SIGNAL_VOLTAGE_180:
+             if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
+                     return -EINVAL;
              /*
               * Enable 1.8V Signal Enable in the Host Control2
               * register

Quoted 9 lines · Adrian Hunter

Quoted 8 lines · Dong Aisheng

Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V which means it can work on DDR mode with either 3.3v IO or 1.8v IO voltage. MMC core will first try 1.8v then 3.3v if host claims MMC_CAP_1_8V_DDR support.
However the host driver voltage switch code does not check NO_1_8_V quirk which may set a wrong 1.8v and causes the card fixed to 3.3v VIO un-work.

Quoted 4 lines

Quoted 1 line

Checking 1.8V quirk before setting it to avoid such issue.

We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.

Yes, i did try to clean up SDHCI_QUIRK2_NO_1_8_V. However, it seems not that simply.
Currently we may still need it before we got a better way.

My point is whether we should stop fixing the exist issue in SDHCI driver just caused by we want to clean up it later?

Quoted 6 lines

There are two possibilities:

1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of MMC_CAP_1_8V_DDR. You'll need Ulf's feedback on that.

Yes, we can do it.
But the point is for DDR50/SD3.0/SDIO3.0/HS200/HS400 cards we have the same situation and still need QUIRK2_NO_1_8_V.

And i somehow a bit wonder whether we should retrieve the speed mode support from device tree since it's actually controller capability. IO range capability is another thing.

Probably we may start another topic to discuss it specificly.

Quoted 9 lines

2. Replace ->start_signal_voltage_switch() i.e.

host->mmc_host_ops.start_signal_voltage_switch =
esdhci_start_signal_voltage_switch;

You will also need to change all calls to sdhci_start_signal_voltage_switch() with host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.

esdhc can fully use the common sdhci_start_signal_voltag_switch. Since it's already support QUIRK_NO_1_8_V, we don't want to invent imx voltage swith currently, but fix and use it first.

Regards
Dong Aisheng

Quoted 22 lines

Quoted 21 lines

CC stable <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

---
  drivers/mmc/host/sdhci.c | 2 ++
  1 file changed, 2 insertions(+)

drivers/mmc/host/sdhci.c +2 -0

index 2338aab..96ccb15 100644
@@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
 
               return -EAGAIN;
       case MMC_SIGNAL_VOLTAGE_180:
+             if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
+                     return -EINVAL;
               /*
                * Enable 1.8V Signal Enable in the Host Control2
                * register

Quoted 19 lines · Dong Aisheng

Quoted 9 lines · Adrian Hunter

Quoted 8 lines · Dong Aisheng

Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V which means it can work on DDR mode with either 3.3v IO or 1.8v IO voltage. MMC core will first try 1.8v then 3.3v if host claims MMC_CAP_1_8V_DDR support.
However the host driver voltage switch code does not check NO_1_8_V quirk which may set a wrong 1.8v and causes the card fixed to 3.3v VIO un-work.

Quoted 4 lines

Quoted 1 line

Checking 1.8V quirk before setting it to avoid such issue.

We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.

Yes, i did try to clean up SDHCI_QUIRK2_NO_1_8_V. However, it seems not that simply.
Currently we may still need it before we got a better way.

Quoted 2 lines

My point is whether we should stop fixing the exist issue in SDHCI driver just caused by we want to clean up it later?

If you want a fix for stable kernels, then I guess it is OK. In that case it is better to have a fix that applies cleanly to older kernels i.e. not on top of re-factoring

Quoted 56 lines

Quoted 6 lines

There are two possibilities:

1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of MMC_CAP_1_8V_DDR. You'll need Ulf's feedback on that.

Yes, we can do it.
But the point is for DDR50/SD3.0/SDIO3.0/HS200/HS400 cards we have the same situation and still need QUIRK2_NO_1_8_V.

And i somehow a bit wonder whether we should retrieve the speed mode support from device tree since it's actually controller capability. IO range capability is another thing.

Probably we may start another topic to discuss it specificly.

Quoted 9 lines

2. Replace ->start_signal_voltage_switch() i.e.

host->mmc_host_ops.start_signal_voltage_switch =
esdhci_start_signal_voltage_switch;

You will also need to change all calls to sdhci_start_signal_voltage_switch() with host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.

esdhc can fully use the common sdhci_start_signal_voltag_switch. Since it's already support QUIRK_NO_1_8_V, we don't want to invent imx voltage swith currently, but fix and use it first.

Regards
Dong Aisheng

Quoted 22 lines

Quoted 21 lines

CC stable <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mmc/host/sdhci.c +2 -0

index 2338aab..96ccb15 100644
@@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
 
              return -EAGAIN;
      case MMC_SIGNAL_VOLTAGE_180:
+             if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
+                     return -EINVAL;
              /*
               * Enable 1.8V Signal Enable in the Host Control2
               * register

Rename ACMD12 quirk to SDHCI_QUIRK_MULTIBLOCK_ACMD12 to avoid confusion since it's needed for both read and write.

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci-pci-o2micro.c | 2 +-
 drivers/mmc/host/sdhci-pltfm.c       | 2 +-
 drivers/mmc/host/sdhci-s3c.c         | 2 +-
 drivers/mmc/host/sdhci.c             | 2 +-
 drivers/mmc/host/sdhci.h             | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

drivers/mmc/host/sdhci-pci-o2micro.c +1 -1

index d48f031..1ed1f995 100644
@@ -162,7 +162,7 @@ int sdhci_pci_o2_probe_slot(struct sdhci_pci_slot *slot)
 	case PCI_DEVICE_ID_O2_FUJIN2:
 		reg = sdhci_readl(host, O2_SD_VENDOR_SETTING);
 		if (reg & 0x1)
-			host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+			host->quirks |= SDHCI_QUIRK_MULTIBLOCK_ACMD12;
 
 		if (chip->pdev->device != PCI_DEVICE_ID_O2_FUJIN2)
 			break;

drivers/mmc/host/sdhci-pltfm.c +1 -1

index 072bb27..206057a 100644
@@ -74,7 +74,7 @@ void sdhci_get_of_property(struct platform_device *pdev)
 	u32 bus_width;
 
 	if (of_get_property(np, "sdhci,auto-cmd12", NULL))
-		host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+		host->quirks |= SDHCI_QUIRK_MULTIBLOCK_ACMD12;
 
 	if (of_get_property(np, "sdhci,1-bit-only", NULL) ||
 	    (of_property_read_u32(np, "bus-width", &bus_width) == 0 &&

drivers/mmc/host/sdhci-s3c.c +1 -1

index 70c724b..756b9bb 100644
@@ -564,7 +564,7 @@ static int sdhci_s3c_probe(struct platform_device *pdev)
 	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
 
 	/* This host supports the Auto CMD12 */
-	host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+	host->quirks |= SDHCI_QUIRK_MULTIBLOCK_ACMD12;
 
 	/* Samsung SoCs need BROKEN_ADMA_ZEROLEN_DESC */
 	host->quirks |= SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC;

drivers/mmc/host/sdhci.c +1 -1

index 96ccb15..40e3551 100644
@@ -2988,7 +2988,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
 	mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
 
-	if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
+	if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_ACMD12)
 		host->flags |= SDHCI_AUTO_CMD12;
 
 	/* Auto-CMD23 stuff only works in ADMA or PIO. */

drivers/mmc/host/sdhci.h +1 -1

index 0f39f4f..afa4de8 100644
@@ -379,7 +379,7 @@ struct sdhci_host {
 /* Controller is missing device caps. Use caps provided by host */
 #define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 /* Controller uses Auto CMD12 command to stop the transfer */
-#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12		(1<<28)
+#define SDHCI_QUIRK_MULTIBLOCK_ACMD12			(1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<29)
 /* Controller treats ADMA descriptors with length 0000h incorrectly */

Quoted 2 lines · Dong Aisheng

Rename ACMD12 quirk to SDHCI_QUIRK_MULTIBLOCK_ACMD12 to avoid confusion since it's needed for both read and write.

I am not in favour of renaming unless the original name is really really bad. In this case it is a quirk which we anyway expect to remove at some point, so let's leave it alone for now.

Quoted 76 lines

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci-pci-o2micro.c | 2 +-
 drivers/mmc/host/sdhci-pltfm.c       | 2 +-
 drivers/mmc/host/sdhci-s3c.c         | 2 +-
 drivers/mmc/host/sdhci.c             | 2 +-
 drivers/mmc/host/sdhci.h             | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

drivers/mmc/host/sdhci-pci-o2micro.c +1 -1

index d48f031..1ed1f995 100644
@@ -162,7 +162,7 @@ int sdhci_pci_o2_probe_slot(struct sdhci_pci_slot *slot)
      case PCI_DEVICE_ID_O2_FUJIN2:
              reg = sdhci_readl(host, O2_SD_VENDOR_SETTING);
              if (reg & 0x1)
-                     host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+                     host->quirks |= SDHCI_QUIRK_MULTIBLOCK_ACMD12;
 
              if (chip->pdev->device != PCI_DEVICE_ID_O2_FUJIN2)
                      break;

drivers/mmc/host/sdhci-pltfm.c +1 -1

index 072bb27..206057a 100644
@@ -74,7 +74,7 @@ void sdhci_get_of_property(struct platform_device *pdev)
      u32 bus_width;
 
      if (of_get_property(np, "sdhci,auto-cmd12", NULL))
-             host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+             host->quirks |= SDHCI_QUIRK_MULTIBLOCK_ACMD12;
 
      if (of_get_property(np, "sdhci,1-bit-only", NULL) ||
          (of_property_read_u32(np, "bus-width", &bus_width) == 0 &&

drivers/mmc/host/sdhci-s3c.c +1 -1

index 70c724b..756b9bb 100644
@@ -564,7 +564,7 @@ static int sdhci_s3c_probe(struct platform_device *pdev)
      host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
 
      /* This host supports the Auto CMD12 */
-     host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+     host->quirks |= SDHCI_QUIRK_MULTIBLOCK_ACMD12;
 
      /* Samsung SoCs need BROKEN_ADMA_ZEROLEN_DESC */
      host->quirks |= SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC;

drivers/mmc/host/sdhci.c +1 -1

index 96ccb15..40e3551 100644
@@ -2988,7 +2988,7 @@ int sdhci_add_host(struct sdhci_host *host)
      mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
      mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
 
-     if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
+     if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_ACMD12)
              host->flags |= SDHCI_AUTO_CMD12;
 
      /* Auto-CMD23 stuff only works in ADMA or PIO. */

drivers/mmc/host/sdhci.h +1 -1

index 0f39f4f..afa4de8 100644
@@ -379,7 +379,7 @@ struct sdhci_host {
 /* Controller is missing device caps. Use caps provided by host */
 #define SDHCI_QUIRK_MISSING_CAPS                     (1<<27)
 /* Controller uses Auto CMD12 command to stop the transfer */
-#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12           (1<<28)
+#define SDHCI_QUIRK_MULTIBLOCK_ACMD12                        (1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT                     (1<<29)
 /* Controller treats ADMA descriptors with length 0000h incorrectly */

Current code will report the wrong data interrupt got when no data operation in progress assumed by getting !host->data in sdhci_data_irq().

For a data command handling process, the driver will call sdhci_finish_data() and clear host->data in case any data error, then card finish_tasklet will do the rest controller reset work.

Before the tasklet got run, however, controllers may report the TC(Transfer Complete) interrupt (SDHCI_INT_DATA_END) a bit later than data CRC error and data end bit error interrupts for single block transfer or the last block of multiblock transfer.

Controller usually detects and generates data CRC/end bit error interrupts once one block on the bus is transferred completely. For single block transfer, since there's only one bock to transfer, the controller will report transfer complete interrupt as well, but until the data in controller FIFO has been successfully transferred to memory. The time gap of TC and CRC interrupt depends on the system busy state at that point and memory bus access speed.

So it is possible when TC interrupt generated, host->data is already equal to NULL due to cleared by former CRC/Data End Bit error which is reasonable.

Thus we DO NOT report the weird data interrupt event for this case.

Else we may easily see warning below during SD3.0 card manually tuning process (calling mmc_send_tuning() which is a single block transfer)
mmc0: Got data interrupt 0x00000002 even though no data operation was in progress

The detailed command log is as follows:

[ 1657.920983] mmc0: starting CMD19 arg 00000000 flags 00000035
[ 1657.921009] mmc0:     blksz 64 blocks 1 flags 00000200 tsac 150 ms nsac 0
[ 1657.921085] sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00200001
[ 1657.921112] sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000002
[ 1657.921131] mmc0: Got data interrupt 0x00000002 even though no data operation was in progress.
[ 1657.929761] sdhci: =========== REGISTER DUMP (mmc0)===========
[ 1657.929780] sdhci: Sys addr: 0x3d5d6380 | Version:  0x00000002
[ 1657.929796] sdhci: Blk size: 0x00000040 | Blk cnt:  0x00000001
[ 1657.929814] sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
[ 1657.929831] sdhci: Present:  0x01fd8008 | Host ctl: 0x00000023
[ 1657.929847] sdhci: Power:    0x00000002 | Blk gap:  0x00000080
[ 1657.929863] sdhci: Wake-up:  0x00000008 | Clock:    0x0000000f
[ 1657.929879] sdhci: Timeout:  0x0000000f | Int stat: 0x00000000
[ 1657.929896] sdhci: Int enab: 0x107f008b | Sig enab: 0x107f008b
[ 1657.929914] sdhci: AC12 err: 0x00000000 | Slot int: 0x00000003
[ 1657.929932] sdhci: Caps:     0x07eb0000 | Caps_1:   0x00002007
[ 1657.929949] sdhci: Cmd:      0x0000133a | Max curr: 0x00ffffff
[ 1657.929965] sdhci: Host ctl2: 0x000000c8
[ 1657.929981] sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x8f042208
[ 1657.929995] sdhci: ===========================================
[ 1657.930156] mmc0: req done (CMD19): 0: 00000900 00000000 00000000 00000000
[ 1657.930179] mmc0:     0 bytes transferred: -84

It shows we first have a data CRC error interrupt then a data transfer complete interrupt.
Then we got the !host->data case in sdhci_data_irq().

CC stable <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

---
 drivers/mmc/host/sdhci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/mmc/host/sdhci.c +0 -0

index 40e3551..2eb0e34 100644
@@ -2325,6 +2325,17 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 			}
 		}
 

drivers/mmc/host/sdhci.c +1 -0

+
 		pr_err("%s: Got data interrupt 0x%08x even though no data operation was in progress.\n",
 		       mmc_hostname(host->mmc), (unsigned)intmask);
 		sdhci_dumpregs(host);