mmc: mmc: Improve reliability of HS200/HS400 selection

PATCH In review Adrian Hunter 3 participants 9 messages Oct 28, 2015 – Nov 9, 2015 MMC, SD and SDIO

lore

Share & tools

lore

Adrian Hunter

author

2015-10-28 12:29 UTC #0 lore raw

cover mmc: mmc: Improve reliability of HS200/HS400 selection

Here are some patches that improve the reliability of selecting eMMC HS200 or HS400 mode.

Generally the improvement is to match the host controller timing setting with the card mode before sending commands.

Adrian Hunter (4):
mmc: mmc: Improve reliability of mmc_select_hs200()
mmc: mmc: Fix HS setting in mmc_select_hs400()
mmc: mmc: Move mmc_switch_status()
mmc: mmc: Improve reliability of mmc_select_hs400()

 drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 24 deletions(-)

Regards
Adrian

Adrian Hunter

author

2015-10-28 12:29 UTC #1 lore raw

PATCH 1/4 mmc: mmc: Improve reliability of mmc_select_hs200()

Currently mmc_select_hs200() uses __mmc_switch() which checks the success of the switch to HS200 mode using CMD13 (SEND_STATUS). The problem is that it does that using the timing settings of legacy mode. That is prone to error, not least because the timing parameters for legacy mode are tighter than the timing parameters for HS200 mode.

In the case when CMD13 polling is used (i.e. not MMC_CAP_WAIT_WHILE_BUSY) with the switch command, it must be assumed that using different modes on the card and host must work.

However in the case when CMD13 polling is not used
(i.e. MMC_CAP_WAIT_WHILE_BUSY) mmc_select_hs200() can be made more reliable by setting the host to the correct timing before sending CMD13.

This patch does that.

A complication is that the caller, mmc_select_timing(), will ignore a switch error (indicated by -EBADMSG), assume the old mode is valid and continue, so the old timing must be restored in that case.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/mmc.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

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

index c793fda27321..2cef40ce9851 100644
@@ -1219,6 +1219,8 @@ static void mmc_select_driver_type(struct mmc_card *card)
 static int mmc_select_hs200(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
+	bool send_status = true;
+	unsigned int old_timing;
 	int err = -EINVAL;
 	u8 val;

@@ -1234,6 +1236,9 @@ static int mmc_select_hs200(struct mmc_card *card)
 
 	mmc_select_driver_type(card);
 
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		send_status = false;
+
 	/*
 	 * Set the bus width(4 or 8) with host's support and
 	 * switch to HS200 mode if bus width is set successfully.

@@ -1245,11 +1250,25 @@ static int mmc_select_hs200(struct mmc_card *card)
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 				   EXT_CSD_HS_TIMING, val,
 				   card->ext_csd.generic_cmd6_time,
-				   true, true, true);
-		if (!err)
-			mmc_set_timing(host, MMC_TIMING_MMC_HS200);
+				   true, send_status, true);
+		if (err)
+			goto err;
+		old_timing = host->ios.timing;
+		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
+		if (!send_status) {
+			err = mmc_switch_status(card);
+			/*
+			 * mmc_select_timing() assumes timing has not changed if
+			 * it is a switch error.
+			 */
+			if (err == -EBADMSG)
+				mmc_set_timing(host, old_timing);
+		}
 	}
 err:
+	if (err)
+		pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
+		       __func__, err);
 	return err;
 }

Signature · 1 line

1.9.1

Adrian Hunter

author

2015-10-28 12:29 UTC #2 lore raw

PATCH 2/4 mmc: mmc: Fix HS setting in mmc_select_hs400()

In reply to Adrian Hunter

mmc_select_hs400() begins with the card and host in HS200 mode. Therefore, any commands sent to the card should use HS200 timing. It is incorrect to set the host to High Speed (HS) timing before sending the switch command. Doing so is unreliable because the timing parameters for HS mode are tighter than the timing parameters for HS200 mode. Thus the HS timings should be set only after the card has switched mode.

However, it is not unreasonable first to reduce the frequency to the HS mode frequency, which should make the switch command and subsequent CMD13 commands more reliable.

This patch does that.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/mmc.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

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

index 2cef40ce9851..14fb767ee8dd 100644
@@ -1043,6 +1043,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 static int mmc_select_hs400(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
+	unsigned int max_dtr;
 	int err = 0;
 	u8 val;

@@ -1053,13 +1054,11 @@ static int mmc_select_hs400(struct mmc_card *card)
 	      host->ios.bus_width == MMC_BUS_WIDTH_8))
 		return 0;
 
-	/*
-	 * Before switching to dual data rate operation for HS400,
-	 * it is required to convert from HS200 mode to HS mode.
-	 */
-	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-	mmc_set_bus_speed(card);
+	/* Reduce frequency to HS frequency */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(host, max_dtr);
 
+	/* Switch card to HS mode */
 	val = EXT_CSD_TIMING_HS |
 	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,

@@ -1072,6 +1071,9 @@ static int mmc_select_hs400(struct mmc_card *card)
 		return err;
 	}
 
+	/* Set host controller to HS timing */
+	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
+
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			 EXT_CSD_BUS_WIDTH,
 			 EXT_CSD_DDR_BUS_WIDTH_8,

Signature · 1 line

1.9.1

Adrian Hunter

author

2015-10-28 12:29 UTC #3 lore raw

PATCH 3/4 mmc: mmc: Move mmc_switch_status()

In reply to Adrian Hunter

Move the mmc_switch_status() function in preparation for calling it in mmc_select_hs400().

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/mmc.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

drivers/mmc/core/mmc.c +13 ~~-13~~

index 14fb767ee8dd..5884c79346da 100644
@@ -1040,6 +1040,19 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 	return err;
 }
 
+/* Caller must hold re-tuning */
+static int mmc_switch_status(struct mmc_card *card)
+{
+	u32 status;
+	int err;
+
+	err = mmc_send_status(card, &status);
+	if (err)
+		return err;
+
+	return mmc_switch_status_error(card->host, status);
+}
+
 static int mmc_select_hs400(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;

@@ -1107,19 +1120,6 @@ int mmc_hs200_to_hs400(struct mmc_card *card)
 	return mmc_select_hs400(card);
 }
 
-/* Caller must hold re-tuning */
-static int mmc_switch_status(struct mmc_card *card)
-{
-	u32 status;
-	int err;
-
-	err = mmc_send_status(card, &status);
-	if (err)
-		return err;
-
-	return mmc_switch_status_error(card->host, status);
-}
-
 int mmc_hs400_to_hs200(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;

Signature · 1 line

1.9.1

Adrian Hunter

author

2015-10-28 12:29 UTC #4 lore raw

PATCH 4/4 mmc: mmc: Improve reliability of mmc_select_hs400()

In reply to Adrian Hunter

mmc_select_hs400() calls __mmc_switch() which checks the switch is successful using CMD13 (SEND_STATUS). The problem is that it does that using the timing settings of the previous mode. That is prone to error, especially when switching from HS to HS400 because the timing parameters for HS mode are tighter than the timing parameters for HS400 mode.

In the case when CMD13 polling is used (i.e. not MMC_CAP_WAIT_WHILE_BUSY) with the switch command, it must be assumed that using different modes on the card and host must work.

However in the case when CMD13 polling is not used
(i.e. MMC_CAP_WAIT_WHILE_BUSY) mmc_select_hs400() can be made more reliable by setting the host to the correct timing before sending CMD13.

This patch does that.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/mmc.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

drivers/mmc/core/mmc.c +26 -2

index 5884c79346da..3a9a79ec4343 100644
@@ -1056,6 +1056,7 @@ static int mmc_switch_status(struct mmc_card *card)
 static int mmc_select_hs400(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
+	bool send_status = true;
 	unsigned int max_dtr;
 	int err = 0;
 	u8 val;

@@ -1067,6 +1068,9 @@ static int mmc_select_hs400(struct mmc_card *card)
 	      host->ios.bus_width == MMC_BUS_WIDTH_8))
 		return 0;
 
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		send_status = false;
+
 	/* Reduce frequency to HS frequency */
 	max_dtr = card->ext_csd.hs_max_dtr;
 	mmc_set_clock(host, max_dtr);

@@ -1077,7 +1081,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time,
-			   true, true, true);
+			   true, send_status, true);
 	if (err) {
 		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
 			mmc_hostname(host), err);

@@ -1087,6 +1091,13 @@ static int mmc_select_hs400(struct mmc_card *card)
 	/* Set host controller to HS timing */
 	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
 
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	/* Switch card to DDR */
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			 EXT_CSD_BUS_WIDTH,
 			 EXT_CSD_DDR_BUS_WIDTH_8,

@@ -1097,22 +1108,35 @@ static int mmc_select_hs400(struct mmc_card *card)
 		return err;
 	}
 
+	/* Switch card to HS400 */
 	val = EXT_CSD_TIMING_HS400 |
 	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time,
-			   true, true, true);
+			   true, send_status, true);
 	if (err) {
 		pr_err("%s: switch to hs400 failed, err:%d\n",
 			 mmc_hostname(host), err);
 		return err;
 	}
 
+	/* Set host controller to HS400 timing and frequency */
 	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
 	mmc_set_bus_speed(card);
 
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
 	return 0;
+
+out_err:
+	pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
+	       __func__, err);
+	return err;
 }
 
 int mmc_hs200_to_hs400(struct mmc_card *card)

Signature · 1 line

1.9.1

Ulf Hansson

2015-11-06 11:04 UTC #5 lore raw

In reply to Adrian Hunter

Quoted 9 lines · Adrian Hunter

Hi
Here are some patches that improve the reliability of selecting eMMC HS200 or HS400 mode.
Generally the improvement is to match the host controller timing setting with the card mode before sending commands.

Quoted 9 lines

Adrian Hunter (4):
mmc: mmc: Improve reliability of mmc_select_hs200()
mmc: mmc: Fix HS setting in mmc_select_hs400()
mmc: mmc: Move mmc_switch_status()
mmc: mmc: Improve reliability of mmc_select_hs400()
 drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 24 deletions(-)

Adrian, these patches looks good to me. I plan to queue them for 4.5 but perhaps those should even go as fixes!?

Additionally, it would be great if someone could help testing this on a few platforms.

Kind regards
Uffe

Adrian Hunter

author

2015-11-06 12:59 UTC #6 lore raw

Quoted 16 lines · Ulf Hansson

Quoted 15 lines · Adrian Hunter
Hi
Here are some patches that improve the reliability of selecting eMMC HS200 or HS400 mode.
Generally the improvement is to match the host controller timing setting with the card mode before sending commands.
Adrian Hunter (4):
mmc: mmc: Improve reliability of mmc_select_hs200()
mmc: mmc: Fix HS setting in mmc_select_hs400()
mmc: mmc: Move mmc_switch_status()
mmc: mmc: Improve reliability of mmc_select_hs400()

Quoted 4 lines

Quoted 3 lines

 drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 24 deletions(-)

Thanks for looking at the patches.

Quoted 2 lines

Adrian, these patches looks good to me. I plan to queue them for 4.5 but perhaps those should even go as fixes!?

4.5 is fine for me. A quick check seemed to show the patches apply cleanly back to 4.2, so I guess you could cc stable and then the patches get exposure in next before going to stable.

Quoted 3 lines

Additionally, it would be great if someone could help testing this on a few platforms.

Yes, indeed.

Ulf Hansson

2015-11-09 12:43 UTC #7 lore raw

Quoted 32 lines · Adrian Hunter

Quoted 16 lines · Ulf Hansson
Quoted 15 lines · Adrian Hunter
Hi
Here are some patches that improve the reliability of selecting eMMC HS200 or HS400 mode.
Generally the improvement is to match the host controller timing setting with the card mode before sending commands.
Adrian Hunter (4):
mmc: mmc: Improve reliability of mmc_select_hs200()
mmc: mmc: Fix HS setting in mmc_select_hs400()
mmc: mmc: Move mmc_switch_status()
mmc: mmc: Improve reliability of mmc_select_hs400()
Quoted 4 lines
Quoted 3 lines
 drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 24 deletions(-)
Thanks for looking at the patches.
Quoted 2 lines
Adrian, these patches looks good to me. I plan to queue them for 4.5 but perhaps those should even go as fixes!?
4.5 is fine for me. A quick check seemed to show the patches apply cleanly back to 4.2, so I guess you could cc stable and then the patches get exposure in next before going to stable.
Quoted 1 line

Quoted 5 lines

Quoted 2 lines
Additionally, it would be great if someone could help testing this on a few platforms.
Yes, indeed.

I queued up them for fixes and added a stable tag. They will be tested in linux-next for a while, before I decide to send a PR.
I also added Alim's tested by tag, thanks for helping out testing!

Thanks!

Kind regards
Uffe

Alim Akhtar

Tested-by on the v1 series

2015-11-06 16:33 UTC #8 lore raw

In reply to Adrian Hunter

Hello Adrian

Quoted 9 lines · Adrian Hunter

Hi
Here are some patches that improve the reliability of selecting eMMC HS200 or HS400 mode.
Generally the improvement is to match the host controller timing setting with the card mode before sending commands.

Quoted 9 lines

Adrian Hunter (4):
mmc: mmc: Improve reliability of mmc_select_hs200()
mmc: mmc: Fix HS setting in mmc_select_hs400()
mmc: mmc: Move mmc_switch_status()
mmc: mmc: Improve reliability of mmc_select_hs400()
 drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 24 deletions(-)

Have tested this series on dw_mmc host controller on exynos5800-peach board. HS400 and HS200 still works with these patches. Have not done a stress testing, but looks like across cold/warm reboot and across suspend/resume cycles, eMMC card still works in hs200/hs400 mode. Feel free to add:

Tested-by Alim Akhtar <[email protected]>

Quoted 7 lines

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

Signature · 2 lines

Regards,
Alim