MMC Core: Drop initialization frequency floor to 50kHz

Patch

commit 8dfd0374be84793360db7fff2e635d2cd3bbcb21

Author: Sascha Hauer <[email protected]>
Date:   Thu Apr 9 08:32:02 2009 +0200

MMC core: limit minimum initialization frequency to 400kHz

Was recently merged. This is too fast for at least one setup permutation - the one on my desk which through trial and error won't initialise at anything above ~350kHz (older Sandisk 256MB SD on atmel-mci).

To avoid a string of "just found card X which requires clock (current_clock - epsilon)" this patch drops the floor right down to 50kHz. This is about the slowest rate before which the discovery process takes a noticeable slowdown.

Signed-off-by Ben Nizette <[email protected]>

---

drivers/mmc/core/core.c +0 -4

index d84c880..6ee1931 100644
@@ -708,12 +708,13 @@ static void mmc_power_up(struct mmc_host *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;

50 kHz seems very low. I'd be more comfortable if we deviate from specified behaviour as little as possible, say 300 kHz. Has anyone checked what USB readers use?

And have you confirmed that the card is actually getting 350 kHz when it fails? Perhaps there is a bug that is causing it to actually run at a higher frequency.

Quoted 18 lines

drivers/mmc/core/core.c +3 -6

index d84c880..6ee1931 100644
@@ -708,12 +708,13 @@ static void mmc_power_up(struct mmc_host *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;
+	/*
+	 * Card discovery is typically done at the controller's minimum
+	 * allowable frequency but for some controllers this is minimum

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

Quoted 5 lines

drivers/mmc/core/core.c +4 -0

+	 * is unreasonably slow.  In that case we limit slow clock rate
+	 * to 50KHz.
+	 */
+	host->ios.clock = max(host->f_min, 50000);
 

I like getting that warning for too fast controllers. It makes it easier to spot possible problems.

Rgds

Was recently merged. This is too fast for at least one setup permutation - the one on my desk which through trial and error won't initialise at anything above ~350kHz (older Sandisk 256MB SD on atmel-mci).

Quoted 4 lines · Ben Nizette

Was recently merged. This is too fast for at least one setup permutation - the one on my desk which through trial and error won't initialise at anything above ~350kHz (older Sandisk 256MB SD on atmel-mci).

What kind of board is this? It sounds a bit like the CMD or DATA0 pull-up is too weak...

Haavard

Almost good: on my Kingston Elite Pro (50x) SD-card, initialization will only succeed if F <= 282258 Hz.

I checked some datasheets of several makes:

Delkin microSD min 0 max 400
Swissbit min 0 max 400
Sandisk 100 - 400
Toshiba 100 - 400
ST electronics 0 400
SD-specification: Issue continues clock in frequency range of 100KHz-400KHz
If the Average frequency of the SDCLK is less than 100KHz then the card may not be able to respond within the 1-second limit

So what about 100 or 150 Khz?

Pierre, thoughts? All my cards were borderline spec but Hein's got a mainstream, common card which seems to completely ignore the spec. Can we just take the few-hundred-uS hit and allow initialization down at the bottom end, say 100kHz?

Quoted 3 lines · On Sat, 2 Jan 2010 20:07:01 +1100 Ben Nizette <[email protected]> wrote:

Pierre, thoughts? All my cards were borderline spec but Hein's got a mainstream, common card which seems to completely ignore the spec. Can we just take the few-hundred-uS hit and allow initialization down at the bottom end, say 100kHz?

Broken cards seem to be all over the spectrum, so I wouldn't be suprised if you find ones that break if you go too low as well.

Quoted 9 lines · Pierre Ossman

Quoted 3 lines · On Sat, 2 Jan 2010 20:07:01 +1100 Ben Nizette<[email protected]> wrote:

Pierre, thoughts? All my cards were borderline spec but Hein's got a mainstream, common card which seems to completely ignore the spec. Can we just take the few-hundred-uS hit and allow initialization down at the bottom end, say 100kHz?

Broken cards seem to be all over the spectrum, so I wouldn't be suprised if you find ones that break if you go too low as well.

Yea good point, though given there might not even be a One Freq to Rule Them All, how about:

---8<---

From: Ben Nizette<[email protected]>
Subject: [PATCH v3] mmc: Make ID freq configurable

While the SD spec specifies a range of frequencies for the ID phase from 100-400kHz, not all cards play by the rules. This patch adds a Kconfig option to allow the user to tweak this to their card/host config, though the default of 250kHz should be fine for all spec-abiding cards and most others besides.

Signed-off-by Ben Nizette<[email protected]>

---

drivers/mmc/core/Kconfig +1 -0

index ab37a6d..a8e5003 100644
@@ -14,3 +14,15 @@ config MMC_UNSAFE_RESUME
           This option is usually just for embedded systems which use
           a MMC/SD card for rootfs. Most people should say N here.
 
+config MMC_ID_FREQ

Quoted 3 lines

Broken cards seem to be all over the spectrum, so I wouldn't be suprised if you find ones that break if you go too low as well.

Yea good point, though given there might not even be a One Freq to Rule Them All, how about:

---8<---

From: Ben Nizette <[email protected]>
Subject: [PATCH v3] mmc: Make ID freq configurable

While the SD spec specifies a range of frequencies for the ID phase from 100-400kHz, not all cards play by the rules. This patch adds a Kconfig option to allow the user to tweak this to their card/host config, though the default of 250kHz should be fine for all spec-abiding cards and most others besides.

Signed-off-by Ben Nizette <[email protected]>

Quoted 5 lines · On Sun, 3 Jan 2010 09:23:30 +1100 Ben Nizette <[email protected]> wrote:

Quoted 3 lines

Broken cards seem to be all over the spectrum, so I wouldn't be suprised if you find ones that break if you go too low as well.

Yea good point, though given there might not even be a One Freq to Rule Them All, how about:

Quoted 10 lines

---8<---

From: Ben Nizette <[email protected]>
Subject: [PATCH v3] mmc: Make ID freq configurable

While the SD spec specifies a range of frequencies for the ID phase from 100-400kHz, not all cards play by the rules. This patch adds a Kconfig option to allow the user to tweak this to their card/host config, though the default of 250kHz should be fine for all spec-abiding cards and most others besides.

Signed-off-by Ben Nizette <[email protected]>

This is not a good solution. We all use the same pool of cards so we should all be using the same init sequence. If there isn't a single frequency where all cards will work, then we'll have to make something more advanced where the kernel will try the init several times with different clocking.

<cut>
We all use the same pool of cards so we
should all be using the same init sequence. If there isn't a single frequency where all cards will work, then we'll have to make something more advanced where the kernel will try the init several times with different clocking.

The problem was that in some setups the mxcmmc driver allows frequencies as low as a few Hz and the card initialization took ages. Increasing the frequency to 400kHz seemed sensible since it's the value the spec describes. 50kHz should be fine as well.

Quoted 7 lines · Pierre Ossman

<cut>
We all use the same pool of cards so we
should all be using the same init sequence. If there isn't a single frequency where all cards will work, then we'll have to make something more advanced where the kernel will try the init several times with different clocking.

Quoted 5 lines · Sascha Hauer

The problem was that in some setups the mxcmmc driver allows frequencies as low as a few Hz and the card initialization took ages. Increasing the frequency to 400kHz seemed sensible since it's the value the spec describes. 50kHz should be fine as well.

I tried out several main-stream sd-cards and with the Atmel (AP7) controller they all initialize with F between 138 (=f_min) and 280 Khz.

Below a patch which tries mmc-initialization using several frequencies from an array 400, 300, 200 and 100. When it comes to 200 Khz, it works. The two failed attempts lasted about 10 ms together.

It looks like a hell-of-a-patch but that's because the indentation changes.

Note that the new mmc_host member f_init will be used later in power_restore and resume. As power_up always comes first, it should have a descend value by then.

But I still wonder, why not just keep using Sascha's patch, changing 400000 to 50000? Has anyone ever had problems with mmc-init on 50 Khz?

In my last mail the spaces in the patch were mistreated, sorry for that.

Below a patch which tries mmc-initialization using several frequencies from an array 400, 300, 200 and 100.

Signed-off-by Hein Tibosch <[email protected]>

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

drivers/mmc/core/core.c +45 -39

@@ -891,12 +891,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);

@@ -1041,6 +1036,8 @@ void mmc_rescan(struct work_struct *work
 		container_of(work, struct mmc_host, detect.work);
 	u32 ocr;
 	int err;
+	int i;
+	unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 
 	mmc_bus_get(host);
 

@@ -1070,46 +1067,55 @@ 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);
-	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);
+		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_power_off(host);
-		goto out;
-	}
+		mmc_send_if_cond(host, host->ocr_avail);
 
-	/*
-	 * ...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;
-	}
+		/*
+		 * First we search for SDIO...
+		 */
+		err = mmc_send_io_op_cond(host, 0, &ocr);
+		if (!err) {
+			if (mmc_attach_sdio(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;
-	}
+		/*
+		 * ...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;
+		}
 
-	mmc_release_host(host);
-	mmc_power_off(host);
+		/*
+		 * ...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;
+		}
 
+		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

@@ -119,6 +119,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;
 
 #define MMC_VDD_165_195		0x00000080	/* VDD voltage 1.65 - 1.95 */
 
 
--

Hi,

This looks like the best of the min_freq patches I've seen, and is implementing a suggestion of Pierre's. Should we take it?

- Chris.

Quoted 6 lines · Hein_Tibosch

Below a patch which tries mmc-initialization using several frequencies from an array 400, 300, 200 and 100.

Signed-off-by Hein Tibosch <[email protected]>

<cut>