[PATCH 0/4] mmc: mmc: Improve reliability of HS200/HS400 selection

[PATCH 0/4] mmc: mmc: Improve reliability of HS200/HS400 selection

[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()

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


Regards
Adrian

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

[Adrian Hunter]

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 <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c793fda27321..2cef40ce9851 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -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;
 }
 
-- 
1.9.1

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

[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 <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 2cef40ce9851..14fb767ee8dd 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -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,
-- 
1.9.1

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

[Adrian Hunter]

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

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 14fb767ee8dd..5884c79346da 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -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;
-- 
1.9.1

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

[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 <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 5884c79346da..3a9a79ec4343 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -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)
-- 
1.9.1

Re: [PATCH 0/4] mmc: mmc: Improve reliability of HS200/HS400 selection

[Ulf Hansson]

On 28 October 2015 at 13:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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()
>
>  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

Re: [PATCH 0/4] mmc: mmc: Improve reliability of HS200/HS400 selection

[Adrian Hunter]

On 06/11/15 13:03, Ulf Hansson wrote:
> On 28 October 2015 at 13:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> 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()
>>
>>  drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 69 insertions(+), 24 deletions(-)
>>
> 

Thanks for looking at the patches.

> 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.

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

Yes, indeed.

Re: [PATCH 0/4] mmc: mmc: Improve reliability of HS200/HS400 selection

[Alim Akhtar]

Hello Adrian

On Wed, Oct 28, 2015 at 5:55 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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()
>
>  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 <alim.akhtar@samsung.com>


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



-- 
Regards,
Alim

Re: [PATCH 0/4] mmc: mmc: Improve reliability of HS200/HS400 selection

[Ulf Hansson]

On 6 November 2015 at 13:55, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 06/11/15 13:03, Ulf Hansson wrote:
>> On 28 October 2015 at 13:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> 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()
>>>
>>>  drivers/mmc/core/mmc.c | 93 +++++++++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 69 insertions(+), 24 deletions(-)
>>>
>>
>
> Thanks for looking at the patches.
>
>> 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.
>
>>
>> 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