[PATCH 0/9] mmc: core: Re-work CMD13 polling method for CMD6 for mmc

[PATCH 0/9] mmc: core: Re-work CMD13 polling method for CMD6 for mmc

[Ulf Hansson]

Several changes has been made for how and when to use CMD13 as a
polling-method, to find out when the mmc cards stops signaling busy after
sending a CMD6. This particularly concerns the cases when switching to new bus
speed modes, such as HS (high-speed), HS DDR, HS200, HS400 and HS400ES.

Currently CMD13 polling is *not* allowed for these cases, but this was recently
changed - and which did cause regressions for card detection for some
platforms.

A simple fix was made to solve these regressions, simply by using a default
CMD6 generic timeout of 500ms, as to avoid using the fall-back timeout in
__mmc_switch() of 10min. That greatly improve the situation and one could
consider the regressions as "solved".

However, this simple fix is still causing unnecessary long card initialization
times, especially for those mmc hosts that doesn't support HW busy detection
(using MMC_CAP_WAIT_WHILE_BUSY and/or implements the ->card_busy() host ops).

This because we wait a fixed amount of time (CMD6 generic timeout) to make sure
the card is done signaling busy, while in practice this happens a lot sooner.

A couple of tests for HS and HS DDR eMMC cards, shows the card only need a few
ms to complete the bus speed mode changes, thus it's far less than the CMD6
generic timeout.

To improve this behaviour and shorten the card initialization time, we need to
allow using CMD13 as polling method to find out when the card stops signaling
busy. Although, enabling CMD13 polling may also introduce other issues as
according to the JEDEC spec, it's not the recommended method. Especially it
mentions that CRC errors may be triggered when sending a CMD13 command during a
bus timing change.

To deal with these issues, we need to change from ignoring those CRC errors but
instead continue to treat the card as being busy and continue to poll with
CMD13. Perhaps this behaviour is one of reasons to why the earlier CMD13 polling
method didn't work out well.

This series requires extensive testing, please help with that!

I have currently tested it with HS and HS DDR eMMC cards, and for combinations
of an mmc hosts (mmci) supporting HW busy detection and not (local hacks in
mmci.c).


Ulf Hansson (9):
  mmc: core: Retry instead of ignore at CRC errors when polling for busy
  mmc: core: Remove redundant __mmc_send_status()
  mmc: core: Rename ignore_crc to retry_crc_err to reflect its purpose
  mmc: core: Enable __mmc_switch() to change bus speed timing for the
    host
  mmc: core: Allow CMD13 polling when switching to HS mode for mmc
  mmc: core: Update CMD13 polling policy when switch to HS DDR mode
  mmc: core: Allow CMD13 polling when switch to HS200 mode
  mmc: core: Allow CMD13 polling when switch to HS400 mode
  mmc: core: Allow CMD13 polling when switch to HS400ES mode

 drivers/mmc/core/core.c    |   2 +-
 drivers/mmc/core/mmc.c     | 156 ++++++++++++++-------------------------------
 drivers/mmc/core/mmc_ops.c |  45 +++++++------
 drivers/mmc/core/mmc_ops.h |   4 +-
 4 files changed, 77 insertions(+), 130 deletions(-)

-- 
1.9.1

[PATCH 1/9] mmc: core: Retry instead of ignore at CRC errors when polling for busy

[Ulf Hansson]

After a CMD6 command has been sent, the __mmc_switch() function might be
advised to poll the card for busy by using CMD13 and also by ignoring CRC
errors.

In the case of ignoring CRC errors, the mmc core tells the mmc host to also
ignore these errors via masking the MMC_RSP_CRC response flag. This seems
wrong, as it leads to that the mmc host could propagate an unreliable
response, instead of a proper error code.

What we really want, is not to ignore CRC errors but instead retry the
polling attempt. So, let's change this by treating a CRC error as the card
is still being busy and thus continue to run the polling loop.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 481bbdb..4773c56 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -503,10 +503,13 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		if (host->ops->card_busy) {
 			busy = host->ops->card_busy(host);
 		} else {
-			err = __mmc_send_status(card, &status, ignore_crc);
-			if (err)
+			err = mmc_send_status(card, &status);
+			if (ignore_crc && err == -EILSEQ)
+				busy = true;
+			else if (err)
 				return err;
-			busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
+			else
+				busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
 		}
 
 		/* Timeout if the device still remains busy. */
-- 
1.9.1

[PATCH 2/9] mmc: core: Remove redundant __mmc_send_status()

[Ulf Hansson]

There are only one users left which calls __mmc_send_status(). Moreover,
the ignore_crc parameter isn't being used, so let's just remove these
redundant parts.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 4773c56..0515748 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -54,8 +54,7 @@
 	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
-				    bool ignore_crc)
+int mmc_send_status(struct mmc_card *card, u32 *status)
 {
 	int err;
 	struct mmc_command cmd = {0};
@@ -67,8 +66,6 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
 	if (!mmc_host_is_spi(card->host))
 		cmd.arg = card->rca << 16;
 	cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
-	if (ignore_crc)
-		cmd.flags &= ~MMC_RSP_CRC;
 
 	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
 	if (err)
@@ -83,11 +80,6 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
 	return 0;
 }
 
-int mmc_send_status(struct mmc_card *card, u32 *status)
-{
-	return __mmc_send_status(card, status, false);
-}
-
 static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
 {
 	struct mmc_command cmd = {0};
-- 
1.9.1

[PATCH 3/9] mmc: core: Rename ignore_crc to retry_crc_err to reflect its purpose

[Ulf Hansson]

The ignore_crc parameter/variable name is used at a couple of places in the
mmc core. Let's rename it to retry_crc_err to reflect its new purpose.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 10 +++++-----
 drivers/mmc/core/mmc_ops.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 0515748..214e734 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -461,7 +461,7 @@ int mmc_switch_status(struct mmc_card *card)
 }
 
 static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
-			bool send_status, bool ignore_crc)
+			bool send_status, bool retry_crc_err)
 {
 	struct mmc_host *host = card->host;
 	int err;
@@ -496,7 +496,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 			busy = host->ops->card_busy(host);
 		} else {
 			err = mmc_send_status(card, &status);
-			if (ignore_crc && err == -EILSEQ)
+			if (retry_crc_err && err == -EILSEQ)
 				busy = true;
 			else if (err)
 				return err;
@@ -528,13 +528,13 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
  *                   timeout of zero implies maximum possible timeout
  *	@use_busy_signal: use the busy signal as response type
  *	@send_status: send status cmd to poll for busy
- *	@ignore_crc: ignore CRC errors when sending status cmd to poll for busy
+ *	@retry_crc_err: retry when CRC errors when polling with CMD13 for busy
  *
  *	Modifies the EXT_CSD register for selected card.
  */
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
-		bool ignore_crc)
+		bool retry_crc_err)
 {
 	struct mmc_host *host = card->host;
 	int err;
@@ -590,7 +590,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	}
 
 	/* Let's try to poll to find out when the command is completed. */
-	err = mmc_poll_for_busy(card, timeout_ms, send_status, ignore_crc);
+	err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);
 out:
 	mmc_retune_release(host);
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 7f6c0e9..9fccddb 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -30,7 +30,7 @@
 int mmc_switch_status(struct mmc_card *card);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
-		bool ignore_crc);
+		bool retry_crc_err);
 
 #endif
 
-- 
1.9.1

[PATCH 4/9] mmc: core: Enable __mmc_switch() to change bus speed timing for the host

[Ulf Hansson]

In cases when a speed mode change is requested for mmc cards, a CMD6 is
sent by calling __mmc_switch() during the card initialization. The CMD6
leads to the card entering a busy period. When that is completed, the host
must parse the CMD6 status to find out whether the change of the speed mode
succeeded.

To enable the mmc core to poll the card by using CMD13 to find out when the
busy period is completed, it's reasonable to make sure polling is done by
having the mmc host and the mmc card, being configured to operate at the
same selected bus speed timing.

Therefore, let's extend __mmc_switch() to take yet another parameter, which
allow its callers to update the bus speed timing of the mmc host. In this
way, __mmc_switch() also becomes capable of reading and validating the CMD6
status by sending a CMD13, in cases when that's desired.

If __mmc_switch() encounters a failure, we make sure to restores the old
bus speed timing for the mmc host, before propagating the error code.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c    |  2 +-
 drivers/mmc/core/mmc.c     | 18 +++++++++---------
 drivers/mmc/core/mmc_ops.c | 20 +++++++++++++++-----
 drivers/mmc/core/mmc_ops.h |  4 ++--
 4 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 50bb9a1..060f767 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -380,7 +380,7 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	mmc_retune_hold(card->host);
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-			EXT_CSD_BKOPS_START, 1, timeout,
+			EXT_CSD_BKOPS_START, 1, timeout, 0,
 			use_busy_signal, true, false);
 	if (err) {
 		pr_warn("%s: Error %d starting bkops\n",
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 9355366..cb1cf4e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1012,7 +1012,7 @@ static int mmc_select_hs(struct mmc_card *card)
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
-			   card->ext_csd.generic_cmd6_time,
+			   card->ext_csd.generic_cmd6_time, 0,
 			   true, false, true);
 	if (!err) {
 		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
@@ -1115,7 +1115,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	val = EXT_CSD_TIMING_HS;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
-			   card->ext_csd.generic_cmd6_time,
+			   card->ext_csd.generic_cmd6_time, 0,
 			   true, false, true);
 	if (err) {
 		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
@@ -1150,7 +1150,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	      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,
+			   card->ext_csd.generic_cmd6_time, 0,
 			   true, false, true);
 	if (err) {
 		pr_err("%s: switch to hs400 failed, err:%d\n",
@@ -1193,7 +1193,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	/* Switch HS400 to HS DDR */
 	val = EXT_CSD_TIMING_HS;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
-			   val, card->ext_csd.generic_cmd6_time,
+			   val, card->ext_csd.generic_cmd6_time, 0,
 			   true, false, true);
 	if (err)
 		goto out_err;
@@ -1207,7 +1207,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	/* Switch HS DDR to HS */
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
 			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
-			   true, false, true);
+			   0, true, false, true);
 	if (err)
 		goto out_err;
 
@@ -1221,7 +1221,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	val = EXT_CSD_TIMING_HS200 |
 	      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,
+			   val, card->ext_csd.generic_cmd6_time, 0,
 			   true, false, true);
 	if (err)
 		goto out_err;
@@ -1295,7 +1295,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	      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,
+			   card->ext_csd.generic_cmd6_time, 0,
 			   true, false, true);
 	if (err) {
 		pr_err("%s: switch to hs400es failed, err:%d\n",
@@ -1377,7 +1377,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 		      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,
+				   card->ext_csd.generic_cmd6_time, 0,
 				   true, false, true);
 		if (err)
 			goto err;
@@ -1841,7 +1841,7 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			EXT_CSD_POWER_OFF_NOTIFICATION,
-			notify_type, timeout, true, false, false);
+			notify_type, timeout, 0, true, false, false);
 	if (err)
 		pr_err("%s: Power Off Notification timed out, %u\n",
 		       mmc_hostname(card->host), timeout);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 214e734..ab77fc3 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -526,6 +526,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
  *	@value: value to program into EXT_CSD register
  *	@timeout_ms: timeout (ms) for operation performed by register write,
  *                   timeout of zero implies maximum possible timeout
+ *	@timing: new timing to change to
  *	@use_busy_signal: use the busy signal as response type
  *	@send_status: send status cmd to poll for busy
  *	@retry_crc_err: retry when CRC errors when polling with CMD13 for busy
@@ -533,13 +534,14 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
  *	Modifies the EXT_CSD register for selected card.
  */
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
-		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
-		bool retry_crc_err)
+		unsigned int timeout_ms, unsigned char timing,
+		bool use_busy_signal, bool send_status,	bool retry_crc_err)
 {
 	struct mmc_host *host = card->host;
 	int err;
 	struct mmc_command cmd = {0};
 	bool use_r1b_resp = use_busy_signal;
+	unsigned char old_timing = host->ios.timing;
 
 	mmc_retune_hold(host);
 
@@ -581,16 +583,24 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	if (!use_busy_signal)
 		goto out;
 
+	/* Switch to new timing before poll and check switch status. */
+	if (timing)
+		mmc_set_timing(host, timing);
+
 	/*If SPI or used HW busy detection above, then we don't need to poll. */
 	if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||
 		mmc_host_is_spi(host)) {
 		if (send_status)
 			err = mmc_switch_status(card);
-		goto out;
+		goto out_tim;
 	}
 
 	/* Let's try to poll to find out when the command is completed. */
 	err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);
+
+out_tim:
+	if (err && timing)
+		mmc_set_timing(host, old_timing);
 out:
 	mmc_retune_release(host);
 
@@ -600,8 +610,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms)
 {
-	return __mmc_switch(card, set, index, value, timeout_ms, true, true,
-				false);
+	return __mmc_switch(card, set, index, value, timeout_ms, 0,
+			true, true, false);
 }
 EXPORT_SYMBOL_GPL(mmc_switch);
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 9fccddb..761cb69 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -29,8 +29,8 @@
 int mmc_can_ext_csd(struct mmc_card *card);
 int mmc_switch_status(struct mmc_card *card);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
-		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
-		bool retry_crc_err);
+		unsigned int timeout_ms, unsigned char timing,
+		bool use_busy_signal, bool send_status,	bool retry_crc_err);
 
 #endif
 
-- 
1.9.1

[PATCH 5/9] mmc: core: Allow CMD13 polling when switching to HS mode for mmc

[Ulf Hansson]

In cases when the mmc host doesn't support HW busy detection, polling for a
card being busy by using CMD13 is beneficial. That is because, instead of
waiting a fixed amount of time, 500ms or the generic CMD6 time from
EXT_CSD, we find out a lot sooner when the card stops signaling busy. This
leads to a significant decreased total initialization time for the mmc
card.

However, to allow polling with CMD13 during a bus timing change operation,
such as switching to HS mode, we first need to update the mmc host's bus
timing before starting to poll. Deal with that, simply by providing
MMC_TIMING_MMC_HS as the timing parameter to __mmc_switch() from
mmc_select_hs().

By telling __mmc_switch() to allow polling with CMD13, also makes it
validate the CMD6 status, thus we can remove the corresponding checks.

When switching to HS400ES, the mmc_select_hs() function is called in one of
the intermediate steps. To still prevent CMD13 polling for HS400ES, let's
call the __mmc_switch() function in this path as it enables us to keep
using the existing method.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index cb1cf4e..15dd51c 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1012,13 +1012,8 @@ static int mmc_select_hs(struct mmc_card *card)
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
-			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
-	if (!err) {
-		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-		err = mmc_switch_status(card);
-	}
-
+			   card->ext_csd.generic_cmd6_time, MMC_TIMING_MMC_HS,
+			   true, true, true);
 	if (err)
 		pr_warn("%s: switch to high-speed failed, err:%d\n",
 			mmc_hostname(card->host), err);
@@ -1268,16 +1263,23 @@ static int mmc_select_hs400es(struct mmc_card *card)
 		goto out_err;
 
 	/* Switch card to HS mode */
-	err = mmc_select_hs(card);
-	if (err)
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
+			   card->ext_csd.generic_cmd6_time, 0,
+			   true, false, true);
+	if (err) {
+		pr_err("%s: switch to hs for hs400es failed, err:%d\n",
+			mmc_hostname(host), err);
 		goto out_err;
+	}
 
-	mmc_set_clock(host, card->ext_csd.hs_max_dtr);
-
+	mmc_set_timing(host, MMC_TIMING_MMC_HS);
 	err = mmc_switch_status(card);
 	if (err)
 		goto out_err;
 
+	mmc_set_clock(host, card->ext_csd.hs_max_dtr);
+
 	/* Switch card to DDR with strobe bit */
 	val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-- 
1.9.1

[PATCH 6/9] mmc: core: Update CMD13 polling policy when switch to HS DDR mode

[Ulf Hansson]

According to the JEDEC specification, during bus timing change operations
for mmc, sending a CMD13 could trigger CRC errors.

As switching to HS DDR mode indeed causes a bus timing change, polling with
CMD13 to detect card busy, may thus potentially trigger CRC errors.
Currently these errors are treated as the switch to HS DDR mode failed.

To improve this behaviour, let's instead tell __mmc_switch() to retry when
it encounters CRC errors during polling.

Moreover, when switching to HS DDR mode, let's make sure the CMD13 polling
is done by having the mmc host and the mmc card, being configured to
operate at the same selected bus speed timing. Fix this by providing
MMC_TIMING_MMC_DDR52 as the timing parameter to __mmc_switch().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 15dd51c..3268fcd 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1040,10 +1040,12 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 	ext_csd_bits = (bus_width == MMC_BUS_WIDTH_8) ?
 		EXT_CSD_DDR_BUS_WIDTH_8 : EXT_CSD_DDR_BUS_WIDTH_4;
 
-	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-			EXT_CSD_BUS_WIDTH,
-			ext_csd_bits,
-			card->ext_csd.generic_cmd6_time);
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+			   EXT_CSD_BUS_WIDTH,
+			   ext_csd_bits,
+			   card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_DDR52,
+			   true, true, true);
 	if (err) {
 		pr_err("%s: switch to bus width %d ddr failed\n",
 			mmc_hostname(host), 1 << bus_width);
@@ -1086,9 +1088,6 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 	if (err)
 		err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330);
 
-	if (!err)
-		mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
-
 	return err;
 }
 
-- 
1.9.1

[PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Ulf Hansson]

In cases when the mmc host doesn't support HW busy detection, polling for
busy by using CMD13 is beneficial. The reasons have already been explained
in earlier change logs.

To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
timing parameter to __mmc_switch(), which makes sure the mmc host and the
mmc card operates at the same bus timing during the polling.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3268fcd..0b26383 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1351,7 +1351,7 @@ 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;
-	unsigned int old_timing, old_signal_voltage;
+	unsigned int old_signal_voltage;
 	int err = -EINVAL;
 	u8 val;
 
@@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
 		      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, 0,
-				   true, false, true);
-		if (err)
-			goto err;
-		old_timing = host->ios.timing;
-		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
-
-		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);
+				   card->ext_csd.generic_cmd6_time,
+				   MMC_TIMING_MMC_HS200,
+				   true, true, true);
 	}
-err:
+
 	if (err) {
 		/* fall back to the old signal voltage, if fails report error */
 		if (__mmc_set_signal_voltage(host, old_signal_voltage))
-- 
1.9.1

[PATCH 8/9] mmc: core: Allow CMD13 polling when switch to HS400 mode

[Ulf Hansson]

In cases when the mmc host doesn't support HW busy detection, polling for
busy by using CMD13 is beneficial. The reasons have already been explained
in earlier change logs.

Moreover, when polling with CMD13 during bus timing changes, we should
retry instead of fail when we get CRC errors.

Switching from HS200 to HS400 and reverse includes several steps, where
each step changes the bus speed timing. Let's improve the behaviour during
these sequences, by allowing CMD13 polling for each of the step. Let's also
make sure the CMD13 polling becomes retried, while receiving a CRC error.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c | 87 +++++++++++++++-----------------------------------
 1 file changed, 26 insertions(+), 61 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 0b26383..24b9e72 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1094,8 +1094,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;
+	int err;
 	u8 val;
 
 	/*
@@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card)
 	      host->ios.bus_width == MMC_BUS_WIDTH_8))
 		return 0;
 
-	/* Switch card to HS mode */
-	val = EXT_CSD_TIMING_HS;
-	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-			   EXT_CSD_HS_TIMING, val,
-			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+	/*
+	 * Switch card to HS mode.
+	 * First reduce to the HS frequency as CMD13 are sent to poll for busy
+	 * and/or to validate the switch status.
+	 */
+	mmc_set_clock(host, card->ext_csd.hs_max_dtr);
+	err = mmc_select_hs(card);
 	if (err) {
 		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
 			mmc_hostname(host), err);
 		return err;
 	}
 
-	/* Set host controller to HS timing */
-	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-
-	/* Reduce frequency to HS frequency */
-	max_dtr = card->ext_csd.hs_max_dtr;
-	mmc_set_clock(host, max_dtr);
-
-	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,
-			 card->ext_csd.generic_cmd6_time);
+	/* Switch card to HS DDR */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+			   EXT_CSD_BUS_WIDTH,
+			   EXT_CSD_DDR_BUS_WIDTH_8,
+			   card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_DDR52,
+			   true, true, true);
 	if (err) {
 		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card)
 	      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, 0,
-			   true, false, true);
+			   card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_HS400,
+			   true, true, 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);
-
-	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)
@@ -1187,27 +1167,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	/* Switch HS400 to HS DDR */
 	val = EXT_CSD_TIMING_HS;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
-			   val, card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
-	if (err)
-		goto out_err;
-
-	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
-
-	err = mmc_switch_status(card);
+			   val, card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_DDR52,
+			   true, true, true);
 	if (err)
 		goto out_err;
 
 	/* Switch HS DDR to HS */
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
 			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
-			   0, true, false, true);
-	if (err)
-		goto out_err;
-
-	mmc_set_timing(host, MMC_TIMING_MMC_HS);
-
-	err = mmc_switch_status(card);
+			   MMC_TIMING_MMC_HS,
+			   true, true, true);
 	if (err)
 		goto out_err;
 
@@ -1215,14 +1185,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	val = EXT_CSD_TIMING_HS200 |
 	      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, 0,
-			   true, false, true);
-	if (err)
-		goto out_err;
-
-	mmc_set_timing(host, MMC_TIMING_MMC_HS200);
-
-	err = mmc_switch_status(card);
+			   val, card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_HS200,
+			   true, true, true);
 	if (err)
 		goto out_err;
 
-- 
1.9.1

[PATCH 9/9] mmc: core: Allow CMD13 polling when switch to HS400ES mode

[Ulf Hansson]

In cases when the mmc host doesn't support HW busy detection, polling for
busy by using CMD13 is beneficial. The reasons have already been explained
in earlier change logs.

Moreover, when polling with CMD13 during bus timing changes, we should
retry instead of fail when we get CRC errors.

Switching to HS400ES includes several steps, where each step changes the
bus speed timing. Let's improve the behaviour during these sequences, by
allowing CMD13 polling for each of the step. Let's also make sure the CMD13
polling becomes retried, while receiving a CRC error.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 24b9e72..b6f0035 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1227,31 +1227,24 @@ static int mmc_select_hs400es(struct mmc_card *card)
 		goto out_err;
 
 	/* Switch card to HS mode */
-	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
-			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+	err = mmc_select_hs(card);
 	if (err) {
 		pr_err("%s: switch to hs for hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
 		goto out_err;
 	}
 
-	mmc_set_timing(host, MMC_TIMING_MMC_HS);
-	err = mmc_switch_status(card);
-	if (err)
-		goto out_err;
-
 	mmc_set_clock(host, card->ext_csd.hs_max_dtr);
 
-	/* Switch card to DDR with strobe bit */
+	/* Switch card to HS DDR with strobe bit */
 	val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
-	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-			 EXT_CSD_BUS_WIDTH,
-			 val,
-			 card->ext_csd.generic_cmd6_time);
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+			   EXT_CSD_BUS_WIDTH, val,
+			   card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_DDR52,
+			   true, true, true);
 	if (err) {
-		pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
+		pr_err("%s: switch to hs ddr for hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
 		goto out_err;
 	}
@@ -1261,26 +1254,20 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	      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, 0,
-			   true, false, true);
+			   card->ext_csd.generic_cmd6_time,
+			   MMC_TIMING_MMC_HS400,
+			   true, true, true);
 	if (err) {
 		pr_err("%s: switch to hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
 		goto out_err;
 	}
 
-	/* Set host controller to HS400 timing and frequency */
-	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
-
 	/* Controller enable enhanced strobe function */
 	host->ios.enhanced_strobe = true;
 	if (host->ops->hs400_enhanced_strobe)
 		host->ops->hs400_enhanced_strobe(host, &host->ios);
 
-	err = mmc_switch_status(card);
-	if (err)
-		goto out_err;
-
 	return 0;
 
 out_err:
-- 
1.9.1

Re: [PATCH 0/9] mmc: core: Re-work CMD13 polling method for CMD6 for mmc

[Linus Walleij]

On Wed, Nov 16, 2016 at 11:51 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> To improve this behaviour and shorten the card initialization time, we need to
> allow using CMD13 as polling method to find out when the card stops signaling
> busy. Although, enabling CMD13 polling may also introduce other issues as
> according to the JEDEC spec, it's not the recommended method. Especially it
> mentions that CRC errors may be triggered when sending a CMD13 command during a
> bus timing change.
>
> To deal with these issues, we need to change from ignoring those CRC errors but
> instead continue to treat the card as being busy and continue to poll with
> CMD13. Perhaps this behaviour is one of reasons to why the earlier CMD13 polling
> method didn't work out well.
>
> This series requires extensive testing, please help with that!

I cloned the MMC tree "next" and applied the patches on top.

(I guess if you want wider testing it's good to publish a branch,
but I'd recommend just putting it into linux-next, it seems solid
to me.)

Tested on my previously problematic APQ8060 Dragonboard, it switches
to highspeed and works like a charm! Now the eMMC comes up smacky
right after registering the host:

[    1.955105] mmci-pl18x 12400000.sdcc: mmc0: PL180 manf 51 rev0 at
0x12400000 irq 197,0 (pio)
[    1.955149] mmci-pl18x 12400000.sdcc: DMA channels RX none, TX none
[    1.964202] mmci-pl18x 12400000.sdcc: Voltage switch failed
[    2.031744] mmci-pl18x 12180000.sdcc: Got CD GPIO
[    2.031829] mmci-pl18x 12180000.sdcc: Got WP GPIO
[    2.040263] mmci-pl18x 12180000.sdcc: DMA channels RX none, TX none
[    2.094161] mmc0: new high speed MMC card at address 0001
[    2.095590] mmcblk0: mmc0:0001 SEM04G 3.69 GiB
[    2.120905] mmcblk0boot0: mmc0:0001 SEM04G partition 1 1.00 MiB
[    2.133068] mmcblk0boot1: mmc0:0001 SEM04G partition 2 1.00 MiB
[    2.139657] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 128 KiB
[    2.158004]  mmcblk0: p1 p2 p3 p4 < p5 p6 p7 p8 p9 p10 p11 p12 p13
p14 p15 p16 p17 p18 p19 p20 p21 >

Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Adrian Hunter]

On 16/11/16 12:51, Ulf Hansson wrote:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
> 
> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
> timing parameter to __mmc_switch(), which makes sure the mmc host and the
> mmc card operates at the same bus timing during the polling.

I have reports of cases where CMD13 always gives CRC errors after switch
to HS200.  Currently we are assuming the low frequency should mean that
won't happen, but it does in some cases.  That is not entirely surprising
since HS200 needs tuning at the final operating frequency.

What I would like to do for hosts that support busy waiting or DAT0 polling
(i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
errors from the CMD13 that checks the switch status.  The main reason for
doing that is that we really expect the switch to succeed and, given HS200
tuning requirement, the CRC error is not a reliable means of determining
that it hasn't.

With the existing code I would just change the err check:


diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3268fcd3378d..c8862c58b60b 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
 
 		err = mmc_switch_status(card);
 		/*
+		 * For HS200, CRC errors are not a reliable way to know the
+		 * switch failed. If there really is a problem, we would expect
+		 * tuning will fail and the result ends up the same.
+		 */
+		if (err == -EILSEQ)
+			err = 0;
+		/*
 		 * mmc_select_timing() assumes timing has not changed if
 		 * it is a switch error.
 		 */


Then to support polling:


diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c8862c58b60b..66d8d57ae2fb 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
 	unsigned int old_timing, old_signal_voltage;
+	bool send_status;
 	int err = -EINVAL;
 	u8 val;
 
@@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
 	 * switch to HS200 mode if bus width is set successfully.
 	 */
 	err = mmc_select_bus_width(card);
-	if (err > 0) {
-		val = EXT_CSD_TIMING_HS200 |
-		      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, 0,
-				   true, false, true);
-		if (err)
-			goto err;
-		old_timing = host->ios.timing;
-		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
+	if (err <= 0)
+		goto err;
+
+	send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
+		      !host->ops->card_busy;
+	old_timing = host->ios.timing;
+
+	val = EXT_CSD_TIMING_HS200 |
+	      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,
+			   MMC_TIMING_MMC_HS200, true, send_status, true);
 
+	if (!err && !send_status) {
 		err = mmc_switch_status(card);
 		/*
 		 * For HS200, CRC errors are not a reliable way to know the



Thoughts?

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Ulf Hansson]

On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/11/16 12:51, Ulf Hansson wrote:
>> In cases when the mmc host doesn't support HW busy detection, polling for
>> busy by using CMD13 is beneficial. The reasons have already been explained
>> in earlier change logs.
>>
>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>> mmc card operates at the same bus timing during the polling.
>
> I have reports of cases where CMD13 always gives CRC errors after switch
> to HS200.  Currently we are assuming the low frequency should mean that
> won't happen, but it does in some cases.  That is not entirely surprising
> since HS200 needs tuning at the final operating frequency.

>From a logical point of view and if tuning is needed also for the CMD
line, this somehow make sense.

However, this is *not* how the JEDEC spec describes the HS200 switch
sequence. It is clearly stated that the host should validate the CM6
status via sending a CMD13 command, *before* performing tuning.

Could it be that the observations about the CRC errors, is related to
a controller/driver issue and not a card issue?

>
> What I would like to do for hosts that support busy waiting or DAT0 polling
> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
> errors from the CMD13 that checks the switch status.  The main reason for
> doing that is that we really expect the switch to succeed and, given HS200
> tuning requirement, the CRC error is not a reliable means of determining
> that it hasn't.

Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
work, as tuning is needed.

So, to me that means we need to fall-back to use the generic CMD6
timeout instead (when HW busy detection isn't supported).

>
> With the existing code I would just change the err check:
>
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 3268fcd3378d..c8862c58b60b 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>
>                 err = mmc_switch_status(card);
>                 /*
> +                * For HS200, CRC errors are not a reliable way to know the
> +                * switch failed. If there really is a problem, we would expect
> +                * tuning will fail and the result ends up the same.
> +                */
> +               if (err == -EILSEQ)
> +                       err = 0;
> +               /*

I don't think ignoring CRC errors is reliable when verifying the CMD6
status. My point is that we must not parse the status, in case of CRC
errors as it can't be trusted.

So, then we might as well just ignore validating the CMD6 status
altogether, but instead always move on to the tuning and hope that it
succeeds.

I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
mode isn't enabled, so we could check that. Anyway, we should fail
with the tuning if the earlier HS200 switch also failed. Don't you
think?

>                  * mmc_select_timing() assumes timing has not changed if
>                  * it is a switch error.
>                  */
>
>
> Then to support polling:
>
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c8862c58b60b..66d8d57ae2fb 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>  {
>         struct mmc_host *host = card->host;
>         unsigned int old_timing, old_signal_voltage;
> +       bool send_status;
>         int err = -EINVAL;
>         u8 val;
>
> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>          * switch to HS200 mode if bus width is set successfully.
>          */
>         err = mmc_select_bus_width(card);
> -       if (err > 0) {
> -               val = EXT_CSD_TIMING_HS200 |
> -                     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, 0,
> -                                  true, false, true);
> -               if (err)
> -                       goto err;
> -               old_timing = host->ios.timing;
> -               mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> +       if (err <= 0)
> +               goto err;
> +
> +       send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
> +                     !host->ops->card_busy;
> +       old_timing = host->ios.timing;
> +
> +       val = EXT_CSD_TIMING_HS200 |
> +             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,
> +                          MMC_TIMING_MMC_HS200, true, send_status, true);
>
> +       if (!err && !send_status) {
>                 err = mmc_switch_status(card);
>                 /*
>                  * For HS200, CRC errors are not a reliable way to know the
>
>
>
> Thoughts?

Well, I think the main problem is that if we have cards that returns
CRC errors even after the HS200 switch, then we can't use polling, as
we can't trust to parse the CMD6 status.

Kind regards
Uffe

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Shawn Lin]

Hi Ulf,

在 2016/11/16 18:51, Ulf Hansson 写道:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
>
> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
> timing parameter to __mmc_switch(), which makes sure the mmc host and the
> mmc card operates at the same bus timing during the polling.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c | 21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 3268fcd..0b26383 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1351,7 +1351,7 @@ 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;
> -	unsigned int old_timing, old_signal_voltage;
> +	unsigned int old_signal_voltage;
>  	int err = -EINVAL;
>  	u8 val;
>
> @@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>  		      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, 0,
> -				   true, false, true);
> -		if (err)
> -			goto err;
> -		old_timing = host->ios.timing;
> -		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> -
> -		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);
> +				   card->ext_csd.generic_cmd6_time,
> +				   MMC_TIMING_MMC_HS200,
> +				   true, true, true);

I was finding a failure from the test last night after applying these 
patches and using HS200 only.

It seems like the controller(sdhci-of-arasan,5.1) continuously generate
response timeout for checking CMD13.. Per TRM, response timeout could
means the device didn't ack the CMD13 or the controller was failing to
latch the response..

The eMMC part number is KLMBG2JENB-B041. I will use two boards to check
if it was indeed related to these patches.

>  	}
> -err:
> +
>  	if (err) {
>  		/* fall back to the old signal voltage, if fails report error */
>  		if (__mmc_set_signal_voltage(host, old_signal_voltage))
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Adrian Hunter]

On 17/11/16 17:02, Ulf Hansson wrote:
> On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 16/11/16 12:51, Ulf Hansson wrote:
>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>> in earlier change logs.
>>>
>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>> mmc card operates at the same bus timing during the polling.
>>
>> I have reports of cases where CMD13 always gives CRC errors after switch
>> to HS200.  Currently we are assuming the low frequency should mean that
>> won't happen, but it does in some cases.  That is not entirely surprising
>> since HS200 needs tuning at the final operating frequency.
> 
>>From a logical point of view and if tuning is needed also for the CMD
> line, this somehow make sense.
> 
> However, this is *not* how the JEDEC spec describes the HS200 switch
> sequence. It is clearly stated that the host should validate the CM6
> status via sending a CMD13 command, *before* performing tuning.

I agree, it seems not to be following spec.

> 
> Could it be that the observations about the CRC errors, is related to
> a controller/driver issue and not a card issue?

I don't know what causes the problem (and I have a sneaking suspicion that
if vendors configured / designed their boards correctly, it wouldn't
happen).  However, while some cards have better signal characteristics than
others, tuning is a host controller issue - the card doesn't care.

> 
>>
>> What I would like to do for hosts that support busy waiting or DAT0 polling
>> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
>> errors from the CMD13 that checks the switch status.  The main reason for
>> doing that is that we really expect the switch to succeed and, given HS200
>> tuning requirement, the CRC error is not a reliable means of determining
>> that it hasn't.
> 
> Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
> work, as tuning is needed.

I would assume that vendors integrate a working combination of eMMC and host
controller, so if polling is the only option, then we could assume it will work.

> 
> So, to me that means we need to fall-back to use the generic CMD6
> timeout instead (when HW busy detection isn't supported).

Or, in the ignore_crc/retry_err_crc case, return -EILSEQ instead -ETIMEOUT,
and catch and ignore the error in the calling code.  Then you get polling if
it works, otherwise getting CRC errors until timeout.

> 
>>
>> With the existing code I would just change the err check:
>>
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 3268fcd3378d..c8862c58b60b 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>>
>>                 err = mmc_switch_status(card);
>>                 /*
>> +                * For HS200, CRC errors are not a reliable way to know the
>> +                * switch failed. If there really is a problem, we would expect
>> +                * tuning will fail and the result ends up the same.
>> +                */
>> +               if (err == -EILSEQ)
>> +                       err = 0;
>> +               /*
> 
> I don't think ignoring CRC errors is reliable when verifying the CMD6
> status. My point is that we must not parse the status, in case of CRC
> errors as it can't be trusted.

I agree, but mmc_switch_status() doesn't look at the response if there is an
error.

> 
> So, then we might as well just ignore validating the CMD6 status
> altogether, but instead always move on to the tuning and hope that it
> succeeds.

That is a possibility, but it seemed to me that is was worth checking for
all the users where it does work. i.e if CMD13 does not give a CRC error
then validate the response, and if CMD13 does give a CRC error then ignore
the response and keep going anyway.

> 
> I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
> mode isn't enabled, so we could check that. Anyway, we should fail
> with the tuning if the earlier HS200 switch also failed. Don't you
> think?

Yes CMD21 is an illegal command if the mode is not HS200.  The card should
set ILLEGAL_COMMAND but also not respond i.e there will be a timeout error.
That could cause a long delay before tuning finally fails.  The only way to
mitigate that would be to make ignoring the CRC error a host-specific option
(e.g. MMC_CAP_... flag).  Arguably, if the switch fails, the mode is broken
and should not have been allowed in the first place.

> 
>>                  * mmc_select_timing() assumes timing has not changed if
>>                  * it is a switch error.
>>                  */
>>
>>
>> Then to support polling:
>>
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index c8862c58b60b..66d8d57ae2fb 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>  {
>>         struct mmc_host *host = card->host;
>>         unsigned int old_timing, old_signal_voltage;
>> +       bool send_status;
>>         int err = -EINVAL;
>>         u8 val;
>>
>> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>>          * switch to HS200 mode if bus width is set successfully.
>>          */
>>         err = mmc_select_bus_width(card);
>> -       if (err > 0) {
>> -               val = EXT_CSD_TIMING_HS200 |
>> -                     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, 0,
>> -                                  true, false, true);
>> -               if (err)
>> -                       goto err;
>> -               old_timing = host->ios.timing;
>> -               mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>> +       if (err <= 0)
>> +               goto err;
>> +
>> +       send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>> +                     !host->ops->card_busy;
>> +       old_timing = host->ios.timing;
>> +
>> +       val = EXT_CSD_TIMING_HS200 |
>> +             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,
>> +                          MMC_TIMING_MMC_HS200, true, send_status, true);
>>
>> +       if (!err && !send_status) {
>>                 err = mmc_switch_status(card);
>>                 /*
>>                  * For HS200, CRC errors are not a reliable way to know the
>>
>>
>>
>> Thoughts?
> 
> Well, I think the main problem is that if we have cards that returns
> CRC errors even after the HS200 switch, then we can't use polling, as
> we can't trust to parse the CMD6 status.

As I wrote above, if there is no option but polling then we could expect it
to work.  And if CMD13 does not give a CRC error then we can validate the
response, only ignoring it if there is a CRC error.

I should point out that retrying CMD13 will clear the error bits in the
status so there is no point retrying when checking for the SWITCH_ERROR bit.
i.e. we need a version of __switch_send_status() that sets retries to zero.

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Ulf Hansson]

Hi Shawn,

On 18 November 2016 at 09:05, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi Ulf,
>
>
> 在 2016/11/16 18:51, Ulf Hansson 写道:
>>
>> In cases when the mmc host doesn't support HW busy detection, polling for
>> busy by using CMD13 is beneficial. The reasons have already been explained
>> in earlier change logs.
>>
>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>> mmc card operates at the same bus timing during the polling.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc.c | 21 +++++----------------
>>  1 file changed, 5 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 3268fcd..0b26383 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1351,7 +1351,7 @@ 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;
>> -       unsigned int old_timing, old_signal_voltage;
>> +       unsigned int old_signal_voltage;
>>         int err = -EINVAL;
>>         u8 val;
>>
>> @@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>>                       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, 0,
>> -                                  true, false, true);
>> -               if (err)
>> -                       goto err;
>> -               old_timing = host->ios.timing;
>> -               mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>> -
>> -               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);
>> +                                  card->ext_csd.generic_cmd6_time,
>> +                                  MMC_TIMING_MMC_HS200,
>> +                                  true, true, true);
>
>
> I was finding a failure from the test last night after applying these
> patches and using HS200 only.
>
> It seems like the controller(sdhci-of-arasan,5.1) continuously generate
> response timeout for checking CMD13.. Per TRM, response timeout could
> means the device didn't ack the CMD13 or the controller was failing to
> latch the response..

Did you make any changes to the sdchi driver while testing this?

What I am wondering is whether you tested this with an implemented
->card_busy() host ops or not, as sdhci has a default implementation
of it.

BTW, some sdhci hosts have lately shown problem [1] with sdhci's
default ->card_busy() host ops.

>
> The eMMC part number is KLMBG2JENB-B041. I will use two boards to check
> if it was indeed related to these patches.
>
>>         }
>> -err:
>> +
>>         if (err) {
>>                 /* fall back to the old signal voltage, if fails report
>> error */
>>                 if (__mmc_set_signal_voltage(host, old_signal_voltage))
>>
>
>
> --
> Best Regards
> Shawn Lin
>

Thanks for helping out with testing etc, I really appreciate it!

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/9429299/

Re: [PATCH 8/9] mmc: core: Allow CMD13 polling when switch to HS400 mode

[Adrian Hunter]

On 16/11/16 12:51, Ulf Hansson wrote:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
> 
> Moreover, when polling with CMD13 during bus timing changes, we should
> retry instead of fail when we get CRC errors.
> 
> Switching from HS200 to HS400 and reverse includes several steps, where
> each step changes the bus speed timing. Let's improve the behaviour during
> these sequences, by allowing CMD13 polling for each of the step. Let's also
> make sure the CMD13 polling becomes retried, while receiving a CRC error.

I don't know we need to try to get polling for HS400.  The support of HS400
suggests a relatively sophisticated host controller which really should
support busy waiting as well.

> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c | 87 +++++++++++++++-----------------------------------
>  1 file changed, 26 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 0b26383..24b9e72 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1094,8 +1094,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;
> +	int err;
>  	u8 val;
>  
>  	/*
> @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>  		return 0;
>  
> -	/* Switch card to HS mode */
> -	val = EXT_CSD_TIMING_HS;
> -	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -			   EXT_CSD_HS_TIMING, val,
> -			   card->ext_csd.generic_cmd6_time, 0,
> -			   true, false, true);
> +	/*
> +	 * Switch card to HS mode.
> +	 * First reduce to the HS frequency as CMD13 are sent to poll for busy
> +	 * and/or to validate the switch status.
> +	 */
> +	mmc_set_clock(host, card->ext_csd.hs_max_dtr);

That was moved by commit 649c6059d2371fef886a8f967e21416204723d63
("mmc: mmc: Fix HS switch failure in mmc_select_hs400()") i.e. if you put it
back I would expect the gru board problem to reappear.

> +	err = mmc_select_hs(card);
>  	if (err) {
>  		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
>  			mmc_hostname(host), err);
>  		return err;
>  	}
>  
> -	/* Set host controller to HS timing */
> -	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
> -
> -	/* Reduce frequency to HS frequency */
> -	max_dtr = card->ext_csd.hs_max_dtr;
> -	mmc_set_clock(host, max_dtr);
> -
> -	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,
> -			 card->ext_csd.generic_cmd6_time);
> +	/* Switch card to HS DDR */
> +	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +			   EXT_CSD_BUS_WIDTH,
> +			   EXT_CSD_DDR_BUS_WIDTH_8,
> +			   card->ext_csd.generic_cmd6_time,
> +			   MMC_TIMING_MMC_DDR52,
> +			   true, true, true);
>  	if (err) {
>  		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>  			mmc_hostname(host), err);
> @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	      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, 0,
> -			   true, false, true);
> +			   card->ext_csd.generic_cmd6_time,
> +			   MMC_TIMING_MMC_HS400,
> +			   true, true, true);

This is not exactly right.  Tuning has been done at the HS400 operating
frequency.  That is why we set the bus speed before sending any more
commands. i.e. mmc_switch_status(card) is below mmc_set_bus_speed(card)

>  	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);
> -
> -	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)
> @@ -1187,27 +1167,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>  	/* Switch HS400 to HS DDR */
>  	val = EXT_CSD_TIMING_HS;
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
> -			   val, card->ext_csd.generic_cmd6_time, 0,
> -			   true, false, true);
> -	if (err)
> -		goto out_err;
> -
> -	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
> -
> -	err = mmc_switch_status(card);
> +			   val, card->ext_csd.generic_cmd6_time,
> +			   MMC_TIMING_MMC_DDR52,
> +			   true, true, true);
>  	if (err)
>  		goto out_err;
>  
>  	/* Switch HS DDR to HS */
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
>  			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
> -			   0, true, false, true);
> -	if (err)
> -		goto out_err;
> -
> -	mmc_set_timing(host, MMC_TIMING_MMC_HS);
> -
> -	err = mmc_switch_status(card);
> +			   MMC_TIMING_MMC_HS,
> +			   true, true, true);
>  	if (err)
>  		goto out_err;
>  
> @@ -1215,14 +1185,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>  	val = EXT_CSD_TIMING_HS200 |
>  	      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, 0,
> -			   true, false, true);
> -	if (err)
> -		goto out_err;
> -
> -	mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> -
> -	err = mmc_switch_status(card);
> +			   val, card->ext_csd.generic_cmd6_time,
> +			   MMC_TIMING_MMC_HS200,
> +			   true, true, true);
>  	if (err)
>  		goto out_err;
>  
> 

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Ulf Hansson]

On 18 November 2016 at 10:30, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 17/11/16 17:02, Ulf Hansson wrote:
>> On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 16/11/16 12:51, Ulf Hansson wrote:
>>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>>> in earlier change logs.
>>>>
>>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>>> mmc card operates at the same bus timing during the polling.
>>>
>>> I have reports of cases where CMD13 always gives CRC errors after switch
>>> to HS200.  Currently we are assuming the low frequency should mean that
>>> won't happen, but it does in some cases.  That is not entirely surprising
>>> since HS200 needs tuning at the final operating frequency.
>>
>>>From a logical point of view and if tuning is needed also for the CMD
>> line, this somehow make sense.
>>
>> However, this is *not* how the JEDEC spec describes the HS200 switch
>> sequence. It is clearly stated that the host should validate the CM6
>> status via sending a CMD13 command, *before* performing tuning.
>
> I agree, it seems not to be following spec.
>
>>
>> Could it be that the observations about the CRC errors, is related to
>> a controller/driver issue and not a card issue?
>
> I don't know what causes the problem (and I have a sneaking suspicion that
> if vendors configured / designed their boards correctly, it wouldn't
> happen).  However, while some cards have better signal characteristics than
> others, tuning is a host controller issue - the card doesn't care.
>
>>
>>>
>>> What I would like to do for hosts that support busy waiting or DAT0 polling
>>> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
>>> errors from the CMD13 that checks the switch status.  The main reason for
>>> doing that is that we really expect the switch to succeed and, given HS200
>>> tuning requirement, the CRC error is not a reliable means of determining
>>> that it hasn't.
>>
>> Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
>> work, as tuning is needed.
>
> I would assume that vendors integrate a working combination of eMMC and host
> controller, so if polling is the only option, then we could assume it will work.
>
>>
>> So, to me that means we need to fall-back to use the generic CMD6
>> timeout instead (when HW busy detection isn't supported).
>
> Or, in the ignore_crc/retry_err_crc case, return -EILSEQ instead -ETIMEOUT,
> and catch and ignore the error in the calling code.  Then you get polling if
> it works, otherwise getting CRC errors until timeout.
>
>>
>>>
>>> With the existing code I would just change the err check:
>>>
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 3268fcd3378d..c8862c58b60b 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>
>>>                 err = mmc_switch_status(card);
>>>                 /*
>>> +                * For HS200, CRC errors are not a reliable way to know the
>>> +                * switch failed. If there really is a problem, we would expect
>>> +                * tuning will fail and the result ends up the same.
>>> +                */
>>> +               if (err == -EILSEQ)
>>> +                       err = 0;
>>> +               /*
>>
>> I don't think ignoring CRC errors is reliable when verifying the CMD6
>> status. My point is that we must not parse the status, in case of CRC
>> errors as it can't be trusted.
>
> I agree, but mmc_switch_status() doesn't look at the response if there is an
> error.

Correct, it's only during CMD13 polling when CRC was ignored.

>
>>
>> So, then we might as well just ignore validating the CMD6 status
>> altogether, but instead always move on to the tuning and hope that it
>> succeeds.
>
> That is a possibility, but it seemed to me that is was worth checking for
> all the users where it does work. i.e if CMD13 does not give a CRC error
> then validate the response, and if CMD13 does give a CRC error then ignore
> the response and keep going anyway.

Okay, let me think about this.

>
>>
>> I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
>> mode isn't enabled, so we could check that. Anyway, we should fail
>> with the tuning if the earlier HS200 switch also failed. Don't you
>> think?
>
> Yes CMD21 is an illegal command if the mode is not HS200.  The card should
> set ILLEGAL_COMMAND but also not respond i.e there will be a timeout error.
> That could cause a long delay before tuning finally fails.  The only way to
> mitigate that would be to make ignoring the CRC error a host-specific option
> (e.g. MMC_CAP_... flag).  Arguably, if the switch fails, the mode is broken
> and should not have been allowed in the first place.

Not sure why there should be a long delay?

If the CMD21 fails with a timeout, it's like any other command that
fails with a timeout, right?

So why should this one take longer to report for the host compared to others?

>
>>
>>>                  * mmc_select_timing() assumes timing has not changed if
>>>                  * it is a switch error.
>>>                  */
>>>
>>>
>>> Then to support polling:
>>>
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index c8862c58b60b..66d8d57ae2fb 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>  {
>>>         struct mmc_host *host = card->host;
>>>         unsigned int old_timing, old_signal_voltage;
>>> +       bool send_status;
>>>         int err = -EINVAL;
>>>         u8 val;
>>>
>>> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>          * switch to HS200 mode if bus width is set successfully.
>>>          */
>>>         err = mmc_select_bus_width(card);
>>> -       if (err > 0) {
>>> -               val = EXT_CSD_TIMING_HS200 |
>>> -                     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, 0,
>>> -                                  true, false, true);
>>> -               if (err)
>>> -                       goto err;
>>> -               old_timing = host->ios.timing;
>>> -               mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>> +       if (err <= 0)
>>> +               goto err;
>>> +
>>> +       send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>> +                     !host->ops->card_busy;
>>> +       old_timing = host->ios.timing;
>>> +
>>> +       val = EXT_CSD_TIMING_HS200 |
>>> +             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,
>>> +                          MMC_TIMING_MMC_HS200, true, send_status, true);
>>>
>>> +       if (!err && !send_status) {
>>>                 err = mmc_switch_status(card);
>>>                 /*
>>>                  * For HS200, CRC errors are not a reliable way to know the
>>>
>>>
>>>
>>> Thoughts?
>>
>> Well, I think the main problem is that if we have cards that returns
>> CRC errors even after the HS200 switch, then we can't use polling, as
>> we can't trust to parse the CMD6 status.
>
> As I wrote above, if there is no option but polling then we could expect it
> to work.  And if CMD13 does not give a CRC error then we can validate the
> response, only ignoring it if there is a CRC error.
>
> I should point out that retrying CMD13 will clear the error bits in the
> status so there is no point retrying when checking for the SWITCH_ERROR bit.
> i.e. we need a version of __switch_send_status() that sets retries to zero.

Are you really sure about this?

I thought the switch status remained present in the device, but got
cleared first when a new CMD6 command is being sent (or a reset of
course), that would make more sense to me. :-)
Anyway, this would mean that old CMD13 polling method was broken even
in this sense.

Okay, some more tests seems to be needed here. I will do some local
hacks to explore this.

Kind regards
Uffe

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Adrian Hunter]

On 18/11/16 14:20, Ulf Hansson wrote:
> On 18 November 2016 at 10:30, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 17/11/16 17:02, Ulf Hansson wrote:
>>> On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 16/11/16 12:51, Ulf Hansson wrote:
>>>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>>>> in earlier change logs.
>>>>>
>>>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>>>> mmc card operates at the same bus timing during the polling.
>>>>
>>>> I have reports of cases where CMD13 always gives CRC errors after switch
>>>> to HS200.  Currently we are assuming the low frequency should mean that
>>>> won't happen, but it does in some cases.  That is not entirely surprising
>>>> since HS200 needs tuning at the final operating frequency.
>>>
>>> >From a logical point of view and if tuning is needed also for the CMD
>>> line, this somehow make sense.
>>>
>>> However, this is *not* how the JEDEC spec describes the HS200 switch
>>> sequence. It is clearly stated that the host should validate the CM6
>>> status via sending a CMD13 command, *before* performing tuning.
>>
>> I agree, it seems not to be following spec.
>>
>>>
>>> Could it be that the observations about the CRC errors, is related to
>>> a controller/driver issue and not a card issue?
>>
>> I don't know what causes the problem (and I have a sneaking suspicion that
>> if vendors configured / designed their boards correctly, it wouldn't
>> happen).  However, while some cards have better signal characteristics than
>> others, tuning is a host controller issue - the card doesn't care.
>>
>>>
>>>>
>>>> What I would like to do for hosts that support busy waiting or DAT0 polling
>>>> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
>>>> errors from the CMD13 that checks the switch status.  The main reason for
>>>> doing that is that we really expect the switch to succeed and, given HS200
>>>> tuning requirement, the CRC error is not a reliable means of determining
>>>> that it hasn't.
>>>
>>> Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
>>> work, as tuning is needed.
>>
>> I would assume that vendors integrate a working combination of eMMC and host
>> controller, so if polling is the only option, then we could assume it will work.
>>
>>>
>>> So, to me that means we need to fall-back to use the generic CMD6
>>> timeout instead (when HW busy detection isn't supported).
>>
>> Or, in the ignore_crc/retry_err_crc case, return -EILSEQ instead -ETIMEOUT,
>> and catch and ignore the error in the calling code.  Then you get polling if
>> it works, otherwise getting CRC errors until timeout.
>>
>>>
>>>>
>>>> With the existing code I would just change the err check:
>>>>
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 3268fcd3378d..c8862c58b60b 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>>
>>>>                 err = mmc_switch_status(card);
>>>>                 /*
>>>> +                * For HS200, CRC errors are not a reliable way to know the
>>>> +                * switch failed. If there really is a problem, we would expect
>>>> +                * tuning will fail and the result ends up the same.
>>>> +                */
>>>> +               if (err == -EILSEQ)
>>>> +                       err = 0;
>>>> +               /*
>>>
>>> I don't think ignoring CRC errors is reliable when verifying the CMD6
>>> status. My point is that we must not parse the status, in case of CRC
>>> errors as it can't be trusted.
>>
>> I agree, but mmc_switch_status() doesn't look at the response if there is an
>> error.
> 
> Correct, it's only during CMD13 polling when CRC was ignored.
> 
>>
>>>
>>> So, then we might as well just ignore validating the CMD6 status
>>> altogether, but instead always move on to the tuning and hope that it
>>> succeeds.
>>
>> That is a possibility, but it seemed to me that is was worth checking for
>> all the users where it does work. i.e if CMD13 does not give a CRC error
>> then validate the response, and if CMD13 does give a CRC error then ignore
>> the response and keep going anyway.
> 
> Okay, let me think about this.
> 
>>
>>>
>>> I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
>>> mode isn't enabled, so we could check that. Anyway, we should fail
>>> with the tuning if the earlier HS200 switch also failed. Don't you
>>> think?
>>
>> Yes CMD21 is an illegal command if the mode is not HS200.  The card should
>> set ILLEGAL_COMMAND but also not respond i.e there will be a timeout error.
>> That could cause a long delay before tuning finally fails.  The only way to
>> mitigate that would be to make ignoring the CRC error a host-specific option
>> (e.g. MMC_CAP_... flag).  Arguably, if the switch fails, the mode is broken
>> and should not have been allowed in the first place.
> 
> Not sure why there should be a long delay?
> 
> If the CMD21 fails with a timeout, it's like any other command that
> fails with a timeout, right?

Ah, right.  I was thinking of the data timeout, but yes the command timeout
will kick in first of course.

> 
> So why should this one take longer to report for the host compared to others?
> 
>>
>>>
>>>>                  * mmc_select_timing() assumes timing has not changed if
>>>>                  * it is a switch error.
>>>>                  */
>>>>
>>>>
>>>> Then to support polling:
>>>>
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index c8862c58b60b..66d8d57ae2fb 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>>  {
>>>>         struct mmc_host *host = card->host;
>>>>         unsigned int old_timing, old_signal_voltage;
>>>> +       bool send_status;
>>>>         int err = -EINVAL;
>>>>         u8 val;
>>>>
>>>> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>>          * switch to HS200 mode if bus width is set successfully.
>>>>          */
>>>>         err = mmc_select_bus_width(card);
>>>> -       if (err > 0) {
>>>> -               val = EXT_CSD_TIMING_HS200 |
>>>> -                     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, 0,
>>>> -                                  true, false, true);
>>>> -               if (err)
>>>> -                       goto err;
>>>> -               old_timing = host->ios.timing;
>>>> -               mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>>> +       if (err <= 0)
>>>> +               goto err;
>>>> +
>>>> +       send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>>> +                     !host->ops->card_busy;
>>>> +       old_timing = host->ios.timing;
>>>> +
>>>> +       val = EXT_CSD_TIMING_HS200 |
>>>> +             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,
>>>> +                          MMC_TIMING_MMC_HS200, true, send_status, true);
>>>>
>>>> +       if (!err && !send_status) {
>>>>                 err = mmc_switch_status(card);
>>>>                 /*
>>>>                  * For HS200, CRC errors are not a reliable way to know the
>>>>
>>>>
>>>>
>>>> Thoughts?
>>>
>>> Well, I think the main problem is that if we have cards that returns
>>> CRC errors even after the HS200 switch, then we can't use polling, as
>>> we can't trust to parse the CMD6 status.
>>
>> As I wrote above, if there is no option but polling then we could expect it
>> to work.  And if CMD13 does not give a CRC error then we can validate the
>> response, only ignoring it if there is a CRC error.
>>
>> I should point out that retrying CMD13 will clear the error bits in the
>> status so there is no point retrying when checking for the SWITCH_ERROR bit.
>> i.e. we need a version of __switch_send_status() that sets retries to zero.
> 
> Are you really sure about this?

I don't think I have ever tested it.  I was going on what the spec says:
"1) Error bit. Signals an error condition was detected by the device. These
bits are cleared as soon as the response (reporting the error) is sent out."

> 
> I thought the switch status remained present in the device, but got
> cleared first when a new CMD6 command is being sent (or a reset of
> course), that would make more sense to me. :-)
> Anyway, this would mean that old CMD13 polling method was broken even
> in this sense.
> 
> Okay, some more tests seems to be needed here. I will do some local
> hacks to explore this.
> 
> Kind regards
> Uffe
> 

Re: [PATCH 8/9] mmc: core: Allow CMD13 polling when switch to HS400 mode

[Ulf Hansson]

On 18 November 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/11/16 12:51, Ulf Hansson wrote:
>> In cases when the mmc host doesn't support HW busy detection, polling for
>> busy by using CMD13 is beneficial. The reasons have already been explained
>> in earlier change logs.
>>
>> Moreover, when polling with CMD13 during bus timing changes, we should
>> retry instead of fail when we get CRC errors.
>>
>> Switching from HS200 to HS400 and reverse includes several steps, where
>> each step changes the bus speed timing. Let's improve the behaviour during
>> these sequences, by allowing CMD13 polling for each of the step. Let's also
>> make sure the CMD13 polling becomes retried, while receiving a CRC error.
>
> I don't know we need to try to get polling for HS400.  The support of HS400
> suggests a relatively sophisticated host controller which really should
> support busy waiting as well.

That's a reasonable argument.

Moreover, I do also think that argument stands for a controller
supporting HS200. So I have no problem dropping the CMD13 polling
support for that bus speed mode as well, if people wants that.

>
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc.c | 87 +++++++++++++++-----------------------------------
>>  1 file changed, 26 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 0b26383..24b9e72 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1094,8 +1094,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;
>> +     int err;
>>       u8 val;
>>
>>       /*
>> @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>>             host->ios.bus_width == MMC_BUS_WIDTH_8))
>>               return 0;
>>
>> -     /* Switch card to HS mode */
>> -     val = EXT_CSD_TIMING_HS;
>> -     err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> -                        EXT_CSD_HS_TIMING, val,
>> -                        card->ext_csd.generic_cmd6_time, 0,
>> -                        true, false, true);
>> +     /*
>> +      * Switch card to HS mode.
>> +      * First reduce to the HS frequency as CMD13 are sent to poll for busy
>> +      * and/or to validate the switch status.
>> +      */
>> +     mmc_set_clock(host, card->ext_csd.hs_max_dtr);
>
> That was moved by commit 649c6059d2371fef886a8f967e21416204723d63
> ("mmc: mmc: Fix HS switch failure in mmc_select_hs400()") i.e. if you put it
> back I would expect the gru board problem to reappear.

Correct, thanks for pointing it out!

According to below comments, it's clear that the JEDEC spec lacks a
good description of the HS200/400 switch behaviour.

>
>> +     err = mmc_select_hs(card);
>>       if (err) {
>>               pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
>>                       mmc_hostname(host), err);
>>               return err;
>>       }
>>
>> -     /* Set host controller to HS timing */
>> -     mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> -
>> -     /* Reduce frequency to HS frequency */
>> -     max_dtr = card->ext_csd.hs_max_dtr;
>> -     mmc_set_clock(host, max_dtr);
>> -
>> -     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,
>> -                      card->ext_csd.generic_cmd6_time);
>> +     /* Switch card to HS DDR */
>> +     err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +                        EXT_CSD_BUS_WIDTH,
>> +                        EXT_CSD_DDR_BUS_WIDTH_8,
>> +                        card->ext_csd.generic_cmd6_time,
>> +                        MMC_TIMING_MMC_DDR52,
>> +                        true, true, true);
>>       if (err) {
>>               pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>>                       mmc_hostname(host), err);
>> @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card)
>>             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, 0,
>> -                        true, false, true);
>> +                        card->ext_csd.generic_cmd6_time,
>> +                        MMC_TIMING_MMC_HS400,
>> +                        true, true, true);
>
> This is not exactly right.  Tuning has been done at the HS400 operating
> frequency.  That is why we set the bus speed before sending any more
> commands. i.e. mmc_switch_status(card) is below mmc_set_bus_speed(card)

Right, I see!

I had another look at the JEDEC spec for HS400 mode switch:

---
6) Perform the Tuning Process at the HS400 target operating frequency,
NOTE Tuning process in HS200 mode is required to synchronize the
command response on the CMD line to CLK for HS400 operation.
---

This confirms your point. Although on the other hand by looking at the
HS400 switch flow chart, it says that checking the switch status shall
be done *before* bumping the clock rate. :-)

Anyway, I will move back to the behaviour where CMD13 polling is not
allowed for HS400, as that seems like the only thing we can do!

Kind regards
Uffe

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Ulf Hansson]

[...]

>>>
>>> I should point out that retrying CMD13 will clear the error bits in the
>>> status so there is no point retrying when checking for the SWITCH_ERROR bit.
>>> i.e. we need a version of __switch_send_status() that sets retries to zero.
>>
>> Are you really sure about this?
>
> I don't think I have ever tested it.  I was going on what the spec says:
> "1) Error bit. Signals an error condition was detected by the device. These
> bits are cleared as soon as the response (reporting the error) is sent out."

Couldn't have been more clear than that.

So when doing CMD13 polling, it's seems like we should be validating
the SWITCH_ERROR bit for each successful CMD13 response, as otherwise
we may end up loosing that information.

[...]

Kind regards
Uffe

Re: [PATCH 9/9] mmc: core: Allow CMD13 polling when switch to HS400ES mode

[Adrian Hunter]

On 16/11/16 12:51, Ulf Hansson wrote:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
> 
> Moreover, when polling with CMD13 during bus timing changes, we should
> retry instead of fail when we get CRC errors.
> 
> Switching to HS400ES includes several steps, where each step changes the
> bus speed timing. Let's improve the behaviour during these sequences, by
> allowing CMD13 polling for each of the step. Let's also make sure the CMD13
> polling becomes retried, while receiving a CRC error.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c | 35 +++++++++++------------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 24b9e72..b6f0035 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1227,31 +1227,24 @@ static int mmc_select_hs400es(struct mmc_card *card)
>  		goto out_err;
>  
>  	/* Switch card to HS mode */
> -	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
> -			   card->ext_csd.generic_cmd6_time, 0,
> -			   true, false, true);
> +	err = mmc_select_hs(card);
>  	if (err) {
>  		pr_err("%s: switch to hs for hs400es failed, err:%d\n",
>  			mmc_hostname(host), err);
>  		goto out_err;
>  	}
>  
> -	mmc_set_timing(host, MMC_TIMING_MMC_HS);
> -	err = mmc_switch_status(card);
> -	if (err)
> -		goto out_err;
> -
>  	mmc_set_clock(host, card->ext_csd.hs_max_dtr);
>  
> -	/* Switch card to DDR with strobe bit */
> +	/* Switch card to HS DDR with strobe bit */
>  	val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
> -	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -			 EXT_CSD_BUS_WIDTH,
> -			 val,
> -			 card->ext_csd.generic_cmd6_time);
> +	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +			   EXT_CSD_BUS_WIDTH, val,
> +			   card->ext_csd.generic_cmd6_time,
> +			   MMC_TIMING_MMC_DDR52,
> +			   true, true, true);
>  	if (err) {
> -		pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
> +		pr_err("%s: switch to hs ddr for hs400es failed, err:%d\n",
>  			mmc_hostname(host), err);
>  		goto out_err;
>  	}
> @@ -1261,26 +1254,20 @@ static int mmc_select_hs400es(struct mmc_card *card)
>  	      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, 0,
> -			   true, false, true);
> +			   card->ext_csd.generic_cmd6_time,
> +			   MMC_TIMING_MMC_HS400,
> +			   true, true, true);

This could be a problem because the CMD13 is being sent to a card in HS400
enhanced strobe mode but we haven't enabled enhanced strobe on the host
controller yet.  Previously mmc_switch_status(card) was below
host->ops->hs400_enhanced_strobe().

>  	if (err) {
>  		pr_err("%s: switch to hs400es failed, err:%d\n",
>  			mmc_hostname(host), err);
>  		goto out_err;
>  	}
>  
> -	/* Set host controller to HS400 timing and frequency */
> -	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
> -
>  	/* Controller enable enhanced strobe function */
>  	host->ios.enhanced_strobe = true;
>  	if (host->ops->hs400_enhanced_strobe)
>  		host->ops->hs400_enhanced_strobe(host, &host->ios);
>  
> -	err = mmc_switch_status(card);
> -	if (err)
> -		goto out_err;
> -
>  	return 0;
>  
>  out_err:
> 

Re: [PATCH 9/9] mmc: core: Allow CMD13 polling when switch to HS400ES mode

[Ulf Hansson]

On 18 November 2016 at 14:35, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/11/16 12:51, Ulf Hansson wrote:
>> In cases when the mmc host doesn't support HW busy detection, polling for
>> busy by using CMD13 is beneficial. The reasons have already been explained
>> in earlier change logs.
>>
>> Moreover, when polling with CMD13 during bus timing changes, we should
>> retry instead of fail when we get CRC errors.
>>
>> Switching to HS400ES includes several steps, where each step changes the
>> bus speed timing. Let's improve the behaviour during these sequences, by
>> allowing CMD13 polling for each of the step. Let's also make sure the CMD13
>> polling becomes retried, while receiving a CRC error.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc.c | 35 +++++++++++------------------------
>>  1 file changed, 11 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 24b9e72..b6f0035 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1227,31 +1227,24 @@ static int mmc_select_hs400es(struct mmc_card *card)
>>               goto out_err;
>>
>>       /* Switch card to HS mode */
>> -     err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> -                        EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
>> -                        card->ext_csd.generic_cmd6_time, 0,
>> -                        true, false, true);
>> +     err = mmc_select_hs(card);
>>       if (err) {
>>               pr_err("%s: switch to hs for hs400es failed, err:%d\n",
>>                       mmc_hostname(host), err);
>>               goto out_err;
>>       }
>>
>> -     mmc_set_timing(host, MMC_TIMING_MMC_HS);
>> -     err = mmc_switch_status(card);
>> -     if (err)
>> -             goto out_err;
>> -
>>       mmc_set_clock(host, card->ext_csd.hs_max_dtr);
>>
>> -     /* Switch card to DDR with strobe bit */
>> +     /* Switch card to HS DDR with strobe bit */
>>       val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
>> -     err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> -                      EXT_CSD_BUS_WIDTH,
>> -                      val,
>> -                      card->ext_csd.generic_cmd6_time);
>> +     err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +                        EXT_CSD_BUS_WIDTH, val,
>> +                        card->ext_csd.generic_cmd6_time,
>> +                        MMC_TIMING_MMC_DDR52,
>> +                        true, true, true);
>>       if (err) {
>> -             pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
>> +             pr_err("%s: switch to hs ddr for hs400es failed, err:%d\n",
>>                       mmc_hostname(host), err);
>>               goto out_err;
>>       }
>> @@ -1261,26 +1254,20 @@ static int mmc_select_hs400es(struct mmc_card *card)
>>             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, 0,
>> -                        true, false, true);
>> +                        card->ext_csd.generic_cmd6_time,
>> +                        MMC_TIMING_MMC_HS400,
>> +                        true, true, true);
>
> This could be a problem because the CMD13 is being sent to a card in HS400
> enhanced strobe mode but we haven't enabled enhanced strobe on the host
> controller yet.  Previously mmc_switch_status(card) was below
> host->ops->hs400_enhanced_strobe().
>

Yes, more or less the same reasons as before. We need the "tuning" (in
this case the "strobing") to be done before validating the switch
status.

Although, the change a little further above, when switching to HS DDR
in the intermediate step - that should be fine, don't you think?

Kind regards
Uffe

Re: [PATCH 9/9] mmc: core: Allow CMD13 polling when switch to HS400ES mode

[Adrian Hunter]

On 18/11/16 16:37, Ulf Hansson wrote:
> On 18 November 2016 at 14:35, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 16/11/16 12:51, Ulf Hansson wrote:
>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>> in earlier change logs.
>>>
>>> Moreover, when polling with CMD13 during bus timing changes, we should
>>> retry instead of fail when we get CRC errors.
>>>
>>> Switching to HS400ES includes several steps, where each step changes the
>>> bus speed timing. Let's improve the behaviour during these sequences, by
>>> allowing CMD13 polling for each of the step. Let's also make sure the CMD13
>>> polling becomes retried, while receiving a CRC error.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/core/mmc.c | 35 +++++++++++------------------------
>>>  1 file changed, 11 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 24b9e72..b6f0035 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1227,31 +1227,24 @@ static int mmc_select_hs400es(struct mmc_card *card)
>>>               goto out_err;
>>>
>>>       /* Switch card to HS mode */
>>> -     err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> -                        EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
>>> -                        card->ext_csd.generic_cmd6_time, 0,
>>> -                        true, false, true);
>>> +     err = mmc_select_hs(card);
>>>       if (err) {
>>>               pr_err("%s: switch to hs for hs400es failed, err:%d\n",
>>>                       mmc_hostname(host), err);
>>>               goto out_err;
>>>       }
>>>
>>> -     mmc_set_timing(host, MMC_TIMING_MMC_HS);
>>> -     err = mmc_switch_status(card);
>>> -     if (err)
>>> -             goto out_err;
>>> -
>>>       mmc_set_clock(host, card->ext_csd.hs_max_dtr);
>>>
>>> -     /* Switch card to DDR with strobe bit */
>>> +     /* Switch card to HS DDR with strobe bit */
>>>       val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
>>> -     err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> -                      EXT_CSD_BUS_WIDTH,
>>> -                      val,
>>> -                      card->ext_csd.generic_cmd6_time);
>>> +     err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> +                        EXT_CSD_BUS_WIDTH, val,
>>> +                        card->ext_csd.generic_cmd6_time,
>>> +                        MMC_TIMING_MMC_DDR52,
>>> +                        true, true, true);
>>>       if (err) {
>>> -             pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
>>> +             pr_err("%s: switch to hs ddr for hs400es failed, err:%d\n",
>>>                       mmc_hostname(host), err);
>>>               goto out_err;
>>>       }
>>> @@ -1261,26 +1254,20 @@ static int mmc_select_hs400es(struct mmc_card *card)
>>>             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, 0,
>>> -                        true, false, true);
>>> +                        card->ext_csd.generic_cmd6_time,
>>> +                        MMC_TIMING_MMC_HS400,
>>> +                        true, true, true);
>>
>> This could be a problem because the CMD13 is being sent to a card in HS400
>> enhanced strobe mode but we haven't enabled enhanced strobe on the host
>> controller yet.  Previously mmc_switch_status(card) was below
>> host->ops->hs400_enhanced_strobe().
>>
> 
> Yes, more or less the same reasons as before. We need the "tuning" (in
> this case the "strobing") to be done before validating the switch
> status.
> 
> Although, the change a little further above, when switching to HS DDR
> in the intermediate step - that should be fine, don't you think?

Yes

Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode

[Shawn Lin]

在 2016/11/18 19:45, Ulf Hansson 写道:
> Hi Shawn,
>
> On 18 November 2016 at 09:05, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> Hi Ulf,
>>
>>
>> 在 2016/11/16 18:51, Ulf Hansson 写道:
>>>
>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>> in earlier change logs.
>>>
>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>> mmc card operates at the same bus timing during the polling.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/core/mmc.c | 21 +++++----------------
>>>  1 file changed, 5 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 3268fcd..0b26383 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1351,7 +1351,7 @@ 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;
>>> -       unsigned int old_timing, old_signal_voltage;
>>> +       unsigned int old_signal_voltage;
>>>         int err = -EINVAL;
>>>         u8 val;
>>>
>>> @@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>                       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, 0,
>>> -                                  true, false, true);
>>> -               if (err)
>>> -                       goto err;
>>> -               old_timing = host->ios.timing;
>>> -               mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>> -
>>> -               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);
>>> +                                  card->ext_csd.generic_cmd6_time,
>>> +                                  MMC_TIMING_MMC_HS200,
>>> +                                  true, true, true);
>>
>>
>> I was finding a failure from the test last night after applying these
>> patches and using HS200 only.
>>
>> It seems like the controller(sdhci-of-arasan,5.1) continuously generate
>> response timeout for checking CMD13.. Per TRM, response timeout could
>> means the device didn't ack the CMD13 or the controller was failing to
>> latch the response..
>
> Did you make any changes to the sdchi driver while testing this?

Nope, I didn't.

>
> What I am wondering is whether you tested this with an implemented
> ->card_busy() host ops or not, as sdhci has a default implementation
> of it.

yes, I was using sdhci's default card_busy callback.

I haven't reproduced that failure until now. :(

>
> BTW, some sdhci hosts have lately shown problem [1] with sdhci's
> default ->card_busy() host ops.
>
>>
>> The eMMC part number is KLMBG2JENB-B041. I will use two boards to check
>> if it was indeed related to these patches.
>>
>>>         }
>>> -err:
>>> +
>>>         if (err) {
>>>                 /* fall back to the old signal voltage, if fails report
>>> error */
>>>                 if (__mmc_set_signal_voltage(host, old_signal_voltage))
>>>
>>
>>
>> --
>> Best Regards
>> Shawn Lin
>>
>
> Thanks for helping out with testing etc, I really appreciate it!
>
> Kind regards
> Uffe
>
> [1]
> https://patchwork.kernel.org/patch/9429299/
>
>
>


-- 
Best Regards
Shawn Lin