Skip to contentSkip to conversation

mmc: Make ID freq configurable

PATCH Superseded Hein_Tibosch 6 participants 13 messages Sep 2, 2010 – Dec 17, 2010 MMC, SD and SDIO

lore
Share & tools
lore
#0 lore raw

PATCH v4 1/1 mmc: Make ID freq configurable

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies.

This patch implements that idea. It will try mmc-initialization using several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel 2.6.36-rc3.

In case SDIO is broken, it'll still try to detect SDMEM, also at different freqs.

---

drivers/mmc/core/core.c +51 -45
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
@@ -907,12 +907,7 @@ static void mmc_power_up(struct mmc_host
 	 */
 	mmc_delay(10);
 
-	if (host->f_min > 400000) {
-		pr_warning("%s: Minimum clock frequency too high for "
-				"identification mode\n", mmc_hostname(host));
-		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
 	u32 ocr;
 	int err;
 	unsigned long flags;
+	int i;
+	unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1443,55 +1440,64 @@ 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);
 
-	mmc_power_up(host);
-	sdio_reset(host);
-	mmc_go_idle(host);
+		if (freqs[i] >= host->f_min)
+			host->f_init = freqs[i];
+		else if (i && freqs[i-1] <= host->f_min)
+			goto out;
+		else
+			host->f_init = host->f_min;
 
-	mmc_send_if_cond(host, host->ocr_avail);
+		printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;
+		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;
 		}
-		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;
-	}
+		/*
+		 * ...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);
-
+		mmc_release_host(host);
+		mmc_power_off(host);
+	}
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
include/linux/mmc/host.h +1 -0
diff -Nurp a/include/linux/mmc/host.h b/include/linux/mmc/host.h
@@ -123,6 +123,7 @@ struct mmc_host {
 	const struct mmc_host_ops *ops;
 	unsigned int		f_min;
 	unsigned int		f_max;
+	unsigned int		f_init;
 	u32			ocr_avail;
 	struct notifier_block	pm_notify;
 

---

#1 lore raw

Hi Hein, thanks for resending this,

Quoted 10 lines · Hein_Tibosch

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies.

This patch implements that idea. It will try mmc-initialization using several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel 2.6.36-rc3.

Quoted 4 lines

In case SDIO is broken, it'll still try to detect SDMEM, also at different freqs.

Reviewed-and-Tested-by Chris Ball <[email protected]>

I tested by modifying mmc_send_app_op_cond() to return timeouts for various frequencies, and verified that the code correctly progresses through the freqs[] table until reaching host->f_min, and can bring up the card successfully on any of those retries.

Quoted 2 lines · you

Here it tries 3 frequencies, which takes 290 ms:
In a meanwhile it is doing other things, so not much time is wasted.

I decided to check this. If my emulated failing of bringing up a card at a given frequency is correct, 14ms per frequency attempt is spent in mdelay() rather than msleep(). I don't think this is a problem, though, because you only take that hit if you have a card inserted that we're unable to bring up at the default frequency -- the only alternative would be to give up on the card.

If a host has no card inserted then we still try to detect a card on all frequencies, but all of the paths in that case are msleep() rather than mdelay(), so that's okay.

Two minor checkpatch fixes below:

Quoted 37 lines

---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c

drivers/mmc/core/core.c +5 -9
@@ -907,12 +907,7 @@ static void mmc_power_up(struct mmc_host
 	 */
 	mmc_delay(10);
 
-	if (host->f_min > 400000) {
-		pr_warning("%s: Minimum clock frequency too high for "
-				"identification mode\n", mmc_hostname(host));
-		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
 	u32 ocr;
 	int err;
 	unsigned long flags;
+	int i;
+	unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1443,55 +1440,64 @@ 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);
 
-	mmc_power_up(host);
-	sdio_reset(host);

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

Quoted 10 lines
drivers/mmc/core/core.c +7 -2
-	mmc_go_idle(host);
+		if (freqs[i] >= host->f_min)
+			host->f_init = freqs[i];
+		else if (i && freqs[i-1] <= host->f_min)
+			goto out;
+		else
+			host->f_init = host->f_min;
 
-	mmc_send_if_cond(host, host->ocr_avail);
+		printk ("mmc_rescan: trying %u Hz\n", host->f_init);

Need a loglevel here, and an mmc_hostname. Something like:

pr_info("%s: %s: trying to init card at %u Hz\n",
mmc_hostname(host), __func__, host->f_init);

Quoted 14 lines
drivers/mmc/core/core.c +3 -10
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;

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

Quoted 10 lines
drivers/mmc/core/core.c +10 -0
+		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 */

This breaks 80-chars, so:

/*
* Try SDMEM (but not MMC) even if SDIO
* is broken.
*/

Quoted 72 lines
drivers/mmc/core/core.c +26 -24
+				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;
 		}
-		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;
-	}
+		/*
+		 * ...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);
-
+		mmc_release_host(host);
+		mmc_power_off(host);
+	}
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);

diff -Nurp a/include/linux/mmc/host.h b/include/linux/mmc/host.h

include/linux/mmc/host.h +1 -1
@@ -123,6 +123,7 @@ struct mmc_host {
 	const struct mmc_host_ops *ops;
 	unsigned int		f_min;
 	unsigned int		f_max;
+	unsigned int		f_init;
 	u32			ocr_avail;
 	struct notifier_block	pm_notify;
 
---
Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#2 lore raw
Quoted 35 lines · On Fri, 3 Sep 2010 02:47:57 +0100 Chris Ball <[email protected]> wrote:

Review

Quoted 2 lines
(quoted diff) +1 -1
-	mmc_send_if_cond(host, host->ocr_avail);
+		printk ("mmc_rescan: trying %u Hz\n", host->f_init);

Need a loglevel here, and an mmc_hostname. Something like:

pr_info("%s: %s: trying to init card at %u Hz\n",
mmc_hostname(host), __func__, host->f_init);

Quoted 14 lines
(quoted diff) +3 -10
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;

Review

Quoted 10 lines
(quoted diff) +10 -0
+		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 */

This breaks 80-chars, so:

Quoted 4 lines

/*
* Try SDMEM (but not MMC) even if SDIO
* is broken.
*/

yup.

drivers/mmc/core/core.c renamed from drivers/mmc/core/core.c~mmc-make-id-freq-configurable-checkpatch-fixes +7 -2
@@ -1450,7 +1450,9 @@ void mmc_rescan(struct work_struct *work
 		else
 			host->f_init = host->f_min;
 
-		printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+		pr_info("%s: %s: trying to init card at %u Hz\n",
+			mmc_hostname(host), __func__, host->f_init);
+
 		mmc_power_up(host);
 		sdio_reset(host);
 		mmc_go_idle(host);
@@ -1464,7 +1466,10 @@ void mmc_rescan(struct work_struct *work
 		if (!err) {
 			if (mmc_attach_sdio(host, ocr)) {
 				mmc_claim_host(host);
-				/* try SDMEM (but not MMC) even if SDIO is broken */
+				/*
+				 * Try SDMEM (but not MMC) even if SDIO is
+				 * broken.
+				 */
 				if (mmc_send_app_op_cond(host, 0, &ocr))
 					goto out_fail;
 

diff -puN include/linux/mmc/host.h~mmc-make-id-freq-configurable-checkpatch-fixes include/linux/mmc/host.h _

#3 lore raw
In reply to Hein_Tibosch
Quoted 47 lines · Hein_Tibosch

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies.

This patch implements that idea. It will try mmc-initialization using several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel 2.6.36-rc3.

In case SDIO is broken, it'll still try to detect SDMEM, also at different freqs.

---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c

drivers/mmc/core/core.c +3 -7
@@ -907,12 +907,7 @@ static void mmc_power_up(struct mmc_host
 	 */
 	mmc_delay(10);
 
-	if (host->f_min > 400000) {
-		pr_warning("%s: Minimum clock frequency too high for "
-				"identification mode\n", mmc_hostname(host));
-		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
 	u32 ocr;
 	int err;
 	unsigned long flags;
+	int i;
+	unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1443,55 +1440,64 @@ void mmc_rescan(struct work_struct *work
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;
 
-	mmc_claim_host(host);

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

Quoted 10 lines
drivers/mmc/core/core.c +6 -3
+	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
+		mmc_claim_host(host);
 
-	mmc_power_up(host);
-	sdio_reset(host);
-	mmc_go_idle(host);
+		if (freqs[i] >= host->f_min)
+			host->f_init = freqs[i];
+		else if (i && freqs[i-1] <= host->f_min)
+			goto out;

The label 'out' does not release the claim on the host.

Quoted 36 lines
drivers/mmc/core/core.c +23 -11
+		else
+			host->f_init = host->f_min;
 
-	mmc_send_if_cond(host, host->ocr_avail);
+		printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;
+		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;

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

Quoted 10 lines
drivers/mmc/core/core.c +7 -0
+		}
 
+		/*
+		 * ...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;

Same problem here.

Quoted 22 lines
drivers/mmc/core/core.c +0 -20
 		}
-		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;

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

Quoted 10 lines
drivers/mmc/core/core.c +9 -1
-	}
+		/*
+		 * ...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;
+		}

And here.

#4 lore raw

Hi Matt,

Quoted 51 lines · Matt Fleming
Quoted 50 lines · Hein_Tibosch

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies.

This patch implements that idea. It will try mmc-initialization using several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel 2.6.36-rc3.

In case SDIO is broken, it'll still try to detect SDMEM, also at different freqs.

---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c

drivers/mmc/core/core.c +5 -7
@@ -907,12 +907,7 @@ static void mmc_power_up(struct mmc_host
 	 */
 	mmc_delay(10);
 
-	if (host->f_min > 400000) {
-		pr_warning("%s: Minimum clock frequency too high for "
-				"identification mode\n", mmc_hostname(host));
-		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
 	u32 ocr;
 	int err;
 	unsigned long flags;
+	int i;
+	unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1443,55 +1440,64 @@ 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);
 
Quoted 9 lines

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

Quoted 7 lines
drivers/mmc/core/core.c +4 -3
-	mmc_power_up(host);
-	sdio_reset(host);
-	mmc_go_idle(host);
+		if (freqs[i] >= host->f_min)
+			host->f_init = freqs[i];
+		else if (i && freqs[i-1] <= host->f_min)
+			goto out;

The label 'out' does not release the claim on the host.

Thanks for catching that.

Quoted 75 lines
Quoted 36 lines
drivers/mmc/core/core.c +23 -11
+		else
+			host->f_init = host->f_min;
 
-	mmc_send_if_cond(host, host->ocr_avail);
+		printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;
+		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;

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

Quoted 10 lines
drivers/mmc/core/core.c +7 -0
+		}
 
+		/*
+		 * ...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;

Same problem here.

Quoted 26 lines
drivers/mmc/core/core.c +3 -21
 		}
-		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;
-	}
+		/*
+		 * ...and finally MMC.
+		 */
Quoted 8 lines

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

Quoted 6 lines
drivers/mmc/core/core.c +6 -0
+		err = mmc_send_op_cond(host, 0, &ocr);
+		if (!err) {
+			if (mmc_attach_mmc(host, ocr))
+				mmc_power_off(host);
+			goto out;
+		}

And here.

Actually, mmc_attach_{sd,mmc}() both do release the host as part of their failure case, so I don't think there's a problem with these two.

- Chris.

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

Actually, mmc_attach_{sd,mmc}() both do release the host as part of their failure case, so I don't think there's a problem with these two.

Ah, yep you're right, I missed that.

#6 lore raw

PATCH v5 1/1 mmc: Make ID freq configurable

Matt, Chris,

Thanks both for reviewing.

I never noticed that the host wasn't released because my f_min was low enough so that all frequencies would be tried. But yes, you're both right.

Before somebody else makes the remark: "unsigned freqs[]" can be declared as const.

Below, I changed the order of the if/else/else, and whenever no more frequencies can be tried, it will release the host before branching out.

Hein Tibosch

---

mmc-make-id-freq-configurable.patch (v5)

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies.

This patch implements that idea. It will try mmc-initialization using several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel 2.6.36-rc3.

In case SDIO is detected but turns out broken, it'll still try to detect SDMEM, also at different freqs.

---

drivers/mmc/core/core.c +57 -45
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
@@ -907,12 +907,7 @@ static void mmc_power_up(struct mmc_host
 	 */
 	mmc_delay(10);
 
-	if (host->f_min > 400000) {
-		pr_warning("%s: Minimum clock frequency too high for "
-				"identification mode\n", mmc_hostname(host));
-		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
 	u32 ocr;
 	int err;
 	unsigned long flags;
+	int i;
+	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1443,55 +1440,70 @@ 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);
 
-	mmc_power_up(host);
-	sdio_reset(host);
-	mmc_go_idle(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;
+		}
+		pr_info("%s: %s: trying to init card at %u Hz\n",
+			mmc_hostname(host), __func__, host->f_init);
 
-	mmc_send_if_cond(host, host->ocr_avail);
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;
+		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;
 		}
-		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;
-	}
+		/*
+		 * ...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);
-
+		mmc_release_host(host);
+		mmc_power_off(host);
+	}
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
include/linux/mmc/host.h +1 -0
diff -Nurp a/include/linux/mmc/host.h b/include/linux/mmc/host.h
@@ -123,6 +123,7 @@ struct mmc_host {
 	const struct mmc_host_ops *ops;
 	unsigned int		f_min;
 	unsigned int		f_max;
+	unsigned int		f_init;
 	u32			ocr_avail;
 	struct notifier_block	pm_notify;
 

--

#7 lore raw
In reply to Hein_Tibosch

Hi Hein Tibosch,

Quoted 77 lines · Hein_Tibosch

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies.

This patch implements that idea. It will try mmc-initialization using several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel 2.6.36-rc3.

In case SDIO is broken, it'll still try to detect SDMEM, also at different freqs.

---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c

drivers/mmc/core/core.c +16 -21
@@ -907,12 +907,7 @@ static void mmc_power_up(struct mmc_host
 	 */
 	mmc_delay(10);
 
-	if (host->f_min > 400000) {
-		pr_warning("%s: Minimum clock frequency too high for "
-				"identification mode\n", mmc_hostname(host));
-		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
 	u32 ocr;
 	int err;
 	unsigned long flags;
+	int i;
+	unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1443,55 +1440,64 @@ 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);
 
-	mmc_power_up(host);
-	sdio_reset(host);
-	mmc_go_idle(host);
+		if (freqs[i] >= host->f_min)
+			host->f_init = freqs[i];
+		else if (i && freqs[i-1] <= host->f_min)
+			goto out;
+		else
+			host->f_init = host->f_min;
 
-	mmc_send_if_cond(host, host->ocr_avail);
+		printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+		mmc_power_up(host);
+		sdio_reset(host);
+		mmc_go_idle(host);
 
-	/*
-	 * 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;
+		mmc_send_if_cond(host, host->ocr_avail);

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

Quoted 10 lines
drivers/mmc/core/core.c +10 -0
+
+		/*
+		 * 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))

In case of SDIO error, mmc_power_off() is getting called as part of mmc_detach_bus(). Shouldn't we power up the host before checking for SDMEM? Any comments?

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

Quoted 7 lines
drivers/mmc/core/core.c +7 -0
+					goto out_fail;
+
+				if (mmc_attach_sd(host, ocr))
+					mmc_power_off(host);
+			}
+			goto out;
+		}

Thanks,
Sahitya.
--
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

#8 lore raw

Hi Sahitya,

Quoted 12 lines · Sahitya Tummala

Hi Hein Tibosch,

Quoted 9 lines · Hein_Tibosch

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies. <cut>

drivers/mmc/core/core.c +4 -0
+		/*
+		 * First we search for SDIO...
+		 */
+		err = mmc_send_io_op_cond(host, 0, &ocr);
Quoted 9 lines

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

Quoted 5 lines
drivers/mmc/core/core.c +5 -0
+		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))

In case of SDIO error, mmc_power_off() is getting called as part of mmc_detach_bus(). Shouldn't we power up the host before checking for SDMEM? Any comments?

I think you should ask Michal Miroslaw, as he wrote the patch for the SD-combo (IO + mem)*

But yes, in case of a failure, mmc_attach_sdio() will call mmc_detach_bus() which in turn will call mmc_power_off()

And so it should be mmc_power_up() again before trying to detect SD memory.

Hein Tibosch

* http://permalink.gmane.org/gmane.linux.kernel.mmc/2052

#9 lore raw
Quoted 22 lines · Hein_Tibosch

Hi Sahitya,

Quoted 12 lines · Sahitya Tummala

Hi Hein Tibosch,

Quoted 9 lines · Hein_Tibosch

In the latest releases of the mmc driver, the freq during initialization is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency which-works-for-all, Pierre suggested to let the driver try several frequencies. <cut>

(quoted diff) +4 -0
+		/*
+		 * First we search for SDIO...
+		 */
+		err = mmc_send_io_op_cond(host, 0, &ocr);
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+		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))

In case of SDIO error, mmc_power_off() is getting called as part of mmc_detach_bus(). Shouldn't we power up the host before checking for SDMEM? Any comments?

Quoted 7 lines

I think you should ask Michal Miroslaw, as he wrote the patch for the SD-combo (IO + mem)*

But yes, in case of a failure, mmc_attach_sdio() will call mmc_detach_bus() which in turn will call mmc_power_off()

And so it should be mmc_power_up() again before trying to detect SD memory.

Yes, you are right! This explains some things...

Please try attached patch. (Not tested, but obvious enough.)

Best Regards,
Michał Mirosław

---

mmc: fix detection of memory part of SD-combo card with broken SDIO

In case of failure, mmc_attach_sdio() will power off the SD bus. Power it up and reinitialize before trying SD memory detection.

---
 drivers/mmc/core/core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/mmc/core/core.c +5 -0
index 6286898..32a4798 100644
@@ -1564,6 +1564,11 @@ void mmc_rescan(struct work_struct *work)
 				 * 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;
 
Signature · 1 line
1.7.2.3
#10 lore raw

Hi Michal,

Quoted 32 lines · Hein_Tibosch
Quoted 31 lines

Hi Sahitya,

Quoted 4 lines · Sahitya Tummala

Hi Hein Tibosch,

Quoted 1 line · Hein_Tibosch

In the latest releases of the mmc driver, the freq during

initialization

Quoted 2 lines
Quoted 2 lines

is set to a fixed 400 Khz. This was reportedly too fast for several users. As there doesn't seem to be an ideal frequency

which-works-for-all,

Quoted 6 lines
Quoted 6 lines

Pierre suggested to let the driver try several frequencies. <cut>

(quoted diff) +4 -0
+		/*
+		 * First we search for SDIO...
+		 */
+		err = mmc_send_io_op_cond(host, 0, &ocr);
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+		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))

In case of SDIO error, mmc_power_off() is getting called as part of mmc_detach_bus(). Shouldn't we power up the host before checking for SDMEM? Any comments?

I think you should ask Michal Miroslaw, as he wrote the patch for the SD-combo (IO + mem)*

But yes, in case of a failure, mmc_attach_sdio() will call mmc_detach_bus()
which in turn will call mmc_power_off()

Quoted 6 lines
Quoted 2 lines

And so it should be mmc_power_up() again before trying to detect SD memory.

Yes, you are right! This explains some things...

Please try attached patch. (Not tested, but obvious enough.)

I was looking at the code and figured out this issue. I don't have any SD-combo card to verify your patch.

Thanks
Sahitya.
--
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Quoted 36 lines

Best Regards,
Micha³ Miros³aw

---

mmc: fix detection of memory part of SD-combo card with broken SDIO

In case of failure, mmc_attach_sdio() will power off the SD bus. Power it up and reinitialize before trying SD memory detection.

---
 drivers/mmc/core/core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/mmc/core/core.c +5 -1
index 6286898..32a4798 100644
@@ -1564,6 +1564,11 @@ void mmc_rescan(struct work_struct *work)
 				 * 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;
 
--

1.7.2.3

#11 lore raw
Quoted 1 line · On Thu, Dec 02, 2010 at 09:05:54PM -0800, [email protected] wrote:

Hi Michal,

[...]

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines

In case of SDIO error, mmc_power_off() is getting called as part of mmc_detach_bus(). Shouldn't we power up the host before checking for SDMEM? Any comments?

I think you should ask Michal Miroslaw, as he wrote the patch for the SD-combo (IO + mem)*

But yes, in case of a failure, mmc_attach_sdio() will call mmc_detach_bus()
which in turn will call mmc_power_off()

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

And so it should be mmc_power_up() again before trying to detect SD memory.

Yes, you are right! This explains some things...

Please try attached patch. (Not tested, but obvious enough.)

I was looking at the code and figured out this issue. I don't have any SD-combo card to verify your patch.

You shouldn't be able to use this code path with normal SD-combo cards. This is relevant if you have a card that responds to CMD5 but then fails SDIO part initialization in some circumstances (i.e. time-outs).

Best Regards,
Michał Mirosław

#12 lore raw
In reply to Michal Miroslaw

Hi Michał, Sahitya,

Quoted 7 lines · Michal Miroslaw

mmc: fix detection of memory part of SD-combo card with broken SDIO

In case of failure, mmc_attach_sdio() will power off the SD bus. Power it up and reinitialize before trying SD memory detection.

Thanks, this looks correct to me too; I've pushed it to mmc-next for .38.

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