[PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[Kevin Liu]

This patchset is based on Johan's below patch series submitted recently:
  mmc: core: Add mmc_power_cycle
  mmc: core: Add card_busy to host_ops
  mmc: core: Fixup signal voltage switch

This patchset mainly update sdhci.c to co-work with Johan's above patches.
The first two patches are some update for the core stack with Johan's patch.
The third patch add the card_busy function and change the voltage_switch function in sdhci driver to work with the new code.

Kevin Liu (3):
 mmc: core: enhance the code for signal voltage setting
 mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid
 mmc: sdhci: update signal voltage switch code

 drivers/mmc/core/core.c  |   71 +++++++++++-------
 drivers/mmc/core/sd.c    |   16 +---
 drivers/mmc/host/sdhci.c |  188 +++++++++++++++++++---------------------------
 3 files changed, 125 insertions(+), 150 deletions(-)

[PATCH 1/3] mmc: core: enhance the code for signal voltage setting

[Kevin Liu]

1. if host does NOT implement signal voltage setting function,
should return error.
2. add the check for data signal before voltage change according
to the spec. If host does NOT detect a low signal level, the host
should abort the voltage switch sequence. (phisical layer spec 4.2.4.2(4))
3. if voltage change failed then no need to restore the clock
before cycle power the card.
4. call mmc_power_cycle here since it's a part of voltage switch.
besides -EAGAIN, any other error returned should also cycle power the card.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/core/core.c |   71 ++++++++++++++++++++++++++++------------------
 1 files changed, 43 insertions(+), 28 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d1aa8ab..da93186 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1226,6 +1226,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
 
 	BUG_ON(!host);
 
+	if (!host->ops->start_signal_voltage_switch)
+		return -EPERM;
+
 	/*
 	 * Send CMD11 only if the request is to switch the card to
 	 * 1.8V signalling.
@@ -1245,47 +1248,59 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
 
 	host->ios.signal_voltage = signal_voltage;
 
-	if (host->ops->start_signal_voltage_switch) {
-		u32 clock;
+	u32 clock;
+
+	mmc_host_clk_hold(host);
+
+	if (cmd11) {
+		if (!host->ops->card_busy)
+			pr_warning("%s: cannot verify signal voltage switch\n",
+					mmc_hostname(host));
 
-		mmc_host_clk_hold(host);
 		/*
 		 * During a signal voltage level switch, the clock must be gated
 		 * for a certain period of time according to the SD spec
 		 */
-		if (cmd11) {
-			clock = host->ios.clock;
-			host->ios.clock = 0;
-			mmc_set_ios(host);
-		}
+		clock = host->ios.clock;
+		host->ios.clock = 0;
+		mmc_set_ios(host);
 
-		err = host->ops->start_signal_voltage_switch(host, &host->ios);
+		if (host->ops->card_busy && !host->ops->card_busy(host)) {
+			err = -EAGAIN;
+		} else {
 
-		if (err && cmd11) {
-			host->ios.clock = clock;
-			mmc_set_ios(host);
-		} else if (cmd11) {
-			/* Keep clock gated for at least 5 ms */
-			mmc_delay(5);
-			host->ios.clock = clock;
-			mmc_set_ios(host);
+			err = host->ops->start_signal_voltage_switch(host, &host->ios);
 
-			/* Wait for at least 1 ms according to spec */
-			mmc_delay(1);
+			if (!err) {
+				/* Keep clock gated for at least 5 ms */
+				mmc_delay(5);
+				host->ios.clock = clock;
+				mmc_set_ios(host);
 
-			/*
-			 * Failure to switch is indicated by the card holding
-			 * dat[0:3] low
-			 */
-			if (!host->ops->card_busy)
-				pr_warning("%s: cannot verify signal voltage switch\n",
+				/* Wait for at least 1 ms according to spec */
+				mmc_delay(1);
+
+				/*
+				 * Failure to switch is indicated by the card holding
+				 * dat[0:3] low
+				 */
+				if (host->ops->card_busy && host->ops->card_busy(host))
+					err = -EAGAIN;
+			}
+		}
+		if (err) {
+			/* Power cycle card */
+			pr_debug("%s: Signal voltage switch failed, "
+					"power cycling card \n",
 					mmc_hostname(host));
-			else if (host->ops->card_busy(host))
-				err = -EAGAIN;
+			mmc_power_cycle(host);
 		}
-		mmc_host_clk_release(host);
+	} else {
+		err = host->ops->start_signal_voltage_switch(host, &host->ios);
 	}
 
+	mmc_host_clk_release(host);
+
 	return err;
 }
 
-- 
1.7.0.4

[PATCH 2/3] mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid

[Kevin Liu]

Cycle power the card is a MUST step when voltage switch sequence failed.
So move this operation to function mmc_set_signal_voltage.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/core/sd.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index c1a700d..53ba4d9 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -757,7 +757,6 @@ try_again:
 	if (max_current > 150)
 		ocr |= SD_OCR_XPC;
 
-try_again:
 	err = mmc_send_app_op_cond(host, ocr, rocr);
 	if (err)
 		return err;
@@ -769,16 +768,11 @@ try_again:
 	if (!mmc_host_is_spi(host) && rocr &&
 	   ((*rocr & 0x41000000) == 0x41000000)) {
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180, true);
-		if (err == -EAGAIN) {
-			/* Power cycle card */
-			pr_debug("%s: Signal voltage switch failed, "
-					"power cycling card (retries = %d)\n",
-					mmc_hostname(host), retries);
-			mmc_power_cycle(host);
-			retries--;
-			goto try_again;
-		} else if (err) {
-			retries = 0;
+		if (err) {
+			if (err == -EAGAIN)
+				retries--;
+			else
+				retries = 0;
 			goto try_again;
 		}
 	}
-- 
1.7.0.4

[PATCH 3/3] mmc: sdhci: update signal voltage switch code

[Kevin Liu]

The protocal related code is moved to core stack. So update the host
driver accordingly.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |  188 +++++++++++++++++++---------------------------
 1 files changed, 77 insertions(+), 111 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6f0bfc0..d19b2aa 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1608,141 +1608,95 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host,
-						u16 ctrl)
+static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
+						struct mmc_ios *ios)
 {
+	u16 ctrl;
 	int ret;
 
-	/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
-	ctrl &= ~SDHCI_CTRL_VDD_180;
-	sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
-
-	if (host->vqmmc) {
-		ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
-		if (ret) {
-			pr_warning("%s: Switching to 3.3V signalling voltage "
-				   " failed\n", mmc_hostname(host->mmc));
-			return -EIO;
-		}
-	}
-	/* Wait for 5ms */
-	usleep_range(5000, 5500);
-
-	/* 3.3V regulator output should be stable within 5 ms */
-	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
-	if (!(ctrl & SDHCI_CTRL_VDD_180))
+	/*
+	 * Signal Voltage Switching is only applicable for Host Controllers
+	 * v3.00 and above.
+	 */
+	if (host->version < SDHCI_SPEC_300)
 		return 0;
 
-	pr_warning("%s: 3.3V regulator output did not became stable\n",
-		   mmc_hostname(host->mmc));
-
-	return -EIO;
-}
+	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 
-static int sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host,
-						u16 ctrl)
-{
-	u8 pwr;
-	u16 clk;
-	u32 present_state;
-	int ret;
+	switch (ios->signal_voltage) {
 
-	/* Stop SDCLK */
-	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-	clk &= ~SDHCI_CLOCK_CARD_EN;
-	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+	case MMC_SIGNAL_VOLTAGE_330:
+		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
+		ctrl &= ~SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
-	/* Check whether DAT[3:0] is 0000 */
-	present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
-	if (!((present_state & SDHCI_DATA_LVL_MASK) >>
-	       SDHCI_DATA_LVL_SHIFT)) {
-		/*
-		 * Enable 1.8V Signal Enable in the Host Control2
-		 * register
-		 */
-		if (host->vqmmc)
-			ret = regulator_set_voltage(host->vqmmc,
-				1700000, 1950000);
-		else
-			ret = 0;
+		if (host->vqmmc) {
+			ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
+			if (ret) {
+				pr_warning("%s: Switching to 3.3V signalling voltage "
+						" failed\n", mmc_hostname(host->mmc));
+				return -EIO;
+			}
+		}
+		/* Wait for 5ms */
+		usleep_range(5000, 5500);
 
-		if (!ret) {
-			ctrl |= SDHCI_CTRL_VDD_180;
-			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+		/* 3.3V regulator output should be stable within 5 ms */
+		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (!(ctrl & SDHCI_CTRL_VDD_180))
+			return 0;
 
-			/* Wait for 5ms */
-			usleep_range(5000, 5500);
+		pr_warning("%s: 3.3V regulator output did not became stable\n",
+				mmc_hostname(host->mmc));
 
-			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
-			if (ctrl & SDHCI_CTRL_VDD_180) {
-				/* Provide SDCLK again and wait for 1ms */
-				clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-				clk |= SDHCI_CLOCK_CARD_EN;
-				sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-				usleep_range(1000, 1500);
+		return -EAGAIN;
 
-				/*
-				 * If DAT[3:0] level is 1111b, then the card
-				 * was successfully switched to 1.8V signaling.
-				 */
-				present_state = sdhci_readl(host,
-							SDHCI_PRESENT_STATE);
-				if ((present_state & SDHCI_DATA_LVL_MASK) ==
-				     SDHCI_DATA_LVL_MASK)
-					return 0;
+	case MMC_SIGNAL_VOLTAGE_180:
+		if (host->vqmmc) {
+			ret = regulator_set_voltage(host->vqmmc,
+					1700000, 1950000);
+			if (ret) {
+				pr_warning("%s: Switching to 1.8V signalling voltage "
+						" failed\n", mmc_hostname(host->mmc));
+				return -EIO;
 			}
 		}
-	}
 
-	/*
-	 * If we are here, that means the switch to 1.8V signaling
-	 * failed. We power cycle the card, and retry initialization
-	 * sequence by setting S18R to 0.
-	 */
-	pwr = sdhci_readb(host, SDHCI_POWER_CONTROL);
-	pwr &= ~SDHCI_POWER_ON;
-	sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
-	if (host->vmmc)
-		regulator_disable(host->vmmc);
+		/*
+		 * Enable 1.8V Signal Enable in the Host Control2
+		 * register
+		 */
+		ctrl |= SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
-	/* Wait for 1ms as per the spec */
-	usleep_range(1000, 1500);
-	pwr |= SDHCI_POWER_ON;
-	sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
-	if (host->vmmc)
-		regulator_enable(host->vmmc);
+		/* Wait for 5ms */
+		usleep_range(5000, 5500);
 
-	pr_warning("%s: Switching to 1.8V signalling voltage failed, "
-		   "retrying with S18R set to 0\n", mmc_hostname(host->mmc));
+		/* 1.8V regulator output should be stable within 5 ms */
+		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (ctrl & SDHCI_CTRL_VDD_180)
+			return 0;
 
-	return -EAGAIN;
-}
+		pr_warning("%s: 1.8V regulator output did not became stable\n",
+				mmc_hostname(host->mmc));
 
-static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
-						struct mmc_ios *ios)
-{
-	u16 ctrl;
+		return -EAGAIN;
 
-	/*
-	 * Signal Voltage Switching is only applicable for Host Controllers
-	 * v3.00 and above.
-	 */
-	if (host->version < SDHCI_SPEC_300)
+	case MMC_SIGNAL_VOLTAGE_120:
+		if (host->vqmmc) {
+			ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
+			if (ret) {
+				pr_warning("%s: Switching to 1.2V signalling voltage "
+						" failed\n", mmc_hostname(host->mmc));
+				return -EIO;
+			}
+		}
 		return 0;
 
-	/*
-	 * We first check whether the request is to set signalling voltage
-	 * to 3.3V. If so, we change the voltage to 3.3V and return quickly.
-	 */
-	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
-	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
-		return sdhci_do_3_3v_signal_voltage_switch(host, ctrl);
-	else if (!(ctrl & SDHCI_CTRL_VDD_180) &&
-			(ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180))
-		return sdhci_do_1_8v_signal_voltage_switch(host, ctrl);
-	else
+	default:
 		/* No signal voltage switch required */
 		return 0;
+	}
 }
 
 static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
@@ -1759,6 +1713,17 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 	return err;
 }
 
+static int sdhci_card_busy(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	u32 present_state;
+
+	/* Check whether DAT[3:0] is 0000 */
+	present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
+
+	return !(present_state & SDHCI_DATA_LVL_MASK);
+}
+
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct sdhci_host *host;
@@ -2029,6 +1994,7 @@ static const struct mmc_host_ops sdhci_ops = {
 	.execute_tuning			= sdhci_execute_tuning,
 	.enable_preset_value		= sdhci_enable_preset_value,
 	.card_event			= sdhci_card_event,
+	.card_busy	= sdhci_card_busy,
 };
 
 /*****************************************************************************\
-- 
1.7.0.4

Re: [PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[Ulf Hansson]

Hi Kevin,

Thanks for looking into this for SDHCI.

On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>
> This patchset is based on Johan's below patch series submitted recently:
>   mmc: core: Add mmc_power_cycle
>   mmc: core: Add card_busy to host_ops
>   mmc: core: Fixup signal voltage switch
>
> This patchset mainly update sdhci.c to co-work with Johan's above patches.
> The first two patches are some update for the core stack with Johan's patch.
> The third patch add the card_busy function and change the voltage_switch function in sdhci driver to work with the new code.
>
> Kevin Liu (3):
>  mmc: core: enhance the code for signal voltage setting
>  mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid

Regarding you patches on the core layer. I think it would make sense
to instead of patching on top of Johan patchset, if needed, it is
better that you put review comment directly on Johans patches. Would
that be possible for you?

>  mmc: sdhci: update signal voltage switch code
>
>  drivers/mmc/core/core.c  |   71 +++++++++++-------
>  drivers/mmc/core/sd.c    |   16 +---
>  drivers/mmc/host/sdhci.c |  188 +++++++++++++++++++---------------------------
>  3 files changed, 125 insertions(+), 150 deletions(-)

Kind regards
Ulf Hansson

Re: [PATCH 1/3] mmc: core: enhance the code for signal voltage setting

[Ulf Hansson]

On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
> 1. if host does NOT implement signal voltage setting function,
> should return error.
> 2. add the check for data signal before voltage change according
> to the spec. If host does NOT detect a low signal level, the host
> should abort the voltage switch sequence. (phisical layer spec 4.2.4.2(4))
> 3. if voltage change failed then no need to restore the clock
> before cycle power the card.
> 4. call mmc_power_cycle here since it's a part of voltage switch.
> besides -EAGAIN, any other error returned should also cycle power the card.
>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/core/core.c |   71 ++++++++++++++++++++++++++++------------------
>  1 files changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index d1aa8ab..da93186 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1226,6 +1226,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>
>         BUG_ON(!host);
>
> +       if (!host->ops->start_signal_voltage_switch)
> +               return -EPERM;
> +

No, this is not OK.
For example, eMMC might default use 1.8V as I/O voltage, you don't
want to force host drivers implementing this function to make this
work.

>         /*
>          * Send CMD11 only if the request is to switch the card to
>          * 1.8V signalling.
> @@ -1245,47 +1248,59 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>
>         host->ios.signal_voltage = signal_voltage;
>
> -       if (host->ops->start_signal_voltage_switch) {
> -               u32 clock;
> +       u32 clock;
> +
> +       mmc_host_clk_hold(host);
> +
> +       if (cmd11) {
> +               if (!host->ops->card_busy)
> +                       pr_warning("%s: cannot verify signal voltage switch\n",
> +                                       mmc_hostname(host));
>
> -               mmc_host_clk_hold(host);
>                 /*
>                  * During a signal voltage level switch, the clock must be gated
>                  * for a certain period of time according to the SD spec
>                  */
> -               if (cmd11) {
> -                       clock = host->ios.clock;
> -                       host->ios.clock = 0;
> -                       mmc_set_ios(host);
> -               }
> +               clock = host->ios.clock;
> +               host->ios.clock = 0;
> +               mmc_set_ios(host);
>
> -               err = host->ops->start_signal_voltage_switch(host, &host->ios);
> +               if (host->ops->card_busy && !host->ops->card_busy(host)) {
> +                       err = -EAGAIN;
> +               } else {

Great that you spotted this! We should check busy before we do the
actual voltage switch.

Although, I think it would make sense to do this before the clock is
set to 0. Especially important for those host controllers that do
support busy-detection, a qualified guess would be that those also
could require the clock to be on to detect this. Or what do you think?
>From spec point of view it shall be safe to do it before setting clk
to 0.

>
> -               if (err && cmd11) {
> -                       host->ios.clock = clock;
> -                       mmc_set_ios(host);
> -               } else if (cmd11) {
> -                       /* Keep clock gated for at least 5 ms */
> -                       mmc_delay(5);
> -                       host->ios.clock = clock;
> -                       mmc_set_ios(host);
> +                       err = host->ops->start_signal_voltage_switch(host, &host->ios);
>
> -                       /* Wait for at least 1 ms according to spec */
> -                       mmc_delay(1);
> +                       if (!err) {
> +                               /* Keep clock gated for at least 5 ms */
> +                               mmc_delay(5);
> +                               host->ios.clock = clock;
> +                               mmc_set_ios(host);
>
> -                       /*
> -                        * Failure to switch is indicated by the card holding
> -                        * dat[0:3] low
> -                        */
> -                       if (!host->ops->card_busy)
> -                               pr_warning("%s: cannot verify signal voltage switch\n",
> +                               /* Wait for at least 1 ms according to spec */
> +                               mmc_delay(1);
> +
> +                               /*
> +                                * Failure to switch is indicated by the card holding
> +                                * dat[0:3] low
> +                                */
> +                               if (host->ops->card_busy && host->ops->card_busy(host))
> +                                       err = -EAGAIN;
> +                       }
> +               }
> +               if (err) {
> +                       /* Power cycle card */
> +                       pr_debug("%s: Signal voltage switch failed, "
> +                                       "power cycling card \n",
>                                         mmc_hostname(host));
> -                       else if (host->ops->card_busy(host))
> -                               err = -EAGAIN;
> +                       mmc_power_cycle(host);
>                 }
> -               mmc_host_clk_release(host);
> +       } else {
> +               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>         }
>
> +       mmc_host_clk_release(host);
> +
>         return err;
>  }
>
> --
> 1.7.0.4
>

Kind regards
Ulf Hansson

Re: [PATCH 2/3] mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid

[Ulf Hansson]

On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
> Cycle power the card is a MUST step when voltage switch sequence failed.
> So move this operation to function mmc_set_signal_voltage.
>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/core/sd.c |   16 +++++-----------
>  1 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index c1a700d..53ba4d9 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -757,7 +757,6 @@ try_again:
>         if (max_current > 150)
>                 ocr |= SD_OCR_XPC;
>
> -try_again:
>         err = mmc_send_app_op_cond(host, ocr, rocr);
>         if (err)
>                 return err;
> @@ -769,16 +768,11 @@ try_again:
>         if (!mmc_host_is_spi(host) && rocr &&
>            ((*rocr & 0x41000000) == 0x41000000)) {
>                 err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180, true);
> -               if (err == -EAGAIN) {
> -                       /* Power cycle card */
> -                       pr_debug("%s: Signal voltage switch failed, "
> -                                       "power cycling card (retries = %d)\n",
> -                                       mmc_hostname(host), retries);
> -                       mmc_power_cycle(host);
> -                       retries--;
> -                       goto try_again;
> -               } else if (err) {
> -                       retries = 0;
> +               if (err) {
> +                       if (err == -EAGAIN)
> +                               retries--;
> +                       else
> +                               retries = 0;
>                         goto try_again;
>                 }
>         }
> --
> 1.7.0.4
>

This looks good to me. I suggest Johan includes this in his patch.
Maybe add Kevins signed off?

Kind regards
Ulf Hansson

Re: [PATCH 1/3] mmc: core: enhance the code for signal voltage setting

[Ulf Hansson]

On 14 December 2012 14:40, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>> 1. if host does NOT implement signal voltage setting function,
>> should return error.
>> 2. add the check for data signal before voltage change according
>> to the spec. If host does NOT detect a low signal level, the host
>> should abort the voltage switch sequence. (phisical layer spec 4.2.4.2(4))
>> 3. if voltage change failed then no need to restore the clock
>> before cycle power the card.
>> 4. call mmc_power_cycle here since it's a part of voltage switch.
>> besides -EAGAIN, any other error returned should also cycle power the card.
>>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/core/core.c |   71 ++++++++++++++++++++++++++++------------------
>>  1 files changed, 43 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index d1aa8ab..da93186 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1226,6 +1226,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>>
>>         BUG_ON(!host);
>>
>> +       if (!host->ops->start_signal_voltage_switch)
>> +               return -EPERM;
>> +
>
> No, this is not OK.
> For example, eMMC might default use 1.8V as I/O voltage, you don't
> want to force host drivers implementing this function to make this
> work.
>
>>         /*
>>          * Send CMD11 only if the request is to switch the card to
>>          * 1.8V signalling.
>> @@ -1245,47 +1248,59 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>>
>>         host->ios.signal_voltage = signal_voltage;
>>
>> -       if (host->ops->start_signal_voltage_switch) {
>> -               u32 clock;
>> +       u32 clock;
>> +
>> +       mmc_host_clk_hold(host);
>> +
>> +       if (cmd11) {
>> +               if (!host->ops->card_busy)
>> +                       pr_warning("%s: cannot verify signal voltage switch\n",
>> +                                       mmc_hostname(host));
>>
>> -               mmc_host_clk_hold(host);
>>                 /*
>>                  * During a signal voltage level switch, the clock must be gated
>>                  * for a certain period of time according to the SD spec
>>                  */
>> -               if (cmd11) {
>> -                       clock = host->ios.clock;
>> -                       host->ios.clock = 0;
>> -                       mmc_set_ios(host);
>> -               }
>> +               clock = host->ios.clock;
>> +               host->ios.clock = 0;
>> +               mmc_set_ios(host);
>>
>> -               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>> +               if (host->ops->card_busy && !host->ops->card_busy(host)) {
>> +                       err = -EAGAIN;
>> +               } else {
>
> Great that you spotted this! We should check busy before we do the
> actual voltage switch.
>
> Although, I think it would make sense to do this before the clock is
> set to 0. Especially important for those host controllers that do
> support busy-detection, a qualified guess would be that those also
> could require the clock to be on to detect this. Or what do you think?
> From spec point of view it shall be safe to do it before setting clk
> to 0.
>
>>
>> -               if (err && cmd11) {
>> -                       host->ios.clock = clock;
>> -                       mmc_set_ios(host);
>> -               } else if (cmd11) {
>> -                       /* Keep clock gated for at least 5 ms */
>> -                       mmc_delay(5);
>> -                       host->ios.clock = clock;
>> -                       mmc_set_ios(host);
>> +                       err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>
>> -                       /* Wait for at least 1 ms according to spec */
>> -                       mmc_delay(1);
>> +                       if (!err) {
>> +                               /* Keep clock gated for at least 5 ms */
>> +                               mmc_delay(5);
>> +                               host->ios.clock = clock;
>> +                               mmc_set_ios(host);
>>
>> -                       /*
>> -                        * Failure to switch is indicated by the card holding
>> -                        * dat[0:3] low
>> -                        */
>> -                       if (!host->ops->card_busy)
>> -                               pr_warning("%s: cannot verify signal voltage switch\n",
>> +                               /* Wait for at least 1 ms according to spec */
>> +                               mmc_delay(1);
>> +
>> +                               /*
>> +                                * Failure to switch is indicated by the card holding
>> +                                * dat[0:3] low
>> +                                */
>> +                               if (host->ops->card_busy && host->ops->card_busy(host))
>> +                                       err = -EAGAIN;
>> +                       }
>> +               }
>> +               if (err) {
>> +                       /* Power cycle card */
>> +                       pr_debug("%s: Signal voltage switch failed, "
>> +                                       "power cycling card \n",
>>                                         mmc_hostname(host));
>> -                       else if (host->ops->card_busy(host))
>> -                               err = -EAGAIN;
>> +                       mmc_power_cycle(host);

I definitely agree, that this should be done here!

Although, this will break the SDIO case, so we need to fix that in the
sdio.c as well.

>>                 }
>> -               mmc_host_clk_release(host);
>> +       } else {
>> +               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>         }
>>
>> +       mmc_host_clk_release(host);
>> +
>>         return err;
>>  }
>>
>> --
>> 1.7.0.4
>>
>
> Kind regards
> Ulf Hansson

Kind regards
Ulf Hansson

Re: [PATCH 3/3] mmc: sdhci: update signal voltage switch code

[Johan Rudholm]

Hi Kevin,

2012/12/14 Kevin Liu <kliu5@marvell.com>:
> The protocal related code is moved to core stack. So update the host
> driver accordingly.
>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |  188 +++++++++++++++++++---------------------------
>  1 files changed, 77 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6f0bfc0..d19b2aa 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c

...

> +static int sdhci_card_busy(struct mmc_host *mmc)
> +{
> +       struct sdhci_host *host = mmc_priv(mmc);
> +       u32 present_state;
> +
> +       /* Check whether DAT[3:0] is 0000 */
> +       present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
> +
> +       return !(present_state & SDHCI_DATA_LVL_MASK);
> +}
> +

I'm not that familiar with SDHCI, but don't you need some calls to
sdhci_runtime_pm_get / put here?

Kind regards, Johan

Re: [PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[Kevin Liu]

2012/12/14 Ulf Hansson <ulf.hansson@linaro.org>:
> Hi Kevin,
>
> Thanks for looking into this for SDHCI.
>
> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>>
>> This patchset is based on Johan's below patch series submitted recently:
>>   mmc: core: Add mmc_power_cycle
>>   mmc: core: Add card_busy to host_ops
>>   mmc: core: Fixup signal voltage switch
>>
>> This patchset mainly update sdhci.c to co-work with Johan's above patches.
>> The first two patches are some update for the core stack with Johan's patch.
>> The third patch add the card_busy function and change the voltage_switch function in sdhci driver to work with the new code.
>>
>> Kevin Liu (3):
>>  mmc: core: enhance the code for signal voltage setting
>>  mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid
>
> Regarding you patches on the core layer. I think it would make sense
> to instead of patching on top of Johan patchset, if needed, it is
> better that you put review comment directly on Johans patches. Would
> that be possible for you?
>

Sure, it's better to integrate related change into one patch.
I can update my patch, then Johan can just integrate my patch if he agree.
Or Johan change his patch directly if needed.

Thanks
Kevin

Re: [PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[Johan Rudholm]

Hi Kevin and Ulf,

2012/12/14 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/14 Ulf Hansson <ulf.hansson@linaro.org>:
>> Hi Kevin,
>>
>> Thanks for looking into this for SDHCI.
>>
>> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>>>
>>> This patchset is based on Johan's below patch series submitted recently:
>>>   mmc: core: Add mmc_power_cycle
>>>   mmc: core: Add card_busy to host_ops
>>>   mmc: core: Fixup signal voltage switch
>>>
>>> This patchset mainly update sdhci.c to co-work with Johan's above patches.
>>> The first two patches are some update for the core stack with Johan's patch.
>>> The third patch add the card_busy function and change the voltage_switch function in sdhci driver to work with the new code.
>>>
>>> Kevin Liu (3):
>>>  mmc: core: enhance the code for signal voltage setting
>>>  mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid
>>
>> Regarding you patches on the core layer. I think it would make sense
>> to instead of patching on top of Johan patchset, if needed, it is
>> better that you put review comment directly on Johans patches. Would
>> that be possible for you?
>>
>
> Sure, it's better to integrate related change into one patch.
> I can update my patch, then Johan can just integrate my patch if he agree.
> Or Johan change his patch directly if needed.

I'll try to have an updated patchset by Monday next week which
integrates your changes and adds your Signed-off-by if that's OK with
you. Thank you for testing this stuff!

Kind regards, Johan

Re: [PATCH 1/3] mmc: core: enhance the code for signal voltage setting

[Kevin Liu]

2012/12/14 Ulf Hansson <ulf.hansson@linaro.org>:
> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>> 1. if host does NOT implement signal voltage setting function,
>> should return error.
>> 2. add the check for data signal before voltage change according
>> to the spec. If host does NOT detect a low signal level, the host
>> should abort the voltage switch sequence. (phisical layer spec 4.2.4.2(4))
>> 3. if voltage change failed then no need to restore the clock
>> before cycle power the card.
>> 4. call mmc_power_cycle here since it's a part of voltage switch.
>> besides -EAGAIN, any other error returned should also cycle power the card.
>>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/core/core.c |   71 ++++++++++++++++++++++++++++------------------
>>  1 files changed, 43 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index d1aa8ab..da93186 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1226,6 +1226,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>>
>>         BUG_ON(!host);
>>
>> +       if (!host->ops->start_signal_voltage_switch)
>> +               return -EPERM;
>> +
>
> No, this is not OK.
> For example, eMMC might default use 1.8V as I/O voltage, you don't
> want to force host drivers implementing this function to make this
> work.
>
I agree.
But then how can we make sure emmc's voltage?
Only depend on the voltage by default?
I will update the patch and move this judgement withine cmd11.

>>         /*
>>          * Send CMD11 only if the request is to switch the card to
>>          * 1.8V signalling.
>> @@ -1245,47 +1248,59 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>>
>>         host->ios.signal_voltage = signal_voltage;
>>
>> -       if (host->ops->start_signal_voltage_switch) {
>> -               u32 clock;
>> +       u32 clock;
>> +
>> +       mmc_host_clk_hold(host);
>> +
>> +       if (cmd11) {
>> +               if (!host->ops->card_busy)
>> +                       pr_warning("%s: cannot verify signal voltage switch\n",
>> +                                       mmc_hostname(host));
>>
>> -               mmc_host_clk_hold(host);
>>                 /*
>>                  * During a signal voltage level switch, the clock must be gated
>>                  * for a certain period of time according to the SD spec
>>                  */
>> -               if (cmd11) {
>> -                       clock = host->ios.clock;
>> -                       host->ios.clock = 0;
>> -                       mmc_set_ios(host);
>> -               }
>> +               clock = host->ios.clock;
>> +               host->ios.clock = 0;
>> +               mmc_set_ios(host);
>>
>> -               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>> +               if (host->ops->card_busy && !host->ops->card_busy(host)) {
>> +                       err = -EAGAIN;
>> +               } else {
>
> Great that you spotted this! We should check busy before we do the
> actual voltage switch.
>
> Although, I think it would make sense to do this before the clock is
> set to 0. Especially important for those host controllers that do
> support busy-detection, a qualified guess would be that those also
> could require the clock to be on to detect this. Or what do you think?
> From spec point of view it shall be safe to do it before setting clk
> to 0.
>
I considered this when I draft this patch.
My only concern is whether DAT level is stable at that point.
According to the spec, the card driver DAT[3:0] to low at the NEXT
clock of the end bit for R1 response after completing the R1 response.
I'm not sure whether the status is stable if we query the DAT level
just after cmd11.

I agree with you that it will be safer to keep clock on when detecting.

>>
>> -               if (err && cmd11) {
>> -                       host->ios.clock = clock;
>> -                       mmc_set_ios(host);
>> -               } else if (cmd11) {
>> -                       /* Keep clock gated for at least 5 ms */
>> -                       mmc_delay(5);
>> -                       host->ios.clock = clock;
>> -                       mmc_set_ios(host);
>> +                       err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>
>> -                       /* Wait for at least 1 ms according to spec */
>> -                       mmc_delay(1);
>> +                       if (!err) {
>> +                               /* Keep clock gated for at least 5 ms */
>> +                               mmc_delay(5);
>> +                               host->ios.clock = clock;
>> +                               mmc_set_ios(host);
>>
>> -                       /*
>> -                        * Failure to switch is indicated by the card holding
>> -                        * dat[0:3] low
>> -                        */
>> -                       if (!host->ops->card_busy)
>> -                               pr_warning("%s: cannot verify signal voltage switch\n",
>> +                               /* Wait for at least 1 ms according to spec */
>> +                               mmc_delay(1);
>> +
>> +                               /*
>> +                                * Failure to switch is indicated by the card holding
>> +                                * dat[0:3] low
>> +                                */
>> +                               if (host->ops->card_busy && host->ops->card_busy(host))
>> +                                       err = -EAGAIN;
>> +                       }
>> +               }
>> +               if (err) {
>> +                       /* Power cycle card */
>> +                       pr_debug("%s: Signal voltage switch failed, "
>> +                                       "power cycling card \n",
>>                                         mmc_hostname(host));
>> -                       else if (host->ops->card_busy(host))
>> -                               err = -EAGAIN;
>> +                       mmc_power_cycle(host);
>>                 }
>> -               mmc_host_clk_release(host);
>> +       } else {
>> +               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>         }
>>
>> +       mmc_host_clk_release(host);
>> +
>>         return err;
>>  }
>>
>> --
>> 1.7.0.4
>>
>
> Kind regards
> Ulf Hansson

Re: [PATCH 3/3] mmc: sdhci: update signal voltage switch code

[Kevin Liu]

2012/12/14 Johan Rudholm <jrudholm@gmail.com>:
> Hi Kevin,
>
> 2012/12/14 Kevin Liu <kliu5@marvell.com>:
>> The protocal related code is moved to core stack. So update the host
>> driver accordingly.
>>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/host/sdhci.c |  188 +++++++++++++++++++---------------------------
>>  1 files changed, 77 insertions(+), 111 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 6f0bfc0..d19b2aa 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>
> ...
>
>> +static int sdhci_card_busy(struct mmc_host *mmc)
>> +{
>> +       struct sdhci_host *host = mmc_priv(mmc);
>> +       u32 present_state;
>> +
>> +       /* Check whether DAT[3:0] is 0000 */
>> +       present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
>> +
>> +       return !(present_state & SDHCI_DATA_LVL_MASK);
>> +}
>> +
>
> I'm not that familiar with SDHCI, but don't you need some calls to
> sdhci_runtime_pm_get / put here?
>

Oh, yes. I will add this.

Thanks
Kevin

Re: [PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[Kevin Liu]

2012/12/14 Johan Rudholm <jrudholm@gmail.com>:
> Hi Kevin and Ulf,
>
> 2012/12/14 Kevin Liu <keyuan.liu@gmail.com>:
>> 2012/12/14 Ulf Hansson <ulf.hansson@linaro.org>:
>>> Hi Kevin,
>>>
>>> Thanks for looking into this for SDHCI.
>>>
>>> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>>>>
>>>> This patchset is based on Johan's below patch series submitted recently:
>>>>   mmc: core: Add mmc_power_cycle
>>>>   mmc: core: Add card_busy to host_ops
>>>>   mmc: core: Fixup signal voltage switch
>>>>
>>>> This patchset mainly update sdhci.c to co-work with Johan's above patches.
>>>> The first two patches are some update for the core stack with Johan's patch.
>>>> The third patch add the card_busy function and change the voltage_switch function in sdhci driver to work with the new code.
>>>>
>>>> Kevin Liu (3):
>>>>  mmc: core: enhance the code for signal voltage setting
>>>>  mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid
>>>
>>> Regarding you patches on the core layer. I think it would make sense
>>> to instead of patching on top of Johan patchset, if needed, it is
>>> better that you put review comment directly on Johans patches. Would
>>> that be possible for you?
>>>
>>
>> Sure, it's better to integrate related change into one patch.
>> I can update my patch, then Johan can just integrate my patch if he agree.
>> Or Johan change his patch directly if needed.
>
> I'll try to have an updated patchset by Monday next week which
> integrates your changes and adds your Signed-off-by if that's OK with
> you. Thank you for testing this stuff!
>

I'm ok.

Thanks
Kevin

Re: [PATCH 1/3] mmc: core: enhance the code for signal voltage setting

[Kevin Liu]

2012/12/14 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/14 Ulf Hansson <ulf.hansson@linaro.org>:
>> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>>> 1. if host does NOT implement signal voltage setting function,
>>> should return error.
>>> 2. add the check for data signal before voltage change according
>>> to the spec. If host does NOT detect a low signal level, the host
>>> should abort the voltage switch sequence. (phisical layer spec 4.2.4.2(4))
>>> 3. if voltage change failed then no need to restore the clock
>>> before cycle power the card.
>>> 4. call mmc_power_cycle here since it's a part of voltage switch.
>>> besides -EAGAIN, any other error returned should also cycle power the card.
>>>
>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>> ---
>>>  drivers/mmc/core/core.c |   71 ++++++++++++++++++++++++++++------------------
>>>  1 files changed, 43 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index d1aa8ab..da93186 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1226,6 +1226,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>>>
>>>         BUG_ON(!host);
>>>
>>> +       if (!host->ops->start_signal_voltage_switch)
>>> +               return -EPERM;
>>> +
>>
>> No, this is not OK.
>> For example, eMMC might default use 1.8V as I/O voltage, you don't
>> want to force host drivers implementing this function to make this
>> work.
>>
> I agree.
> But then how can we make sure emmc's voltage?
> Only depend on the voltage by default?
> I will update the patch and move this judgement withine cmd11.
>
>>>         /*
>>>          * Send CMD11 only if the request is to switch the card to
>>>          * 1.8V signalling.
>>> @@ -1245,47 +1248,59 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
>>>
>>>         host->ios.signal_voltage = signal_voltage;
>>>
>>> -       if (host->ops->start_signal_voltage_switch) {
>>> -               u32 clock;
>>> +       u32 clock;
>>> +
>>> +       mmc_host_clk_hold(host);
>>> +
>>> +       if (cmd11) {
>>> +               if (!host->ops->card_busy)
>>> +                       pr_warning("%s: cannot verify signal voltage switch\n",
>>> +                                       mmc_hostname(host));
>>>
>>> -               mmc_host_clk_hold(host);
>>>                 /*
>>>                  * During a signal voltage level switch, the clock must be gated
>>>                  * for a certain period of time according to the SD spec
>>>                  */
>>> -               if (cmd11) {
>>> -                       clock = host->ios.clock;
>>> -                       host->ios.clock = 0;
>>> -                       mmc_set_ios(host);
>>> -               }
>>> +               clock = host->ios.clock;
>>> +               host->ios.clock = 0;
>>> +               mmc_set_ios(host);
>>>
>>> -               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>> +               if (host->ops->card_busy && !host->ops->card_busy(host)) {
>>> +                       err = -EAGAIN;
>>> +               } else {
>>
>> Great that you spotted this! We should check busy before we do the
>> actual voltage switch.
>>
>> Although, I think it would make sense to do this before the clock is
>> set to 0. Especially important for those host controllers that do
>> support busy-detection, a qualified guess would be that those also
>> could require the clock to be on to detect this. Or what do you think?
>> From spec point of view it shall be safe to do it before setting clk
>> to 0.
>>
> I considered this when I draft this patch.
> My only concern is whether DAT level is stable at that point.
> According to the spec, the card driver DAT[3:0] to low at the NEXT
> clock of the end bit for R1 response after completing the R1 response.
> I'm not sure whether the status is stable if we query the DAT level
> just after cmd11.
>

How about adding a 1ms delay between cmd11 response and calling card_busy?
And we can do this before the clock is set to 0.
As spec said the timing

> I agree with you that it will be safer to keep clock on when detecting.
>
>>>
>>> -               if (err && cmd11) {
>>> -                       host->ios.clock = clock;
>>> -                       mmc_set_ios(host);
>>> -               } else if (cmd11) {
>>> -                       /* Keep clock gated for at least 5 ms */
>>> -                       mmc_delay(5);
>>> -                       host->ios.clock = clock;
>>> -                       mmc_set_ios(host);
>>> +                       err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>>
>>> -                       /* Wait for at least 1 ms according to spec */
>>> -                       mmc_delay(1);
>>> +                       if (!err) {
>>> +                               /* Keep clock gated for at least 5 ms */
>>> +                               mmc_delay(5);
>>> +                               host->ios.clock = clock;
>>> +                               mmc_set_ios(host);
>>>
>>> -                       /*
>>> -                        * Failure to switch is indicated by the card holding
>>> -                        * dat[0:3] low
>>> -                        */
>>> -                       if (!host->ops->card_busy)
>>> -                               pr_warning("%s: cannot verify signal voltage switch\n",
>>> +                               /* Wait for at least 1 ms according to spec */
>>> +                               mmc_delay(1);
>>> +
>>> +                               /*
>>> +                                * Failure to switch is indicated by the card holding
>>> +                                * dat[0:3] low
>>> +                                */
>>> +                               if (host->ops->card_busy && host->ops->card_busy(host))
>>> +                                       err = -EAGAIN;
>>> +                       }
>>> +               }
>>> +               if (err) {
>>> +                       /* Power cycle card */
>>> +                       pr_debug("%s: Signal voltage switch failed, "
>>> +                                       "power cycling card \n",
>>>                                         mmc_hostname(host));
>>> -                       else if (host->ops->card_busy(host))
>>> -                               err = -EAGAIN;
>>> +                       mmc_power_cycle(host);
>>>                 }
>>> -               mmc_host_clk_release(host);
>>> +       } else {
>>> +               err = host->ops->start_signal_voltage_switch(host, &host->ios);
>>>         }
>>>
>>> +       mmc_host_clk_release(host);
>>> +
>>>         return err;
>>>  }
>>>
>>> --
>>> 1.7.0.4
>>>
>>
>> Kind regards
>> Ulf Hansson

Re: [PATCH 0/3] mmc: Signal voltage switch procedure for UHS mode (sdhci)

[Kevin Liu]

2012/12/14 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/14 Johan Rudholm <jrudholm@gmail.com>:
>> Hi Kevin and Ulf,
>>
>> 2012/12/14 Kevin Liu <keyuan.liu@gmail.com>:
>>> 2012/12/14 Ulf Hansson <ulf.hansson@linaro.org>:
>>>> Hi Kevin,
>>>>
>>>> Thanks for looking into this for SDHCI.
>>>>
>>>> On 14 December 2012 11:53, Kevin Liu <kliu5@marvell.com> wrote:
>>>>>
>>>>> This patchset is based on Johan's below patch series submitted recently:
>>>>>   mmc: core: Add mmc_power_cycle
>>>>>   mmc: core: Add card_busy to host_ops
>>>>>   mmc: core: Fixup signal voltage switch
>>>>>
>>>>> This patchset mainly update sdhci.c to co-work with Johan's above patches.
>>>>> The first two patches are some update for the core stack with Johan's patch.
>>>>> The third patch add the card_busy function and change the voltage_switch function in sdhci driver to work with the new code.
>>>>>
>>>>> Kevin Liu (3):
>>>>>  mmc: core: enhance the code for signal voltage setting
>>>>>  mmc: core: cycle power the card in voltage switch rather than mmc_sd_get_cid
>>>>
>>>> Regarding you patches on the core layer. I think it would make sense
>>>> to instead of patching on top of Johan patchset, if needed, it is
>>>> better that you put review comment directly on Johans patches. Would
>>>> that be possible for you?
>>>>
>>>
>>> Sure, it's better to integrate related change into one patch.
>>> I can update my patch, then Johan can just integrate my patch if he agree.
>>> Or Johan change his patch directly if needed.
>>
>> I'll try to have an updated patchset by Monday next week which
>> integrates your changes and adds your Signed-off-by if that's OK with
>> you. Thank you for testing this stuff!
>>
>
> I'm ok.
>
Johan and Ulf,

I just sent the v2 patchset which include your review comments.
Please review and integrate based on this version.

Thanks
Kevin