* [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout
2020-11-20 15:06 [PATCH RFT v2 0/3] mmc: tmio: honor busy timeouts properly Wolfram Sang
@ 2020-11-20 15:06 ` Wolfram Sang
2020-11-24 12:41 ` Yoshihiro Shimoda
2020-11-20 15:06 ` [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection Wolfram Sang
2020-11-20 15:06 ` [PATCH RFT v2 3/3] mmc: renesas_sdhi: enable WAIT_WHILE_BUSY Wolfram Sang
2 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2020-11-20 15:06 UTC (permalink / raw)
To: linux-mmc
Cc: linux-renesas-soc, Yoshihiro Shimoda, Masahiro Yamada,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout
2020-11-20 15:06 ` [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout Wolfram Sang
@ 2020-11-24 12:41 ` Yoshihiro Shimoda
2020-11-25 8:17 ` Wolfram Sang
0 siblings, 1 reply; 9+ messages in thread
From: Yoshihiro Shimoda @ 2020-11-24 12:41 UTC (permalink / raw)
To: Wolfram Sang, linux-mmc@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org, Masahiro Yamada
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection
2020-11-20 15:06 [PATCH RFT v2 0/3] mmc: tmio: honor busy timeouts properly Wolfram Sang
2020-11-20 15:06 ` [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout Wolfram Sang
@ 2020-11-20 15:06 ` Wolfram Sang
2020-11-24 12:41 ` Yoshihiro Shimoda
2020-11-20 15:06 ` [PATCH RFT v2 3/3] mmc: renesas_sdhi: enable WAIT_WHILE_BUSY Wolfram Sang
2 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2020-11-20 15:06 UTC (permalink / raw)
To: linux-mmc
Cc: linux-renesas-soc, Yoshihiro Shimoda, Masahiro Yamada,
Masaharu Hayakawa, Takeshi Saito, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection
2020-11-20 15:06 ` [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection Wolfram Sang
@ 2020-11-24 12:41 ` Yoshihiro Shimoda
2020-11-25 8:56 ` Wolfram Sang
0 siblings, 1 reply; 9+ messages in thread
From: Yoshihiro Shimoda @ 2020-11-24 12:41 UTC (permalink / raw)
To: Wolfram Sang, linux-mmc@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org, Masahiro Yamada,
Masaharu Hayakawa, Takeshi Saito
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection
2020-11-24 12:41 ` Yoshihiro Shimoda
@ 2020-11-25 8:56 ` Wolfram Sang
2020-11-25 11:05 ` Yoshihiro Shimoda
0 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2020-11-25 8:56 UTC (permalink / raw)
To: Yoshihiro Shimoda
Cc: linux-mmc@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
Masahiro Yamada, Masaharu Hayakawa, Takeshi Saito
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection
2020-11-25 8:56 ` Wolfram Sang
@ 2020-11-25 11:05 ` Yoshihiro Shimoda
0 siblings, 0 replies; 9+ messages in thread
From: Yoshihiro Shimoda @ 2020-11-25 11:05 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-mmc@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
Masahiro Yamada, Masaharu Hayakawa, Takeshi Saito
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFT v2 3/3] mmc: renesas_sdhi: enable WAIT_WHILE_BUSY
2020-11-20 15:06 [PATCH RFT v2 0/3] mmc: tmio: honor busy timeouts properly Wolfram Sang
2020-11-20 15:06 ` [PATCH RFT v2 1/3] mmc: tmio: set max_busy_timeout Wolfram Sang
2020-11-20 15:06 ` [PATCH RFT v2 2/3] mmc: tmio: Add data timeout error detection Wolfram Sang
@ 2020-11-20 15:06 ` Wolfram Sang
2 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2020-11-20 15:06 UTC (permalink / raw)
To: linux-mmc
Cc: linux-renesas-soc, Yoshihiro Shimoda, Masahiro Yamada,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread