[PATCH 0/3] Signal voltage switch procedure for UHS mode

[PATCH 0/3] Signal voltage switch procedure for UHS mode

[Johan Rudholm]

This patch series attempts to make the 1.8V signal voltage switch
required for UHS mode work according to the SD specification.

These patches have been tested with a couple of UHS SD cards, one of
which sometimes requires up to five power cycles before it accepts the
signal voltage switch. The patches have also been tested with various
other SD cards, as well as one SDIO WLAN chip (cw1200) to check
for regressions. The patches have also been tested with
CONFIG_MMC_CLKGATE.

As usual, I'd be very grateful if someone could help me test this patch
with an UHS SDIO card and perhaps also a combo card (which does seem to
be rare these days?)?

This patch series is based on previous RFC/patch:

[RFC/PATCH v2] mmc: core: Fixup signal voltage switch

Changelog:
	v2 -> This patch series
	- Removed the extra argument to the card_busy host_ops function
	- Added mmc_power_cycle
	- Some clarifying comments
	v1 -> v2
	- Removed reset of signal voltage in mmc_sd_get_cid, since
	  "mmc: core: reset signal voltage on power up"
	previous two patches -> v1:
	- Keep calls to mmc_host_clk_hold / mmc_host_clk_release
	- Add retry-loop / power cycle in sdio.c
	- Fall back to 3.3 V if the switch repeatedly fails
	- Add an extra argument to the card_busy host_ops function,
	  which can be used to signal polling use of the function

Johan Rudholm (3):
  mmc: core: Add mmc_power_cycle
  mmc: core: Add card_busy to host_ops
  mmc: core: Fixup signal voltage switch

 drivers/mmc/core/core.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/core.h  |    1 +
 drivers/mmc/core/sd.c    |   26 +++++++++++++++++++++-----
 drivers/mmc/core/sdio.c  |   25 +++++++++++++++++++++++--
 include/linux/mmc/host.h |    3 +++
 5 files changed, 91 insertions(+), 7 deletions(-)

-- 
1.7.10

[PATCH 1/3] mmc: core: Add mmc_power_cycle

[Johan Rudholm]

Add mmc_power_cycle which can be used to power cycle for instance
SD-cards.

Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
---
 drivers/mmc/core/core.c |    8 ++++++++
 drivers/mmc/core/core.h |    1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index aaed768..285f064 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1372,6 +1372,14 @@ void mmc_power_off(struct mmc_host *host)
 	mmc_host_clk_release(host);
 }
 
+void mmc_power_cycle(struct mmc_host *host)
+{
+	mmc_power_off(host);
+	/* Wait at least 1 ms according to SD spec */
+	mmc_delay(1);
+	mmc_power_up(host);
+}
+
 /*
  * Cleanup when the last reference to the bus operator is dropped.
  */
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 3bdafbc..4a90067 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -45,6 +45,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage,
 void mmc_set_timing(struct mmc_host *host, unsigned int timing);
 void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
 void mmc_power_off(struct mmc_host *host);
+void mmc_power_cycle(struct mmc_host *host);
 
 static inline void mmc_delay(unsigned int ms)
 {
-- 
1.7.10

[PATCH 2/3] mmc: core: Add card_busy to host_ops

[Johan Rudholm]

This host_ops member is used to test if the card is signaling busy by
pulling dat[0:3] low.

Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
---
 include/linux/mmc/host.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 23df21e..e2ed8c5 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -131,6 +131,9 @@ struct mmc_host_ops {
 
 	int	(*start_signal_voltage_switch)(struct mmc_host *host, struct mmc_ios *ios);
 
+	/* Check if the card is pulling dat[0:3] low */
+	int	(*card_busy)(struct mmc_host *host);
+
 	/* The tuning command opcode value is different for SD and eMMC cards */
 	int	(*execute_tuning)(struct mmc_host *host, u32 opcode);
 	void	(*enable_preset_value)(struct mmc_host *host, bool enable);
-- 
1.7.10

[PATCH 3/3] mmc: core: Fixup signal voltage switch

[Johan Rudholm]

When switching SD and SDIO cards from 3.3V to 1.8V signal levels, the
clock should be gated for 5 ms during the step. After enabling the
clock, the host should wait for at least 1 ms before checking for
failure. Failure by the card to switch is indicated by dat[0:3] being
pulled low. The host should check for this condition and power-cycle
the card if failure is indicated.

Add a retry mechanism for the SDIO case.

If the voltage switch fails repeatedly, give up and continue the
initialization using the original voltage.

This patch places a couple of requirements on the host driver:

 1) mmc_set_ios with ios.clock = 0 must gate the clock
 2) mmc_power_off must actually cut the power to the card

if these requirements are not fulfilled, the 1.8V signal voltage switch
may not be successful.

Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
---
 drivers/mmc/core/core.c |   35 +++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sd.c   |   26 +++++++++++++++++++++-----
 drivers/mmc/core/sdio.c |   25 +++++++++++++++++++++++--
 3 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 285f064..c9a7a8a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1246,8 +1246,43 @@ 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;
+
 		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);
+		}
+
 		err = host->ops->start_signal_voltage_switch(host, &host->ios);
+
+		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);
+
+			/* 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)
+				pr_warning("%s: cannot verify signal voltage switch\n",
+					mmc_hostname(host));
+			else if (host->ops->card_busy(host))
+				err = -EAGAIN;
+		}
 		mmc_host_clk_release(host);
 	}
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 74972c2..eb299bc 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -713,6 +713,14 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
 {
 	int err;
 	u32 max_current;
+	int retries = 10;
+
+try_again:
+	if (!retries) {
+		ocr &= ~SD_OCR_S18R;
+		pr_warning("%s: Skipping voltage switch\n",
+			mmc_hostname(host));
+	}
 
 	/*
 	 * Since we're changing the OCR value, we seem to
@@ -734,9 +742,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
 
 	/*
 	 * If the host supports one of UHS-I modes, request the card
-	 * to switch to 1.8V signaling level.
+	 * to switch to 1.8V signaling level. If the card has failed
+	 * repeatedly to switch however, skip this.
 	 */
-	if (host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
+	if (retries && host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
 	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50))
 		ocr |= SD_OCR_S18R;
 
@@ -748,7 +757,6 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
 	if (max_current > 150)
 		ocr |= SD_OCR_XPC;
 
-try_again:
 	err = mmc_send_app_op_cond(host, ocr, rocr);
 	if (err)
 		return err;
@@ -760,8 +768,16 @@ 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) {
-			ocr &= ~SD_OCR_S18R;
+		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;
 			goto try_again;
 		}
 	}
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2273ce6..573ab06 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -583,10 +583,19 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 {
 	struct mmc_card *card;
 	int err;
+	int retries = 10;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+try_again:
+	if (!retries) {
+		pr_warning("%s: Skipping voltage switch\n",
+				mmc_hostname(host));
+		ocr &= ~R4_18V_PRESENT;
+		host->ocr &= ~R4_18V_PRESENT;
+	}
+
 	/*
 	 * Inform the card of the voltage
 	 */
@@ -645,14 +654,26 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 * systems that claim 1.8v signalling in fact do not support
 	 * it.
 	 */
-	if ((ocr & R4_18V_PRESENT) &&
+	if (!powered_resume && (ocr & R4_18V_PRESENT) &&
 		(host->caps &
 			(MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
 			 MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
 			 MMC_CAP_UHS_DDR50))) {
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
 				true);
-		if (err) {
+		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);
+			sdio_reset(host);
+			mmc_go_idle(host);
+			mmc_send_if_cond(host, host->ocr_avail);
+			mmc_remove_card(card);
+			retries--;
+			goto try_again;
+		} else if (err) {
 			ocr &= ~R4_18V_PRESENT;
 			host->ocr &= ~R4_18V_PRESENT;
 		}
-- 
1.7.10

Re: [PATCH 3/3] mmc: core: Fixup signal voltage switch

[Subhash Jadavani]

On 12/7/2012 9:49 PM, Johan Rudholm wrote:
> When switching SD and SDIO cards from 3.3V to 1.8V signal levels, the
> clock should be gated for 5 ms during the step. After enabling the
> clock, the host should wait for at least 1 ms before checking for
> failure. Failure by the card to switch is indicated by dat[0:3] being
> pulled low. The host should check for this condition and power-cycle
> the card if failure is indicated.
>
> Add a retry mechanism for the SDIO case.
>
> If the voltage switch fails repeatedly, give up and continue the
> initialization using the original voltage.
>
> This patch places a couple of requirements on the host driver:
>
>   1) mmc_set_ios with ios.clock = 0 must gate the clock
>   2) mmc_power_off must actually cut the power to the card
>
> if these requirements are not fulfilled, the 1.8V signal voltage switch
> may not be successful.
>
> Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
> ---
>   drivers/mmc/core/core.c |   35 +++++++++++++++++++++++++++++++++++
>   drivers/mmc/core/sd.c   |   26 +++++++++++++++++++++-----
>   drivers/mmc/core/sdio.c |   25 +++++++++++++++++++++++--
>   3 files changed, 79 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 285f064..c9a7a8a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1246,8 +1246,43 @@ 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;
> +
>   		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);
> +		}
> +
>   		err = host->ops->start_signal_voltage_switch(host, &host->ios);

Shouldn't you fix all the existing host drivers who have already 
implemented start_signal_voltage_switch host ops? If you don't change 
them as part of this patch then
i afraid UHS functionality would be broken on those platforms. Also, 
it's not just changing the start_signal_voltage_switch hot op 
implementation, you may also need to add card_busy() host op 
implementation for those drivers.

> +
> +		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);
> +
> +			/* 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)
> +				pr_warning("%s: cannot verify signal voltage switch\n",
> +					mmc_hostname(host));
> +			else if (host->ops->card_busy(host))
> +				err = -EAGAIN;
> +		}
>   		mmc_host_clk_release(host);
>   	}
>   
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 74972c2..eb299bc 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -713,6 +713,14 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>   {
>   	int err;
>   	u32 max_current;
> +	int retries = 10;
> +
> +try_again:
> +	if (!retries) {
> +		ocr &= ~SD_OCR_S18R;
> +		pr_warning("%s: Skipping voltage switch\n",
> +			mmc_hostname(host));
> +	}
>   
>   	/*
>   	 * Since we're changing the OCR value, we seem to
> @@ -734,9 +742,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>   
>   	/*
>   	 * If the host supports one of UHS-I modes, request the card
> -	 * to switch to 1.8V signaling level.
> +	 * to switch to 1.8V signaling level. If the card has failed
> +	 * repeatedly to switch however, skip this.
>   	 */
> -	if (host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> +	if (retries && host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
>   	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50))
>   		ocr |= SD_OCR_S18R;
>   
> @@ -748,7 +757,6 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>   	if (max_current > 150)
>   		ocr |= SD_OCR_XPC;
>   
> -try_again:
>   	err = mmc_send_app_op_cond(host, ocr, rocr);
>   	if (err)
>   		return err;
> @@ -760,8 +768,16 @@ 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) {
> -			ocr &= ~SD_OCR_S18R;
> +		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;
>   			goto try_again;
>   		}
>   	}
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 2273ce6..573ab06 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -583,10 +583,19 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>   {
>   	struct mmc_card *card;
>   	int err;
> +	int retries = 10;
>   
>   	BUG_ON(!host);
>   	WARN_ON(!host->claimed);
>   
> +try_again:
> +	if (!retries) {
> +		pr_warning("%s: Skipping voltage switch\n",
> +				mmc_hostname(host));
> +		ocr &= ~R4_18V_PRESENT;
> +		host->ocr &= ~R4_18V_PRESENT;
> +	}
> +
>   	/*
>   	 * Inform the card of the voltage
>   	 */
> @@ -645,14 +654,26 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>   	 * systems that claim 1.8v signalling in fact do not support
>   	 * it.
>   	 */
> -	if ((ocr & R4_18V_PRESENT) &&
> +	if (!powered_resume && (ocr & R4_18V_PRESENT) &&
>   		(host->caps &
>   			(MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
>   			 MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
>   			 MMC_CAP_UHS_DDR50))) {
>   		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
>   				true);
> -		if (err) {
> +		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);
> +			sdio_reset(host);
> +			mmc_go_idle(host);
> +			mmc_send_if_cond(host, host->ocr_avail);
> +			mmc_remove_card(card);
> +			retries--;
> +			goto try_again;
> +		} else if (err) {
>   			ocr &= ~R4_18V_PRESENT;
>   			host->ocr &= ~R4_18V_PRESENT;
>   		}

Re: [PATCH 3/3] mmc: core: Fixup signal voltage switch

[Johan Rudholm]

Hi Subhash,

2012/12/8 Subhash Jadavani <subhashj@codeaurora.org>:
> On 12/7/2012 9:49 PM, Johan Rudholm wrote:
>>
>> When switching SD and SDIO cards from 3.3V to 1.8V signal levels, the
>> clock should be gated for 5 ms during the step. After enabling the
>> clock, the host should wait for at least 1 ms before checking for
>> failure. Failure by the card to switch is indicated by dat[0:3] being
>> pulled low. The host should check for this condition and power-cycle
>> the card if failure is indicated.
>>
>> Add a retry mechanism for the SDIO case.
>>
>> If the voltage switch fails repeatedly, give up and continue the
>> initialization using the original voltage.
>>
>> This patch places a couple of requirements on the host driver:
>>
>>   1) mmc_set_ios with ios.clock = 0 must gate the clock
>>   2) mmc_power_off must actually cut the power to the card
>>
>> if these requirements are not fulfilled, the 1.8V signal voltage switch
>> may not be successful.

<snip>

>>                 err = host->ops->start_signal_voltage_switch(host,
>> &host->ios);
>
>
> Shouldn't you fix all the existing host drivers who have already implemented
> start_signal_voltage_switch host ops? If you don't change them as part of
> this patch then
> i afraid UHS functionality would be broken on those platforms. Also, it's
> not just changing the start_signal_voltage_switch hot op implementation, you
> may also need to add card_busy() host op implementation for those drivers.

This is true, and I did actually make an RFC for the sdhci driver
("[RFC] mmc: sdhci: Let core handle UHS switch failure":
https://patchwork.kernel.org/patch/1517211/). Daniel Drake tried this
code for a problem related to the 1.8V switch (his board could
actually not perform the switch to 1.8V even though this capability
was announced), and I think this pointed out two areas for further
investigation before a proper patch for the sdhci driver can be
created:

1) the sdhci driver may not gate the clock when setting ios.clock = 0
and calling mmc_set_ios
2) mmc_power_off may not cut power to the card

maybe 2) was only for Daniel's board, I'm not sure, but this needs to
be investigated further anyway. Since I don't have any sdhci hardware
with UHS support, this must either be done by some other kind soul or
it will have to wait until I get the hardware.

sdhci is the only driver I'm aware of that's got (mainlined) support
for start_signal_voltage_switch, are you thinking of any other driver?
I'm developing for the MMCI driver, but the UHS support code is still
pending mainlining.

Kind regards, Johan

Re: [PATCH 3/3] mmc: core: Fixup signal voltage switch

[Subhash Jadavani]

On 12/10/2012 1:51 PM, Johan Rudholm wrote:
> Hi Subhash,
>
> 2012/12/8 Subhash Jadavani <subhashj@codeaurora.org>:
>> On 12/7/2012 9:49 PM, Johan Rudholm wrote:
>>> When switching SD and SDIO cards from 3.3V to 1.8V signal levels, the
>>> clock should be gated for 5 ms during the step. After enabling the
>>> clock, the host should wait for at least 1 ms before checking for
>>> failure. Failure by the card to switch is indicated by dat[0:3] being
>>> pulled low. The host should check for this condition and power-cycle
>>> the card if failure is indicated.
>>>
>>> Add a retry mechanism for the SDIO case.
>>>
>>> If the voltage switch fails repeatedly, give up and continue the
>>> initialization using the original voltage.
>>>
>>> This patch places a couple of requirements on the host driver:
>>>
>>>    1) mmc_set_ios with ios.clock = 0 must gate the clock
>>>    2) mmc_power_off must actually cut the power to the card
>>>
>>> if these requirements are not fulfilled, the 1.8V signal voltage switch
>>> may not be successful.
> <snip>
>
>>>                  err = host->ops->start_signal_voltage_switch(host,
>>> &host->ios);
>>
>> Shouldn't you fix all the existing host drivers who have already implemented
>> start_signal_voltage_switch host ops? If you don't change them as part of
>> this patch then
>> i afraid UHS functionality would be broken on those platforms. Also, it's
>> not just changing the start_signal_voltage_switch hot op implementation, you
>> may also need to add card_busy() host op implementation for those drivers.
> This is true, and I did actually make an RFC for the sdhci driver
> ("[RFC] mmc: sdhci: Let core handle UHS switch failure":
> https://patchwork.kernel.org/patch/1517211/). Daniel Drake tried this
> code for a problem related to the 1.8V switch (his board could
> actually not perform the switch to 1.8V even though this capability
> was announced), and I think this pointed out two areas for further
> investigation before a proper patch for the sdhci driver can be
> created:
>
> 1) the sdhci driver may not gate the clock when setting ios.clock = 0
> and calling mmc_set_ios
> 2) mmc_power_off may not cut power to the card
>
> maybe 2) was only for Daniel's board, I'm not sure, but this needs to
> be investigated further anyway. Since I don't have any sdhci hardware
> with UHS support, this must either be done by some other kind soul or
> it will have to wait until I get the hardware.
>
> sdhci is the only driver I'm aware of that's got (mainlined) support
> for start_signal_voltage_switch, are you thinking of any other driver?
You may look at only in tree drivers who have implemented 
start_singnal_voltage_switch() ops. Our msm_sdcc* driver has also 
implemented it but it's not in tree driver so i may
need to take care of that later once we are synced to kernel version 
which will have your patch series.

Yes, SDHCi seeems to be only one in tree driver implemented the 
start_singnal_voltage_switch() ops. So you might be good fixing the same.
> I'm developing for the MMCI driver, but the UHS support code is still
> pending mainlining.
>
> Kind regards, Johan

Re: [PATCH 3/3] mmc: core: Fixup signal voltage switch

[Johan Rudholm]

Hi Subhash,

2012/12/11 Subhash Jadavani <subhashj@codeaurora.org>:
> On 12/10/2012 1:51 PM, Johan Rudholm wrote:
>>
>> 2012/12/8 Subhash Jadavani <subhashj@codeaurora.org>:
>>>
>>> On 12/7/2012 9:49 PM, Johan Rudholm wrote:
>>>>
>>>> When switching SD and SDIO cards from 3.3V to 1.8V signal levels, the
>>>> clock should be gated for 5 ms during the step. After enabling the
>>>> clock, the host should wait for at least 1 ms before checking for
>>>> failure. Failure by the card to switch is indicated by dat[0:3] being
>>>> pulled low. The host should check for this condition and power-cycle
>>>> the card if failure is indicated.
>>>>
>>>> Add a retry mechanism for the SDIO case.
>>>>
>>>> If the voltage switch fails repeatedly, give up and continue the
>>>> initialization using the original voltage.
>>>>
>>>> This patch places a couple of requirements on the host driver:
>>>>
>>>>    1) mmc_set_ios with ios.clock = 0 must gate the clock
>>>>    2) mmc_power_off must actually cut the power to the card
>>>>
>>>> if these requirements are not fulfilled, the 1.8V signal voltage switch
>>>> may not be successful.
>>
>> <snip>
>>
>>>>                  err = host->ops->start_signal_voltage_switch(host,
>>>> &host->ios);
>>>
>>>
>>> Shouldn't you fix all the existing host drivers who have already
>>> implemented
>>> start_signal_voltage_switch host ops? If you don't change them as part of
>>> this patch then
>>> i afraid UHS functionality would be broken on those platforms. Also, it's
>>> not just changing the start_signal_voltage_switch hot op implementation,
>>> you
>>> may also need to add card_busy() host op implementation for those
>>> drivers.
>>
>> This is true, and I did actually make an RFC for the sdhci driver
>> ("[RFC] mmc: sdhci: Let core handle UHS switch failure":
>> https://patchwork.kernel.org/patch/1517211/). Daniel Drake tried this
>> code for a problem related to the 1.8V switch (his board could
>> actually not perform the switch to 1.8V even though this capability
>> was announced), and I think this pointed out two areas for further
>> investigation before a proper patch for the sdhci driver can be
>> created:
>>
>> 1) the sdhci driver may not gate the clock when setting ios.clock = 0
>> and calling mmc_set_ios
>> 2) mmc_power_off may not cut power to the card
>>
>> maybe 2) was only for Daniel's board, I'm not sure, but this needs to
>> be investigated further anyway. Since I don't have any sdhci hardware
>> with UHS support, this must either be done by some other kind soul or
>> it will have to wait until I get the hardware.
>>
>> sdhci is the only driver I'm aware of that's got (mainlined) support
>> for start_signal_voltage_switch, are you thinking of any other driver?
>
> You may look at only in tree drivers who have implemented
> start_singnal_voltage_switch() ops. Our msm_sdcc* driver has also
> implemented it but it's not in tree driver so i may
> need to take care of that later once we are synced to kernel version which
> will have your patch series.

Ok, good.

> Yes, SDHCi seeems to be only one in tree driver implemented the
> start_singnal_voltage_switch() ops. So you might be good fixing the same.

It seems the UHS support in the SDHCI driver is a complex issue,
judging from Daniel's experience described above, so I think the best
way forward is to get these patches in and then take care of the SDHCI
driver (perhaps based on my previous RFC), unless any of the SDHCI
guys thinks differently? Naturally, I will be happy to assist in
fixing the SDHCI driver.

Kind regards, Johan

Re: [PATCH 3/3] mmc: core: Fixup signal voltage switch

[Kevin Liu]

2012/12/14 Johan Rudholm <jrudholm@gmail.com>:
> Hi Subhash,
>
> 2012/12/11 Subhash Jadavani <subhashj@codeaurora.org>:
>> On 12/10/2012 1:51 PM, Johan Rudholm wrote:
>>>
>>> 2012/12/8 Subhash Jadavani <subhashj@codeaurora.org>:
>>>>
>>>> On 12/7/2012 9:49 PM, Johan Rudholm wrote:
>>>>>
>>>>> When switching SD and SDIO cards from 3.3V to 1.8V signal levels, the
>>>>> clock should be gated for 5 ms during the step. After enabling the
>>>>> clock, the host should wait for at least 1 ms before checking for
>>>>> failure. Failure by the card to switch is indicated by dat[0:3] being
>>>>> pulled low. The host should check for this condition and power-cycle
>>>>> the card if failure is indicated.
>>>>>
>>>>> Add a retry mechanism for the SDIO case.
>>>>>
>>>>> If the voltage switch fails repeatedly, give up and continue the
>>>>> initialization using the original voltage.
>>>>>
>>>>> This patch places a couple of requirements on the host driver:
>>>>>
>>>>>    1) mmc_set_ios with ios.clock = 0 must gate the clock
>>>>>    2) mmc_power_off must actually cut the power to the card
>>>>>
>>>>> if these requirements are not fulfilled, the 1.8V signal voltage switch
>>>>> may not be successful.
>>>
>>> <snip>
>>>
>>>>>                  err = host->ops->start_signal_voltage_switch(host,
>>>>> &host->ios);
>>>>
>>>>
>>>> Shouldn't you fix all the existing host drivers who have already
>>>> implemented
>>>> start_signal_voltage_switch host ops? If you don't change them as part of
>>>> this patch then
>>>> i afraid UHS functionality would be broken on those platforms. Also, it's
>>>> not just changing the start_signal_voltage_switch hot op implementation,
>>>> you
>>>> may also need to add card_busy() host op implementation for those
>>>> drivers.
>>>
>>> This is true, and I did actually make an RFC for the sdhci driver
>>> ("[RFC] mmc: sdhci: Let core handle UHS switch failure":
>>> https://patchwork.kernel.org/patch/1517211/). Daniel Drake tried this
>>> code for a problem related to the 1.8V switch (his board could
>>> actually not perform the switch to 1.8V even though this capability
>>> was announced), and I think this pointed out two areas for further
>>> investigation before a proper patch for the sdhci driver can be
>>> created:
>>>
>>> 1) the sdhci driver may not gate the clock when setting ios.clock = 0
>>> and calling mmc_set_ios
>>> 2) mmc_power_off may not cut power to the card
>>>
>>> maybe 2) was only for Daniel's board, I'm not sure, but this needs to
>>> be investigated further anyway. Since I don't have any sdhci hardware
>>> with UHS support, this must either be done by some other kind soul or
>>> it will have to wait until I get the hardware.
>>>
>>> sdhci is the only driver I'm aware of that's got (mainlined) support
>>> for start_signal_voltage_switch, are you thinking of any other driver?
>>
>> You may look at only in tree drivers who have implemented
>> start_singnal_voltage_switch() ops. Our msm_sdcc* driver has also
>> implemented it but it's not in tree driver so i may
>> need to take care of that later once we are synced to kernel version which
>> will have your patch series.
>
> Ok, good.
>
>> Yes, SDHCi seeems to be only one in tree driver implemented the
>> start_singnal_voltage_switch() ops. So you might be good fixing the same.
>
> It seems the UHS support in the SDHCI driver is a complex issue,
> judging from Daniel's experience described above, so I think the best
> way forward is to get these patches in and then take care of the SDHCI
> driver (perhaps based on my previous RFC), unless any of the SDHCI
> guys thinks differently? Naturally, I will be happy to assist in
> fixing the SDHCI driver.
>

Johan,

Just sent a patchset for sdhci based on your patch. Please help to review.
Tested with both UHS and non-UHS sd card on sdhci-pxav3 platform.

Thanks
Kevin

Re: [PATCH 3/3] mmc: core: Fixup signal voltage switch

[Johan Rudholm]

Hi Kevin,

2012/12/14 Kevin Liu <keyuan.liu@gmail.com>:
>
> Just sent a patchset for sdhci based on your patch. Please help to review.
> Tested with both UHS and non-UHS sd card on sdhci-pxav3 platform.

Great! I'll get on it right away.

Kind regards, Johan

Re: [PATCH 0/3] Signal voltage switch procedure for UHS mode

[Ulf Hansson]

On 7 December 2012 17:19, Johan Rudholm <johan.rudholm@stericsson.com> wrote:
> This patch series attempts to make the 1.8V signal voltage switch
> required for UHS mode work according to the SD specification.
>
> These patches have been tested with a couple of UHS SD cards, one of
> which sometimes requires up to five power cycles before it accepts the
> signal voltage switch. The patches have also been tested with various
> other SD cards, as well as one SDIO WLAN chip (cw1200) to check
> for regressions. The patches have also been tested with
> CONFIG_MMC_CLKGATE.
>
> As usual, I'd be very grateful if someone could help me test this patch
> with an UHS SDIO card and perhaps also a combo card (which does seem to
> be rare these days?)?
>
> This patch series is based on previous RFC/patch:
>
> [RFC/PATCH v2] mmc: core: Fixup signal voltage switch
>
> Changelog:
>         v2 -> This patch series
>         - Removed the extra argument to the card_busy host_ops function
>         - Added mmc_power_cycle
>         - Some clarifying comments
>         v1 -> v2
>         - Removed reset of signal voltage in mmc_sd_get_cid, since
>           "mmc: core: reset signal voltage on power up"
>         previous two patches -> v1:
>         - Keep calls to mmc_host_clk_hold / mmc_host_clk_release
>         - Add retry-loop / power cycle in sdio.c
>         - Fall back to 3.3 V if the switch repeatedly fails
>         - Add an extra argument to the card_busy host_ops function,
>           which can be used to signal polling use of the function
>
> Johan Rudholm (3):
>   mmc: core: Add mmc_power_cycle
>   mmc: core: Add card_busy to host_ops
>   mmc: core: Fixup signal voltage switch
>
>  drivers/mmc/core/core.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/core.h  |    1 +
>  drivers/mmc/core/sd.c    |   26 +++++++++++++++++++++-----
>  drivers/mmc/core/sdio.c  |   25 +++++++++++++++++++++++--
>  include/linux/mmc/host.h |    3 +++
>  5 files changed, 91 insertions(+), 7 deletions(-)
>
> --
> 1.7.10
>

Great Johan! Thanks for fixing this up.

Now, I can go ahead with upstreaming the patches for mmci host driver
for this feature.
If some of the sdhci guys needs some assistance for fixing this more
properly for sdhci as well, I can also happily help out.

So for the hole series:

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Ulf Hansson

RE: [PATCH 0/3] Signal voltage switch procedure for UHS mode

[Shen, Jackey]

Hi Johan,

Is the SDIO WLAN chip (cw1200), you mentioned, SDIO 3.0 device with SDIO interface?

Thanks,
Jackey 

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Johan Rudholm
Sent: Saturday, December 08, 2012 12:20 AM
To: linux-mmc@vger.kernel.org; Chris Ball
Cc: Per Forlin; Ulf Hansson; Fredrik Soderstedt; Kevin Liu; Philip Rakity; Daniel Drake; Aaron; Subhash Jadavani; Johan Rudholm
Subject: [PATCH 0/3] Signal voltage switch procedure for UHS mode

This patch series attempts to make the 1.8V signal voltage switch required for UHS mode work according to the SD specification.

These patches have been tested with a couple of UHS SD cards, one of which sometimes requires up to five power cycles before it accepts the signal voltage switch. The patches have also been tested with various other SD cards, as well as one SDIO WLAN chip (cw1200) to check for regressions. The patches have also been tested with CONFIG_MMC_CLKGATE.

As usual, I'd be very grateful if someone could help me test this patch with an UHS SDIO card and perhaps also a combo card (which does seem to be rare these days?)?

This patch series is based on previous RFC/patch:

[RFC/PATCH v2] mmc: core: Fixup signal voltage switch

Changelog:
	v2 -> This patch series
	- Removed the extra argument to the card_busy host_ops function
	- Added mmc_power_cycle
	- Some clarifying comments
	v1 -> v2
	- Removed reset of signal voltage in mmc_sd_get_cid, since
	  "mmc: core: reset signal voltage on power up"
	previous two patches -> v1:
	- Keep calls to mmc_host_clk_hold / mmc_host_clk_release
	- Add retry-loop / power cycle in sdio.c
	- Fall back to 3.3 V if the switch repeatedly fails
	- Add an extra argument to the card_busy host_ops function,
	  which can be used to signal polling use of the function

Johan Rudholm (3):
  mmc: core: Add mmc_power_cycle
  mmc: core: Add card_busy to host_ops
  mmc: core: Fixup signal voltage switch

 drivers/mmc/core/core.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/core.h  |    1 +
 drivers/mmc/core/sd.c    |   26 +++++++++++++++++++++-----
 drivers/mmc/core/sdio.c  |   25 +++++++++++++++++++++++--
 include/linux/mmc/host.h |    3 +++
 5 files changed, 91 insertions(+), 7 deletions(-)

--
1.7.10

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Signal voltage switch procedure for UHS mode

[Johan Rudholm]

Hi Jackey,

no, the cw1200 chip is a SDIO < 2.0 device.

Kind regards, Johan

2012/12/14 Shen, Jackey <Jackey.Shen@amd.com>:
> Hi Johan,
>
> Is the SDIO WLAN chip (cw1200), you mentioned, SDIO 3.0 device with SDIO interface?
>
> Thanks,
> Jackey
>
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Johan Rudholm
> Sent: Saturday, December 08, 2012 12:20 AM
> To: linux-mmc@vger.kernel.org; Chris Ball
> Cc: Per Forlin; Ulf Hansson; Fredrik Soderstedt; Kevin Liu; Philip Rakity; Daniel Drake; Aaron; Subhash Jadavani; Johan Rudholm
> Subject: [PATCH 0/3] Signal voltage switch procedure for UHS mode
>
> This patch series attempts to make the 1.8V signal voltage switch required for UHS mode work according to the SD specification.
>
> These patches have been tested with a couple of UHS SD cards, one of which sometimes requires up to five power cycles before it accepts the signal voltage switch. The patches have also been tested with various other SD cards, as well as one SDIO WLAN chip (cw1200) to check for regressions. The patches have also been tested with CONFIG_MMC_CLKGATE.
>
> As usual, I'd be very grateful if someone could help me test this patch with an UHS SDIO card and perhaps also a combo card (which does seem to be rare these days?)?
>
> This patch series is based on previous RFC/patch:
>
> [RFC/PATCH v2] mmc: core: Fixup signal voltage switch
>
> Changelog:
>         v2 -> This patch series
>         - Removed the extra argument to the card_busy host_ops function
>         - Added mmc_power_cycle
>         - Some clarifying comments
>         v1 -> v2
>         - Removed reset of signal voltage in mmc_sd_get_cid, since
>           "mmc: core: reset signal voltage on power up"
>         previous two patches -> v1:
>         - Keep calls to mmc_host_clk_hold / mmc_host_clk_release
>         - Add retry-loop / power cycle in sdio.c
>         - Fall back to 3.3 V if the switch repeatedly fails
>         - Add an extra argument to the card_busy host_ops function,
>           which can be used to signal polling use of the function
>
> Johan Rudholm (3):
>   mmc: core: Add mmc_power_cycle
>   mmc: core: Add card_busy to host_ops
>   mmc: core: Fixup signal voltage switch
>
>  drivers/mmc/core/core.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/core.h  |    1 +
>  drivers/mmc/core/sd.c    |   26 +++++++++++++++++++++-----
>  drivers/mmc/core/sdio.c  |   25 +++++++++++++++++++++++--
>  include/linux/mmc/host.h |    3 +++
>  5 files changed, 91 insertions(+), 7 deletions(-)
>
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html