Skip to contentSkip to conversation

Fix sd/sdio/mmc initialization frequency retries

PATCH In review Andy Ross 3 participants 7 messages Dec 28, 2010 – Jan 4, 2011 MMC, SD and SDIO

lore
Share & tools
lore

Andy Ross

author
#0 lore raw

PATCH 1/1 Fix sd/sdio/mmc initialization frequency retries

I'm working with a eMMC device that broke after this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=88ae8b86648

The older hack I'd been given hard-coded the initialisation frequency at 200kHz (instead of 400kHz in the original upstream) and works fine.

But the new code tries to dynamically detect the highest frequency that will work, but as far as I can tell the logic is wrong. It will continue trying after failed mmc_send_*_cond() calls, but *not* after failures later on in mmc_attach_{sd,sdio,mmc}. My boards (at 400kHz) are getting successful returns from mmc_send_op_cond(), but failing in mmc_attach_mmc(). If I retry at 300, it works.

Tested on the same controller using a sdhc card. No idea how the extra retries will affect other hardware (though as the original code was fixed at 400, presumably very few devices will care?).

The attached patch fixes that error, and does some rewriting that should hopefully make review easier:

+ The freqs array should be static
+ No need for locking around atomic rescan_disable test
+ Remove adjacent bus_put/get calls.
+ Move repeated claim/release host calls out of mmc_rescan loop
+ Hopefully clearer iteration logic for f_init
+ Remove duplicate mmc_attach_sd() logic after sdio failures
+ Move mmc_send_*_cond() calls into mmc_attach_*() functions, as they are only called from one place and are the only consumers of the ocr return value.
+ Make mmc_attach_*() functions symmetric with respect to mmc_claim/release_host(). The existing code is called with the lock and releases it before returning, which was really confusing to me when debugging this issue. They now hold the lock throughout, releasing across some interior calls to preserve existing behavior.

Quoted 1 line

From 0357d0d07d4985de43c814a9fea02d13753c121c Mon Sep 17 00:00:00 2001

From: Andy Ross <[email protected]>
Date: Tue, 28 Dec 2010 09:04:10 -0800
Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries

Rewrite and clean up mmc_rescan() to properly retry frequencies lower than 400kHz. Failures can happen both in sd_send_* calls and mmc_attach_*. Symmetrize claim/release logic in mmc_attach_* API, and move the sd_send_* calls there to make mmc_rescan easier to read.

Signed-off-by Andy Ross <[email protected]>
---
 drivers/mmc/core/core.c |   86 ++++++++--------------------------------------
 drivers/mmc/core/core.h |    6 ++--
 drivers/mmc/core/mmc.c  |   12 ++++--
 drivers/mmc/core/sd.c   |   10 ++++--
 drivers/mmc/core/sdio.c |   17 ++++++---
 5 files changed, 44 insertions(+), 87 deletions(-)
drivers/mmc/core/core.c +15 -71
index 31ae07a..c92de04 100644
@@ -1426,23 +1426,13 @@ EXPORT_SYMBOL(mmc_set_blocklen);
 
 void mmc_rescan(struct work_struct *work)
 {
+	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 	struct mmc_host *host =
 		container_of(work, struct mmc_host, detect.work);
-	u32 ocr;
-	int err;
-	unsigned long flags;
 	int i;
-	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
-	spin_lock_irqsave(&host->lock, flags);
-
-	if (host->rescan_disable) {
-		spin_unlock_irqrestore(&host->lock, flags);
+	if (host->rescan_disable)
 		return;
-	}
-
-	spin_unlock_irqrestore(&host->lock, flags);
-
 
 	mmc_bus_get(host);
 

@@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
 	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
 		host->bus_ops->detect(host);
 
-	mmc_bus_put(host);
-
-
-	mmc_bus_get(host);
-
 	/* if there still is a card present, stop here */
 	if (host->bus_ops != NULL) {
 		mmc_bus_put(host);
 		goto out;
 	}
 
-	/* detect a newly inserted card */
-
 	/*
 	 * Only we can add a new handler, so it's safe to
 	 * release the lock here.

@@ -1472,17 +1455,11 @@ void mmc_rescan(struct work_struct *work)
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;
 
-	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-		mmc_claim_host(host);
+	mmc_claim_host(host);
+	host->f_init = freqs[0];
+	for (i=0; i<ARRAY_SIZE(freqs) && host->f_init > host->f_min; i++) {
+		host->f_init = max(freqs[i], host->f_min);
 
-		if (freqs[i] >= host->f_min)
-			host->f_init = freqs[i];
-		else if (!i || freqs[i-1] > host->f_min)
-			host->f_init = host->f_min;
-		else {
-			mmc_release_host(host);
-			goto out;
-		}
 #ifdef CONFIG_MMC_DEBUG
 		pr_info("%s: %s: trying to init card at %u Hz\n",
 			mmc_hostname(host), __func__, host->f_init);

@@ -1493,50 +1470,17 @@ void mmc_rescan(struct work_struct *work)
 
 		mmc_send_if_cond(host, host->ocr_avail);
 
-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr)) {
-				mmc_claim_host(host);
-				/*
-				 * Try SDMEM (but not MMC) even if SDIO
-				 * is broken.
-				 */
-				if (mmc_send_app_op_cond(host, 0, &ocr))
-					goto out_fail;
-
-				if (mmc_attach_sd(host, ocr))
-					mmc_power_off(host);
-			}
-			goto out;
-		}
-
-		/*
-		 * ...then normal SD...
-		 */
-		err = mmc_send_app_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sd(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-		/*
-		 * ...and finally MMC.
-		 */
-		err = mmc_send_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_mmc(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
+		/* Try SDIO, then SD, then MMC */
+		if(mmc_attach_sdio(host) == 0)
+			break;
+		else if(mmc_attach_sd(host) == 0)
+			break;
+		else if(mmc_attach_mmc(host) == 0)
+			break;
 
-out_fail:
-		mmc_release_host(host);
-		mmc_power_off(host);
+                mmc_power_off(host);
 	}
+	mmc_release_host(host);
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
drivers/mmc/core/core.h +3 -3
index 77240cd..54de886 100644
@@ -54,9 +54,9 @@ void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);
 
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
-int mmc_attach_sd(struct mmc_host *host, u32 ocr);
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
+int mmc_attach_mmc(struct mmc_host *host);
+int mmc_attach_sd(struct mmc_host *host);
+int mmc_attach_sdio(struct mmc_host *host);
 
 /* Module parameters */
 extern int use_spi_crc;
drivers/mmc/core/mmc.c +8 -4
index 77f93c3..cad2eaf 100644
@@ -737,13 +737,17 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for MMC card init.
  */
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
+int mmc_attach_mmc(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+ 	if((err = mmc_send_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_attach_bus_ops(host);
 
 	/*

@@ -784,20 +788,20 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
 		goto err;
 
 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;
 
 	return 0;
 
 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
-	host->card = NULL;
 	mmc_claim_host(host);
+	host->card = NULL;
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising MMC card\n",
 		mmc_hostname(host), err);
drivers/mmc/core/sd.c +7 -3
index 49da4df..9fad37a 100644
@@ -764,13 +764,17 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	if((err = mmc_send_app_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_sd_attach_bus_ops(host);
 
 	/*

@@ -820,20 +824,20 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 		goto err;
 
 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;
 
 	return 0;
 
 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
 	host->card = NULL;
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
drivers/mmc/core/sdio.c +11 -6
index efef5f9..5d2f33d 100644
@@ -690,15 +690,18 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 /*
  * Starting point for SDIO card init.
  */
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
+int mmc_attach_sdio(struct mmc_host *host)
 {
-	int err;
-	int i, funcs;
+	int err, i, funcs;
+	u32 ocr;
 	struct mmc_card *card;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	if((err =mmc_send_io_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_attach_bus(host, &mmc_sdio_ops);
 
 	/*

@@ -769,12 +772,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}
 
-	mmc_release_host(host);
-
 	/*
 	 * First add the card to the driver model...
 	 */
+	mmc_release_host(host);
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_added;
 

@@ -792,15 +795,17 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 
 remove_added:
 	/* Remove without lock if the device has been added. */
+	mmc_release_host(host);
 	mmc_sdio_remove(host);
 	mmc_claim_host(host);
 remove:
 	/* And with lock if it hasn't been added. */
+	mmc_release_host(host);
 	if (host->card)
 		mmc_sdio_remove(host);
+	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SDIO card\n",
 		mmc_hostname(host), err);
Signature · 1 line
1.7.1
#1 lore raw

Hi Andy,

This patch doesn't apply against mmc-next, which makes it hard to review properly; could you rebase/resend?

I like the look of these changes, just a few comments:

Quoted 4 lines · Andy Ross
drivers/mmc/core/core.c +0 -0
@@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
 	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
 		host->bus_ops->detect(host);

drivers/mmc/core/core.c void mmc_rescan(struct work_struct *work) · line 1447

Quoted 10 lines
drivers/mmc/core/core.c +0 -5
-	mmc_bus_put(host);
-
-
-	mmc_bus_get(host);
-
 	/* if there still is a card present, stop here */
 	if (host->bus_ops != NULL) {
 		mmc_bus_put(host);
 		goto out;
 	}

Let's leave these in -- I think we're depending on the side-effect of mmc_bus_put() clearing out host->bus_ops. I'll check and submit a separate patch adding a comment there explaining what's going on.

drivers/mmc/core/core.c void mmc_rescan(struct work_struct *work) · line 1479

Quoted 7 lines
drivers/mmc/core/core.c +7 -0
+		/* Try SDIO, then SD, then MMC */
+		if(mmc_attach_sdio(host) == 0)
+			break;
+		else if(mmc_attach_sd(host) == 0)
+			break;
+		else if(mmc_attach_mmc(host) == 0)
+			break;

Space after if, and perhaps let's use !mmc_attach_*() rather than == 0 for brevity. I guess we could also consider:

		/* Try SDIO, then SD, then MMC */
		if (mmc_attach_sdio(host) && mmc_attach_sd(host) &&
		    mmc_attach_mmc(host))
			mmc_power_off(host);

	mmc_release_host(host);

which would save quite a few lines, but perhaps it's less clear (or not equivalent somehow). What do you think?

drivers/mmc/core/core.c void mmc_rescan(struct work_struct *work) · line 1481

Quoted 4 lines
drivers/mmc/core/core.c +1 -3
-out_fail:
-		mmc_release_host(host);
-		mmc_power_off(host);
+              mmc_power_off(host);

This looks misaligned.

drivers/mmc/core/mmc.c static void mmc_attach_bus_ops(struct mmc_host *host) · line 749

Quoted 2 lines
drivers/mmc/core/mmc.c +2 -0
+ 	if((err = mmc_send_op_cond(host, 0, &ocr)))
+		return err;

Redundant parens, and space after if.

drivers/mmc/core/sdio.c static const struct mmc_bus_ops mmc_sdio_ops = { · line 702

Quoted 1 line
drivers/mmc/core/sdio.c +1 -0
+	if((err =mmc_send_io_op_cond(host, 0, &ocr)))

As above, plus space after =.

Thanks! You might also consider splitting this up into two separate patches, but I'll leave that up to you.

- Chris.

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
author
#2 lore raw

[Combined response to Chris and Hein, hope that's not too confusing. And sorry about the delay, had to wait until this morning to have access to my board to test.]

Quoted 2 lines · Chris Ball

This patch doesn't apply against mmc-next, which makes it hard to review properly;

Yeah. And you got the pre-checkpatch version too, thus the syntax goofs. Apologies, I do kernel stuff only occasionally. New patch
below. Applies and builds vs. mmc-next, tested on my board this
morning with the problematic eMMC device and a sdhc card.

Quoted 3 lines

Let's leave these in -- I think we're depending on the side-effect of mmc_bus_put() clearing out host->bus_ops. I'll check and submit a separate patch adding a comment there explaining what's going on.

OK, I put exactly that text in as a placeholder. :)

Quoted 3 lines

Space after if, and perhaps let's use !mmc_attach_*() rather than == 0 for brevity. I guess we could also consider:

    if (mmc_attach_sdio(host) && mmc_attach_sd(host) &&

Personally, I use the "== 0" syntax because my brain wants to read "!" as a test for failure, not success. But I'm fine with whatever. I didn't do the folded test though -- that not only flips my internal sense of the boolean, it makes me think through the short circuit behavior of "&&" in the transformed gauge.

Quoted 1 line · Hein Tibosch

Suppose f_min > 400 Khz, no initialization will ever take place?

Uh... yeah. That's a bug. Fixed in this version; I split out the "try_freq" step so the loop contains only the frequency selection logic. Hopefully this is even clearer.

Quoted 2 lines

The "else" after break is syntactically redundant, this would do the same:

Quibble: the else is *semantically* redundant. :)

The syntactic meaning of "if ... if" vs. "if ... else if" is definitely different, and the only reason those two cases are equivalent is because of the special behavior of the break statement *inside* the if expressions. I like mine for this reason (it's robust against changes to the code inside the block), but I'm not the maintainer. Fixed.

Quoted 3 lines

Don't know why all these additions of claim/release? In some cases it will lead to unnecessary releases, just because later in the branch claim will be called.

It's voodoo, basically, because I didn't want to break something. The original code for these functions was called with the claim held, and released it before returning. This confused me terribly and made the exit conditions complicated in mmc_rescan(), so I modified the functions to hold the cookie across the call.

But some of the calls in the original were made after the release, so I do an explicit release/claim around the calls that did *not* hold the lock in the original because I wasn't sure what the assumptions were. I'm pretty sure it preserves the original semantics, but I'm not expert enough to know whether it can be optimized.

Andy

Quoted 1 line

From 411a58a232e4266877d0af4840226661dd687474 Mon Sep 17 00:00:00 2001

From: Andy Ross <[email protected]>
Date: Mon, 3 Jan 2011 10:30:23 -0800
Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries

Rewrite and clean up mmc_rescan() to properly retry frequencies lower than 400kHz. Failures can happen both in sd_send_* calls and mmc_attach_*. Break out "mmc_rescan_try_freq" from the frequency selection loop. Symmetrize claim/release logic in mmc_attach_* API, and move the sd_send_* calls there to make mmc_rescan easier to read.

Signed-off-by Andy Ross <[email protected]>
---
 drivers/mmc/core/core.c |  124 +++++++++++++++--------------------------------
 drivers/mmc/core/core.h |    6 +-
 drivers/mmc/core/mmc.c  |   13 +++--
 drivers/mmc/core/sd.c   |   11 +++-
 drivers/mmc/core/sdio.c |   18 +++++--
 5 files changed, 71 insertions(+), 101 deletions(-)
drivers/mmc/core/core.c +39 -85
index a8e89f3..ef7f8cc 100644
@@ -1485,25 +1485,41 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
 }
 EXPORT_SYMBOL(mmc_set_blocklen);
 
+static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
+{
+	host->f_init = freq;
+
+#ifdef CONFIG_MMC_DEBUG
+	pr_info("%s: %s: trying to init card at %u Hz\n",
+		mmc_hostname(host), __func__, host->f_init);
+#endif
+	mmc_power_up(host);
+	sdio_reset(host);
+	mmc_go_idle(host);
+
+	mmc_send_if_cond(host, host->ocr_avail);
+
+	/* Order's important: probe SDIO, then SD, then MMC */
+	if (!mmc_attach_sdio(host))
+		return 0;
+	if (!mmc_attach_sd(host))
+		return 0;
+	if (!mmc_attach_mmc(host))
+		return 0;
+
+	mmc_power_off(host);
+	return -EIO;
+}
+
 void mmc_rescan(struct work_struct *work)
 {
+	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 	struct mmc_host *host =
 		container_of(work, struct mmc_host, detect.work);
-	u32 ocr;
-	int err;
-	unsigned long flags;
 	int i;
-	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
-
-	spin_lock_irqsave(&host->lock, flags);
 
-	if (host->rescan_disable) {
-		spin_unlock_irqrestore(&host->lock, flags);
+	if (host->rescan_disable)
 		return;
-	}
-
-	spin_unlock_irqrestore(&host->lock, flags);
-
 
 	mmc_bus_get(host);
 

@@ -1515,9 +1531,10 @@ void mmc_rescan(struct work_struct *work)
 	    && mmc_card_is_removable(host))
 		host->bus_ops->detect(host);
 
+	/* "Let's leave these in -- I think we're depending on the
+	 * side-effect of mmc_bus_put() clearing out host->bus_ops."
+	 * -cjb 2010-12-30 */
 	mmc_bus_put(host);
-
-
 	mmc_bus_get(host);
 
 	/* if there still is a card present, stop here */

@@ -1526,8 +1543,6 @@ void mmc_rescan(struct work_struct *work)
 		goto out;
 	}
 
-	/* detect a newly inserted card */
-
 	/*
 	 * Only we can add a new handler, so it's safe to
 	 * release the lock here.

@@ -1537,77 +1552,16 @@ void mmc_rescan(struct work_struct *work)
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;
 
+	mmc_claim_host(host);
 	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-		mmc_claim_host(host);
-
-		if (freqs[i] >= host->f_min)
-			host->f_init = freqs[i];
-		else if (!i || freqs[i-1] > host->f_min)
-			host->f_init = host->f_min;
-		else {
-			mmc_release_host(host);
-			goto out;
-		}
-#ifdef CONFIG_MMC_DEBUG
-		pr_info("%s: %s: trying to init card at %u Hz\n",
-			mmc_hostname(host), __func__, host->f_init);
-#endif
-		mmc_power_up(host);
-		sdio_reset(host);
-		mmc_go_idle(host);
-
-		mmc_send_if_cond(host, host->ocr_avail);
-
-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr)) {
-				mmc_claim_host(host);
-				/*
-				 * Try SDMEM (but not MMC) even if SDIO
-				 * is broken.
-				 */
-				mmc_power_up(host);
-				sdio_reset(host);
-				mmc_go_idle(host);
-				mmc_send_if_cond(host, host->ocr_avail);
-
-				if (mmc_send_app_op_cond(host, 0, &ocr))
-					goto out_fail;
-
-				if (mmc_attach_sd(host, ocr))
-					mmc_power_off(host);
-			}
-			goto out;
-		}
-
-		/*
-		 * ...then normal SD...
-		 */
-		err = mmc_send_app_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sd(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-		/*
-		 * ...and finally MMC.
-		 */
-		err = mmc_send_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_mmc(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-out_fail:
-		mmc_release_host(host);
-		mmc_power_off(host);
+		if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
+			break;
+		if (freqs[i] < host->f_min)
+			break;
 	}
-out:
+	mmc_release_host(host);
+
+ out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
 }
drivers/mmc/core/core.h +3 -3
index 026c975..ca1fdde 100644
@@ -57,9 +57,9 @@ void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);
 
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
-int mmc_attach_sd(struct mmc_host *host, u32 ocr);
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
+int mmc_attach_mmc(struct mmc_host *host);
+int mmc_attach_sd(struct mmc_host *host);
+int mmc_attach_sdio(struct mmc_host *host);
 
 /* Module parameters */
 extern int use_spi_crc;
drivers/mmc/core/mmc.c +9 -4
index c86dd73..16006ef 100644
@@ -755,13 +755,18 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for MMC card init.
  */
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
+int mmc_attach_mmc(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	err = mmc_send_op_cond(host, 0, &ocr);
+	if (err)
+		return err;
+
 	mmc_attach_bus_ops(host);
 	if (host->ocr_avail_mmc)
 		host->ocr_avail = host->ocr_avail_mmc;

@@ -804,20 +809,20 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
 		goto err;
 
 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;
 
 	return 0;
 
 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
-	host->card = NULL;
 	mmc_claim_host(host);
+	host->card = NULL;
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising MMC card\n",
 		mmc_hostname(host), err);
drivers/mmc/core/sd.c +8 -3
index de062eb..d18c32b 100644
@@ -764,13 +764,18 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	err = mmc_send_app_op_cond(host, 0, &ocr);
+	if (err)
+		return err;
+
 	mmc_sd_attach_bus_ops(host);
 	if (host->ocr_avail_sd)
 		host->ocr_avail = host->ocr_avail_sd;

@@ -823,20 +828,20 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 		goto err;
 
 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;
 
 	return 0;
 
 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
 	host->card = NULL;
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
drivers/mmc/core/sdio.c +12 -6
index 82f4b90..5c4a54d 100644
@@ -702,15 +702,19 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 /*
  * Starting point for SDIO card init.
  */
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
+int mmc_attach_sdio(struct mmc_host *host)
 {
-	int err;
-	int i, funcs;
+	int err, i, funcs;
+	u32 ocr;
 	struct mmc_card *card;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	err = mmc_send_io_op_cond(host, 0, &ocr);
+	if (err)
+		return err;
+
 	mmc_attach_bus(host, &mmc_sdio_ops);
 	if (host->ocr_avail_sdio)
 		host->ocr_avail = host->ocr_avail_sdio;

@@ -783,12 +787,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}
 
-	mmc_release_host(host);
-
 	/*
 	 * First add the card to the driver model...
 	 */
+	mmc_release_host(host);
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_added;
 

@@ -806,15 +810,17 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 
 remove_added:
 	/* Remove without lock if the device has been added. */
+	mmc_release_host(host);
 	mmc_sdio_remove(host);
 	mmc_claim_host(host);
 remove:
 	/* And with lock if it hasn't been added. */
+	mmc_release_host(host);
 	if (host->card)
 		mmc_sdio_remove(host);
+	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SDIO card\n",
 		mmc_hostname(host), err);
Signature · 1 line
1.7.1
#3 lore raw

Hi Andy,

Quoted 9 lines · Andy Ross

Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries

Rewrite and clean up mmc_rescan() to properly retry frequencies lower than 400kHz. Failures can happen both in sd_send_* calls and mmc_attach_*. Break out "mmc_rescan_try_freq" from the frequency selection loop. Symmetrize claim/release logic in mmc_attach_* API, and move the sd_send_* calls there to make mmc_rescan easier to read.

Signed-off-by Andy Ross <[email protected]>

Thanks! Pushed to mmc-next for .38, after adding Reviewed-by: tags and removing the mmc_bus_{put,get}() comment, since I'll take care of that separately.

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#4 lore raw

Hi,

Quoted 3 lines · Chris Ball

Thanks! Pushed to mmc-next for .38, after adding Reviewed-by: tags and removing the mmc_bus_{put,get}() comment, since I'll take care of that separately.

And here's the comment patch:

From: Chris Ball <[email protected]>
Date: Tue, 4 Jan 2011 12:20:22 -0500
Subject: [PATCH] mmc: Explain why we make adjacent mmc_bus_{put,get} calls during rescan.

Signed-off-by Chris Ball <[email protected]>
---
 drivers/mmc/core/core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/mmc/core/core.c +4 -2
index 286e6ce..b8e3b97 100644
@@ -1529,13 +1529,15 @@ void mmc_rescan(struct work_struct *work)
 	 */
 	if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
 	    && mmc_card_is_removable(host))
 		host->bus_ops->detect(host);
 
+	/*
+	 * Let mmc_bus_put() free the bus/bus_ops if we've found that
+	 * the card is no longer present.
+	 */
 	mmc_bus_put(host);
-
-
 	mmc_bus_get(host);
 
 	/* if there still is a card present, stop here */
 	if (host->bus_ops != NULL) {
 		mmc_bus_put(host);
Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#5 lore raw
In reply to Andy Ross

Hi Andy,

It looks like a good cleanup!

Quoted 14 lines · Andy Ross

I'm working with a eMMC device that broke after this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=88ae8b86648

The older hack I'd been given hard-coded the initialisation frequency at 200kHz (instead of 400kHz in the original upstream) and works fine.

But the new code tries to dynamically detect the highest frequency that will work, but as far as I can tell the logic is wrong. It will continue trying after failed mmc_send_*_cond() calls, but *not* after failures later on in mmc_attach_{sd,sdio,mmc}. My boards (at 400kHz) are getting successful returns from mmc_send_op_cond(), but failing in mmc_attach_mmc(). If I retry at 300, it works.

Quoted 3 lines

Tested on the same controller using a sdhc card. No idea how the extra retries will affect other hardware (though as the original code was fixed at 400, presumably very few devices will care?).

And before the fixed 400 Khz, it went like this:

    host->ios.clock = host->f_min;

So with the older versions, your card(s) would have initialized fine, and mine as well.

Quoted 88 lines

The attached patch fixes that error, and does some rewriting that should hopefully make review easier:

+ The freqs array should be static
+ No need for locking around atomic rescan_disable test
+ Remove adjacent bus_put/get calls.
+ Move repeated claim/release host calls out of mmc_rescan loop
+ Hopefully clearer iteration logic for f_init
+ Remove duplicate mmc_attach_sd() logic after sdio failures
+ Move mmc_send_*_cond() calls into mmc_attach_*() functions, as they are only called from one place and are the only consumers of the ocr return value.
+ Make mmc_attach_*() functions symmetric with respect to mmc_claim/release_host(). The existing code is called with the lock and releases it before returning, which was really confusing to me when debugging this issue. They now hold the lock throughout, releasing across some interior calls to preserve existing behavior.

Quoted 1 line

From 0357d0d07d4985de43c814a9fea02d13753c121c Mon Sep 17 00:00:00 2001

From: Andy Ross <[email protected]>
Date: Tue, 28 Dec 2010 09:04:10 -0800
Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries

Rewrite and clean up mmc_rescan() to properly retry frequencies lower than 400kHz. Failures can happen both in sd_send_* calls and mmc_attach_*. Symmetrize claim/release logic in mmc_attach_* API, and move the sd_send_* calls there to make mmc_rescan easier to read.

Signed-off-by Andy Ross <[email protected]>
---
 drivers/mmc/core/core.c |   86 ++++++++--------------------------------------
 drivers/mmc/core/core.h |    6 ++--
 drivers/mmc/core/mmc.c  |   12 ++++--
 drivers/mmc/core/sd.c   |   10 ++++--
 drivers/mmc/core/sdio.c |   17 ++++++---
 5 files changed, 44 insertions(+), 87 deletions(-)
drivers/mmc/core/core.c +2 -19
index 31ae07a..c92de04 100644
@@ -1426,23 +1426,13 @@ EXPORT_SYMBOL(mmc_set_blocklen);
 
 void mmc_rescan(struct work_struct *work)
 {
+	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 	struct mmc_host *host =
 		container_of(work, struct mmc_host, detect.work);
-	u32 ocr;
-	int err;
-	unsigned long flags;
 	int i;
-	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
-	spin_lock_irqsave(&host->lock, flags);
-
-	if (host->rescan_disable) {
-		spin_unlock_irqrestore(&host->lock, flags);
+	if (host->rescan_disable)
 		return;
-	}
-
-	spin_unlock_irqrestore(&host->lock, flags);
-
 
 	mmc_bus_get(host);
 

@@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
 	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
 		host->bus_ops->detect(host);
 
-	mmc_bus_put(host);
-
-
-	mmc_bus_get(host);
-
 	/* if there still is a card present, stop here */
 	if (host->bus_ops != NULL) {
 		mmc_bus_put(host);
 		goto out;
 	}
 
-	/* detect a newly inserted card */
-
 	/*
 	 * Only we can add a new handler, so it's safe to
 	 * release the lock here.

drivers/mmc/core/core.c void mmc_rescan(struct work_struct *work) · line 1460

Quoted 8 lines
drivers/mmc/core/core.c +3 -2
@@ -1472,17 +1455,11 @@ void mmc_rescan(struct work_struct *work)
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;
 
-	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-		mmc_claim_host(host);
+	mmc_claim_host(host);
+	host->f_init = freqs[0];
+	for (i=0; i<ARRAY_SIZE(freqs) && host->f_init > host->f_min; i++) {

Suppose f_min > 400 Khz, no initialization will ever take place?

Can't you change it to:

+ for (i=0;
+ i<ARRAY_SIZE(freqs) && (host->f_init > host->f_min || !i);
+ i++) {

ans so at least f_min will be tried?

Quoted 54 lines
drivers/mmc/core/core.c +1 -45
+		host->f_init = max(freqs[i], host->f_min);
 
-		if (freqs[i] >= host->f_min)
-			host->f_init = freqs[i];
-		else if (!i || freqs[i-1] > host->f_min)
-			host->f_init = host->f_min;
-		else {
-			mmc_release_host(host);
-			goto out;
-		}
 #ifdef CONFIG_MMC_DEBUG
 		pr_info("%s: %s: trying to init card at %u Hz\n",
 			mmc_hostname(host), __func__, host->f_init);

@@ -1493,50 +1470,17 @@ void mmc_rescan(struct work_struct *work)
 
 		mmc_send_if_cond(host, host->ocr_avail);
 
-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr)) {
-				mmc_claim_host(host);
-				/*
-				 * Try SDMEM (but not MMC) even if SDIO
-				 * is broken.
-				 */
-				if (mmc_send_app_op_cond(host, 0, &ocr))
-					goto out_fail;
-
-				if (mmc_attach_sd(host, ocr))
-					mmc_power_off(host);
-			}
-			goto out;
-		}
-
-		/*
-		 * ...then normal SD...
-		 */
-		err = mmc_send_app_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sd(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-		/*
-		 * ...and finally MMC.
-		 */
-		err = mmc_send_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_mmc(host, ocr))
-				mmc_power_off(host);

drivers/mmc/core/core.c void mmc_rescan(struct work_struct *work) · line 1480

Quoted 10 lines
drivers/mmc/core/core.c +7 -2
-			goto out;
-		}
+		/* Try SDIO, then SD, then MMC */
+		if(mmc_attach_sdio(host) == 0)
+			break;
+		else if(mmc_attach_sd(host) == 0)
+			break;
+		else if(mmc_attach_mmc(host) == 0)
+			break;

The "else" after break is syntactically redundant, this would do the same:

+ if (!mmc_attach_sdio(host))
+ break;
+ if (!mmc_attach_sd(host))
+ break;
+ if (!mmc_attach_mmc(host))
+ break;

Quoted 165 lines
drivers/mmc/core/core.c +2 -3
-out_fail:
-		mmc_release_host(host);
-		mmc_power_off(host);
+                mmc_power_off(host);
 	}
+	mmc_release_host(host);
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
drivers/mmc/core/core.h +3 -3
index 77240cd..54de886 100644
@@ -54,9 +54,9 @@ void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);
 
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
-int mmc_attach_sd(struct mmc_host *host, u32 ocr);
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
+int mmc_attach_mmc(struct mmc_host *host);
+int mmc_attach_sd(struct mmc_host *host);
+int mmc_attach_sdio(struct mmc_host *host);
 
 /* Module parameters */
 extern int use_spi_crc;
drivers/mmc/core/mmc.c +8 -4
index 77f93c3..cad2eaf 100644
@@ -737,13 +737,17 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for MMC card init.
  */
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
+int mmc_attach_mmc(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+ 	if((err = mmc_send_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_attach_bus_ops(host);
 
 	/*

@@ -784,20 +788,20 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
 		goto err;
 
 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;
 
 	return 0;
 
 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
-	host->card = NULL;
 	mmc_claim_host(host);
+	host->card = NULL;
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising MMC card\n",
 		mmc_hostname(host), err);
drivers/mmc/core/sd.c +6 -3
index 49da4df..9fad37a 100644
@@ -764,13 +764,17 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	if((err = mmc_send_app_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_sd_attach_bus_ops(host);
 
 	/*

@@ -820,20 +824,20 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 		goto err;
 
 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
 	return 0;
 
 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
 	host->card = NULL;
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
drivers/mmc/core/sdio.c +9 -5
index efef5f9..5d2f33d 100644
@@ -690,15 +690,18 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 /*
  * Starting point for SDIO card init.
  */
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
+int mmc_attach_sdio(struct mmc_host *host)
 {
-	int err;
-	int i, funcs;
+	int err, i, funcs;
+	u32 ocr;
 	struct mmc_card *card;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	if((err =mmc_send_io_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_attach_bus(host, &mmc_sdio_ops);
 
 	/*

@@ -769,12 +772,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}
 
-	mmc_release_host(host);
-
 	/*
 	 * First add the card to the driver model...
 	 */
+	mmc_release_host(host);
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_added;
 

@@ -792,15 +795,17 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 
 remove_added:
 	/* Remove without lock if the device has been added. */
+	mmc_release_host(host);
 	mmc_sdio_remove(host);
 	mmc_claim_host(host);
 remove:
 	/* And with lock if it hasn't been added. */

drivers/mmc/core/sdio.c int mmc_attach_sdio(struct mmc_host *host, u32 ocr) · line 811

Quoted 10 lines
drivers/mmc/core/sdio.c +2 -1
+	mmc_release_host(host);
 	if (host->card)
 		mmc_sdio_remove(host);
+	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SDIO card\n",
 		mmc_hostname(host), err);

Don't know why all these additions of claim/release? In some cases it will lead to unnecessary releases, just because later in the branch claim will be called.

I think in all 3 cases (sd/sdio/mmc) the change can be much simpler. For instance for sd.c I would do this:

---

drivers/mmc/core/sd.c +6 -2
index 49da4df..18331eb
@@ -764,13 +764,17 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	if((err = mmc_send_app_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_sd_attach_bus_ops(host);
 
 	/*

@@ -825,6 +829,7 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 	if (err)
 		goto remove_card;
 
+	mmc_claim_host(host);
 	return 0;
 
 remove_card:

@@ -833,7 +838,6 @@ remove_card:
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);

--

Or in other words: add a claim before "return 0" and remove release after "err".

In the next days I'll test your patch on my system, which also had problems with the fixed 400 Khz. SD-cards will initialize at less than 280 Khz.

Hein Tibosch

#6 lore raw
Quoted 46 lines · Hein_Tibosch
Quoted 14 lines · Andy Ross

I'm working with a eMMC device that broke after this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=88ae8b86648

The older hack I'd been given hard-coded the initialisation frequency at 200kHz (instead of 400kHz in the original upstream) and works fine.

But the new code tries to dynamically detect the highest frequency that will work, but as far as I can tell the logic is wrong. It will continue trying after failed mmc_send_*_cond() calls, but *not* after failures later on in mmc_attach_{sd,sdio,mmc}. My boards (at 400kHz) are getting successful returns from mmc_send_op_cond(), but failing in mmc_attach_mmc(). If I retry at 300, it works.

Quoted 3 lines

Tested on the same controller using a sdhc card. No idea how the extra retries will affect other hardware (though as the original code was fixed at 400, presumably very few devices will care?).

<snip>

Don't know why all these additions of claim/release? In some cases it will lead to unnecessary releases, just because later in the branch claim will be called.

I think in all 3 cases (sd/sdio/mmc) the change can be much simpler. For instance for sd.c I would do this:

---

drivers/mmc/core/sd.c +1 -0
index 49da4df..18331eb
 
<snip>

@@ -825,6 +829,7 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 	if (err)
 		goto remove_card;
 
+	mmc_claim_host(host);
 	return 0;
 
 remove_card:

drivers/mmc/core/sd.c remove_card:

Quoted 8 lines
drivers/mmc/core/sd.c +0 -2
@@ -833,7 +838,6 @@ remove_card:
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);
 
 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
--

I tested the simplified patch as described above on AVR32 with an sd-card.

It worked well and the timing was the same as before: each frequency tried takes about 22 ms. The third time, at 200Khz, the card got initialized.

But not much time is lost, in a meanwhile it was starting up the USB host

Hein