[PATCH RFC 0/3] mmc: core: Do not change frequency before switch from HS400

[PATCH RFC 0/3] mmc: core: Do not change frequency before switch from HS400

[Adrian Hunter]

Hi

Patches to improve mmc_hs400_to_hs200() as discussed here:

	https://lore.kernel.org/linux-mmc/d355ac11-21d6-e8f7-d03f-65c55e10aea2@intel.com/


Adrian Hunter (3):
      mmc: core: Try harder if transfer mode switch fails
      mmc: core: Do not check CRC response for switch from HS400 to HS200
      mmc: core: Do not change frequency before switch from HS400

 drivers/mmc/core/mmc.c     | 41 ++++++++++++++++++++++++-----------------
 drivers/mmc/core/mmc_ops.c | 13 +++++++++++--
 drivers/mmc/core/mmc_ops.h |  2 +-
 3 files changed, 36 insertions(+), 20 deletions(-)


Regards
Adrian

[PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Adrian Hunter]

Add extra busy wait and retries if transfer mode switch fails.

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

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index aa0cab190cd8..619088a94688 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		cmd.sanitize_busy = true;
 
 	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
+	if (err && index == EXT_CSD_HS_TIMING) {
+		/* Try harder for timing changes */
+		__mmc_poll_for_busy(card, timeout_ms, send_status,
+				    retry_crc_err, MMC_BUSY_CMD6);
+		err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
+	}
 	if (err)
 		goto out;
 
-- 
2.17.1

[PATCH RFC 2/3] mmc: core: Do not check CRC response for switch from HS400 to HS200

[Adrian Hunter]

Switching from HS400 to HS200 may experience CRC errors. Do not check
CRC response in that case.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c     | 22 +++++++++++-----------
 drivers/mmc/core/mmc_ops.c |  7 +++++--
 drivers/mmc/core/mmc_ops.h |  2 +-
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c2abd417a84a..3e2ad728b55e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1055,7 +1055,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, MMC_TIMING_MMC_HS,
-			   true, true);
+			   true, true, false);
 	if (err)
 		pr_warn("%s: switch to high-speed failed, err:%d\n",
 			mmc_hostname(card->host), err);
@@ -1087,7 +1087,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 			   ext_csd_bits,
 			   card->ext_csd.generic_cmd6_time,
 			   MMC_TIMING_MMC_DDR52,
-			   true, true);
+			   true, true, false);
 	if (err) {
 		pr_err("%s: switch to bus width %d ddr failed\n",
 			mmc_hostname(host), 1 << bus_width);
@@ -1155,7 +1155,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   false, true);
+			   false, true, false);
 	if (err) {
 		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1197,7 +1197,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   false, true);
+			   false, true, false);
 	if (err) {
 		pr_err("%s: switch to hs400 failed, err:%d\n",
 			 mmc_hostname(host), err);
@@ -1243,7 +1243,7 @@ int mmc_hs400_to_hs200(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, 0,
-			   false, true);
+			   false, true, true);
 	if (err)
 		goto out_err;
 
@@ -1256,7 +1256,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,
-			   0, false, true);
+			   0, false, true, false);
 	if (err)
 		goto out_err;
 
@@ -1274,7 +1274,7 @@ int mmc_hs400_to_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,
-			   false, true);
+			   false, true, false);
 	if (err)
 		goto out_err;
 
@@ -1358,7 +1358,7 @@ static int mmc_select_hs400es(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,
-			   false, true);
+			   false, true, false);
 	if (err) {
 		pr_err("%s: switch to hs for hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1392,7 +1392,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   false, true);
+			   false, true, false);
 	if (err) {
 		pr_err("%s: switch to hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1457,7 +1457,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 				   EXT_CSD_HS_TIMING, val,
 				   card->ext_csd.generic_cmd6_time, 0,
-				   false, true);
+				   false, true, false);
 		if (err)
 			goto err;
 		old_timing = host->ios.timing;
@@ -1959,7 +1959,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, 0, false, false);
+			notify_type, timeout, 0, false, 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 619088a94688..2baaa66e491d 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -552,12 +552,13 @@ int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
  *	@timing: new timing to change to
  *	@send_status: send status cmd to poll for busy
  *	@retry_crc_err: retry when CRC errors when polling with CMD13 for busy
+ *	@no_crc_resp: do not require to check CRC response
  *
  *	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, unsigned char timing,
-		bool send_status, bool retry_crc_err)
+		bool send_status, bool retry_crc_err, bool no_crc_resp)
 {
 	struct mmc_host *host = card->host;
 	int err;
@@ -594,6 +595,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	} else {
 		cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
 	}
+	if (no_crc_resp)
+		cmd.flags &= ~MMC_RSP_CRC;
 
 	if (index == EXT_CSD_SANITIZE_START)
 		cmd.sanitize_busy = true;
@@ -639,7 +642,7 @@ 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, 0,
-			    true, false);
+			    true, false, false);
 }
 EXPORT_SYMBOL_GPL(mmc_switch);
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 38dcfeeaf6d5..1a75c347b8a7 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -40,7 +40,7 @@ int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		      enum mmc_busy_cmd busy_cmd);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, unsigned char timing,
-		bool send_status, bool retry_crc_err);
+		bool send_status, bool retry_crc_err, bool no_crc_resp);
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms);
 void mmc_run_bkops(struct mmc_card *card);
-- 
2.17.1

[PATCH RFC 3/3] mmc: core: Do not change frequency before switch from HS400

[Adrian Hunter]

Do not change frequency before switch from HS400 but in case of error
try again after frequency change.

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

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3e2ad728b55e..e2b3d7505d6c 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1235,20 +1235,27 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	int err;
 	u8 val;
 
-	/* Reduce frequency to HS */
-	max_dtr = card->ext_csd.hs_max_dtr;
-	mmc_set_clock(host, max_dtr);
-
 	/* 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,
 			   false, true, true);
-	if (err)
-		goto out_err;
 
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
+	/* Reduce frequency to HS */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(host, max_dtr);
+
+	if (err) {
+		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				   EXT_CSD_HS_TIMING, val,
+				   card->ext_csd.generic_cmd6_time, 0, false,
+				   true, true);
+	}
+	if (err)
+		goto out_err;
+
 	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
-- 
2.17.1

Re: [PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Ulf Hansson]

On Thu, 12 Mar 2020 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> Add extra busy wait and retries if transfer mode switch fails.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/mmc_ops.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index aa0cab190cd8..619088a94688 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>                 cmd.sanitize_busy = true;
>
>         err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> +       if (err && index == EXT_CSD_HS_TIMING) {
> +               /* Try harder for timing changes */
> +               __mmc_poll_for_busy(card, timeout_ms, send_status,
> +                                   retry_crc_err, MMC_BUSY_CMD6);
> +               err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> +       }

Hmm, what do you think of moving this to the caller(s) of
__mmc_switch() and in particular only at those places were we find it
useful. Me personally, would prefer that option.

To do that, we may need to have the possibility of specifying the
number of retries that should be used in the mmc_wait_for_cmd() call
to the caller can check the error code better.

Moreover, it looks a bit risky to do the polling for all kinds of
errors - shouldn't we do for CRC errors?

>         if (err)
>                 goto out;
>
> --
> 2.17.1
>

Kind regards
Uffe

Re: [PATCH RFC 2/3] mmc: core: Do not check CRC response for switch from HS400 to HS200

[Ulf Hansson]

On Thu, 12 Mar 2020 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> Switching from HS400 to HS200 may experience CRC errors. Do not check
> CRC response in that case.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

[...]

> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 619088a94688..2baaa66e491d 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -552,12 +552,13 @@ int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>   *     @timing: new timing to change to
>   *     @send_status: send status cmd to poll for busy
>   *     @retry_crc_err: retry when CRC errors when polling with CMD13 for busy
> + *     @no_crc_resp: do not require to check CRC response
>   *
>   *     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, unsigned char timing,
> -               bool send_status, bool retry_crc_err)
> +               bool send_status, bool retry_crc_err, bool no_crc_resp)
>  {
>         struct mmc_host *host = card->host;
>         int err;
> @@ -594,6 +595,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>         } else {
>                 cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
>         }
> +       if (no_crc_resp)
> +               cmd.flags &= ~MMC_RSP_CRC;

So potentially this means the host driver will complete the command
successfully, even if it receives a CRC error.

As I understood it, the idea was to poll for busy, *if* we encountered
CRC errors, so then how would you know about that?

Seems like we should drop the above code and be checking for -EILSEQ
of the request instead? No?

>
>         if (index == EXT_CSD_SANITIZE_START)
>                 cmd.sanitize_busy = true;
> @@ -639,7 +642,7 @@ 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, 0,
> -                           true, false);
> +                           true, false, false);
>  }
>  EXPORT_SYMBOL_GPL(mmc_switch);
>
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 38dcfeeaf6d5..1a75c347b8a7 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -40,7 +40,7 @@ int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>                       enum mmc_busy_cmd busy_cmd);
>  int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>                 unsigned int timeout_ms, unsigned char timing,
> -               bool send_status, bool retry_crc_err);
> +               bool send_status, bool retry_crc_err, bool no_crc_resp);
>  int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>                 unsigned int timeout_ms);
>  void mmc_run_bkops(struct mmc_card *card);
> --
> 2.17.1
>

Kind regards
Uffe

Re: [PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Adrian Hunter]

On 12/03/20 5:45 pm, Ulf Hansson wrote:
> On Thu, 12 Mar 2020 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> Add extra busy wait and retries if transfer mode switch fails.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/mmc_ops.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index aa0cab190cd8..619088a94688 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>                 cmd.sanitize_busy = true;
>>
>>         err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>> +       if (err && index == EXT_CSD_HS_TIMING) {
>> +               /* Try harder for timing changes */
>> +               __mmc_poll_for_busy(card, timeout_ms, send_status,
>> +                                   retry_crc_err, MMC_BUSY_CMD6);
>> +               err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>> +       }
> 
> Hmm, what do you think of moving this to the caller(s) of
> __mmc_switch() and in particular only at those places were we find it
> useful. Me personally, would prefer that option.
> 
> To do that, we may need to have the possibility of specifying the
> number of retries that should be used in the mmc_wait_for_cmd() call
> to the caller can check the error code better.
> 
> Moreover, it looks a bit risky to do the polling for all kinds of
> errors - shouldn't we do for CRC errors?
> 

What about this then?


diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c2abd417a84a..faa5d30ed891 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1235,20 +1235,35 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	int err;
 	u8 val;
 
-	/* Reduce frequency to HS */
-	max_dtr = card->ext_csd.hs_max_dtr;
-	mmc_set_clock(host, max_dtr);
-
 	/* 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,
 			   false, true);
-	if (err)
-		goto out_err;
+	if (err == -EILSEQ) {
+		__mmc_poll_for_busy(card, card->ext_csd.generic_cmd6_time,
+				    false, true, MMC_BUSY_CMD6);
+		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				   EXT_CSD_HS_TIMING, val,
+				   card->ext_csd.generic_cmd6_time, 0, false,
+				   true);
+	}
 
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
+	/* Reduce frequency to HS */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(host, max_dtr);
+
+	if (err) {
+		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				   EXT_CSD_HS_TIMING, val,
+				   card->ext_csd.generic_cmd6_time, 0, false,
+				   true);
+	}
+	if (err)
+		goto out_err;
+
 	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 6eb87833d478..54afee7c34ae 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -484,9 +484,9 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
 	return 0;
 }
 
-static int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
-			       bool send_status, bool retry_crc_err,
-			       enum mmc_busy_cmd busy_cmd)
+int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+			bool send_status, bool retry_crc_err,
+			enum mmc_busy_cmd busy_cmd)
 {
 	struct mmc_host *host = card->host;
 	int err;
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 38dcfeeaf6d5..649985507f87 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -36,6 +36,9 @@ int mmc_interrupt_hpi(struct mmc_card *card);
 int mmc_can_ext_csd(struct mmc_card *card);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
 int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
+int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+			bool send_status, bool retry_crc_err,
+			enum mmc_busy_cmd busy_cmd);
 int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		      enum mmc_busy_cmd busy_cmd);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,

Re: [PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Ulf Hansson]

On Fri, 13 Mar 2020 at 10:53, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 12/03/20 5:45 pm, Ulf Hansson wrote:
> > On Thu, 12 Mar 2020 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> Add extra busy wait and retries if transfer mode switch fails.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> ---
> >>  drivers/mmc/core/mmc_ops.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> >> index aa0cab190cd8..619088a94688 100644
> >> --- a/drivers/mmc/core/mmc_ops.c
> >> +++ b/drivers/mmc/core/mmc_ops.c
> >> @@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
> >>                 cmd.sanitize_busy = true;
> >>
> >>         err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> >> +       if (err && index == EXT_CSD_HS_TIMING) {
> >> +               /* Try harder for timing changes */
> >> +               __mmc_poll_for_busy(card, timeout_ms, send_status,
> >> +                                   retry_crc_err, MMC_BUSY_CMD6);
> >> +               err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> >> +       }
> >
> > Hmm, what do you think of moving this to the caller(s) of
> > __mmc_switch() and in particular only at those places were we find it
> > useful. Me personally, would prefer that option.
> >
> > To do that, we may need to have the possibility of specifying the
> > number of retries that should be used in the mmc_wait_for_cmd() call
> > to the caller can check the error code better.
> >
> > Moreover, it looks a bit risky to do the polling for all kinds of
> > errors - shouldn't we do for CRC errors?
> >
>
> What about this then?

Looks good to me!

Is the retry in __mmc_switch() with MMC_CMD_RETRIES okay for now you think?

Kind regards
Uffe

>
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c2abd417a84a..faa5d30ed891 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1235,20 +1235,35 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>         int err;
>         u8 val;
>
> -       /* Reduce frequency to HS */
> -       max_dtr = card->ext_csd.hs_max_dtr;
> -       mmc_set_clock(host, max_dtr);
> -
>         /* 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,
>                            false, true);
> -       if (err)
> -               goto out_err;
> +       if (err == -EILSEQ) {
> +               __mmc_poll_for_busy(card, card->ext_csd.generic_cmd6_time,
> +                                   false, true, MMC_BUSY_CMD6);
> +               err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +                                  EXT_CSD_HS_TIMING, val,
> +                                  card->ext_csd.generic_cmd6_time, 0, false,
> +                                  true);
> +       }
>
>         mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
>
> +       /* Reduce frequency to HS */
> +       max_dtr = card->ext_csd.hs_max_dtr;
> +       mmc_set_clock(host, max_dtr);
> +
> +       if (err) {
> +               err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +                                  EXT_CSD_HS_TIMING, val,
> +                                  card->ext_csd.generic_cmd6_time, 0, false,
> +                                  true);
> +       }
> +       if (err)
> +               goto out_err;
> +
>         err = mmc_switch_status(card, true);
>         if (err)
>                 goto out_err;
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 6eb87833d478..54afee7c34ae 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -484,9 +484,9 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
>         return 0;
>  }
>
> -static int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> -                              bool send_status, bool retry_crc_err,
> -                              enum mmc_busy_cmd busy_cmd)
> +int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> +                       bool send_status, bool retry_crc_err,
> +                       enum mmc_busy_cmd busy_cmd)
>  {
>         struct mmc_host *host = card->host;
>         int err;
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 38dcfeeaf6d5..649985507f87 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -36,6 +36,9 @@ int mmc_interrupt_hpi(struct mmc_card *card);
>  int mmc_can_ext_csd(struct mmc_card *card);
>  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>  int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
> +int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> +                       bool send_status, bool retry_crc_err,
> +                       enum mmc_busy_cmd busy_cmd);
>  int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>                       enum mmc_busy_cmd busy_cmd);
>  int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>

Re: [PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Adrian Hunter]

On 13/03/20 12:40 pm, Ulf Hansson wrote:
> On Fri, 13 Mar 2020 at 10:53, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 12/03/20 5:45 pm, Ulf Hansson wrote:
>>> On Thu, 12 Mar 2020 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> Add extra busy wait and retries if transfer mode switch fails.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/core/mmc_ops.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>>> index aa0cab190cd8..619088a94688 100644
>>>> --- a/drivers/mmc/core/mmc_ops.c
>>>> +++ b/drivers/mmc/core/mmc_ops.c
>>>> @@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>>                 cmd.sanitize_busy = true;
>>>>
>>>>         err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>>>> +       if (err && index == EXT_CSD_HS_TIMING) {
>>>> +               /* Try harder for timing changes */
>>>> +               __mmc_poll_for_busy(card, timeout_ms, send_status,
>>>> +                                   retry_crc_err, MMC_BUSY_CMD6);
>>>> +               err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>>>> +       }
>>>
>>> Hmm, what do you think of moving this to the caller(s) of
>>> __mmc_switch() and in particular only at those places were we find it
>>> useful. Me personally, would prefer that option.
>>>
>>> To do that, we may need to have the possibility of specifying the
>>> number of retries that should be used in the mmc_wait_for_cmd() call
>>> to the caller can check the error code better.
>>>
>>> Moreover, it looks a bit risky to do the polling for all kinds of
>>> errors - shouldn't we do for CRC errors?
>>>
>>
>> What about this then?
> 
> Looks good to me!
> 
> Is the retry in __mmc_switch() with MMC_CMD_RETRIES okay for now you think?

Yes, the additional error handling effectively gives a few more retries, so
that should do for now.


From: Adrian Hunter <adrian.hunter@intel.com>
Date: Fri, 13 Mar 2020 16:01:06 +0200
Subject: [PATCH] mmc: core: Do not change frequency before switch from HS400

Host controllers may support HS400 only at the expected frequency, so
do not change frequency before switch from HS400 but in case of CRC
error, wait for a busy card and then try again. Also try again after
frequency change if the error persists.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c     | 27 +++++++++++++++++++++------
 drivers/mmc/core/mmc_ops.c |  6 +++---
 drivers/mmc/core/mmc_ops.h |  3 +++
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c2abd417a84a..faa5d30ed891 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1235,20 +1235,35 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	int err;
 	u8 val;
 
-	/* Reduce frequency to HS */
-	max_dtr = card->ext_csd.hs_max_dtr;
-	mmc_set_clock(host, max_dtr);
-
 	/* 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,
 			   false, true);
-	if (err)
-		goto out_err;
+	if (err == -EILSEQ) {
+		__mmc_poll_for_busy(card, card->ext_csd.generic_cmd6_time,
+				    false, true, MMC_BUSY_CMD6);
+		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				   EXT_CSD_HS_TIMING, val,
+				   card->ext_csd.generic_cmd6_time, 0, false,
+				   true);
+	}
 
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
+	/* Reduce frequency to HS */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(host, max_dtr);
+
+	if (err) {
+		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				   EXT_CSD_HS_TIMING, val,
+				   card->ext_csd.generic_cmd6_time, 0, false,
+				   true);
+	}
+	if (err)
+		goto out_err;
+
 	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index aa0cab190cd8..8d9c3b97269c 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -484,9 +484,9 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
 	return 0;
 }
 
-static int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
-			       bool send_status, bool retry_crc_err,
-			       enum mmc_busy_cmd busy_cmd)
+int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+			bool send_status, bool retry_crc_err,
+			enum mmc_busy_cmd busy_cmd)
 {
 	struct mmc_host *host = card->host;
 	int err;
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 38dcfeeaf6d5..649985507f87 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -36,6 +36,9 @@ int mmc_interrupt_hpi(struct mmc_card *card);
 int mmc_can_ext_csd(struct mmc_card *card);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
 int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
+int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+			bool send_status, bool retry_crc_err,
+			enum mmc_busy_cmd busy_cmd);
 int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		      enum mmc_busy_cmd busy_cmd);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
-- 
2.17.1

[PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Adrian Hunter]

Add extra busy wait and retries if transfer mode switch fails.

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

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index aa0cab190cd8..619088a94688 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		cmd.sanitize_busy = true;
 
 	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
+	if (err && index == EXT_CSD_HS_TIMING) {
+		/* Try harder for timing changes */
+		__mmc_poll_for_busy(card, timeout_ms, send_status,
+				    retry_crc_err, MMC_BUSY_CMD6);
+		err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
+	}
 	if (err)
 		goto out;
 
-- 
2.17.1