[PATCH RFT v2 0/3] mmc: tmio: honor busy timeouts properly

[PATCH RFT v2 0/3] mmc: tmio: honor busy timeouts properly

[Wolfram Sang]

(I fixed the typo in patch 1 and retested again. Sorry for the noise!)

TMIO has a HW busy timeout detection which was always running but the
driver did not handle it yet in a proper way. This series fixes that.

Patch 1 sets max_busy_timeout, so the MMC core will not use R1B when it
is beyond the maximum HW timeout. Patch 2 adds proper handling of the
timeout interrupt. After ensuring both, we can finally enable the
WAIT_WHILE_BUSY capability to save the polling.

This was tested using my Renesas Salvator-XS (R-Car M3-N) and Lager
(R-Car H2). However, more advanced testing from the BSP team and/or
Shimoda-san would be much appreciated.

The branch is based on mmc/next plus one patch "[PATCH RFT v2] mmc: tmio: Fix
command error processing". A branch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/busy_timeout

Yamada-san: I enabled setting max_busy_timeout for Uniphier as well
because I am quite sure this HW supports it and it will fix timeout
issues with long erase timeouts. I did not enable WAIT_WHILE_BUSY
because this is "only" an improvement and not a fix. I think opt-in
after some testing is good for that case.

I think all patches are 5.11 material to get enough testing. Patch 1
is IMHO stable material, nonetheless. I am open for discussions, of
course.

Looking forward to comments, reviews, tags...

Thanks and happy hacking,

   Wolfram


Masaharu Hayakawa (1):
  mmc: tmio: Add data timeout error detection

Wolfram Sang (2):
  mmc: tmio: set max_busy_timeout
  mmc: renesas_sdhi: enable WAIT_WHILE_BUSY

 drivers/mmc/host/renesas_sdhi_core.c          |  3 ++
 drivers/mmc/host/renesas_sdhi_internal_dmac.c |  4 +--
 drivers/mmc/host/renesas_sdhi_sys_dmac.c      |  7 ++--
 drivers/mmc/host/tmio_mmc.h                   |  8 +++--
 drivers/mmc/host/tmio_mmc_core.c              | 32 +++++++++++++++----
 drivers/mmc/host/uniphier-sd.c                |  1 +
 include/linux/mfd/tmio.h                      |  7 +++-
 7 files changed, 48 insertions(+), 14 deletions(-)

-- 
2.28.0

[PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout

[Wolfram Sang]

Set max_busy_timeouts for variants known to support the TOPxx bits in
the SD_OPTION register. The timeout mechanism was running in the
background but not yet properly handled in the driver. So, let the MMC
core know when to not use R1B to avoid unhandled timeouts.

My datasheets for older variants (tmio_mmc.c) suggest that they support
it, too. However, actual bit descriptions are lacking, so I chose an
opt-in approach.

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

Change since v1:
* fix wrong multiplication to correct division due to a bad merge
  conflict resolution 

 drivers/mmc/host/renesas_sdhi_core.c |  3 +++
 drivers/mmc/host/tmio_mmc.h          |  2 ++
 drivers/mmc/host/tmio_mmc_core.c     | 15 +++++++++++++++
 drivers/mmc/host/uniphier-sd.c       |  1 +
 include/linux/mfd/tmio.h             |  7 ++++++-
 5 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index bb937411c2ec..153767054c05 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -1041,6 +1041,9 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	/* All SDHI have SDIO status bits which must be 1 */
 	mmc_data->flags |= TMIO_MMC_SDIO_STATUS_SETBITS;
 
+	/* All SDHI support HW busy detection */
+	mmc_data->flags |= TMIO_MMC_USE_BUSY_TIMEOUT;
+
 	dev_pm_domain_start(&pdev->dev);
 
 	ret = renesas_sdhi_clk_enable(host);
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 7ff41185896a..819198af17f4 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -80,6 +80,8 @@
 #define	CLK_CTL_SCLKEN		BIT(8)
 
 /* Definitions for values the CTL_SD_MEM_CARD_OPT register can take */
+#define CARD_OPT_TOP_MASK	0xf0
+#define CARD_OPT_TOP_SHIFT	4
 #define CARD_OPT_WIDTH8		BIT(13)
 #define CARD_OPT_WIDTH		BIT(15)
 
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 7f4a28125010..4727fcfdf95f 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -887,6 +887,18 @@ static void tmio_mmc_set_bus_width(struct tmio_mmc_host *host,
 	sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, reg);
 }
 
+static void tmio_mmc_max_busy_timeout(struct tmio_mmc_host *host)
+{
+	u16 val = sd_ctrl_read16(host, CTL_SD_MEM_CARD_OPT);
+	unsigned int clk_rate = host->mmc->actual_clock ?: host->mmc->f_max;
+	unsigned int cycles;
+
+	val = (val & CARD_OPT_TOP_MASK) >> CARD_OPT_TOP_SHIFT;
+	cycles = 1 << (13  + val);
+
+	host->mmc->max_busy_timeout = cycles / (clk_rate / MSEC_PER_SEC);
+}
+
 /* Set MMC clock / power.
  * Note: This controller uses a simple divider scheme therefore it cannot
  * run a MMC card at full speed (20MHz). The max clock is 24MHz on SD, but as
@@ -945,6 +957,9 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		break;
 	}
 
+	if (host->pdata->flags & TMIO_MMC_USE_BUSY_TIMEOUT)
+		tmio_mmc_max_busy_timeout(host);
+
 	/* Let things settle. delay taken from winCE driver */
 	usleep_range(140, 200);
 	if (PTR_ERR(host->mrq) == -EINTR)
diff --git a/drivers/mmc/host/uniphier-sd.c b/drivers/mmc/host/uniphier-sd.c
index 3092466a99ab..a6cd16771d4e 100644
--- a/drivers/mmc/host/uniphier-sd.c
+++ b/drivers/mmc/host/uniphier-sd.c
@@ -586,6 +586,7 @@ static int uniphier_sd_probe(struct platform_device *pdev)
 
 	tmio_data = &priv->tmio_data;
 	tmio_data->flags |= TMIO_MMC_32BIT_DATA_PORT;
+	tmio_data->flags |= TMIO_MMC_USE_BUSY_TIMEOUT;
 
 	host = tmio_mmc_host_alloc(pdev, tmio_data);
 	if (IS_ERR(host))
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 8ba042430d8e..27264fe4b3b9 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -55,7 +55,12 @@
  */
 #define TMIO_MMC_HAS_IDLE_WAIT		BIT(4)
 
-/* BIT(5) is unused */
+/*
+ * Use the busy timeout feature. Probably all TMIO versions support it. Yet,
+ * we don't have documentation for old variants, so we enable only known good
+ * variants with this flag. Can be removed once all variants are known good.
+ */
+#define TMIO_MMC_USE_BUSY_TIMEOUT	BIT(5)
 
 /*
  * Some controllers have CMD12 automatically
-- 
2.28.0

[PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection

[Wolfram Sang]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

Currently, busy timeout is not checked for data transfer command. But,
if the temperature condition changes, the data cannot be acquired
correctly and timeout may occur. Also, we could reproduce an issue by
using mmc_test driver (e.g. "Correct xfer_size at write (start
failure)"). Therefore, this adds timeout error check.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[saito: rework commit message.]
Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
[shimoda: rebase, add commit description]
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc.h      |  6 ++++--
 drivers/mmc/host/tmio_mmc_core.c | 17 +++++++++++------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 819198af17f4..15cf850e75d3 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -100,8 +100,10 @@
 /* This is the mask used at reset by the chip */
 #define TMIO_MASK_INIT_RCAR2	0x8b7f031d /* Initial value for R-Car Gen2+ */
 #define TMIO_MASK_ALL           0x837f031d
-#define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND)
-#define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND)
+#define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND | \
+			   TMIO_STAT_DATATIMEOUT)
+#define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND | \
+			   TMIO_STAT_DATATIMEOUT)
 #define TMIO_MASK_CMD     (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT | \
 		TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT)
 #define TMIO_MASK_IRQ     (TMIO_MASK_READOP | TMIO_MASK_WRITEOP | TMIO_MASK_CMD)
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 4727fcfdf95f..541a0cf4a9fa 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -477,8 +477,10 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat)
 	if (!data)
 		goto out;
 
-	if (stat & TMIO_STAT_CRCFAIL || stat & TMIO_STAT_STOPBIT_ERR ||
-	    stat & TMIO_STAT_TXUNDERRUN)
+	if (stat & TMIO_STAT_DATATIMEOUT)
+		data->error = -ETIMEDOUT;
+	else if (stat & TMIO_STAT_CRCFAIL || stat & TMIO_STAT_STOPBIT_ERR ||
+		 stat & TMIO_STAT_TXUNDERRUN)
 		data->error = -EILSEQ;
 	if (host->dma_on && (data->flags & MMC_DATA_WRITE)) {
 		u32 status = sd_ctrl_read16_and_16_as_32(host, CTL_STATUS);
@@ -501,11 +503,13 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat)
 		}
 
 		if (done) {
-			tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
+			tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND |
+						  TMIO_STAT_DATATIMEOUT);
 			tmio_mmc_dataend_dma(host);
 		}
 	} else if (host->dma_on && (data->flags & MMC_DATA_READ)) {
-		tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
+		tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND |
+					  TMIO_STAT_DATATIMEOUT);
 		tmio_mmc_dataend_dma(host);
 	} else {
 		tmio_mmc_do_data_irq(host);
@@ -619,8 +623,9 @@ static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host, int ireg,
 	}
 
 	/* Data transfer completion */
-	if (ireg & TMIO_STAT_DATAEND) {
-		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
+	if (ireg & (TMIO_STAT_DATAEND | TMIO_STAT_DATATIMEOUT)) {
+		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND |
+				      TMIO_STAT_DATATIMEOUT);
 		tmio_mmc_data_irq(host, status);
 		return true;
 	}
-- 
2.28.0

[PATCH RFT v2 3/3] mmc: renesas_sdhi: enable WAIT_WHILE_BUSY

[Wolfram Sang]

Now that we got the timeout handling in the driver correct, we can use
this capability to avoid polling via the MMC core.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 4 ++--
 drivers/mmc/host/renesas_sdhi_sys_dmac.c      | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index fe13e1ea22dc..d8b811c46628 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -88,7 +88,7 @@ static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
 
 static const struct renesas_sdhi_of_data of_rza2_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
-			  TMIO_MMC_HAVE_CBSY,
+			  TMIO_MMC_HAVE_CBSY | MMC_CAP_WAIT_WHILE_BUSY,
 	.tmio_ocr_mask	= MMC_VDD_32_33,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_CMD23,
@@ -105,7 +105,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
-			  MMC_CAP_CMD23,
+			  MMC_CAP_CMD23 | MMC_CAP_WAIT_WHILE_BUSY,
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT | MMC_CAP2_MERGE_CAPABLE,
 	.bus_shift	= 2,
 	.scc_offset	= 0x1000,
diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index c5f789675302..0a3494fcc5e8 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -31,13 +31,14 @@ static const struct renesas_sdhi_of_data of_default_cfg = {
 
 static const struct renesas_sdhi_of_data of_rz_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_32BIT_DATA_PORT |
-			  TMIO_MMC_HAVE_CBSY,
+			  TMIO_MMC_HAVE_CBSY | MMC_CAP_WAIT_WHILE_BUSY,
 	.tmio_ocr_mask	= MMC_VDD_32_33,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 };
 
 static const struct renesas_sdhi_of_data of_rcar_gen1_compatible = {
-	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL,
+	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
+			  MMC_CAP_WAIT_WHILE_BUSY,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 };
@@ -58,7 +59,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
-			  MMC_CAP_CMD23,
+			  MMC_CAP_CMD23 | MMC_CAP_WAIT_WHILE_BUSY,
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
 	.dma_rx_offset	= 0x2000,
-- 
2.28.0

RE: [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Saturday, November 21, 2020 12:07 AM
> 
> Set max_busy_timeouts for variants known to support the TOPxx bits in
> the SD_OPTION register. The timeout mechanism was running in the
> background but not yet properly handled in the driver. So, let the MMC
> core know when to not use R1B to avoid unhandled timeouts.
> 
> My datasheets for older variants (tmio_mmc.c) suggest that they support
> it, too. However, actual bit descriptions are lacking, so I chose an
> opt-in approach.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
<snip>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 7f4a28125010..4727fcfdf95f 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -887,6 +887,18 @@ static void tmio_mmc_set_bus_width(struct tmio_mmc_host *host,
>  	sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, reg);
>  }
> 
> +static void tmio_mmc_max_busy_timeout(struct tmio_mmc_host *host)
> +{
> +	u16 val = sd_ctrl_read16(host, CTL_SD_MEM_CARD_OPT);
> +	unsigned int clk_rate = host->mmc->actual_clock ?: host->mmc->f_max;
> +	unsigned int cycles;
> +
> +	val = (val & CARD_OPT_TOP_MASK) >> CARD_OPT_TOP_SHIFT;
> +	cycles = 1 << (13  + val);

nit: we can remove double-spaces like (13 + val).

Best regards,
Yoshihiro Shimoda

RE: [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Saturday, November 21, 2020 12:07 AM
> 
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> Currently, busy timeout is not checked for data transfer command. But,
> if the temperature condition changes, the data cannot be acquired
> correctly and timeout may occur. Also, we could reproduce an issue by
> using mmc_test driver (e.g. "Correct xfer_size at write (start
> failure)"). Therefore, this adds timeout error check.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [saito: rework commit message.]
> Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
> [shimoda: rebase, add commit description]
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/host/tmio_mmc.h      |  6 ++++--
>  drivers/mmc/host/tmio_mmc_core.c | 17 +++++++++++------
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 819198af17f4..15cf850e75d3 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -100,8 +100,10 @@
>  /* This is the mask used at reset by the chip */
>  #define TMIO_MASK_INIT_RCAR2	0x8b7f031d /* Initial value for R-Car Gen2+ */
>  #define TMIO_MASK_ALL           0x837f031d
> -#define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND)
> -#define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND)
> +#define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND | \
> +			   TMIO_STAT_DATATIMEOUT)
> +#define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND | \
> +			   TMIO_STAT_DATATIMEOUT)

I talked Saito-san about this patch locally and we can drop these lines
because this driver can detect data timeout in Access End interrupt
and the driver didn't enable error related interrupts like CRCFAIL.

>  #define TMIO_MASK_CMD     (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT | \
>  		TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT)
>  #define TMIO_MASK_IRQ     (TMIO_MASK_READOP | TMIO_MASK_WRITEOP | TMIO_MASK_CMD)
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 4727fcfdf95f..541a0cf4a9fa 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -477,8 +477,10 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat)
>  	if (!data)
>  		goto out;
> 
> -	if (stat & TMIO_STAT_CRCFAIL || stat & TMIO_STAT_STOPBIT_ERR ||
> -	    stat & TMIO_STAT_TXUNDERRUN)
> +	if (stat & TMIO_STAT_DATATIMEOUT)
> +		data->error = -ETIMEDOUT;

The following commit [1] is a BSP local patch though,
we need to set -EILSEQ to retune a card for R-Car Gen3 [2]
by MMC core driver [3].

[1]
https://github.com/renesas-rcar/linux-bsp/commit/6f7519552fbed1474561ff423acb967eb03994e3
Remarks (please look the [2] below at first):
 - The patch checks whether a card is removed or not by using MMC_CAP_NONREMOVABLE and
   get_cd() to avoid retuning by MMC core driver for R-Car Gen3.
 - The patch also change the tmio_mmc_cmd_irq() when CMDTIMEOUT happens for R-Car Gen3.
   But, for upstream, we should make a separated patch for it.
 - These "for R-Car Gen3" means I'm thinking we need additional condition:
    1) to set -EILSRQ or -ETIMEDOUT for R-Car Gen3
    2) to set -ETIMEDOUT anyway for other SoCs.
   # These are complex conditions a little though...

[2]
This is related to R-Car Gen3 tuning flow (e.g. Figure 70.32).

[3]
---
drivers/mmc/core/core.c:
void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
{
        struct mmc_command *cmd = mrq->cmd;
        int err = cmd->error;

        /* Flag re-tuning needed on CRC errors */
        if (cmd->opcode != MMC_SEND_TUNING_BLOCK &&
            cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200 &&
            !host->retune_crc_disable &&
            (err == -EILSEQ || (mrq->sbc && mrq->sbc->error == -EILSEQ) ||
            (mrq->data && mrq->data->error == -EILSEQ) ||              <-- here
            (mrq->stop && mrq->stop->error == -EILSEQ)))
                mmc_retune_needed(host);
---

> +	else if (stat & TMIO_STAT_CRCFAIL || stat & TMIO_STAT_STOPBIT_ERR ||
> +		 stat & TMIO_STAT_TXUNDERRUN)
>  		data->error = -EILSEQ;
>  	if (host->dma_on && (data->flags & MMC_DATA_WRITE)) {
>  		u32 status = sd_ctrl_read16_and_16_as_32(host, CTL_STATUS);
> @@ -501,11 +503,13 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat)
>  		}
> 
>  		if (done) {
> -			tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
> +			tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND |
> +						  TMIO_STAT_DATATIMEOUT);
>  			tmio_mmc_dataend_dma(host);
>  		}
>  	} else if (host->dma_on && (data->flags & MMC_DATA_READ)) {
> -		tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
> +		tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND |
> +					  TMIO_STAT_DATATIMEOUT);
>  		tmio_mmc_dataend_dma(host);
>  	} else {
>  		tmio_mmc_do_data_irq(host);
> @@ -619,8 +623,9 @@ static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host, int ireg,
>  	}
> 
>  	/* Data transfer completion */
> -	if (ireg & TMIO_STAT_DATAEND) {
> -		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
> +	if (ireg & (TMIO_STAT_DATAEND | TMIO_STAT_DATATIMEOUT)) {
> +		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND |
> +				      TMIO_STAT_DATATIMEOUT);

So, we can also drop these adding "TMIO_STAT_DATATIMEOUT".

Best regards,
Yoshihiro Shimoda

Re: [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout

[Wolfram Sang]

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


> > +	val = (val & CARD_OPT_TOP_MASK) >> CARD_OPT_TOP_SHIFT;
> > +	cycles = 1 << (13  + val);
> 
> nit: we can remove double-spaces like (13 + val).

Thanks, I will fix it.


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

Re: [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection

[Wolfram Sang]

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

Hi Shimoda-san,

> I talked Saito-san about this patch locally and we can drop these lines
> because this driver can detect data timeout in Access End interrupt
> and the driver didn't enable error related interrupts like CRCFAIL.

I see. I already wondered why the BSP patch
6f7519552fbed1474561ff423acb967eb03994e3 did not have these lines.

> The following commit [1] is a BSP local patch though,
> we need to set -EILSEQ to retune a card for R-Car Gen3 [2]
> by MMC core driver [3].

So, if there is a non-removable eMMC or a SD card inserted, then we need
to EILSEQ to enforce a retune. Otherwise it is a data timeout. Is my
understanding correct?

I wonder, though, if "Gen3" is a complete description? There are SDHI
instances on Gen2 which can also do SDR104. Won't they need the same
treatment? Then we could say that every SDHI which has an SCC will need
this treatment.

>  - The patch also change the tmio_mmc_cmd_irq() when CMDTIMEOUT happens for R-Car Gen3.
>    But, for upstream, we should make a separated patch for it.

I am sorry. I don't fully understand. Why does the change to
tmio_mmc_cmd_irq() need a seperate patch?

>  - These "for R-Car Gen3" means I'm thinking we need additional condition:
>     1) to set -EILSRQ or -ETIMEDOUT for R-Car Gen3
>     2) to set -ETIMEDOUT anyway for other SoCs.
>    # These are complex conditions a little though...

Well, from what I understood this sounds not too hard. Let's hope I just
got it correctly :)

However, there is something in this patch which makes mmc_test #15 work,
though. We still want this in this series, or do you think it is better
to move it to a seperate series?

Kind regards,

   Wolfram


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

RE: [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, November 25, 2020 5:56 PM
<snip>
> > The following commit [1] is a BSP local patch though,
> > we need to set -EILSEQ to retune a card for R-Car Gen3 [2]
> > by MMC core driver [3].
> 
> So, if there is a non-removable eMMC or a SD card inserted, then we need
> to EILSEQ to enforce a retune. Otherwise it is a data timeout. Is my
> understanding correct?

Yes.

> I wonder, though, if "Gen3" is a complete description? There are SDHI
> instances on Gen2 which can also do SDR104. Won't they need the same
> treatment? Then we could say that every SDHI which has an SCC will need
> this treatment.

Hmm, since Gen2 datasheet didn't update the flowchart so that I thought
we need special handling for "Gen3". But, I'll ask hardware team whether
my thought is correct or not.

> >  - The patch also change the tmio_mmc_cmd_irq() when CMDTIMEOUT happens for R-Car Gen3.
> >    But, for upstream, we should make a separated patch for it.
> 
> I am sorry. I don't fully understand. Why does the change to
> tmio_mmc_cmd_irq() need a seperate patch?

The patch subject is "Add data timeout error detection".
That's why I thought so.

> >  - These "for R-Car Gen3" means I'm thinking we need additional condition:
> >     1) to set -EILSRQ or -ETIMEDOUT for R-Car Gen3
> >     2) to set -ETIMEDOUT anyway for other SoCs.
> >    # These are complex conditions a little though...
> 
> Well, from what I understood this sounds not too hard. Let's hope I just
> got it correctly :)

I got it :)

> However, there is something in this patch which makes mmc_test #15 work,
> though. We still want this in this series, or do you think it is better
> to move it to a seperate series?

Thank you for the comments. I realized mmc_test #15 failed if tmio driver
returned -EILSEQ when I applied the BSP patch as the following:
---
[  206.016193] mmc0: Starting tests of card mmc0:0001...
[  206.022708] mmc0: Test case 15. Proper xfer_size at write (start failure)...
[  206.854082] mmc0: Result: ERROR (-84)
[  206.858632] mmc0: Tests completed.
---

So, I'm thinking retuning -EILSEQ in cmd/data timeout should be a separate series now.
# Perhaps we need other way to follow the retuning sequence for R-Car Gen3...

Best regards,
Yoshihiro Shimoda