[PATCH v2] mmc: mmc: do not use CMD13 to get status after speed mode

[PATCH v2] mmc: mmc: do not use CMD13 to get status after speed mode

[Chaotian Jing]

base on Adrian's comment to split it to 2 patches
fix switch timeout issue caused by jiffies

[PATCH 1/3] mmc: mmc: use ops->card_busy() to check card status in __mmc_switch()

[Chaotian Jing]

some MMC host do not support MMC_CAP_WAIT_WHILE_BUSY but provides
ops->card_busy(), So, add this method to check card status after
switch command.

Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
 drivers/mmc/core/mmc_ops.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 62355bd..32de144 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -480,6 +480,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	u32 status = 0;
 	bool use_r1b_resp = use_busy_signal;
 	bool expired = false;
+	bool busy = false;
 
 	mmc_retune_hold(host);
 
@@ -535,19 +536,24 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	/* Must check status to be sure of no errors. */
 	timeout = jiffies + msecs_to_jiffies(timeout_ms);
 	do {
+		/*
+		 * Due to the possibility of being preempted after
+		 * sending the status command, check the expiration
+		 * time first.
+		 */
+		expired = time_after(jiffies, timeout);
 		if (send_status) {
-			/*
-			 * Due to the possibility of being preempted after
-			 * sending the status command, check the expiration
-			 * time first.
-			 */
-			expired = time_after(jiffies, timeout);
 			err = __mmc_send_status(card, &status, ignore_crc);
 			if (err)
 				goto out;
 		}
 		if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
 			break;
+		if (host->ops->card_busy) {
+			if (!host->ops->card_busy(host))
+				break;
+			busy = true;
+		}
 		if (mmc_host_is_spi(host))
 			break;
 
@@ -556,19 +562,20 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		 * does'nt support MMC_CAP_WAIT_WHILE_BUSY, then we can only
 		 * rely on waiting for the stated timeout to be sufficient.
 		 */
-		if (!send_status) {
+		if (!send_status && !host->ops->card_busy) {
 			mmc_delay(timeout_ms);
 			goto out;
 		}
 
 		/* Timeout if the device never leaves the program state. */
-		if (expired && R1_CURRENT_STATE(status) == R1_STATE_PRG) {
+		if (expired &&
+		    (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
 			pr_err("%s: Card stuck in programming state! %s\n",
 				mmc_hostname(host), __func__);
 			err = -ETIMEDOUT;
 			goto out;
 		}
-	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
+	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
 
 	err = mmc_switch_status_error(host, status);
 out:
-- 
1.8.1.1.dirty

Re: [PATCH 1/3] mmc: mmc: use ops->card_busy() to check card status in __mmc_switch()

[Ulf Hansson]

On 19 May 2016 at 10:47, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> some MMC host do not support MMC_CAP_WAIT_WHILE_BUSY but provides
> ops->card_busy(), So, add this method to check card status after
> switch command.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>

Thanks, applied for next! (With some minor updates to the change log)

Kind regards
Uffe

> ---
>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 62355bd..32de144 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -480,6 +480,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>         u32 status = 0;
>         bool use_r1b_resp = use_busy_signal;
>         bool expired = false;
> +       bool busy = false;
>
>         mmc_retune_hold(host);
>
> @@ -535,19 +536,24 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>         /* Must check status to be sure of no errors. */
>         timeout = jiffies + msecs_to_jiffies(timeout_ms);
>         do {
> +               /*
> +                * Due to the possibility of being preempted after
> +                * sending the status command, check the expiration
> +                * time first.
> +                */
> +               expired = time_after(jiffies, timeout);
>                 if (send_status) {
> -                       /*
> -                        * Due to the possibility of being preempted after
> -                        * sending the status command, check the expiration
> -                        * time first.
> -                        */
> -                       expired = time_after(jiffies, timeout);
>                         err = __mmc_send_status(card, &status, ignore_crc);
>                         if (err)
>                                 goto out;
>                 }
>                 if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>                         break;
> +               if (host->ops->card_busy) {
> +                       if (!host->ops->card_busy(host))
> +                               break;
> +                       busy = true;
> +               }
>                 if (mmc_host_is_spi(host))
>                         break;
>
> @@ -556,19 +562,20 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>                  * does'nt support MMC_CAP_WAIT_WHILE_BUSY, then we can only
>                  * rely on waiting for the stated timeout to be sufficient.
>                  */
> -               if (!send_status) {
> +               if (!send_status && !host->ops->card_busy) {
>                         mmc_delay(timeout_ms);
>                         goto out;
>                 }
>
>                 /* Timeout if the device never leaves the program state. */
> -               if (expired && R1_CURRENT_STATE(status) == R1_STATE_PRG) {
> +               if (expired &&
> +                   (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
>                         pr_err("%s: Card stuck in programming state! %s\n",
>                                 mmc_hostname(host), __func__);
>                         err = -ETIMEDOUT;
>                         goto out;
>                 }
> -       } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> +       } while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
>
>         err = mmc_switch_status_error(host, status);
>  out:
> --
> 1.8.1.1.dirty
>

[PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Chaotian Jing]

Per JEDEC spec, it is not recommended to use CMD13 to get card status
after speed mode switch. below are two reason about this:
1. CMD13 cannot be guaranteed due to the asynchronous operation.
Therefore it is not recommended to use CMD13 to check busy completion
of the timing change indication.
2. After switch to HS200, CMD13 will get response of 0x800, and even the
busy signal gets de-asserted, the response of CMD13 is aslo 0x800.

Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
 drivers/mmc/core/mmc.c | 112 ++++++++++++++++++++-----------------------------
 1 file changed, 45 insertions(+), 67 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 4dbe3df..97403a6 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -952,6 +952,19 @@ static int mmc_select_bus_width(struct mmc_card *card)
 	return err;
 }
 
+/* Caller must hold re-tuning */
+static int mmc_switch_status(struct mmc_card *card)
+{
+	u32 status;
+	int err;
+
+	err = mmc_send_status(card, &status);
+	if (err)
+		return err;
+
+	return mmc_switch_status_error(card->host, status);
+}
+
 /*
  * Switch to the high-speed mode
  */
@@ -962,9 +975,11 @@ 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,
-			   true, true, true);
-	if (!err)
+			   true, false, true);
+	if (!err) {
 		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
+		err = mmc_switch_status(card);
+	}
 
 	return err;
 }
@@ -1040,23 +1055,9 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 	return err;
 }
 
-/* Caller must hold re-tuning */
-static int mmc_switch_status(struct mmc_card *card)
-{
-	u32 status;
-	int err;
-
-	err = mmc_send_status(card, &status);
-	if (err)
-		return err;
-
-	return mmc_switch_status_error(card->host, status);
-}
-
 static int mmc_select_hs400(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
-	bool send_status = true;
 	unsigned int max_dtr;
 	int err = 0;
 	u8 val;
@@ -1068,9 +1069,6 @@ static int mmc_select_hs400(struct mmc_card *card)
 	      host->ios.bus_width == MMC_BUS_WIDTH_8))
 		return 0;
 
-	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
-		send_status = false;
-
 	/* Reduce frequency to HS frequency */
 	max_dtr = card->ext_csd.hs_max_dtr;
 	mmc_set_clock(host, max_dtr);
@@ -1080,7 +1078,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time,
-			   true, send_status, true);
+			   true, false, true);
 	if (err) {
 		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1090,11 +1088,9 @@ static int mmc_select_hs400(struct mmc_card *card)
 	/* Set host controller to HS timing */
 	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
 
-	if (!send_status) {
-		err = mmc_switch_status(card);
-		if (err)
-			goto out_err;
-	}
+	err = mmc_switch_status(card);
+	if (err)
+		goto out_err;
 
 	/* Switch card to DDR */
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -1113,7 +1109,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time,
-			   true, send_status, true);
+			   true, false, true);
 	if (err) {
 		pr_err("%s: switch to hs400 failed, err:%d\n",
 			 mmc_hostname(host), err);
@@ -1124,11 +1120,9 @@ static int mmc_select_hs400(struct mmc_card *card)
 	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
 	mmc_set_bus_speed(card);
 
-	if (!send_status) {
-		err = mmc_switch_status(card);
-		if (err)
-			goto out_err;
-	}
+	err = mmc_switch_status(card);
+	if (err)
+		goto out_err;
 
 	return 0;
 
@@ -1146,14 +1140,10 @@ int mmc_hs200_to_hs400(struct mmc_card *card)
 int mmc_hs400_to_hs200(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
-	bool send_status = true;
 	unsigned int max_dtr;
 	int err;
 	u8 val;
 
-	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
-		send_status = false;
-
 	/* Reduce frequency to HS */
 	max_dtr = card->ext_csd.hs_max_dtr;
 	mmc_set_clock(host, max_dtr);
@@ -1162,49 +1152,43 @@ 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,
-			   true, send_status, true);
+			   true, false, true);
 	if (err)
 		goto out_err;
 
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
-	if (!send_status) {
-		err = mmc_switch_status(card);
-		if (err)
-			goto out_err;
-	}
+	err = mmc_switch_status(card);
+	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,
-			   true, send_status, true);
+			   true, false, true);
 	if (err)
 		goto out_err;
 
 	mmc_set_timing(host, MMC_TIMING_MMC_HS);
 
-	if (!send_status) {
-		err = mmc_switch_status(card);
-		if (err)
-			goto out_err;
-	}
+	err = mmc_switch_status(card);
+	if (err)
+		goto out_err;
 
 	/* Switch HS to HS200 */
 	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, true,
-			   send_status, true);
+			   false, true);
 	if (err)
 		goto out_err;
 
 	mmc_set_timing(host, MMC_TIMING_MMC_HS200);
 
-	if (!send_status) {
-		err = mmc_switch_status(card);
-		if (err)
-			goto out_err;
-	}
+	err = mmc_switch_status(card);
+	if (err)
+		goto out_err;
 
 	mmc_set_bus_speed(card);
 
@@ -1243,7 +1227,6 @@ static void mmc_select_driver_type(struct mmc_card *card)
 static int mmc_select_hs200(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
-	bool send_status = true;
 	unsigned int old_timing;
 	int err = -EINVAL;
 	u8 val;
@@ -1260,9 +1243,6 @@ static int mmc_select_hs200(struct mmc_card *card)
 
 	mmc_select_driver_type(card);
 
-	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
-		send_status = false;
-
 	/*
 	 * Set the bus width(4 or 8) with host's support and
 	 * switch to HS200 mode if bus width is set successfully.
@@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 				   EXT_CSD_HS_TIMING, val,
 				   card->ext_csd.generic_cmd6_time,
-				   true, send_status, true);
+				   true, false, true);
 		if (err)
 			goto err;
 		old_timing = host->ios.timing;
 		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
-		if (!send_status) {
-			err = mmc_switch_status(card);
-			/*
-			 * mmc_select_timing() assumes timing has not changed if
-			 * it is a switch error.
-			 */
-			if (err == -EBADMSG)
-				mmc_set_timing(host, old_timing);
-		}
+		err = mmc_switch_status(card);
+		/*
+		 * mmc_select_timing() assumes timing has not changed if
+		 * it is a switch error.
+		 */
+		if (err == -EBADMSG)
+			mmc_set_timing(host, old_timing);
 	}
 err:
 	if (err)
-- 
1.8.1.1.dirty

Re: [PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Ulf Hansson]

On 19 May 2016 at 10:47, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> Per JEDEC spec, it is not recommended to use CMD13 to get card status
> after speed mode switch. below are two reason about this:
> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
> Therefore it is not recommended to use CMD13 to check busy completion
> of the timing change indication.
> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>

Thanks, applied for next! (With a minor change, see below)

[...]

>
>         /* Switch HS to HS200 */
>         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, true,
> -                          send_status, true);
> +                          false, true);

To keep consistency with other calls to __mmc_switch(), I change these lines to:

err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
                       val, card->ext_csd.generic_cmd6_time,
                       true, false, true);

>         if (err)
>                 goto out_err;
>
>         mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>
> -       if (!send_status) {
> -               err = mmc_switch_status(card);
> -               if (err)
> -                       goto out_err;
> -       }
> +       err = mmc_switch_status(card);
> +       if (err)
> +               goto out_err;
>
>         mmc_set_bus_speed(card);
>
> @@ -1243,7 +1227,6 @@ static void mmc_select_driver_type(struct mmc_card *card)
>  static int mmc_select_hs200(struct mmc_card *card)
>  {
>         struct mmc_host *host = card->host;
> -       bool send_status = true;
>         unsigned int old_timing;
>         int err = -EINVAL;
>         u8 val;
> @@ -1260,9 +1243,6 @@ static int mmc_select_hs200(struct mmc_card *card)
>
>         mmc_select_driver_type(card);
>
> -       if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
> -               send_status = false;
> -
>         /*
>          * Set the bus width(4 or 8) with host's support and
>          * switch to HS200 mode if bus width is set successfully.
> @@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                                    EXT_CSD_HS_TIMING, val,
>                                    card->ext_csd.generic_cmd6_time,
> -                                  true, send_status, true);
> +                                  true, false, true);

[...]

Kind regards
Uffe

Re: [PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Bjorn Andersson]

On Thu, May 19, 2016 at 1:47 AM, Chaotian Jing
<chaotian.jing@mediatek.com> wrote:
> Per JEDEC spec, it is not recommended to use CMD13 to get card status
> after speed mode switch. below are two reason about this:
> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
> Therefore it is not recommended to use CMD13 to check busy completion
> of the timing change indication.
> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/core/mmc.c | 112 ++++++++++++++++++++-----------------------------
>  1 file changed, 45 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
[..]
> @@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                                    EXT_CSD_HS_TIMING, val,
>                                    card->ext_csd.generic_cmd6_time,
> -                                  true, send_status, true);
> +                                  true, false, true);
>                 if (err)
>                         goto err;
>                 old_timing = host->ios.timing;
>                 mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> -               if (!send_status) {
> -                       err = mmc_switch_status(card);
> -                       /*
> -                        * mmc_select_timing() assumes timing has not changed if
> -                        * it is a switch error.
> -                        */
> -                       if (err == -EBADMSG)
> -                               mmc_set_timing(host, old_timing);
> -               }
> +               err = 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);

Sorry for not spotting this earlier, but with the move of the call of
mmc_switch_status() to after mmc_set_timing() we get following error
with sdhci-msm:

mmc0: mmc_select_hs200 failed, error -110
mmc0: error -110 whilst initialising MMC card
mmc0: Reset 0x1 never completed.
sdhci: =========== REGISTER DUMP (mmc0)===========
sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
sdhci: Power: 0x00000000 | Blk gap: 0x00000000
sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
sdhci: Host ctl2: 0x00000000
sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
sdhci: ===========================================

But I if I understand the commit correctly this is the intention of
the patch (not the error, but the move).

Regards,
Bjorn

>         }
>  err:
>         if (err)
> --
> 1.8.1.1.dirty
>

Re: [PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Ulf Hansson]

On 16 July 2016 at 00:10, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> On Thu, May 19, 2016 at 1:47 AM, Chaotian Jing
> <chaotian.jing@mediatek.com> wrote:
>> Per JEDEC spec, it is not recommended to use CMD13 to get card status
>> after speed mode switch. below are two reason about this:
>> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
>> Therefore it is not recommended to use CMD13 to check busy completion
>> of the timing change indication.
>> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
>> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
>>
>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>> ---
>>  drivers/mmc/core/mmc.c | 112 ++++++++++++++++++++-----------------------------
>>  1 file changed, 45 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> [..]
>> @@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>                                    EXT_CSD_HS_TIMING, val,
>>                                    card->ext_csd.generic_cmd6_time,
>> -                                  true, send_status, true);
>> +                                  true, false, true);
>>                 if (err)
>>                         goto err;
>>                 old_timing = host->ios.timing;
>>                 mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>> -               if (!send_status) {
>> -                       err = mmc_switch_status(card);
>> -                       /*
>> -                        * mmc_select_timing() assumes timing has not changed if
>> -                        * it is a switch error.
>> -                        */
>> -                       if (err == -EBADMSG)
>> -                               mmc_set_timing(host, old_timing);
>> -               }
>> +               err = 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);
>
> Sorry for not spotting this earlier, but with the move of the call of
> mmc_switch_status() to after mmc_set_timing() we get following error
> with sdhci-msm:

Thanks for reporting!

>
> mmc0: mmc_select_hs200 failed, error -110
> mmc0: error -110 whilst initialising MMC card
> mmc0: Reset 0x1 never completed.
> sdhci: =========== REGISTER DUMP (mmc0)===========
> sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
> sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
> sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
> sdhci: Power: 0x00000000 | Blk gap: 0x00000000
> sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
> sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
> sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
> sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
> sdhci: Host ctl2: 0x00000000
> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
> sdhci: ===========================================
>
> But I if I understand the commit correctly this is the intention of
> the patch (not the error, but the move).

I tried dropping this change from my next branch, but there are some
more changes on top that prevents a "clean" drop. It's certainly
doable, but perhaps we can try to narrow down the problem to see if
this could/should be fixed in the sdhci msm driver instead!?

I also noticed that below submitted change, which *isn't* applied for
next, might be related.
https://patchwork.kernel.org/patch/9197881/

>
> Regards,
> Bjorn
>
>>         }
>>  err:
>>         if (err)
>> --
>> 1.8.1.1.dirty
>>

Kind regards
Uffe

Re: [PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Georgi Djakov]

On 07/18/2016 03:50 PM, Ulf Hansson wrote:
> On 16 July 2016 at 00:10, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>> On Thu, May 19, 2016 at 1:47 AM, Chaotian Jing
>> <chaotian.jing@mediatek.com> wrote:
>>> Per JEDEC spec, it is not recommended to use CMD13 to get card status
>>> after speed mode switch. below are two reason about this:
>>> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
>>> Therefore it is not recommended to use CMD13 to check busy completion
>>> of the timing change indication.
>>> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
>>> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
>>>
>>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>>> ---
>>>  drivers/mmc/core/mmc.c | 112 ++++++++++++++++++++-----------------------------
>>>  1 file changed, 45 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> [..]
>>> @@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>                                    EXT_CSD_HS_TIMING, val,
>>>                                    card->ext_csd.generic_cmd6_time,
>>> -                                  true, send_status, true);
>>> +                                  true, false, true);
>>>                 if (err)
>>>                         goto err;
>>>                 old_timing = host->ios.timing;
>>>                 mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>> -               if (!send_status) {
>>> -                       err = mmc_switch_status(card);
>>> -                       /*
>>> -                        * mmc_select_timing() assumes timing has not changed if
>>> -                        * it is a switch error.
>>> -                        */
>>> -                       if (err == -EBADMSG)
>>> -                               mmc_set_timing(host, old_timing);
>>> -               }
>>> +               err = 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);
>>
>> Sorry for not spotting this earlier, but with the move of the call of
>> mmc_switch_status() to after mmc_set_timing() we get following error
>> with sdhci-msm:
> 
> Thanks for reporting!
> 
>>
>> mmc0: mmc_select_hs200 failed, error -110
>> mmc0: error -110 whilst initialising MMC card
>> mmc0: Reset 0x1 never completed.
>> sdhci: =========== REGISTER DUMP (mmc0)===========
>> sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
>> sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>> sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
>> sdhci: Power: 0x00000000 | Blk gap: 0x00000000
>> sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
>> sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>> sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
>> sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
>> sdhci: Host ctl2: 0x00000000
>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>> sdhci: ===========================================
>>
>> But I if I understand the commit correctly this is the intention of
>> the patch (not the error, but the move).
> 
> I tried dropping this change from my next branch, but there are some
> more changes on top that prevents a "clean" drop. It's certainly
> doable, but perhaps we can try to narrow down the problem to see if
> this could/should be fixed in the sdhci msm driver instead!?

Yes, i am working on it. Seems that if we just ignore the -ETIMEDOUT
returned by mmc_switch_status() everything works as before. Currently
i am expecting more information about this controller specifics, in
order to understand if this could be fixed with a change in the driver
only.

> 
> I also noticed that below submitted change, which *isn't* applied for
> next, might be related.
> https://patchwork.kernel.org/patch/9197881/
> 

It is not related to this issue, but it is good to have when this is
resolved, otherwise we will see the dumpregs output (pr_err now),
when controller is initialized without a card in the SD slot.

Thanks,
Georgi

Re: [PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Ulf Hansson]

On 18 July 2016 at 15:38, Georgi Djakov <georgi.djakov@linaro.org> wrote:
> On 07/18/2016 03:50 PM, Ulf Hansson wrote:
>> On 16 July 2016 at 00:10, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>> On Thu, May 19, 2016 at 1:47 AM, Chaotian Jing
>>> <chaotian.jing@mediatek.com> wrote:
>>>> Per JEDEC spec, it is not recommended to use CMD13 to get card status
>>>> after speed mode switch. below are two reason about this:
>>>> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
>>>> Therefore it is not recommended to use CMD13 to check busy completion
>>>> of the timing change indication.
>>>> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
>>>> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
>>>>
>>>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>>>> ---
>>>>  drivers/mmc/core/mmc.c | 112 ++++++++++++++++++++-----------------------------
>>>>  1 file changed, 45 insertions(+), 67 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> [..]
>>>> @@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>>                                    EXT_CSD_HS_TIMING, val,
>>>>                                    card->ext_csd.generic_cmd6_time,
>>>> -                                  true, send_status, true);
>>>> +                                  true, false, true);
>>>>                 if (err)
>>>>                         goto err;
>>>>                 old_timing = host->ios.timing;
>>>>                 mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>>> -               if (!send_status) {
>>>> -                       err = mmc_switch_status(card);
>>>> -                       /*
>>>> -                        * mmc_select_timing() assumes timing has not changed if
>>>> -                        * it is a switch error.
>>>> -                        */
>>>> -                       if (err == -EBADMSG)
>>>> -                               mmc_set_timing(host, old_timing);
>>>> -               }
>>>> +               err = 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);
>>>
>>> Sorry for not spotting this earlier, but with the move of the call of
>>> mmc_switch_status() to after mmc_set_timing() we get following error
>>> with sdhci-msm:
>>
>> Thanks for reporting!
>>
>>>
>>> mmc0: mmc_select_hs200 failed, error -110
>>> mmc0: error -110 whilst initialising MMC card
>>> mmc0: Reset 0x1 never completed.
>>> sdhci: =========== REGISTER DUMP (mmc0)===========
>>> sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
>>> sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
>>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>>> sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
>>> sdhci: Power: 0x00000000 | Blk gap: 0x00000000
>>> sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
>>> sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
>>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>>> sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
>>> sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
>>> sdhci: Host ctl2: 0x00000000
>>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>>> sdhci: ===========================================
>>>
>>> But I if I understand the commit correctly this is the intention of
>>> the patch (not the error, but the move).
>>
>> I tried dropping this change from my next branch, but there are some
>> more changes on top that prevents a "clean" drop. It's certainly
>> doable, but perhaps we can try to narrow down the problem to see if
>> this could/should be fixed in the sdhci msm driver instead!?
>
> Yes, i am working on it. Seems that if we just ignore the -ETIMEDOUT
> returned by mmc_switch_status() everything works as before. Currently
> i am expecting more information about this controller specifics, in
> order to understand if this could be fixed with a change in the driver
> only.

Thanks for looking into this!

>
>>
>> I also noticed that below submitted change, which *isn't* applied for
>> next, might be related.
>> https://patchwork.kernel.org/patch/9197881/
>>
>
> It is not related to this issue, but it is good to have when this is
> resolved, otherwise we will see the dumpregs output (pr_err now),
> when controller is initialized without a card in the SD slot.

Okay, thanks for clarifying. Although, I need an ack from Adrian
before I apply it.

>
> Thanks,
> Georgi

Kind regards
Uffe

Re: [PATCH 2/3] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Georgi Djakov]

On 07/19/2016 12:31 PM, Ulf Hansson wrote:
> On 18 July 2016 at 15:38, Georgi Djakov <georgi.djakov@linaro.org> wrote:
>> On 07/18/2016 03:50 PM, Ulf Hansson wrote:
>>> On 16 July 2016 at 00:10, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>>> On Thu, May 19, 2016 at 1:47 AM, Chaotian Jing
>>>> <chaotian.jing@mediatek.com> wrote:
>>>>> Per JEDEC spec, it is not recommended to use CMD13 to get card status
>>>>> after speed mode switch. below are two reason about this:
>>>>> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
>>>>> Therefore it is not recommended to use CMD13 to check busy completion
>>>>> of the timing change indication.
>>>>> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
>>>>> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
>>>>>
>>>>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>>>>> ---
>>>>>  drivers/mmc/core/mmc.c | 112 ++++++++++++++++++++-----------------------------
>>>>>  1 file changed, 45 insertions(+), 67 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> [..]
>>>>> @@ -1274,20 +1254,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>>>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>>>                                    EXT_CSD_HS_TIMING, val,
>>>>>                                    card->ext_csd.generic_cmd6_time,
>>>>> -                                  true, send_status, true);
>>>>> +                                  true, false, true);
>>>>>                 if (err)
>>>>>                         goto err;
>>>>>                 old_timing = host->ios.timing;
>>>>>                 mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>>>> -               if (!send_status) {
>>>>> -                       err = mmc_switch_status(card);
>>>>> -                       /*
>>>>> -                        * mmc_select_timing() assumes timing has not changed if
>>>>> -                        * it is a switch error.
>>>>> -                        */
>>>>> -                       if (err == -EBADMSG)
>>>>> -                               mmc_set_timing(host, old_timing);
>>>>> -               }
>>>>> +               err = 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);
>>>>
>>>> Sorry for not spotting this earlier, but with the move of the call of
>>>> mmc_switch_status() to after mmc_set_timing() we get following error
>>>> with sdhci-msm:
>>>
>>> Thanks for reporting!
>>>
>>>>
>>>> mmc0: mmc_select_hs200 failed, error -110
>>>> mmc0: error -110 whilst initialising MMC card
>>>> mmc0: Reset 0x1 never completed.
>>>> sdhci: =========== REGISTER DUMP (mmc0)===========
>>>> sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
>>>> sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
>>>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>>>> sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
>>>> sdhci: Power: 0x00000000 | Blk gap: 0x00000000
>>>> sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
>>>> sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
>>>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>>>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>>>> sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
>>>> sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
>>>> sdhci: Host ctl2: 0x00000000
>>>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>>>> sdhci: ===========================================
>>>>
>>>> But I if I understand the commit correctly this is the intention of
>>>> the patch (not the error, but the move).
>>>
>>> I tried dropping this change from my next branch, but there are some
>>> more changes on top that prevents a "clean" drop. It's certainly
>>> doable, but perhaps we can try to narrow down the problem to see if
>>> this could/should be fixed in the sdhci msm driver instead!?
>>
>> Yes, i am working on it. Seems that if we just ignore the -ETIMEDOUT
>> returned by mmc_switch_status() everything works as before. Currently
>> i am expecting more information about this controller specifics, in
>> order to understand if this could be fixed with a change in the driver
>> only.
> 
> Thanks for looking into this!
> 

Here is a partial fix, as it resolves the issue for the eMMC only.
https://patchwork.kernel.org/patch/9237679/
More investigation ongoing.

Thanks,
Georgi

>>
>>>
>>> I also noticed that below submitted change, which *isn't* applied for
>>> next, might be related.
>>> https://patchwork.kernel.org/patch/9197881/
>>>
>>
>> It is not related to this issue, but it is good to have when this is
>> resolved, otherwise we will see the dumpregs output (pr_err now),
>> when controller is initialized without a card in the SD slot.
> 
> Okay, thanks for clarifying. Although, I need an ack from Adrian
> before I apply it.
> 
>>
>> Thanks,
>> Georgi
> 
> Kind regards
> Uffe
> 

[PATCH 3/3] mmc: mmc: fix switch timeout issue caused by jiffies precision

[Chaotian Jing]

with CONFIG_HZ=100, the precision of jiffies is 10ms, and the
generic_cmd6_time of some card is also 10ms. then, may be current
time is only 5ms, but already timed out caused by jiffies precision.

Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
 drivers/mmc/core/mmc_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 32de144..ad6e979 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -534,7 +534,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		timeout_ms = MMC_OPS_TIMEOUT_MS;
 
 	/* Must check status to be sure of no errors. */
-	timeout = jiffies + msecs_to_jiffies(timeout_ms);
+	timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
 	do {
 		/*
 		 * Due to the possibility of being preempted after
-- 
1.8.1.1.dirty

Re: [PATCH 3/3] mmc: mmc: fix switch timeout issue caused by jiffies precision

[Ulf Hansson]

On 19 May 2016 at 10:47, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> with CONFIG_HZ=100, the precision of jiffies is 10ms, and the
> generic_cmd6_time of some card is also 10ms. then, may be current
> time is only 5ms, but already timed out caused by jiffies precision.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/core/mmc_ops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 32de144..ad6e979 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -534,7 +534,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>                 timeout_ms = MMC_OPS_TIMEOUT_MS;
>
>         /* Must check status to be sure of no errors. */
> -       timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +       timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
>         do {
>                 /*
>                  * Due to the possibility of being preempted after
> --
> 1.8.1.1.dirty
>