[PATCH v2] Fix HS setting in mmc_hs400_to_hs200()

[PATCH v2] Fix HS setting in mmc_hs400_to_hs200()

[Chaotian Jing]

Change vs v1:
  do not retry CMD6 in __mmc_switch()
  do not move the reduce clock operation after switch HS

Chaotian Jing (2):
  mmc: core: do not retry CMD6 in __mmc_switch()
  mmc: mmc: Fix HS setting in mmc_hs400_to_hs200()

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

-- 
1.8.1.1.dirty

[PATCH v2 1/2] mmc: core: do not retry CMD6 in __mmc_switch()

[Chaotian Jing]

the response type of CMD6 is R1B, when the first CMD6 gets response
CRC error, do retry may get timeout error due to card may still in
busy state, which cause this retry make no sense.

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 9054329..c5208fb 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -562,7 +562,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	if (index == EXT_CSD_SANITIZE_START)
 		cmd.sanitize_busy = true;
 
-	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
+	err = mmc_wait_for_cmd(host, &cmd, 0);
 	if (err)
 		goto out;
 
-- 
1.8.1.1.dirty

[PATCH v2 2/2] mmc: mmc: Fix HS setting in mmc_hs400_to_hs200()

[Chaotian Jing]

mmc_hs400_to_hs200() begins with the card and host in HS400 mode.
before send CMD6 to switch card's timing to HS mode, it reduce clock
frequency to 50Mhz firstly, the original intention of reduce clock
is to make "more stable" when doing HS switch. however,reduce clock
frequency to 50Mhz but without host timming change may cause CMD6
response CRC error. because host is still running at hs400 mode,
and it's hard to find a suitable setting for all eMMC cards when
clock frequency reduced to 50Mhz but card & host still in hs400 mode.

so that We consider that CMD6 response CRC error is not a fatal error,
if host gets CMD6 response CRC error, it means that card has already
received this command.

Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
Fixes: ef3d232245ab ("mmc: mmc: Relax checking for switch errors after HS200 switch")
---
 drivers/mmc/core/mmc.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 09c688f..03d1c17 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1248,8 +1248,25 @@ int mmc_hs400_to_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,
 			   true, false, true);
-	if (err)
-		goto out_err;
+	/*
+	 * as we are on the way to do re-tune, so if the CMD6 got response CRC
+	 * error, do not treat it as error.
+	 */
+	if (err) {
+		if (err == -EILSEQ) {
+			/*
+			 * card will busy after sending out response and host
+			 * driver may not wait busy de-assert when get
+			 * response CRC error. so just wait enough time to
+			 * ensure card leave busy state.
+			 */
+			mmc_delay(card->ext_csd.generic_cmd6_time);
+			pr_debug("%s: %s switch to HS got CRC error\n",
+				 mmc_hostname(host), __func__);
+		} else {
+			goto out_err;
+		}
+	}
 
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
-- 
1.8.1.1.dirty

Re: [PATCH v2] Fix HS setting in mmc_hs400_to_hs200()

[Chaotian Jing]

Hi Ulf,

this is just a ping, do you have any comments of this series of
patches ?

Thx!

On Fri, 2019-02-15 at 13:59 +0800, Chaotian Jing wrote:
> Change vs v1:
>   do not retry CMD6 in __mmc_switch()
>   do not move the reduce clock operation after switch HS
> 
> Chaotian Jing (2):
>   mmc: core: do not retry CMD6 in __mmc_switch()
>   mmc: mmc: Fix HS setting in mmc_hs400_to_hs200()
> 
>  drivers/mmc/core/mmc.c     | 21 +++++++++++++++++++--
>  drivers/mmc/core/mmc_ops.c |  2 +-
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 

Re: [PATCH v2 2/2] mmc: mmc: Fix HS setting in mmc_hs400_to_hs200()

[Ulf Hansson]

On Fri, 15 Feb 2019 at 06:59, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>
> mmc_hs400_to_hs200() begins with the card and host in HS400 mode.
> before send CMD6 to switch card's timing to HS mode, it reduce clock
> frequency to 50Mhz firstly, the original intention of reduce clock
> is to make "more stable" when doing HS switch. however,reduce clock
> frequency to 50Mhz but without host timming change may cause CMD6
> response CRC error. because host is still running at hs400 mode,
> and it's hard to find a suitable setting for all eMMC cards when
> clock frequency reduced to 50Mhz but card & host still in hs400 mode.
>
> so that We consider that CMD6 response CRC error is not a fatal error,
> if host gets CMD6 response CRC error, it means that card has already
> received this command.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> Fixes: ef3d232245ab ("mmc: mmc: Relax checking for switch errors after HS200 switch")
> ---
>  drivers/mmc/core/mmc.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 09c688f..03d1c17 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1248,8 +1248,25 @@ int mmc_hs400_to_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,
>                            true, false, true);
> -       if (err)
> -               goto out_err;
> +       /*
> +        * as we are on the way to do re-tune, so if the CMD6 got response CRC
> +        * error, do not treat it as error.
> +        */
> +       if (err) {
> +               if (err == -EILSEQ) {

Well, I am having seconds thoughts about this. Sorry.

There are different scenarios of why we end up doing a re-tune and
thus call mmc_hs400_to_hs200(). These are:

1) Because of a periodic re-tuning - to avoid CRC errors in the future.
2) Because the host controller needs to do a re-tune when it gets
runtime resumed, due to that it lost its tuning data (and is unable to
restore it).
3) Because we encountered a CRC (-EILSEQ) error for a block I/O
request and want to try re-cover.

I assume you are looking at case 3), because mtk-sd don't use periodic
re-tuning and nor does it need re-tune at runtime resume, right?

So, when thinking a bit about these cases, more closely, I think we
need to admit that these are actually quite fundamentally different
cases. Therefore, we should probably start treat them like that as
well.

For 1), There are no reason to decrease the clock rate before invoking
__mmc_switch(), but rather we should decrease it afterwards instead.
For 2), The clock rate must to be decreased to HS speed (50 MHz),
before invoking __mmc_switch(), as there are really no tuning
parameters available at all to use for the controller.
For 3). Similar to 1) and for the reasons you also have brought up
earlier, the clock rate should remain at HS400 speed before invoking
__mmc_switch(). This is because the controller are still using the
tuned data for the HS400 clock rate. So, even if the conditions
started to become fragile, I think it's better to try with the HS400
MHz than on any other rate.

Moreover, as I stated in an earlier reply, if the call to
__mmc_switch() ends up with a CRC error, how can we ever be sure that
the command was accepted by the card? We can't, hence that solution
will probably never fly.

> +                       /*
> +                        * card will busy after sending out response and host
> +                        * driver may not wait busy de-assert when get
> +                        * response CRC error. so just wait enough time to
> +                        * ensure card leave busy state.
> +                        */
> +                       mmc_delay(card->ext_csd.generic_cmd6_time);
> +                       pr_debug("%s: %s switch to HS got CRC error\n",
> +                                mmc_hostname(host), __func__);
> +               } else {
> +                       goto out_err;
> +               }
> +       }
>
>         mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
>
> --
> 1.8.1.1.dirty
>

If you take into account my comments above, it seems like we need to
add a new tuning flag somewhere, to tell the reason for why the tuning
is needed. Then we can let mmc_hs400_to_hs200() act on that flag, and
depending on its value it can either change the clock rate before or
after the call to __mmc_switch(). If we can make this work, that would
be the best solution, I think.

The fallback method, is to add a specific host cap bit, that instructs
the mmc core to adjust clock rate before/after the switch, but I would
rather avoid this solution, but perhaps there is no other way. Let's
see.

Kind regards
Uffe

Re: [PATCH v2 1/2] mmc: core: do not retry CMD6 in __mmc_switch()

[Ulf Hansson]

On Fri, 15 Feb 2019 at 06:59, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>
> the response type of CMD6 is R1B, when the first CMD6 gets response
> CRC error, do retry may get timeout error due to card may still in
> busy state, which cause this retry make no sense.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>

Applied for next, thanks!

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 9054329..c5208fb 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -562,7 +562,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>         if (index == EXT_CSD_SANITIZE_START)
>                 cmd.sanitize_busy = true;
>
> -       err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> +       err = mmc_wait_for_cmd(host, &cmd, 0);
>         if (err)
>                 goto out;
>
> --
> 1.8.1.1.dirty
>