[PATCH 00/19] mmc: Improve host driver's support for R1B responses

[PATCH 00/19] mmc: Improve host driver's support for R1B responses

[Ulf Hansson]

Some MMC/SD commands uses R1B responses, which means the card may assert the DAT0 line
to signal busy for a period of time, after it has received the command. The mmc
core normally specifies the busy period for the command in the
cmd->busy_timeout. Ideally the driver should respect it, but that isn't always
the case, for several reasons.

For example, the mmc host may not even support HW busy signal
detection, the HW may have a build-in upper limit of the timeout and the driver
simply uses that or it uses some other internal command/request SW timeout
mechanism with another timeout value.

In cases when the host driver can't support a cmd->busy_timeout, the core
implements a fallback method, which is based upon sending CMD13 to poll the card
about the status instead. However, to make this work, the host driver need to
specify the mmc->max_busy_timeout, as to make the core aware of when to use the
fallback method with CMD13 polling.

Step by step, host drivers has been improved to better cope with the above
scenarios. Although in this series, I have walked through *all* host drivers and
those that looked particularly suspicious I have fixed.

Note, none of the changes has been tested on HW, so I am relying on help with
this.

The next step beyond this series is to make MMC_CAP_ERASE default enabled, as
that would trigger CMD38 to be used. CMD38 uses the R1B response and may
typically require longer busy periods to be supported.

Kind regards
Ulf Hanssom


Ulf Hansson (19):
  mmc: atmel-mci: Keep timer enabled when queuing a next request
  mmc: atmel-mci: Set the timer per command rather than per request
  mmc: atmel-mci: Respect the cmd->busy_timeout from the mmc core
  mmc: jz4740: Inform the mmc core about the maximum busy timeout
  mmc: usdhi6rol0: Inform the mmc core about the maximum busy timeout
  mmc: cb710: Inform the mmc core about the maximum busy timeout
  mmc: owl-mmc: Respect the cmd->busy_timeout from the mmc core
  mmc: sdricoh_cs: Drop unused defines
  mmc: sdricoh_cs: Use MMC_APP_CMD rather than a hardcoded number
  mmc: sdricoh_cs: Move MMC_APP_CMD handling to sdricoh_mmc_cmd()
  mmc: sdricoh_cs: Drop redundant in-parameter to sdricoh_query_status()
  mmc: sdricoh_cs: Throttle polling rate for data transfers
  mmc: sdricoh_cs: Throttle polling rate for commands
  mmc: sdricoh_cs: Respect the cmd->busy_timeout from the mmc core
  mmc: tifm_sd: Inform the mmc core about the maximum busy timeout
  mmc: via-sdmmc: Respect the cmd->busy_timeout from the mmc core
  mmc: mmc_spi: Add/rename defines for timeouts
  mmc: mmc_spi: Respect the cmd->busy_timeout from the mmc core
  staging: greybus: sdio: Respect the cmd->busy_timeout from the mmc
    core

 drivers/mmc/host/atmel-mci.c   |  12 ++--
 drivers/mmc/host/cb710-mmc.c   |   8 +++
 drivers/mmc/host/jz4740_mmc.c  |  13 ++++-
 drivers/mmc/host/mmc_spi.c     |  20 +++----
 drivers/mmc/host/owl-mmc.c     |   8 ++-
 drivers/mmc/host/sdricoh_cs.c  | 103 +++++++++++++++++----------------
 drivers/mmc/host/tifm_sd.c     |   9 ++-
 drivers/mmc/host/usdhi6rol0.c  |   9 ++-
 drivers/mmc/host/via-sdmmc.c   |   7 ++-
 drivers/staging/greybus/sdio.c |  10 +++-
 10 files changed, 125 insertions(+), 74 deletions(-)

-- 
2.20.1

[PATCH 01/19] mmc: atmel-mci: Keep timer enabled when queuing a next request

[Ulf Hansson]

When atmci_request_end() is about to finish a request for one slot, there
is a possibility that there is new request queued for another slot. If this
turns out to be the case, the new request is started and the timer is
re-programmed for it.

Although, a few lines below in atmci_request_end(), this timer becomes
deleted, likely corresponding to the other recently completed request. This
looks wrong, so let's fix it.

Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/atmel-mci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index aeaaa5314924..0472df8391b5 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -1557,6 +1557,8 @@ static void atmci_request_end(struct atmel_mci *host, struct mmc_request *mrq)
 
 	WARN_ON(host->cmd || host->data);
 
+	del_timer(&host->timer);
+
 	/*
 	 * Update the MMC clock rate if necessary. This may be
 	 * necessary if set_ios() is called when a different slot is
@@ -1583,8 +1585,6 @@ static void atmci_request_end(struct atmel_mci *host, struct mmc_request *mrq)
 		host->state = STATE_IDLE;
 	}
 
-	del_timer(&host->timer);
-
 	spin_unlock(&host->lock);
 	mmc_request_done(prev_mmc, mrq);
 	spin_lock(&host->lock);
-- 
2.20.1

Re: [PATCH 01/19] mmc: atmel-mci: Keep timer enabled when queuing a next request

[ludovic.desroches]

On Tue, Apr 14, 2020 at 06:13:55PM +0200, Ulf Hansson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> When atmci_request_end() is about to finish a request for one slot, there
> is a possibility that there is new request queued for another slot. If this
> turns out to be the case, the new request is started and the timer is
> re-programmed for it.
> 
> Although, a few lines below in atmci_request_end(), this timer becomes
> deleted, likely corresponding to the other recently completed request. This
> looks wrong, so let's fix it.
> 
> Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Nice catch.

Thanks

Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

> ---
>  drivers/mmc/host/atmel-mci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index aeaaa5314924..0472df8391b5 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -1557,6 +1557,8 @@ static void atmci_request_end(struct atmel_mci *host, struct mmc_request *mrq)
> 
>         WARN_ON(host->cmd || host->data);
> 
> +       del_timer(&host->timer);
> +
>         /*
>          * Update the MMC clock rate if necessary. This may be
>          * necessary if set_ios() is called when a different slot is
> @@ -1583,8 +1585,6 @@ static void atmci_request_end(struct atmel_mci *host, struct mmc_request *mrq)
>                 host->state = STATE_IDLE;
>         }
> 
> -       del_timer(&host->timer);
> -
>         spin_unlock(&host->lock);
>         mmc_request_done(prev_mmc, mrq);
>         spin_lock(&host->lock);
> --
> 2.20.1
> 

[PATCH 02/19] mmc: atmel-mci: Set the timer per command rather than per request

[Ulf Hansson]

Setting the timer on a per request basis, is rather limiting as the timer
really depends on what commands that is to be sent as part of the request.

Therefore improve the behaviour by programming the timer per command basis
instead.

Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/atmel-mci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 0472df8391b5..7292970065b6 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -169,6 +169,7 @@
 #define	atmci_writel(port, reg, value)			\
 	__raw_writel((value), (port)->regs + reg)
 
+#define ATMCI_CMD_TIMEOUT_MS	2000
 #define AUTOSUSPEND_DELAY	50
 
 #define ATMCI_DATA_ERROR_FLAGS	(ATMCI_DCRCE | ATMCI_DTOE | ATMCI_OVRE | ATMCI_UNRE)
@@ -817,6 +818,9 @@ static void atmci_send_command(struct atmel_mci *host,
 
 	atmci_writel(host, ATMCI_ARGR, cmd->arg);
 	atmci_writel(host, ATMCI_CMDR, cmd_flags);
+
+	mod_timer(&host->timer,
+		  jiffies + msecs_to_jiffies(ATMCI_CMD_TIMEOUT_MS));
 }
 
 static void atmci_send_stop_cmd(struct atmel_mci *host, struct mmc_data *data)
@@ -1314,8 +1318,6 @@ static void atmci_start_request(struct atmel_mci *host,
 	 * prepared yet.)
 	 */
 	atmci_writel(host, ATMCI_IER, iflags);
-
-	mod_timer(&host->timer, jiffies +  msecs_to_jiffies(2000));
 }
 
 static void atmci_queue_request(struct atmel_mci *host,
-- 
2.20.1

Re: [PATCH 02/19] mmc: atmel-mci: Set the timer per command rather than per request

[ludovic.desroches]

On Tue, Apr 14, 2020 at 06:13:56PM +0200, Ulf Hansson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Setting the timer on a per request basis, is rather limiting as the timer
> really depends on what commands that is to be sent as part of the request.
> 
> Therefore improve the behaviour by programming the timer per command basis
> instead.
> 
> Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Makes sense.

Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

Thanks

> ---
>  drivers/mmc/host/atmel-mci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index 0472df8391b5..7292970065b6 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -169,6 +169,7 @@
>  #define        atmci_writel(port, reg, value)                  \
>         __raw_writel((value), (port)->regs + reg)
> 
> +#define ATMCI_CMD_TIMEOUT_MS   2000
>  #define AUTOSUSPEND_DELAY      50
> 
>  #define ATMCI_DATA_ERROR_FLAGS (ATMCI_DCRCE | ATMCI_DTOE | ATMCI_OVRE | ATMCI_UNRE)
> @@ -817,6 +818,9 @@ static void atmci_send_command(struct atmel_mci *host,
> 
>         atmci_writel(host, ATMCI_ARGR, cmd->arg);
>         atmci_writel(host, ATMCI_CMDR, cmd_flags);
> +
> +       mod_timer(&host->timer,
> +                 jiffies + msecs_to_jiffies(ATMCI_CMD_TIMEOUT_MS));
>  }
> 
>  static void atmci_send_stop_cmd(struct atmel_mci *host, struct mmc_data *data)
> @@ -1314,8 +1318,6 @@ static void atmci_start_request(struct atmel_mci *host,
>          * prepared yet.)
>          */
>         atmci_writel(host, ATMCI_IER, iflags);
> -
> -       mod_timer(&host->timer, jiffies +  msecs_to_jiffies(2000));
>  }
> 
>  static void atmci_queue_request(struct atmel_mci *host,
> --
> 2.20.1
> 

[PATCH 03/19] mmc: atmel-mci: Respect the cmd->busy_timeout from the mmc core

[Ulf Hansson]

Using a fixed 2s timeout for all commands is a bit problematic.

For some commands it means waiting longer than needed for the timer to
expire, which may not a big issue, but still. For other commands, like for
an erase (CMD38) that uses a R1B response, may require longer timeouts than
2s. In these cases, we may end up treating the command as it failed, while
it just needed some more time to complete successfully.

Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.

Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/atmel-mci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 7292970065b6..5cb692687698 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -809,6 +809,9 @@ static u32 atmci_prepare_command(struct mmc_host *mmc,
 static void atmci_send_command(struct atmel_mci *host,
 		struct mmc_command *cmd, u32 cmd_flags)
 {
+	unsigned int timeout_ms = cmd->busy_timeout ? cmd->busy_timeout :
+		ATMCI_CMD_TIMEOUT_MS;
+
 	WARN_ON(host->cmd);
 	host->cmd = cmd;
 
@@ -819,8 +822,7 @@ static void atmci_send_command(struct atmel_mci *host,
 	atmci_writel(host, ATMCI_ARGR, cmd->arg);
 	atmci_writel(host, ATMCI_CMDR, cmd_flags);
 
-	mod_timer(&host->timer,
-		  jiffies + msecs_to_jiffies(ATMCI_CMD_TIMEOUT_MS));
+	mod_timer(&host->timer, jiffies + msecs_to_jiffies(timeout_ms));
 }
 
 static void atmci_send_stop_cmd(struct atmel_mci *host, struct mmc_data *data)
-- 
2.20.1

Re: [PATCH 03/19] mmc: atmel-mci: Respect the cmd->busy_timeout from the mmc core

[ludovic.desroches]

On Tue, Apr 14, 2020 at 06:13:57PM +0200, Ulf Hansson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Using a fixed 2s timeout for all commands is a bit problematic.
> 
> For some commands it means waiting longer than needed for the timer to
> expire, which may not a big issue, but still. For other commands, like for
> an erase (CMD38) that uses a R1B response, may require longer timeouts than
> 2s. In these cases, we may end up treating the command as it failed, while
> it just needed some more time to complete successfully.
> 
> Fix the problem by respecting the cmd->busy_timeout, which is provided by
> the mmc core.
> 
> Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Far better than a fixed timeout.

This driver covers many versions of the IP. I could test it with only one
version. As I didn't any regression and these three patches make sense

Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

Thanks

> ---
>  drivers/mmc/host/atmel-mci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index 7292970065b6..5cb692687698 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -809,6 +809,9 @@ static u32 atmci_prepare_command(struct mmc_host *mmc,
>  static void atmci_send_command(struct atmel_mci *host,
>                 struct mmc_command *cmd, u32 cmd_flags)
>  {
> +       unsigned int timeout_ms = cmd->busy_timeout ? cmd->busy_timeout :
> +               ATMCI_CMD_TIMEOUT_MS;
> +
>         WARN_ON(host->cmd);
>         host->cmd = cmd;
> 
> @@ -819,8 +822,7 @@ static void atmci_send_command(struct atmel_mci *host,
>         atmci_writel(host, ATMCI_ARGR, cmd->arg);
>         atmci_writel(host, ATMCI_CMDR, cmd_flags);
> 
> -       mod_timer(&host->timer,
> -                 jiffies + msecs_to_jiffies(ATMCI_CMD_TIMEOUT_MS));
> +       mod_timer(&host->timer, jiffies + msecs_to_jiffies(timeout_ms));
>  }
> 
>  static void atmci_send_stop_cmd(struct atmel_mci *host, struct mmc_data *data)
> --
> 2.20.1
> 

[PATCH 04/19] mmc: jz4740: Inform the mmc core about the maximum busy timeout

[Ulf Hansson]

Some commands uses R1B responses, which means the card may assert the DAT0
line to signal busy for a period of time, after it has received the
command. The mmc core normally specifies the busy period for the command in
the cmd->busy_timeout. Ideally the driver should respect it, but that
requires quite some update of the code, so let's defer that to someone with
the HW at hand.

Instead, let's inform the mmc core about the maximum supported busy timeout
in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
5s timeout used by jz4740. In this way, we let the mmc core validate the
needed timeout, which may lead to that it converts from a R1B into a R1
response and then use CMD13 to poll for busy completion.

In other words, this change enables support for commands with longer busy
periods than 5s, like erase (CMD38) for example.

Cc: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/jz4740_mmc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index fbae87d1f017..cba7a6fcd178 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -108,6 +108,7 @@
 #define	JZ_MMC_LPM_LOW_POWER_MODE_EN BIT(0)
 
 #define JZ_MMC_CLK_RATE 24000000
+#define JZ_MMC_REQ_TIMEOUT_MS 5000
 
 enum jz4740_mmc_version {
 	JZ_MMC_JZ4740,
@@ -440,7 +441,8 @@ static unsigned int jz4740_mmc_poll_irq(struct jz4740_mmc_host *host,
 
 	if (timeout == 0) {
 		set_bit(0, &host->waiting);
-		mod_timer(&host->timeout_timer, jiffies + 5*HZ);
+		mod_timer(&host->timeout_timer,
+			  jiffies + msecs_to_jiffies(JZ_MMC_REQ_TIMEOUT_MS));
 		jz4740_mmc_set_irq_enabled(host, irq, true);
 		return true;
 	}
@@ -893,7 +895,8 @@ static void jz4740_mmc_request(struct mmc_host *mmc, struct mmc_request *req)
 
 	host->state = JZ4740_MMC_STATE_READ_RESPONSE;
 	set_bit(0, &host->waiting);
-	mod_timer(&host->timeout_timer, jiffies + 5*HZ);
+	mod_timer(&host->timeout_timer,
+		  jiffies + msecs_to_jiffies(JZ_MMC_REQ_TIMEOUT_MS));
 	jz4740_mmc_send_command(host, req->cmd);
 }
 
@@ -1023,6 +1026,12 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	mmc->f_min = mmc->f_max / 128;
 	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
 
+	/*
+	 * We use a fixed timeout of 5s, hence inform the core about it. A
+	 * future improvement should instead respect the cmd->busy_timeout.
+	 */
+	mmc->max_busy_timeout = JZ_MMC_REQ_TIMEOUT_MS;
+
 	mmc->max_blk_size = (1 << 10) - 1;
 	mmc->max_blk_count = (1 << 15) - 1;
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
-- 
2.20.1

[PATCH 05/19] mmc: usdhi6rol0: Inform the mmc core about the maximum busy timeout

[Ulf Hansson]

Some commands uses R1B responses, which means the card may assert the DAT0
line to signal busy for a period of time, after it has received the
command. The mmc core normally specifies the busy period for the command in
the cmd->busy_timeout. Ideally the driver should respect it, but that
requires quite some update of the code, so let's defer that to someone with
the HW at hand.

Instead, let's inform the mmc core about the maximum supported busy timeout
in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
4s timeout used by usdhi6rol0. In this way, we let the mmc core validate
the needed timeout, which may lead to that it converts from a R1B into a R1
response and then use CMD13 to poll for busy completion.

In other words, this change enables support for commands with longer busy
periods than 4s, like erase (CMD38) for example.

Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: Lars Persson <lars.persson@axis.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/usdhi6rol0.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c
index 9a0b1e4e405d..369b8dee2e3d 100644
--- a/drivers/mmc/host/usdhi6rol0.c
+++ b/drivers/mmc/host/usdhi6rol0.c
@@ -136,6 +136,8 @@
 
 #define USDHI6_MIN_DMA 64
 
+#define USDHI6_REQ_TIMEOUT_MS 4000
+
 enum usdhi6_wait_for {
 	USDHI6_WAIT_FOR_REQUEST,
 	USDHI6_WAIT_FOR_CMD,
@@ -1763,7 +1765,12 @@ static int usdhi6_probe(struct platform_device *pdev)
 	host		= mmc_priv(mmc);
 	host->mmc	= mmc;
 	host->wait	= USDHI6_WAIT_FOR_REQUEST;
-	host->timeout	= msecs_to_jiffies(4000);
+	host->timeout	= msecs_to_jiffies(USDHI6_REQ_TIMEOUT_MS);
+	/*
+	 * We use a fixed timeout of 4s, hence inform the core about it. A
+	 * future improvement should instead respect the cmd->busy_timeout.
+	 */
+	mmc->max_busy_timeout = USDHI6_REQ_TIMEOUT_MS;
 
 	host->pinctrl = devm_pinctrl_get(&pdev->dev);
 	if (IS_ERR(host->pinctrl)) {
-- 
2.20.1

Re: [PATCH 05/19] mmc: usdhi6rol0: Inform the mmc core about the maximum busy timeout

[Jesper Nilsson]

On Tue, Apr 14, 2020 at 06:13:59PM +0200, Ulf Hansson wrote:
> Some commands uses R1B responses, which means the card may assert the DAT0
> line to signal busy for a period of time, after it has received the
> command. The mmc core normally specifies the busy period for the command in
> the cmd->busy_timeout. Ideally the driver should respect it, but that
> requires quite some update of the code, so let's defer that to someone with
> the HW at hand.
> 
> Instead, let's inform the mmc core about the maximum supported busy timeout
> in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
> 4s timeout used by usdhi6rol0. In this way, we let the mmc core validate
> the needed timeout, which may lead to that it converts from a R1B into a R1
> response and then use CMD13 to poll for busy completion.
> 
> In other words, this change enables support for commands with longer busy
> periods than 4s, like erase (CMD38) for example.

Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>

> Cc: Lars Persson <lars.persson@axis.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/usdhi6rol0.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c
> index 9a0b1e4e405d..369b8dee2e3d 100644
> --- a/drivers/mmc/host/usdhi6rol0.c
> +++ b/drivers/mmc/host/usdhi6rol0.c
> @@ -136,6 +136,8 @@
>  
>  #define USDHI6_MIN_DMA 64
>  
> +#define USDHI6_REQ_TIMEOUT_MS 4000
> +
>  enum usdhi6_wait_for {
>  	USDHI6_WAIT_FOR_REQUEST,
>  	USDHI6_WAIT_FOR_CMD,
> @@ -1763,7 +1765,12 @@ static int usdhi6_probe(struct platform_device *pdev)
>  	host		= mmc_priv(mmc);
>  	host->mmc	= mmc;
>  	host->wait	= USDHI6_WAIT_FOR_REQUEST;
> -	host->timeout	= msecs_to_jiffies(4000);
> +	host->timeout	= msecs_to_jiffies(USDHI6_REQ_TIMEOUT_MS);
> +	/*
> +	 * We use a fixed timeout of 4s, hence inform the core about it. A
> +	 * future improvement should instead respect the cmd->busy_timeout.
> +	 */
> +	mmc->max_busy_timeout = USDHI6_REQ_TIMEOUT_MS;
>  
>  	host->pinctrl = devm_pinctrl_get(&pdev->dev);
>  	if (IS_ERR(host->pinctrl)) {
> -- 
> 2.20.1
> 

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

[PATCH 06/19] mmc: cb710: Inform the mmc core about the maximum busy timeout

[Ulf Hansson]

Some commands uses R1B responses, which means the card may assert the DAT0
line to signal busy for a period of time, after it has received the
command. The mmc core normally specifies the busy period for the command in
the cmd->busy_timeout. Ideally the driver should respect it, but that
requires quite some update of the code, so let's defer that to someone with
the HW at hand.

Instead, let's inform the mmc core about the maximum supported busy timeout
in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
~2s timeout of the polling loop, implemented in cb710_wait_for_event(). In
this way, we let the mmc core validate the needed timeout, which may lead
to that it converts from a R1B into a R1 response and then use CMD13 to
poll for busy completion.

In other words, this change enables support for commands with longer busy
periods than 2s, like erase (CMD38) for example.

Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/cb710-mmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/cb710-mmc.c b/drivers/mmc/host/cb710-mmc.c
index e33270e40539..e84ed84ea4cc 100644
--- a/drivers/mmc/host/cb710-mmc.c
+++ b/drivers/mmc/host/cb710-mmc.c
@@ -10,6 +10,8 @@
 #include <linux/delay.h>
 #include "cb710-mmc.h"
 
+#define CB710_MMC_REQ_TIMEOUT_MS	2000
+
 static const u8 cb710_clock_divider_log2[8] = {
 /*	1, 2, 4, 8, 16, 32, 128, 512 */
 	0, 1, 2, 3,  4,  5,   7,   9
@@ -707,6 +709,12 @@ static int cb710_mmc_init(struct platform_device *pdev)
 	mmc->f_min = val >> cb710_clock_divider_log2[CB710_MAX_DIVIDER_IDX];
 	mmc->ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
+	/*
+	 * In cb710_wait_for_event() we use a fixed timeout of ~2s, hence let's
+	 * inform the core about it. A future improvement should instead make
+	 * use of the cmd->busy_timeout.
+	 */
+	mmc->max_busy_timeout = CB710_MMC_REQ_TIMEOUT_MS;
 
 	reader = mmc_priv(mmc);
 
-- 
2.20.1

Re: [PATCH 06/19] mmc: cb710: Inform the mmc core about the maximum busy timeout

[Michał Mirosław]

On Tue, Apr 14, 2020 at 06:14:00PM +0200, Ulf Hansson wrote:
> Some commands uses R1B responses, which means the card may assert the DAT0
> line to signal busy for a period of time, after it has received the
> command. The mmc core normally specifies the busy period for the command in
> the cmd->busy_timeout. Ideally the driver should respect it, but that
> requires quite some update of the code, so let's defer that to someone with
> the HW at hand.
> 
> Instead, let's inform the mmc core about the maximum supported busy timeout
> in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
> ~2s timeout of the polling loop, implemented in cb710_wait_for_event(). In
> this way, we let the mmc core validate the needed timeout, which may lead
> to that it converts from a R1B into a R1 response and then use CMD13 to
> poll for busy completion.
> 
> In other words, this change enables support for commands with longer busy
> periods than 2s, like erase (CMD38) for example.
[...]
> +	/*
> +	 * In cb710_wait_for_event() we use a fixed timeout of ~2s, hence let's
> +	 * inform the core about it. A future improvement should instead make
> +	 * use of the cmd->busy_timeout.
> +	 */
[...]

I'm not sure whether the controller limits busy signalling time.
Since this is rare HW now, I would just pass the timeout to
cb710_wait_for_event() and see if someone reports a bug.

Best Regards,
Michał Mirosław

Re: [PATCH 06/19] mmc: cb710: Inform the mmc core about the maximum busy timeout

[Ulf Hansson]

On Tue, 14 Apr 2020 at 20:40, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> On Tue, Apr 14, 2020 at 06:14:00PM +0200, Ulf Hansson wrote:
> > Some commands uses R1B responses, which means the card may assert the DAT0
> > line to signal busy for a period of time, after it has received the
> > command. The mmc core normally specifies the busy period for the command in
> > the cmd->busy_timeout. Ideally the driver should respect it, but that
> > requires quite some update of the code, so let's defer that to someone with
> > the HW at hand.
> >
> > Instead, let's inform the mmc core about the maximum supported busy timeout
> > in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
> > ~2s timeout of the polling loop, implemented in cb710_wait_for_event(). In
> > this way, we let the mmc core validate the needed timeout, which may lead
> > to that it converts from a R1B into a R1 response and then use CMD13 to
> > poll for busy completion.
> >
> > In other words, this change enables support for commands with longer busy
> > periods than 2s, like erase (CMD38) for example.
> [...]
> > +     /*
> > +      * In cb710_wait_for_event() we use a fixed timeout of ~2s, hence let's
> > +      * inform the core about it. A future improvement should instead make
> > +      * use of the cmd->busy_timeout.
> > +      */
> [...]
>
> I'm not sure whether the controller limits busy signalling time.
> Since this is rare HW now, I would just pass the timeout to
> cb710_wait_for_event() and see if someone reports a bug.

Alright, let me try something like that. Thanks for your suggestion.

Kind regards
Uffe

Re: [PATCH 06/19] mmc: cb710: Inform the mmc core about the maximum busy timeout

[Ulf Hansson]

On Wed, 15 Apr 2020 at 09:41, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 14 Apr 2020 at 20:40, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> >
> > On Tue, Apr 14, 2020 at 06:14:00PM +0200, Ulf Hansson wrote:
> > > Some commands uses R1B responses, which means the card may assert the DAT0
> > > line to signal busy for a period of time, after it has received the
> > > command. The mmc core normally specifies the busy period for the command in
> > > the cmd->busy_timeout. Ideally the driver should respect it, but that
> > > requires quite some update of the code, so let's defer that to someone with
> > > the HW at hand.
> > >
> > > Instead, let's inform the mmc core about the maximum supported busy timeout
> > > in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
> > > ~2s timeout of the polling loop, implemented in cb710_wait_for_event(). In
> > > this way, we let the mmc core validate the needed timeout, which may lead
> > > to that it converts from a R1B into a R1 response and then use CMD13 to
> > > poll for busy completion.
> > >
> > > In other words, this change enables support for commands with longer busy
> > > periods than 2s, like erase (CMD38) for example.
> > [...]
> > > +     /*
> > > +      * In cb710_wait_for_event() we use a fixed timeout of ~2s, hence let's
> > > +      * inform the core about it. A future improvement should instead make
> > > +      * use of the cmd->busy_timeout.
> > > +      */
> > [...]
> >
> > I'm not sure whether the controller limits busy signalling time.
> > Since this is rare HW now, I would just pass the timeout to
> > cb710_wait_for_event() and see if someone reports a bug.
>
> Alright, let me try something like that. Thanks for your suggestion.

I looked a bit more closely at your suggestion, but unfortunately it
requires quite some changes to the code.

To pass a timeout and need to move cb710_wait_for_event() from using a
fixed number of polling loops into using "timeout_ms" value instead.
Although, rather than open coding yet another new polling loop and
perhaps introduce new errors, I prefer moving to readx_poll_timeout().
However, to do that, even more changes are needed around
cb710_check_event().

Once that is done, we also have cb710_wait_while_busy(), that uses a
similar polling loop as cb710_wait_for_event(). So then it seems
reasonable to convert this one too.

To conclude, for now, I am going to apply $subject patch as is. Then
we can always do the above conversion later on, if we see that there
is a need for it.

Kind regards
Uffe

[PATCH 07/19] mmc: owl-mmc: Respect the cmd->busy_timeout from the mmc core

[Ulf Hansson]

For commands that doesn't involve to prepare a data transfer, owl-mmc is
using a fixed 30s response timeout. This is a bit problematic.

For some commands it means waiting longer than needed for the completion to
expire, which may not a big issue, but still. For other commands, like for
an erase (CMD38) that uses a R1B response, may require longer timeouts than
30s. In these cases, we may end up treating the command as it failed, while
it just needed some more time to complete successfully.

Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/owl-mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/owl-mmc.c b/drivers/mmc/host/owl-mmc.c
index 01ffe51f413d..5e20c099fe03 100644
--- a/drivers/mmc/host/owl-mmc.c
+++ b/drivers/mmc/host/owl-mmc.c
@@ -92,6 +92,8 @@
 #define OWL_SD_STATE_RC16ER		BIT(1)
 #define OWL_SD_STATE_CRC7ER		BIT(0)
 
+#define OWL_CMD_TIMEOUT_MS		30000
+
 struct owl_mmc_host {
 	struct device *dev;
 	struct reset_control *reset;
@@ -172,6 +174,7 @@ static void owl_mmc_send_cmd(struct owl_mmc_host *owl_host,
 			     struct mmc_command *cmd,
 			     struct mmc_data *data)
 {
+	unsigned long timeout;
 	u32 mode, state, resp[2];
 	u32 cmd_rsp_mask = 0;
 
@@ -239,7 +242,10 @@ static void owl_mmc_send_cmd(struct owl_mmc_host *owl_host,
 	if (data)
 		return;
 
-	if (!wait_for_completion_timeout(&owl_host->sdc_complete, 30 * HZ)) {
+	timeout = msecs_to_jiffies(cmd->busy_timeout ? cmd->busy_timeout :
+		OWL_CMD_TIMEOUT_MS);
+
+	if (!wait_for_completion_timeout(&owl_host->sdc_complete, timeout)) {
 		dev_err(owl_host->dev, "CMD interrupt timeout\n");
 		cmd->error = -ETIMEDOUT;
 		return;
-- 
2.20.1

[PATCH 08/19] mmc: sdricoh_cs: Drop unused defines

[Ulf Hansson]

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index a38b8b2a4e5c..1fc4db713ef5 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -57,10 +57,8 @@ static unsigned int switchlocked;
 #define STATUS_BUSY              0x40000000
 
 /* timeouts */
-#define INIT_TIMEOUT      100
 #define CMD_TIMEOUT       100000
 #define TRANSFER_TIMEOUT  100000
-#define BUSY_TIMEOUT      32767
 
 /* list of supported pcmcia devices */
 static const struct pcmcia_device_id pcmcia_ids[] = {
-- 
2.20.1

[PATCH 09/19] mmc: sdricoh_cs: Use MMC_APP_CMD rather than a hardcoded number

[Ulf Hansson]

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index 1fc4db713ef5..a41c0660abbf 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -22,6 +22,7 @@
 #include <linux/io.h>
 
 #include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
 
 #define DRIVER_NAME "sdricoh_cs"
 
@@ -261,7 +262,7 @@ static void sdricoh_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	if (host->app_cmd) {
 		opcode |= 64;
 		host->app_cmd = 0;
-	} else if (opcode == 55)
+	} else if (opcode == MMC_APP_CMD)
 		host->app_cmd = 1;
 
 	/* read/write commands seem to require this */
-- 
2.20.1

[PATCH 10/19] mmc: sdricoh_cs: Move MMC_APP_CMD handling to sdricoh_mmc_cmd()

[Ulf Hansson]

Move MMC_APP_CMD specific handling to be managed by sdricoh_mmc_cmd(), as
this makes the code a bit cleaner.

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index a41c0660abbf..e7d74db95b57 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -149,16 +149,25 @@ static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted,
 
 }
 
-static int sdricoh_mmc_cmd(struct sdricoh_host *host, unsigned char opcode,
-			   unsigned int arg)
+static int sdricoh_mmc_cmd(struct sdricoh_host *host, struct mmc_command *cmd)
 {
 	unsigned int status;
 	int result = 0;
 	unsigned int loop = 0;
+	unsigned char opcode = cmd->opcode;
+
 	/* reset status reg? */
 	sdricoh_writel(host, R21C_STATUS, 0x18);
+
+	/* MMC_APP_CMDs need some special handling */
+	if (host->app_cmd) {
+		opcode |= 64;
+		host->app_cmd = 0;
+	} else if (opcode == MMC_APP_CMD)
+		host->app_cmd = 1;
+
 	/* fill parameters */
-	sdricoh_writel(host, R204_CMD_ARG, arg);
+	sdricoh_writel(host, R204_CMD_ARG, cmd->arg);
 	sdricoh_writel(host, R200_CMD, (0x10000 << 8) | opcode);
 	/* wait for command completion */
 	if (opcode) {
@@ -250,28 +259,20 @@ static void sdricoh_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	struct mmc_command *cmd = mrq->cmd;
 	struct mmc_data *data = cmd->data;
 	struct device *dev = host->dev;
-	unsigned char opcode = cmd->opcode;
 	int i;
 
 	dev_dbg(dev, "=============================\n");
-	dev_dbg(dev, "sdricoh_request opcode=%i\n", opcode);
+	dev_dbg(dev, "sdricoh_request opcode=%i\n", cmd->opcode);
 
 	sdricoh_writel(host, R21C_STATUS, 0x18);
 
-	/* MMC_APP_CMDs need some special handling */
-	if (host->app_cmd) {
-		opcode |= 64;
-		host->app_cmd = 0;
-	} else if (opcode == MMC_APP_CMD)
-		host->app_cmd = 1;
-
 	/* read/write commands seem to require this */
 	if (data) {
 		sdricoh_writew(host, R226_BLOCKSIZE, data->blksz);
 		sdricoh_writel(host, R208_DATAIO, 0);
 	}
 
-	cmd->error = sdricoh_mmc_cmd(host, opcode, cmd->arg);
+	cmd->error = sdricoh_mmc_cmd(host, cmd);
 
 	/* read response buffer */
 	if (cmd->flags & MMC_RSP_PRESENT) {
-- 
2.20.1

[PATCH 11/19] mmc: sdricoh_cs: Drop redundant in-parameter to sdricoh_query_status()

[Ulf Hansson]

The in-parameter timeout is always set to TRANSFER_TIMEOUT by the callers
of sdricoh_query_status(), hence let's drop it.

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index e7d74db95b57..97ef7d71375a 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -123,11 +123,13 @@ static inline unsigned int sdricoh_readb(struct sdricoh_host *host,
 	return value;
 }
 
-static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted,
-				unsigned int timeout){
+static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted)
+{
 	unsigned int loop;
 	unsigned int status = 0;
+	unsigned int timeout = TRANSFER_TIMEOUT;
 	struct device *dev = host->dev;
+
 	for (loop = 0; loop < timeout; loop++) {
 		status = sdricoh_readl(host, R21C_STATUS);
 		sdricoh_writel(host, R2E4_STATUS_RESP, status);
@@ -215,8 +217,7 @@ static int sdricoh_blockio(struct sdricoh_host *host, int read,
 	u32 data = 0;
 	/* wait until the data is available */
 	if (read) {
-		if (sdricoh_query_status(host, STATUS_READY_TO_READ,
-						TRANSFER_TIMEOUT))
+		if (sdricoh_query_status(host, STATUS_READY_TO_READ))
 			return -ETIMEDOUT;
 		sdricoh_writel(host, R21C_STATUS, 0x18);
 		/* read data */
@@ -232,8 +233,7 @@ static int sdricoh_blockio(struct sdricoh_host *host, int read,
 			}
 		}
 	} else {
-		if (sdricoh_query_status(host, STATUS_READY_TO_WRITE,
-						TRANSFER_TIMEOUT))
+		if (sdricoh_query_status(host, STATUS_READY_TO_WRITE))
 			return -ETIMEDOUT;
 		sdricoh_writel(host, R21C_STATUS, 0x18);
 		/* write data */
@@ -323,8 +323,7 @@ static void sdricoh_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 		sdricoh_writel(host, R208_DATAIO, 1);
 
-		if (sdricoh_query_status(host, STATUS_TRANSFER_FINISHED,
-					TRANSFER_TIMEOUT)) {
+		if (sdricoh_query_status(host, STATUS_TRANSFER_FINISHED)) {
 			dev_err(dev, "sdricoh_request: transfer end error\n");
 			cmd->error = -EINVAL;
 		}
-- 
2.20.1

[PATCH 12/19] mmc: sdricoh_cs: Throttle polling rate for data transfers

[Ulf Hansson]

Rather than to poll in a busy-loop, let's convert into using
readl_poll_timeout() and insert a small delay between each polling
attempts. In particular, this avoids hogging the CPU.

Additionally, to convert to readl_poll_timeout() we also need to switch
from using a specific number of polling attempts, into a specific timeout
in us instead. The previous 100000 attempts, is translated into a total
timeout of total 1s, as that seemed like reasonable value to pick.

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index 97ef7d71375a..7e407fb6dab8 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/ioport.h>
+#include <linux/iopoll.h>
 #include <linux/scatterlist.h>
 
 #include <pcmcia/cistpl.h>
@@ -59,7 +60,7 @@ static unsigned int switchlocked;
 
 /* timeouts */
 #define CMD_TIMEOUT       100000
-#define TRANSFER_TIMEOUT  100000
+#define SDRICOH_DATA_TIMEOUT_US	1000000
 
 /* list of supported pcmcia devices */
 static const struct pcmcia_device_id pcmcia_ids[] = {
@@ -123,21 +124,23 @@ static inline unsigned int sdricoh_readb(struct sdricoh_host *host,
 	return value;
 }
 
+static bool sdricoh_status_ok(struct sdricoh_host *host, unsigned int status,
+			      unsigned int wanted)
+{
+	sdricoh_writel(host, R2E4_STATUS_RESP, status);
+	return status & wanted;
+}
+
 static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted)
 {
-	unsigned int loop;
+	int ret;
 	unsigned int status = 0;
-	unsigned int timeout = TRANSFER_TIMEOUT;
 	struct device *dev = host->dev;
 
-	for (loop = 0; loop < timeout; loop++) {
-		status = sdricoh_readl(host, R21C_STATUS);
-		sdricoh_writel(host, R2E4_STATUS_RESP, status);
-		if (status & wanted)
-			break;
-	}
-
-	if (loop == timeout) {
+	ret = readl_poll_timeout(host->iobase + R21C_STATUS, status,
+				 sdricoh_status_ok(host, status, wanted),
+				 32, SDRICOH_DATA_TIMEOUT_US);
+	if (ret) {
 		dev_err(dev, "query_status: timeout waiting for %x\n", wanted);
 		return -ETIMEDOUT;
 	}
-- 
2.20.1

[PATCH 13/19] mmc: sdricoh_cs: Throttle polling rate for commands

[Ulf Hansson]

Rather than to poll in a busy-loop, let's convert into using
readl_poll_timeout() and insert a small delay between each polling
attempts. In particular, this avoids hogging the CPU.

Additionally, to convert to readl_poll_timeout() we also need to switch
from using a specific number of polling attempts, into a specific timeout
in us instead. The previous 100000 attempts, is translated into a total
timeout of total 1s, as that seemed like reasonable value to pick.

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index 7e407fb6dab8..d7a191f14337 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -59,7 +59,7 @@ static unsigned int switchlocked;
 #define STATUS_BUSY              0x40000000
 
 /* timeouts */
-#define CMD_TIMEOUT       100000
+#define SDRICOH_CMD_TIMEOUT_US	1000000
 #define SDRICOH_DATA_TIMEOUT_US	1000000
 
 /* list of supported pcmcia devices */
@@ -157,8 +157,7 @@ static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted)
 static int sdricoh_mmc_cmd(struct sdricoh_host *host, struct mmc_command *cmd)
 {
 	unsigned int status;
-	int result = 0;
-	unsigned int loop = 0;
+	int ret;
 	unsigned char opcode = cmd->opcode;
 
 	/* reset status reg? */
@@ -174,24 +173,23 @@ static int sdricoh_mmc_cmd(struct sdricoh_host *host, struct mmc_command *cmd)
 	/* fill parameters */
 	sdricoh_writel(host, R204_CMD_ARG, cmd->arg);
 	sdricoh_writel(host, R200_CMD, (0x10000 << 8) | opcode);
+
 	/* wait for command completion */
-	if (opcode) {
-		for (loop = 0; loop < CMD_TIMEOUT; loop++) {
-			status = sdricoh_readl(host, R21C_STATUS);
-			sdricoh_writel(host, R2E4_STATUS_RESP, status);
-			if (status  & STATUS_CMD_FINISHED)
-				break;
-		}
-		/* don't check for timeout in the loop it is not always
-		   reset correctly
-		*/
-		if (loop == CMD_TIMEOUT || status & STATUS_CMD_TIMEOUT)
-			result = -ETIMEDOUT;
+	if (!opcode)
+		return 0;
 
-	}
+	ret = readl_poll_timeout(host->iobase + R21C_STATUS, status,
+			sdricoh_status_ok(host, status, STATUS_CMD_FINISHED),
+			32, SDRICOH_CMD_TIMEOUT_US);
 
-	return result;
+	/*
+	 * Don't check for timeout status in the loop, as it's not always reset
+	 * correctly.
+	 */
+	if (ret || status & STATUS_CMD_TIMEOUT)
+		return -ETIMEDOUT;
 
+	return 0;
 }
 
 static int sdricoh_reset(struct sdricoh_host *host)
-- 
2.20.1

[PATCH 14/19] mmc: sdricoh_cs: Respect the cmd->busy_timeout from the mmc core

[Ulf Hansson]

Using a fixed 1s polling timeout for all commands is a bit problematic.

For some commands it means waiting longer than needed for the polling to be
aborted, which may not a big issue, but still. For other commands, like for
an erase (CMD38) that uses a R1B response, may require longer timeouts than
1s. In these cases, we may end up treating the command as it failed, while
it just needed some more time to complete successfully.

Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.

Note that, even if the sdricoh_cs driver may currently not support HW busy
detection on DAT0, some comments in the code refer to that the HW may
support it. Therefore, it seems better to be proactive in this case.

Cc: Sascha Sommer <saschasommer@freenet.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdricoh_cs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index d7a191f14337..dfd395ff7c3e 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -156,7 +156,7 @@ static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted)
 
 static int sdricoh_mmc_cmd(struct sdricoh_host *host, struct mmc_command *cmd)
 {
-	unsigned int status;
+	unsigned int status, timeout_us;
 	int ret;
 	unsigned char opcode = cmd->opcode;
 
@@ -178,9 +178,12 @@ static int sdricoh_mmc_cmd(struct sdricoh_host *host, struct mmc_command *cmd)
 	if (!opcode)
 		return 0;
 
+	timeout_us = cmd->busy_timeout ? cmd->busy_timeout * 1000 :
+		SDRICOH_CMD_TIMEOUT_US;
+
 	ret = readl_poll_timeout(host->iobase + R21C_STATUS, status,
 			sdricoh_status_ok(host, status, STATUS_CMD_FINISHED),
-			32, SDRICOH_CMD_TIMEOUT_US);
+			32, timeout_us);
 
 	/*
 	 * Don't check for timeout status in the loop, as it's not always reset
-- 
2.20.1

[PATCH 15/19] mmc: tifm_sd: Inform the mmc core about the maximum busy timeout

[Ulf Hansson]

Some commands uses R1B responses, which means the card may assert the DAT0
line to signal busy for a period of time, after it has received the
command. The mmc core normally specifies the busy period for the command in
the cmd->busy_timeout. Ideally the driver should respect it, but that
requires quite some update of the code, so let's defer that to someone with
the HW at hand.

Instead, let's inform the mmc core about the maximum supported busy timeout
in ->max_busy_timeout during ->probe(). This value corresponds to the fixed
1s timeout used by tifm_sd. In this way, we let the mmc core validate the
needed timeout, which may lead to that it converts from a R1B into a R1
response and then use CMD13 to poll for busy completion.

In other words, this change enables support for commands with longer busy
periods than 1s, like erase (CMD38) for example.

Cc: Alex Dubov <oakad@yahoo.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/tifm_sd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/tifm_sd.c b/drivers/mmc/host/tifm_sd.c
index 54271b92ee59..5987656e0474 100644
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -73,6 +73,8 @@ module_param(fixed_timeout, bool, 0644);
 
 #define TIFM_MMCSD_MAX_BLOCK_SIZE  0x0800UL
 
+#define TIFM_MMCSD_REQ_TIMEOUT_MS  1000
+
 enum {
 	CMD_READY    = 0x0001,
 	FIFO_READY   = 0x0002,
@@ -959,7 +961,12 @@ static int tifm_sd_probe(struct tifm_dev *sock)
 	host = mmc_priv(mmc);
 	tifm_set_drvdata(sock, mmc);
 	host->dev = sock;
-	host->timeout_jiffies = msecs_to_jiffies(1000);
+	host->timeout_jiffies = msecs_to_jiffies(TIFM_MMCSD_REQ_TIMEOUT_MS);
+	/*
+	 * We use a fixed request timeout of 1s, hence inform the core about it.
+	 * A future improvement should instead respect the cmd->busy_timeout.
+	 */
+	mmc->max_busy_timeout = TIFM_MMCSD_REQ_TIMEOUT_MS;
 
 	tasklet_init(&host->finish_tasklet, tifm_sd_end_cmd,
 		     (unsigned long)host);
-- 
2.20.1

[PATCH 16/19] mmc: via-sdmmc: Respect the cmd->busy_timeout from the mmc core

[Ulf Hansson]

Using a fixed 1s timeout for all commands (and data transfers) is a bit
problematic.

For some commands it means waiting longer than needed for the timer to
expire, which may not a big issue, but still. For other commands, like for
an erase (CMD38) that uses a R1B response, may require longer timeouts than
1s. In these cases, we may end up treating the command as it failed, while
it just needed some more time to complete successfully.

Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.

Cc: Bruce Chang <brucechang@via.com.tw>
Cc: Harald Welte <HaraldWelte@viatech.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/via-sdmmc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/via-sdmmc.c b/drivers/mmc/host/via-sdmmc.c
index e48bddd95ce6..ef95bce50889 100644
--- a/drivers/mmc/host/via-sdmmc.c
+++ b/drivers/mmc/host/via-sdmmc.c
@@ -319,6 +319,8 @@ struct via_crdr_mmc_host {
 /* some devices need a very long delay for power to stabilize */
 #define VIA_CRDR_QUIRK_300MS_PWRDELAY	0x0001
 
+#define VIA_CMD_TIMEOUT_MS		1000
+
 static const struct pci_device_id via_ids[] = {
 	{PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_9530,
 	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0,},
@@ -551,14 +553,17 @@ static void via_sdc_send_command(struct via_crdr_mmc_host *host,
 {
 	void __iomem *addrbase;
 	struct mmc_data *data;
+	unsigned int timeout_ms;
 	u32 cmdctrl = 0;
 
 	WARN_ON(host->cmd);
 
 	data = cmd->data;
-	mod_timer(&host->timer, jiffies + HZ);
 	host->cmd = cmd;
 
+	timeout_ms = cmd->busy_timeout ? cmd->busy_timeout : VIA_CMD_TIMEOUT_MS;
+	mod_timer(&host->timer, jiffies + msecs_to_jiffies(timeout_ms));
+
 	/*Command index*/
 	cmdctrl = cmd->opcode << 8;
 
-- 
2.20.1

[PATCH 17/19] mmc: mmc_spi: Add/rename defines for timeouts

[Ulf Hansson]

Clarify the use of r1b_timeout, by renaming it to MMC_SPI_R1B_TIMEOUT_MS
and by dropping the corresponding confusing comment about it.

Additionally, let's also add a new define, MMC_SPI_INIT_TIMEOUT_MS and use
it during the initialization. Even if these two defines are given the same
value, the split makes it easier to understand them.

Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmc_spi.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 951f76dc1ddd..5768fe9f8f6f 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -77,14 +77,8 @@
 
 #define MMC_SPI_BLOCKSIZE	512
 
-
-/* These fixed timeouts come from the latest SD specs, which say to ignore
- * the CSD values.  The R1B value is for card erase (e.g. the "I forgot the
- * card's password" scenario); it's mostly applied to STOP_TRANSMISSION after
- * reads which takes nowhere near that long.  Older cards may be able to use
- * shorter timeouts ... but why bother?
- */
-#define r1b_timeout		(HZ * 3)
+#define MMC_SPI_R1B_TIMEOUT_MS	3000
+#define MMC_SPI_INIT_TIMEOUT_MS	3000
 
 /* One of the critical speed parameters is the amount of data which may
  * be transferred in one command. If this value is too low, the SD card
@@ -347,7 +341,8 @@ static int mmc_spi_response_get(struct mmc_spi_host *host,
 		while (cp < end && *cp == 0)
 			cp++;
 		if (cp == end)
-			mmc_spi_wait_unbusy(host, r1b_timeout);
+			mmc_spi_wait_unbusy(host,
+				msecs_to_jiffies(MMC_SPI_R1B_TIMEOUT_MS));
 		break;
 
 	/* SPI R2 == R1 + second status byte; SEND_STATUS
@@ -1118,7 +1113,7 @@ static void mmc_spi_initsequence(struct mmc_spi_host *host)
 	/* Try to be very sure any previous command has completed;
 	 * wait till not-busy, skip debris from any old commands.
 	 */
-	mmc_spi_wait_unbusy(host, r1b_timeout);
+	mmc_spi_wait_unbusy(host, msecs_to_jiffies(MMC_SPI_INIT_TIMEOUT_MS));
 	mmc_spi_readbytes(host, 10);
 
 	/*
-- 
2.20.1

[PATCH 18/19] mmc: mmc_spi: Respect the cmd->busy_timeout from the mmc core

[Ulf Hansson]

Using a fixed 3s polling timeout for all commands with R1B responses is a
bit problematic.

For some commands it means waiting longer than needed for the polling to be
aborted, which may not a big issue, but still. For other commands, like for
an erase (CMD38), may require longer timeouts than 3s. In these cases, we
may end up treating the command as it failed, while it just needed some
more time to complete successfully.

Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.

Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmc_spi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 5768fe9f8f6f..39bb1e30c2d7 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -242,6 +242,7 @@ static char *maptype(struct mmc_command *cmd)
 static int mmc_spi_response_get(struct mmc_spi_host *host,
 		struct mmc_command *cmd, int cs_on)
 {
+	unsigned long timeout_ms;
 	u8	*cp = host->data->status;
 	u8	*end = cp + host->t.len;
 	int	value = 0;
@@ -340,9 +341,11 @@ static int mmc_spi_response_get(struct mmc_spi_host *host,
 		/* maybe we read all the busy tokens already */
 		while (cp < end && *cp == 0)
 			cp++;
-		if (cp == end)
-			mmc_spi_wait_unbusy(host,
-				msecs_to_jiffies(MMC_SPI_R1B_TIMEOUT_MS));
+		if (cp == end) {
+			timeout_ms = cmd->busy_timeout ? cmd->busy_timeout :
+				MMC_SPI_R1B_TIMEOUT_MS;
+			mmc_spi_wait_unbusy(host, msecs_to_jiffies(timeout_ms));
+		}
 		break;
 
 	/* SPI R2 == R1 + second status byte; SEND_STATUS
-- 
2.20.1

[PATCH 19/19] staging: greybus: sdio: Respect the cmd->busy_timeout from the mmc core

[Ulf Hansson]

Using a fixed 1s timeout for all commands is a bit problematic.

For some commands it means waiting longer than needed for the timeout to
expire, which may not a big issue, but still. For other commands, like for
an erase (CMD38) that uses a R1B response, may require longer timeouts than
1s. In these cases, we may end up treating the command as it failed, while
it just needed some more time to complete successfully.

Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.

Cc: Rui Miguel Silva <rmfrfs@gmail.com>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: greybus-dev@lists.linaro.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/staging/greybus/sdio.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/sdio.c b/drivers/staging/greybus/sdio.c
index 68c5718be827..c4b16bb5c1a4 100644
--- a/drivers/staging/greybus/sdio.c
+++ b/drivers/staging/greybus/sdio.c
@@ -411,6 +411,7 @@ static int gb_sdio_command(struct gb_sdio_host *host, struct mmc_command *cmd)
 	struct gb_sdio_command_request request = {0};
 	struct gb_sdio_command_response response;
 	struct mmc_data *data = host->mrq->data;
+	unsigned int timeout_ms;
 	u8 cmd_flags;
 	u8 cmd_type;
 	int i;
@@ -469,9 +470,12 @@ static int gb_sdio_command(struct gb_sdio_host *host, struct mmc_command *cmd)
 		request.data_blksz = cpu_to_le16(data->blksz);
 	}
 
-	ret = gb_operation_sync(host->connection, GB_SDIO_TYPE_COMMAND,
-				&request, sizeof(request), &response,
-				sizeof(response));
+	timeout_ms = cmd->busy_timeout ? cmd->busy_timeout :
+		GB_OPERATION_TIMEOUT_DEFAULT;
+
+	ret = gb_operation_sync_timeout(host->connection, GB_SDIO_TYPE_COMMAND,
+					&request, sizeof(request), &response,
+					sizeof(response), timeout_ms);
 	if (ret < 0)
 		goto out;
 
-- 
2.20.1

Re: [PATCH 19/19] staging: greybus: sdio: Respect the cmd->busy_timeout from the mmc core

[Rui Miguel Silva]

Hi Ulf,
Thanks for the patch.

On Tue, Apr 14, 2020 at 06:14:13PM +0200, Ulf Hansson wrote:
> Using a fixed 1s timeout for all commands is a bit problematic.
> 
> For some commands it means waiting longer than needed for the timeout to
> expire, which may not a big issue, but still. For other commands, like for
> an erase (CMD38) that uses a R1B response, may require longer timeouts than
> 1s. In these cases, we may end up treating the command as it failed, while
> it just needed some more time to complete successfully.
> 
> Fix the problem by respecting the cmd->busy_timeout, which is provided by
> the mmc core.
> 
> Cc: Rui Miguel Silva <rmfrfs@gmail.com>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: greybus-dev@lists.linaro.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
LGTM

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui

> ---
>  drivers/staging/greybus/sdio.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/sdio.c b/drivers/staging/greybus/sdio.c
> index 68c5718be827..c4b16bb5c1a4 100644
> --- a/drivers/staging/greybus/sdio.c
> +++ b/drivers/staging/greybus/sdio.c
> @@ -411,6 +411,7 @@ static int gb_sdio_command(struct gb_sdio_host *host, struct mmc_command *cmd)
>  	struct gb_sdio_command_request request = {0};
>  	struct gb_sdio_command_response response;
>  	struct mmc_data *data = host->mrq->data;
> +	unsigned int timeout_ms;
>  	u8 cmd_flags;
>  	u8 cmd_type;
>  	int i;
> @@ -469,9 +470,12 @@ static int gb_sdio_command(struct gb_sdio_host *host, struct mmc_command *cmd)
>  		request.data_blksz = cpu_to_le16(data->blksz);
>  	}
>  
> -	ret = gb_operation_sync(host->connection, GB_SDIO_TYPE_COMMAND,
> -				&request, sizeof(request), &response,
> -				sizeof(response));
> +	timeout_ms = cmd->busy_timeout ? cmd->busy_timeout :
> +		GB_OPERATION_TIMEOUT_DEFAULT;
> +
> +	ret = gb_operation_sync_timeout(host->connection, GB_SDIO_TYPE_COMMAND,
> +					&request, sizeof(request), &response,
> +					sizeof(response), timeout_ms);
>  	if (ret < 0)
>  		goto out;
>  
> -- 
> 2.20.1
> 

Re: [PATCH 19/19] staging: greybus: sdio: Respect the cmd->busy_timeout from the mmc core

[Greg Kroah-Hartman]

On Tue, Apr 14, 2020 at 06:14:13PM +0200, Ulf Hansson wrote:
> Using a fixed 1s timeout for all commands is a bit problematic.
> 
> For some commands it means waiting longer than needed for the timeout to
> expire, which may not a big issue, but still. For other commands, like for
> an erase (CMD38) that uses a R1B response, may require longer timeouts than
> 1s. In these cases, we may end up treating the command as it failed, while
> it just needed some more time to complete successfully.
> 
> Fix the problem by respecting the cmd->busy_timeout, which is provided by
> the mmc core.
> 
> Cc: Rui Miguel Silva <rmfrfs@gmail.com>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: greybus-dev@lists.linaro.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>