diff mbox

[2/2] mmc: core: Power cycle card on voltage switch fail

Message ID CAMLZHHRdz-==wgqvkoest61eaAdSHWTA0x-JTBgLWdO3B2gKJQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Drake Oct. 23, 2012, 10:48 p.m. UTC
On Tue, Oct 23, 2012 at 2:39 PM, Chris Ball <cjb@laptop.org> wrote:
> Dan, maybe you could see if this patch (there's a 1/2 patch too) solves
> your UHS problem?

I tested [RFC/PATCH] mmc: core: Fixup signal voltage switch
https://patchwork.kernel.org/patch/1514691/

This was previously failing on both XO-1.75 (kernel 3.0) and XO-4
(kernel 3.5) - the 1.8V switch would fail, but it would not
successfully switch back to 3.3V.

Testing on XO-4, it worked fine: the 1.8V failed switch was detected,
it came back as 3.3V and everything was fine.

Testing on XO-1.75, it didn't work: it thought that the 1.8V switch
was successful so it left it at that, then the card reacted in a very
unstable manner (failed/retried reads, huge amount of kernel log spam,
etc).

So I came up with the attached sdhci patch. That fixes the XO-1.75
case, which now correctly detects the 1.8V switch failure and goes to
3.3V. However, that broke XO-4, which now just does:
  mmc2: Signal voltage switch failed, power cycling card (retries = 10)
  mmc2: error -110 whilst initialising SD card
(no more time to debug exactly whats happening)

All tests with the same 32GB SD storage card.

I wonder if this difference in behaviour is related to the difference
in kernel versions on each platform (3.0 vs 3.5). I would like to test
with 3.5 or newer running on both, but this requires a bit of setup
work for the XO-1.75 first (long story). And I'm out of time at this
point :( this was a borrowed SD card.

Daniel
From 664afe4de295d3783d335a737ce239cebf5885f2 Mon Sep 17 00:00:00 2001
From: Daniel Drake <dsd@laptop.org>
Date: Tue, 23 Oct 2012 15:58:33 -0600
Subject: [PATCH] update sdhci for voltage changing fixes

---
 drivers/mmc/host/sdhci.c | 72 +++++++++---------------------------------------
 1 file changed, 13 insertions(+), 59 deletions(-)

Comments

Johan Rudholm Oct. 24, 2012, 10:56 a.m. UTC | #1
Hi Chris and Daniel,

Chris: Thank you for commenting on this patch! Yes, the sdhci power
cycle code needs to be removed just as Daniel's patch does, and as I
suggested in another patch: [RFC] mmc: sdhci: Let core handle UHS
switch failure (https://patchwork.kernel.org/patch/1517211/).

Daniel: Thank you for testing the patch! Comments below.

2012/10/24 Daniel Drake <dsd@laptop.org>:
> On Tue, Oct 23, 2012 at 2:39 PM, Chris Ball <cjb@laptop.org> wrote:
>> Dan, maybe you could see if this patch (there's a 1/2 patch too) solves
>> your UHS problem?
>
> I tested [RFC/PATCH] mmc: core: Fixup signal voltage switch
> https://patchwork.kernel.org/patch/1514691/
>
> This was previously failing on both XO-1.75 (kernel 3.0) and XO-4
> (kernel 3.5) - the 1.8V switch would fail, but it would not
> successfully switch back to 3.3V.

Can you enlighten me a bit; what is XO? :) Host controller hardware?

> Testing on XO-4, it worked fine: the 1.8V failed switch was detected,
> it came back as 3.3V and everything was fine.

Ok, so since you didn't have the card_busy function at this point, the
failure was detected by the sdhci code, right? It power-cycles the
card, returns -EAGAIN, the new code in mmc_sd_get_cid will try 10
times and then come back to 3.3V. Is this what happened, did you get
this print?

                pr_warning("%s: Skipping voltage switch\n",
                        mmc_hostname(host));

> Testing on XO-1.75, it didn't work: it thought that the 1.8V switch
> was successful so it left it at that, then the card reacted in a very
> unstable manner (failed/retried reads, huge amount of kernel log spam,
> etc).

This points to that the way of checking if the card is busy or not may
not work on XO-1.75? But then it seems strange that the card was
semi-usable...

> So I came up with the attached sdhci patch. That fixes the XO-1.75
> case, which now correctly detects the 1.8V switch failure and goes to
> 3.3V. However, that broke XO-4, which now just does:
>   mmc2: Signal voltage switch failed, power cycling card (retries = 10)
>   mmc2: error -110 whilst initialising SD card
> (no more time to debug exactly whats happening)

Ok, so failure is detected in mmc_set_signal_voltage by your card_busy
function, good. Then the card is power cycled, and should be
initialized again, but this fails, it's probably mmc_send_app_op_cond
which returns -110. Maybe the power cycle fails?

So the strange thing here is that on kernel 3.5, without my patch,
1.8V switch fails and fall-back to 3.3V fails. With my patch, 1.8V
fails and fall-back to 3.3V succeeds. But, when we move the detection
and error-handling to my patch (the upper layer), the fall-back fails.

I'm thinking of a couple of things that could have gone wrong here...
If you used v2 of the patch, is assumes that the signal voltage is
reset to 3.30 V in mmc_power_up, but this was introduced quite
recently, in v3.6 (mmc: core: reset signal voltage on power up), but
you used v1, right? So then there are at least two other differences:

1) The way the card is power cycled.
    sdhci toggles the SDHCI_POWER_ON bit in SDHCI_POWER_CONTROL, but
my patch calls mmc_power_off / mmc_power_up.
    Do you know if mmc_power_off / up is a good way to power-cycle the card?

2) The way the clock is stopped.
    sdhci clears SDHCI_CLOCK_CARD_EN in SDHCI_CLOCK_CONTROL, my patch
sets ios.clock = 0 and calls mmc_set_ios.
    But maybe this is not the issue, since the 1.8V switch fails in all cases.

I don't really know how to proceed. Do you have the complete dmesgs
from the 3.5 kernel: with my patch and without your patch + with my
patch and with your patch? With these I could try to do a deeper
analysis.

By the way, in your patch
0001-update-sdhci-for-voltage-changing-fixes.txt you don't check if
the signal voltage switch succeeds electrically:

                        ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
                        if (ctrl & SDHCI_CTRL_VDD_180) {

but maybe we can assume that this will be fine after the delay of 5 ms
in mmc_set_signal_voltage? Also in sdhci_card_busy you don't do
runtime_pm_get before you check the busy status, but since the busy
check returns busy, perhaps this is not the issue here either.

> All tests with the same 32GB SD storage card.
>
> I wonder if this difference in behaviour is related to the difference
> in kernel versions on each platform (3.0 vs 3.5). I would like to test
> with 3.5 or newer running on both, but this requires a bit of setup
> work for the XO-1.75 first (long story). And I'm out of time at this
> point :( this was a borrowed SD card.

Ok, I hope you'll be able to get time and SD-card in the future. :)
Btw, what brand was the SD-card?

Kind regards, Johan
--
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
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 07a5346..1955406 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1587,9 +1587,7 @@  static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 						struct mmc_ios *ios)
 {
-	u8 pwr;
-	u16 clk, ctrl;
-	u32 present_state;
+	u16 ctrl;
 
 	/*
 	 * Signal Voltage Switching is only applicable for Host Controllers
@@ -1622,62 +1620,9 @@  static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		}
 	} else if (!(ctrl & SDHCI_CTRL_VDD_180) &&
 		  (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180)) {
-		/* Stop SDCLK */
-		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-		clk &= ~SDHCI_CLOCK_CARD_EN;
-		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-
-		/* 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
-			 */
-			ctrl |= SDHCI_CTRL_VDD_180;
-			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
-
-			/* Wait for 5ms */
-			usleep_range(5000, 5500);
-
-			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);
-
-				/*
-				 * 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;
-			}
-		}
-
-		/*
-		 * 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);
-
-		/* Wait for 1ms as per the spec */
-		usleep_range(1000, 1500);
-		pwr |= SDHCI_POWER_ON;
-		sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
-
-		pr_info(DRIVER_NAME ": Switching to 1.8V signalling "
-			"voltage failed, retrying with S18R set to 0\n");
-		return -EAGAIN;
+		ctrl |= SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+		return 0;
 	} else
 		/* No signal voltage switch required */
 		return 0;
@@ -1931,11 +1876,20 @@  static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
 	sdhci_runtime_pm_put(host);
 }
 
+static int sdhci_card_busy(struct mmc_host *mmc, int keep_busy)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	u32 present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
+
+	return (present_state & SDHCI_DATA_LVL_MASK) != SDHCI_DATA_LVL_MASK;
+}
+
 static const struct mmc_host_ops sdhci_ops = {
 	.request	= sdhci_request,
 	.set_ios	= sdhci_set_ios,
 	.get_ro		= sdhci_get_ro,
 	.hw_reset	= sdhci_hw_reset,
+	.card_busy	= sdhci_card_busy,
 	.enable_sdio_irq = sdhci_enable_sdio_irq,
 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
 	.execute_tuning			= sdhci_execute_tuning,