[PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Wolfram Sang]

After my hackish RFC patch, here is a small series implementing
(hopefully) the solution we discussed. It will make
mmc_send_abort_tuning() also send CMD12 for SD cards which makes more SD
cards work for us. Details are in the patch descriptions.

Please let me know what you think.

Thanks, and happy hacking!


Wolfram Sang (3):
  mmc: core: add helper to send STOP
  mmc: core: also abort tuning with CMD12 for SD
  mmc: core: remove obsolete parameter from mmc_send_abort_tuning

 drivers/mmc/core/block.c             | 14 +-------------
 drivers/mmc/core/core.h              |  1 +
 drivers/mmc/core/mmc.c               |  6 ++++++
 drivers/mmc/core/mmc_ops.c           | 23 ++++-------------------
 drivers/mmc/core/mmc_ops.h           | 14 ++++++++++++++
 drivers/mmc/core/sd.c                |  6 ++++++
 drivers/mmc/host/renesas_sdhi_core.c |  2 +-
 drivers/mmc/host/sdhci.c             |  2 +-
 include/linux/mmc/host.h             |  2 +-
 9 files changed, 35 insertions(+), 35 deletions(-)

-- 
2.30.2

[PATCH 1/3] mmc: core: add helper to send STOP

[Wolfram Sang]

There was a helper in the block layer already, but we need it in other
parts soon as well. So, make it more generic by adding the 'retries'
parameter and add the helper to mmc_ops.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/block.c   | 14 +-------------
 drivers/mmc/core/mmc_ops.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 431af5e8be2f..58f1aa5ac33f 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1642,18 +1642,6 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 #define MMC_DATA_RETRIES	2
 #define MMC_NO_RETRIES		(MMC_MAX_RETRIES + 1)
 
-static int mmc_blk_send_stop(struct mmc_card *card, unsigned int timeout)
-{
-	struct mmc_command cmd = {
-		.opcode = MMC_STOP_TRANSMISSION,
-		.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC,
-		/* Some hosts wait for busy anyway, so provide a busy timeout */
-		.busy_timeout = timeout,
-	};
-
-	return mmc_wait_for_cmd(card->host, &cmd, 5);
-}
-
 static int mmc_blk_fix_state(struct mmc_card *card, struct request *req)
 {
 	struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
@@ -1663,7 +1651,7 @@ static int mmc_blk_fix_state(struct mmc_card *card, struct request *req)
 
 	mmc_retune_hold_now(card->host);
 
-	mmc_blk_send_stop(card, timeout);
+	mmc_send_stop(card->host, timeout, 5);
 
 	err = mmc_poll_for_busy(card, timeout, false, MMC_BUSY_IO);
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index ae25ffc2e870..6e9d1b6b9e55 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -9,6 +9,7 @@
 #define _MMC_MMC_OPS_H
 
 #include <linux/types.h>
+#include <linux/mmc/mmc.h>
 
 enum mmc_busy_cmd {
 	MMC_BUSY_CMD6,
@@ -57,5 +58,18 @@ int mmc_cmdq_enable(struct mmc_card *card);
 int mmc_cmdq_disable(struct mmc_card *card);
 int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms);
 
+static inline int mmc_send_stop(struct mmc_host *host, unsigned int timeout,
+			   unsigned int retries)
+{
+	struct mmc_command cmd = {
+		.opcode = MMC_STOP_TRANSMISSION,
+		.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC,
+		/* Some hosts wait for busy anyway, so provide a busy timeout */
+		.busy_timeout = timeout,
+	};
+
+	return mmc_wait_for_cmd(host, &cmd, retries);
+}
+
 #endif
 
-- 
2.30.2

Re: [PATCH 1/3] mmc: core: add helper to send STOP

[Ulf Hansson]

On Tue, 14 Sept 2021 at 20:20, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> There was a helper in the block layer already, but we need it in other
> parts soon as well. So, make it more generic by adding the 'retries'
> parameter and add the helper to mmc_ops.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/core/block.c   | 14 +-------------
>  drivers/mmc/core/mmc_ops.h | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 431af5e8be2f..58f1aa5ac33f 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1642,18 +1642,6 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>  #define MMC_DATA_RETRIES       2
>  #define MMC_NO_RETRIES         (MMC_MAX_RETRIES + 1)
>
> -static int mmc_blk_send_stop(struct mmc_card *card, unsigned int timeout)
> -{
> -       struct mmc_command cmd = {
> -               .opcode = MMC_STOP_TRANSMISSION,
> -               .flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC,
> -               /* Some hosts wait for busy anyway, so provide a busy timeout */
> -               .busy_timeout = timeout,
> -       };
> -
> -       return mmc_wait_for_cmd(card->host, &cmd, 5);
> -}
> -
>  static int mmc_blk_fix_state(struct mmc_card *card, struct request *req)
>  {
>         struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
> @@ -1663,7 +1651,7 @@ static int mmc_blk_fix_state(struct mmc_card *card, struct request *req)
>
>         mmc_retune_hold_now(card->host);
>
> -       mmc_blk_send_stop(card, timeout);
> +       mmc_send_stop(card->host, timeout, 5);
>
>         err = mmc_poll_for_busy(card, timeout, false, MMC_BUSY_IO);
>
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index ae25ffc2e870..6e9d1b6b9e55 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -9,6 +9,7 @@
>  #define _MMC_MMC_OPS_H
>
>  #include <linux/types.h>
> +#include <linux/mmc/mmc.h>
>
>  enum mmc_busy_cmd {
>         MMC_BUSY_CMD6,
> @@ -57,5 +58,18 @@ int mmc_cmdq_enable(struct mmc_card *card);
>  int mmc_cmdq_disable(struct mmc_card *card);
>  int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms);
>
> +static inline int mmc_send_stop(struct mmc_host *host, unsigned int timeout,

Nitpick: Would you mind renaming timeout to timeout_ms, as to clarify its unit.

> +                          unsigned int retries)
> +{
> +       struct mmc_command cmd = {
> +               .opcode = MMC_STOP_TRANSMISSION,
> +               .flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC,
> +               /* Some hosts wait for busy anyway, so provide a busy timeout */
> +               .busy_timeout = timeout,
> +       };
> +
> +       return mmc_wait_for_cmd(host, &cmd, retries);
> +}
> +
>  #endif
>
> --
> 2.30.2
>

Otherwise, this looks good to me!

Kind regards
Uffe

[PATCH 2/3] mmc: core: also abort tuning with CMD12 for SD

[Wolfram Sang]

We have various SanDisk cards which fail tuning to SDR104 unless we
allow a CMD12 also to be sent to abort a broken tuning. It is true that
the SD specs do not mention that CMD12 is allowed, but they also don't
say it is forbidden. And now reality tells that it is needed to make
some cards work. Other cards I tried did not regress.

So, add a new callback to the bus_ops which allows us to send STOP for
MMC and SD cards but not SDIO which does not support CMD12.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Tested with a Renesas Salvator-XS board (R-Car H3 ES2.0) and a SanDisk
Ultra 64GB card.

 drivers/mmc/core/core.h    |  1 +
 drivers/mmc/core/mmc.c     |  6 ++++++
 drivers/mmc/core/mmc_ops.c | 21 +++------------------
 drivers/mmc/core/sd.c      |  6 ++++++
 4 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 7931a4f0137d..660873ba13ed 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -31,6 +31,7 @@ struct mmc_bus_ops {
 	int (*sw_reset)(struct mmc_host *);
 	bool (*cache_enabled)(struct mmc_host *);
 	int (*flush_cache)(struct mmc_host *);
+	int (*send_abort_tuning)(struct mmc_host *);
 };
 
 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 29e58ffae379..d638477e0a49 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2227,6 +2227,11 @@ static int _mmc_hw_reset(struct mmc_host *host)
 	return mmc_init_card(host, card->ocr, card);
 }
 
+static int _mmc_send_abort_tuning(struct mmc_host *host)
+{
+	return mmc_send_stop(host, 150, 0);
+}
+
 static const struct mmc_bus_ops mmc_ops = {
 	.remove = mmc_remove,
 	.detect = mmc_detect,
@@ -2239,6 +2244,7 @@ static const struct mmc_bus_ops mmc_ops = {
 	.hw_reset = _mmc_hw_reset,
 	.cache_enabled = _mmc_cache_enabled,
 	.flush_cache = _mmc_flush_cache,
+	.send_abort_tuning = _mmc_send_abort_tuning,
 };
 
 /*
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 0c54858e89c0..bc794419d443 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -705,26 +705,11 @@ EXPORT_SYMBOL_GPL(mmc_send_tuning);
 
 int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode)
 {
-	struct mmc_command cmd = {};
-
-	/*
-	 * eMMC specification specifies that CMD12 can be used to stop a tuning
-	 * command, but SD specification does not, so do nothing unless it is
-	 * eMMC.
-	 */
-	if (opcode != MMC_SEND_TUNING_BLOCK_HS200)
-		return 0;
+	if (host->bus_ops->send_abort_tuning)
+		return host->bus_ops->send_abort_tuning(host);
 
-	cmd.opcode = MMC_STOP_TRANSMISSION;
-	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
-
-	/*
-	 * For drivers that override R1 to R1b, set an arbitrary timeout based
-	 * on the tuning timeout i.e. 150ms.
-	 */
-	cmd.busy_timeout = 150;
+	return 0;
 
-	return mmc_wait_for_cmd(host, &cmd, 0);
 }
 EXPORT_SYMBOL_GPL(mmc_send_abort_tuning);
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 4646b7a03db6..19c85e6088f4 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1784,6 +1784,11 @@ static int mmc_sd_hw_reset(struct mmc_host *host)
 	return mmc_sd_init_card(host, host->card->ocr, host->card);
 }
 
+static int sd_send_abort_tuning(struct mmc_host *host)
+{
+	return mmc_send_stop(host, 150, 0);
+}
+
 static const struct mmc_bus_ops mmc_sd_ops = {
 	.remove = mmc_sd_remove,
 	.detect = mmc_sd_detect,
@@ -1796,6 +1801,7 @@ static const struct mmc_bus_ops mmc_sd_ops = {
 	.hw_reset = mmc_sd_hw_reset,
 	.cache_enabled = sd_cache_enabled,
 	.flush_cache = sd_flush_cache,
+	.send_abort_tuning = sd_send_abort_tuning,
 };
 
 /*
-- 
2.30.2

Re: [PATCH 2/3] mmc: core: also abort tuning with CMD12 for SD

[Ulf Hansson]

On Tue, 14 Sept 2021 at 20:21, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> We have various SanDisk cards which fail tuning to SDR104 unless we
> allow a CMD12 also to be sent to abort a broken tuning. It is true that
> the SD specs do not mention that CMD12 is allowed, but they also don't
> say it is forbidden. And now reality tells that it is needed to make
> some cards work. Other cards I tried did not regress.
>
> So, add a new callback to the bus_ops which allows us to send STOP for
> MMC and SD cards but not SDIO which does not support CMD12.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Tested with a Renesas Salvator-XS board (R-Car H3 ES2.0) and a SanDisk
> Ultra 64GB card.
>
>  drivers/mmc/core/core.h    |  1 +
>  drivers/mmc/core/mmc.c     |  6 ++++++
>  drivers/mmc/core/mmc_ops.c | 21 +++------------------
>  drivers/mmc/core/sd.c      |  6 ++++++
>  4 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 7931a4f0137d..660873ba13ed 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -31,6 +31,7 @@ struct mmc_bus_ops {
>         int (*sw_reset)(struct mmc_host *);
>         bool (*cache_enabled)(struct mmc_host *);
>         int (*flush_cache)(struct mmc_host *);
> +       int (*send_abort_tuning)(struct mmc_host *);
>  };
>
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 29e58ffae379..d638477e0a49 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -2227,6 +2227,11 @@ static int _mmc_hw_reset(struct mmc_host *host)
>         return mmc_init_card(host, card->ocr, card);
>  }
>
> +static int _mmc_send_abort_tuning(struct mmc_host *host)
> +{
> +       return mmc_send_stop(host, 150, 0);

Perhaps also add/copy the comment about why we use 150 ms as timeout here.

> +}
> +
>  static const struct mmc_bus_ops mmc_ops = {
>         .remove = mmc_remove,
>         .detect = mmc_detect,
> @@ -2239,6 +2244,7 @@ static const struct mmc_bus_ops mmc_ops = {
>         .hw_reset = _mmc_hw_reset,
>         .cache_enabled = _mmc_cache_enabled,
>         .flush_cache = _mmc_flush_cache,
> +       .send_abort_tuning = _mmc_send_abort_tuning,
>  };
>
>  /*
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 0c54858e89c0..bc794419d443 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -705,26 +705,11 @@ EXPORT_SYMBOL_GPL(mmc_send_tuning);
>
>  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode)
>  {
> -       struct mmc_command cmd = {};
> -
> -       /*
> -        * eMMC specification specifies that CMD12 can be used to stop a tuning
> -        * command, but SD specification does not, so do nothing unless it is
> -        * eMMC.
> -        */
> -       if (opcode != MMC_SEND_TUNING_BLOCK_HS200)
> -               return 0;
> +       if (host->bus_ops->send_abort_tuning)
> +               return host->bus_ops->send_abort_tuning(host);
>
> -       cmd.opcode = MMC_STOP_TRANSMISSION;
> -       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> -
> -       /*
> -        * For drivers that override R1 to R1b, set an arbitrary timeout based
> -        * on the tuning timeout i.e. 150ms.
> -        */
> -       cmd.busy_timeout = 150;
> +       return 0;
>
> -       return mmc_wait_for_cmd(host, &cmd, 0);
>  }
>  EXPORT_SYMBOL_GPL(mmc_send_abort_tuning);
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 4646b7a03db6..19c85e6088f4 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1784,6 +1784,11 @@ static int mmc_sd_hw_reset(struct mmc_host *host)
>         return mmc_sd_init_card(host, host->card->ocr, host->card);
>  }
>
> +static int sd_send_abort_tuning(struct mmc_host *host)
> +{
> +       return mmc_send_stop(host, 150, 0);

Perhaps also add/copy the comment about why we use 150 ms as timeout here.

> +}
> +
>  static const struct mmc_bus_ops mmc_sd_ops = {
>         .remove = mmc_sd_remove,
>         .detect = mmc_sd_detect,
> @@ -1796,6 +1801,7 @@ static const struct mmc_bus_ops mmc_sd_ops = {
>         .hw_reset = mmc_sd_hw_reset,
>         .cache_enabled = sd_cache_enabled,
>         .flush_cache = sd_flush_cache,
> +       .send_abort_tuning = sd_send_abort_tuning,
>  };
>
>  /*
> --
> 2.30.2
>

Besides the minor nitpicks, this looks good to me!

Kind regards
Uffe

Re: [PATCH 2/3] mmc: core: also abort tuning with CMD12 for SD

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

Hi Ulf,

> Besides the minor nitpicks, this looks good to me!

Thanks for the review! However, I acquired new data which could mean
that this series is maybe really a workaround. Let's put it on hold for
now until I have found out more.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] mmc: core: remove obsolete parameter from mmc_send_abort_tuning

[Wolfram Sang]

Due to refactoring, this is not needed anymore. Remove it from the core
and from drivers.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/mmc_ops.c           | 2 +-
 drivers/mmc/host/renesas_sdhi_core.c | 2 +-
 drivers/mmc/host/sdhci.c             | 2 +-
 include/linux/mmc/host.h             | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index bc794419d443..d60750d0f1b9 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -703,7 +703,7 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error)
 }
 EXPORT_SYMBOL_GPL(mmc_send_tuning);
 
-int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode)
+int mmc_send_abort_tuning(struct mmc_host *host)
 {
 	if (host->bus_ops->send_abort_tuning)
 		return host->bus_ops->send_abort_tuning(host);
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 6fc4cf3c9dce..8884ff392935 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -683,7 +683,7 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode)
 			set_bit(i, priv->smpcmp);
 
 		if (cmd_error)
-			mmc_send_abort_tuning(mmc, opcode);
+			mmc_send_abort_tuning(mmc);
 	}
 
 	ret = renesas_sdhi_select_tuning(host);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8eefa7d5fe85..8efc029ee21b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2685,7 +2685,7 @@ void sdhci_abort_tuning(struct sdhci_host *host, u32 opcode)
 
 	sdhci_end_tuning(host);
 
-	mmc_send_abort_tuning(host->mmc, opcode);
+	mmc_send_abort_tuning(host->mmc);
 }
 EXPORT_SYMBOL_GPL(sdhci_abort_tuning);
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c0c9a0fdf57..890dc5c5ffce 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -633,6 +633,6 @@ static inline enum dma_data_direction mmc_get_dma_dir(struct mmc_data *data)
 }
 
 int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
-int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
+int mmc_send_abort_tuning(struct mmc_host *host);
 
 #endif /* LINUX_MMC_HOST_H */
-- 
2.30.2

Re: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Sep 14, 2021 at 8:22 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> After my hackish RFC patch, here is a small series implementing
> (hopefully) the solution we discussed. It will make
> mmc_send_abort_tuning() also send CMD12 for SD cards which makes more SD
> cards work for us. Details are in the patch descriptions.
>
> Please let me know what you think.
>
> Thanks, and happy hacking!
>
>
> Wolfram Sang (3):
>   mmc: core: add helper to send STOP
>   mmc: core: also abort tuning with CMD12 for SD
>   mmc: core: remove obsolete parameter from mmc_send_abort_tuning

Thanks for your series!

I gave this a quick boot test on the Gose and ALT in Magnus' farm, but
the issues are still there.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]


> I gave this a quick boot test on the Gose and ALT in Magnus' farm, but
> the issues are still there.

Pity :( It doesn't fix all cards, but it fixes the SanDisk cards which
the BSP team and I have.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Christian Löhle]

I did not test the patch but want to make you aware of the comment in dw_mmc:
/*
* During UHS tuning sequence, sending the stop
* command after the response CRC error would
* throw the system into a confused state
* causing all future tuning phases to report
* failure.
*
* In such case controller will move into a data
* transfer state after a response error or
* response CRC error. Let's let that finish
* before trying to send a stop, so we'll go to
* STATE_SENDING_DATA.
*
* Although letting the data transfer take place
* will waste a bit of time (we already know
* the command was bad), it can't cause any
* errors since it's possible it would have
* taken place anyway if this tasklet got
* delayed. Allowing the transfer to take place
* avoids races and keeps things simple.
*/
The message in 46d179525a1f6d16957dcb4624517bc04142b3e7
does not mention which card was causing problem, unfortunately.




From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Sent: Tuesday, September 14, 2021 8:20 PM
To: linux-mmc@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org; Yoshihiro Shimoda; Wolfram Sang
Subject: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD
    
After my hackish RFC patch, here is a small series implementing
(hopefully) the solution we discussed. It will make
mmc_send_abort_tuning() also send CMD12 for SD cards which makes more SD
cards work for us. Details are in the patch descriptions.

Please let me know what you think.

Thanks, and happy hacking!


Wolfram Sang (3):
  mmc: core: add helper to send STOP
  mmc: core: also abort tuning with CMD12 for SD
  mmc: core: remove obsolete parameter from mmc_send_abort_tuning

 drivers/mmc/core/block.c             | 14 +-------------
 drivers/mmc/core/core.h              |  1 +
 drivers/mmc/core/mmc.c               |  6 ++++++
 drivers/mmc/core/mmc_ops.c           | 23 ++++-------------------
 drivers/mmc/core/mmc_ops.h           | 14 ++++++++++++++
 drivers/mmc/core/sd.c                |  6 ++++++
 drivers/mmc/host/renesas_sdhi_core.c |  2 +-
 drivers/mmc/host/sdhci.c             |  2 +-
 include/linux/mmc/host.h             |  2 +-
 9 files changed, 35 insertions(+), 35 deletions(-)

-- 
2.30.2

    =
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

On Wed, Sep 15, 2021 at 08:50:21AM +0000, Christian Löhle wrote:
> I did not test the patch but want to make you aware of the comment in dw_mmc:
> /*
> * During UHS tuning sequence, sending the stop
> * command after the response CRC error would
> * throw the system into a confused state
> * causing all future tuning phases to report
> * failure.
> *
> * In such case controller will move into a data
> * transfer state after a response error or
> * response CRC error. Let's let that finish
> * before trying to send a stop, so we'll go to
> * STATE_SENDING_DATA.
> *
> * Although letting the data transfer take place
> * will waste a bit of time (we already know
> * the command was bad), it can't cause any
> * errors since it's possible it would have
> * taken place anyway if this tasklet got
> * delayed. Allowing the transfer to take place
> * avoids races and keeps things simple.
> */
> The message in 46d179525a1f6d16957dcb4624517bc04142b3e7
> does not mention which card was causing problem, unfortunately.

Thank you for the pointer! This is interesting, in deed.

As I understand, it is a limitation of the controller which always goes
into data transfer state even after CRC errors. However, because the
controller driver is not using mmc_send_abort_data(), it will not be
affected by my patch series. My patch series only extends the optional
MMC core helper to be used for SD cards, too, in addition to eMMC.

If I missed something else, please let me know.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Avri Altman]

Hi,
 
> After my hackish RFC patch, here is a small series implementing
> (hopefully) the solution we discussed. It will make
> mmc_send_abort_tuning() also send CMD12 for SD cards which makes more
> SD
> cards work for us. Details are in the patch descriptions.
> 
> Please let me know what you think.
> 
> Thanks, and happy hacking!
I made note of your patch series to our SD (hw) guys, and here is what they say:

"We are ok with host sending CMD12 to abort data transfer when they discover failure with response / incoming data. 
In both SD/eMMC spec, stop transmission command is allowed during data transfer phase ('data' state).
Sometimes, the CMD12 would have been received by card while in 'tran' state. As long host is able to handle the 'illegal command' error indication for this situation, we don't see any other problem. 

Per SD Spec, CMD12 is allowed in 'tran' state only for SDUC card. In non SDUC cards, CMD12 received while in 'tran' state will be treated as illegal command.

However we could not understand how aborting the data transfer is helping host to complete the tuning scheme and have successful read / write operations."

They also think that :
" we believe this hack was added to avoid the data transfer after response crc error...
Receiving CRC error with the tuning pattern would be normal as long as the tuning was not complete."

My 5 cents are, maybe you should try retries > 0 in sd_send_abort_tuning,
If indeed it's a crc while tuning is not complete.

Thanks,
Avri 

> 
> 
> Wolfram Sang (3):
>   mmc: core: add helper to send STOP
>   mmc: core: also abort tuning with CMD12 for SD
>   mmc: core: remove obsolete parameter from mmc_send_abort_tuning
> 
>  drivers/mmc/core/block.c             | 14 +-------------
>  drivers/mmc/core/core.h              |  1 +
>  drivers/mmc/core/mmc.c               |  6 ++++++
>  drivers/mmc/core/mmc_ops.c           | 23 ++++-------------------
>  drivers/mmc/core/mmc_ops.h           | 14 ++++++++++++++
>  drivers/mmc/core/sd.c                |  6 ++++++
>  drivers/mmc/host/renesas_sdhi_core.c |  2 +-
>  drivers/mmc/host/sdhci.c             |  2 +-
>  include/linux/mmc/host.h             |  2 +-
>  9 files changed, 35 insertions(+), 35 deletions(-)
> 
> --
> 2.30.2

Re: [PATCH 0/3] mmc: also abort tuning with CMD12 for SD

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2438 bytes --]

Hi Avri,

first of all, thank you very much for asking your SD experts and
providing this valuable information! This is much appreciated. Sadly, I
have only indirect access to the SD specs and more advanced measurement
techniques.

> "We are ok with host sending CMD12 to abort data transfer when they
> discover failure with response / incoming data. In both SD/eMMC spec,
> stop transmission command is allowed during data transfer phase
> ('data' state).

Yes. The spec is clear about the data state.

> Sometimes, the CMD12 would have been received by card while in 'tran'
> state. As long host is able to handle the 'illegal command' error
> indication for this situation, we don't see any other problem. 

Confirmed. The call to mmc_send_abort_tuning() returns -EILSEQ. But it
still makes the card work. Leaving out the call to
mmc_send_abort_tuning() gives:

	mmc1: error -5 whilst initialising SD card

> Per SD Spec, CMD12 is allowed in 'tran' state only for SDUC card. In
> non SDUC cards, CMD12 received while in 'tran' state will be treated
> as illegal command.

I didn't know about SDUC. The advantage of the approach Ulf suggested is
that we can distinguish the type of cards in the SD callback if needed.

> However we could not understand how aborting the data transfer is
> helping host to complete the tuning scheme and have successful read /
> write operations."

I also didn't know why. But read on, there is a pointer now.

> They also think that :
> " we believe this hack was added to avoid the data transfer after response crc error...
> Receiving CRC error with the tuning pattern would be normal as long as the tuning was not complete."

Yes, I get CRC and CMD_IDX errors for CMD19.

> My 5 cents are, maybe you should try retries > 0 in sd_send_abort_tuning,
> If indeed it's a crc while tuning is not complete.

Interesting. If I do this, I get a timeout error from
mmc_send_abort_tuning(). And bingo! If I replace mmc_send_abort_tuning()
with mdelay(100) the card also probes fine. Also, if I skip this patch
series (so abort_tuning is only for MMC) and add the delay in my host
driver, then the card also comes up. I don't think, though, I should fix
it in the host driver.

Is there a delay defined somewhere which ones need to wait after a CRC
error in tuning? I couldn't find it in the simplified spec.

Thanks for the help again,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]