[PATCH 00/10] mmc: Improve busy detection for MMC_CAP_WAIT_WHILE_BUSY

[PATCH 00/10] mmc: Improve busy detection for MMC_CAP_WAIT_WHILE_BUSY

[Ulf Hansson]

This patchset improves the handling around busy detection in the mmc core layer
while operating on host supporting MMC_CAP_WAIT_WHILE_BUSY.

A R1B response is for an mmc command, specified as and R1 but with an optional
busy assertion on the DAT0 line. Hosts supporting MMC_CAP_WAIT_WHILE_BUSY,
normally has a busy detection mechanism build in it's controller HW.

Using such a feature decreases the need for polling of the card's status using
CMD13, which is the fallback method used by the mmc core for hosts that don't
support MMC_CAP_WAIT_WHILE_BUSY.

Typcial commands that expects R1B responses are CMD6 (SWITCH), CMD12 (STOP),
CMD38 (ERASE) and CMD5 (SLEEP). This patchset adresses CMD6, CMD5 and improves
some parts where CMD12 are used. If the implemented approach becomes accepted,
a future patchset for CMD38 can be based on top if this patchset.

Do note, the final two patches implements support for busy detection for the
mmci host driver, since some of it's HW variants do supports busy detection.

Future suggested improvements related to this patchset: (Please, feel free to
implement any of them :-) ).

a) For CMD38, select a fixed number maximum blocks to accept for
erase/discard/trim operations. Compute the needed timeout depending on each
card's erase information provided through it's CSD/EXT_CSD registers. Then
follow the same principle as for sending a CMD6.

b) At least for CMD38, but likely for other commands as well, we could benefit
from doing a _periodic_ CMD13 polling to handle the busy completion. This will
also be useful for hosts supporting MMC_CAP_WAIT_WHILE_BUSY, in particular for
cases where the host are unable to support the needed busy timeout.

c) Handle timeouts while polling for card's status with CMD13 in cases where
a CMD12 has been used to finalize a data DATA_WRITE transfer.


Ulf Hansson (10):
  mmc: core: Rename max_discard_to to max_busy_timeout
  mmc: core: Rename cmd_timeout_ms to busy_timeout
  mmc: core: Add ignore_crc flag to __mmc_switch
  mmc: core: Fixup busy detection for mmc switch operations
  mmc: core: Use generic CMD6 time while switching to eMMC HS200 mode
  mmc: core: Respect host's max_busy_timeout when sending sleep cmd
  mmc: card: Use R1 responses for stop cmds for read requests
  mmc: card: Use R1 response for the stop cmd at recovery path
  mmc: mmci: Handle CMD irq before DATA irq
  mmc: mmci: Enable support for busy detection for ux500 variant

 drivers/mmc/card/block.c   |    9 +++++--
 drivers/mmc/core/core.c    |   11 ++++----
 drivers/mmc/core/mmc.c     |   34 ++++++++++++++++-------
 drivers/mmc/core/mmc_ops.c |   64 +++++++++++++++++++++++++++++---------------
 drivers/mmc/host/mmci.c    |   54 ++++++++++++++++++++++++++++++-------
 drivers/mmc/host/mmci.h    |    2 ++
 drivers/mmc/host/sdhci.c   |   10 +++----
 include/linux/mmc/core.h   |    4 +--
 include/linux/mmc/host.h   |    2 +-
 9 files changed, 135 insertions(+), 55 deletions(-)

-- 
1.7.9.5

[PATCH 01/10] mmc: core: Rename max_discard_to to max_busy_timeout

[Ulf Hansson]

Rename host->max_discard_to to host->max_busy_timeout, to reflect that
it tells the mmc core layer about the maximum supported busy detection
timeout by the host.

This timeout is at the moment only applicable to erase/trim/discard
commands. By the renaming we provide the option of make use of it for
other commands that cares about busy detection. In other words, those
commands that wants an R1B response, like for example the mmc switch
command.

Do note that the max_busy_timeout is supposed to be specified only by
hosts supporting MMC_CAP_WAIT_WHILE_BUSY.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c  |    6 +++---
 drivers/mmc/host/sdhci.c |    2 +-
 include/linux/mmc/host.h |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 098374b..a81d754 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2137,7 +2137,7 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 		y = 0;
 		for (x = 1; x && x <= max_qty && max_qty - x >= qty; x <<= 1) {
 			timeout = mmc_erase_timeout(card, arg, qty + x);
-			if (timeout > host->max_discard_to)
+			if (timeout > host->max_busy_timeout)
 				break;
 			if (timeout < last_timeout)
 				break;
@@ -2169,7 +2169,7 @@ unsigned int mmc_calc_max_discard(struct mmc_card *card)
 	struct mmc_host *host = card->host;
 	unsigned int max_discard, max_trim;
 
-	if (!host->max_discard_to)
+	if (!host->max_busy_timeout)
 		return UINT_MAX;
 
 	/*
@@ -2189,7 +2189,7 @@ unsigned int mmc_calc_max_discard(struct mmc_card *card)
 		max_discard = 0;
 	}
 	pr_debug("%s: calculated max. discard sectors %u for timeout %u ms\n",
-		 mmc_hostname(host), max_discard, host->max_discard_to);
+		 mmc_hostname(host), max_discard, host->max_busy_timeout);
 	return max_discard;
 }
 EXPORT_SYMBOL(mmc_calc_max_discard);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9ddef47..ec57c6c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2941,7 +2941,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
 		host->timeout_clk = mmc->f_max / 1000;
 
-	mmc->max_discard_to = (1 << 27) / host->timeout_clk;
+	mmc->max_busy_timeout = (1 << 27) / host->timeout_clk;
 
 	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 99f5709..9805522 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -304,7 +304,7 @@ struct mmc_host {
 	unsigned int		max_req_size;	/* maximum number of bytes in one req */
 	unsigned int		max_blk_size;	/* maximum size of one mmc block */
 	unsigned int		max_blk_count;	/* maximum number of blocks in one req */
-	unsigned int		max_discard_to;	/* max. discard timeout in ms */
+	unsigned int		max_busy_timeout; /* max busy timeout in ms */
 
 	/* private data */
 	spinlock_t		lock;		/* lock for claim and bus ops */
-- 
1.7.9.5

[PATCH 02/10] mmc: core: Rename cmd_timeout_ms to busy_timeout

[Ulf Hansson]

To better reflect that the cmd_timeout_ms is directly related to the
busy detection timeout, let's rename it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c    |    2 +-
 drivers/mmc/core/mmc_ops.c |    2 +-
 drivers/mmc/host/sdhci.c   |    8 ++++----
 include/linux/mmc/core.h   |    2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index a81d754..90de094 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	cmd.opcode = MMC_ERASE;
 	cmd.arg = arg;
 	cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
-	cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
+	cmd.busy_timeout = mmc_erase_timeout(card, arg, qty);
 	err = mmc_wait_for_cmd(card->host, &cmd, 0);
 	if (err) {
 		pr_err("mmc_erase: erase error %d, status %#x\n",
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index e5b5eeb..3377bbf 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -432,7 +432,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
 
 
-	cmd.cmd_timeout_ms = timeout_ms;
+	cmd.busy_timeout = timeout_ms;
 	if (index == EXT_CSD_SANITIZE_START)
 		cmd.sanitize_busy = true;
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ec57c6c..1853255 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -675,12 +675,12 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		return 0xE;
 
 	/* Unspecified timeout, assume max */
-	if (!data && !cmd->cmd_timeout_ms)
+	if (!data && !cmd->busy_timeout)
 		return 0xE;
 
 	/* timeout in us */
 	if (!data)
-		target_timeout = cmd->cmd_timeout_ms * 1000;
+		target_timeout = cmd->busy_timeout * 1000;
 	else {
 		target_timeout = data->timeout_ns / 1000;
 		if (host->clock)
@@ -1019,8 +1019,8 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 
 	timeout = jiffies;
-	if (!cmd->data && cmd->cmd_timeout_ms > 9000)
-		timeout += DIV_ROUND_UP(cmd->cmd_timeout_ms, 1000) * HZ + HZ;
+	if (!cmd->data && cmd->busy_timeout > 9000)
+		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
 	else
 		timeout += 10 * HZ;
 	mod_timer(&host->timer, timeout);
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 87079fc..b276996 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -95,7 +95,7 @@ struct mmc_command {
  *              actively failing requests
  */
 
-	unsigned int		cmd_timeout_ms;	/* in milliseconds */
+	unsigned int		busy_timeout;	/* busy detect timeout in ms */
 	/* Set this flag only for blocking sanitize request */
 	bool			sanitize_busy;
 
-- 
1.7.9.5

[PATCH 03/10] mmc: core: Add ignore_crc flag to __mmc_switch

[Ulf Hansson]

Instead of handle specific adaptations, releated to certain switch
operations, inside __mmc_switch, push this to be handled by the caller
instead.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c    |    3 ++-
 drivers/mmc/core/mmc.c     |   13 +++++++------
 drivers/mmc/core/mmc_ops.c |   19 ++++++++++---------
 include/linux/mmc/core.h   |    2 +-
 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 90de094..75ce01d 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -302,7 +302,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	}
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-			EXT_CSD_BKOPS_START, 1, timeout, use_busy_signal, true);
+			EXT_CSD_BKOPS_START, 1, timeout,
+			use_busy_signal, true, false);
 	if (err) {
 		pr_warn("%s: Error %d starting bkops\n",
 			mmc_hostname(card->host), err);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 98e9eb0..ae3489a 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -856,8 +856,8 @@ static int mmc_select_hs200(struct mmc_card *card)
 
 	/* switch to HS200 mode if bus width set successfully */
 	if (!err)
-		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-				 EXT_CSD_HS_TIMING, 2, 0);
+		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				EXT_CSD_HS_TIMING, 2, 0, true, true, true);
 err:
 	return err;
 }
@@ -1074,9 +1074,10 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 		    host->caps2 & MMC_CAP2_HS200)
 			err = mmc_select_hs200(card);
 		else if	(host->caps & MMC_CAP_MMC_HIGHSPEED)
-			err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-					 EXT_CSD_HS_TIMING, 1,
-					 card->ext_csd.generic_cmd6_time);
+			err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+					EXT_CSD_HS_TIMING, 1,
+					card->ext_csd.generic_cmd6_time,
+					true, true, true);
 
 		if (err && err != -EBADMSG)
 			goto free_card;
@@ -1404,7 +1405,7 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			EXT_CSD_POWER_OFF_NOTIFICATION,
-			notify_type, timeout, true, false);
+			notify_type, timeout, true, false, false);
 	if (err)
 		pr_err("%s: Power Off Notification timed out, %u\n",
 		       mmc_hostname(card->host), timeout);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 3377bbf..5e1a2cb 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -405,17 +405,18 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc)
  *                   timeout of zero implies maximum possible timeout
  *	@use_busy_signal: use the busy signal as response type
  *	@send_status: send status cmd to poll for busy
+ *	@ignore_crc: ignore CRC errors when sending status cmd to poll for busy
  *
  *	Modifies the EXT_CSD register for selected card.
  */
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
-		unsigned int timeout_ms, bool use_busy_signal, bool send_status)
+		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
+		bool ignore_crc)
 {
 	int err;
 	struct mmc_command cmd = {0};
 	unsigned long timeout;
 	u32 status = 0;
-	bool ignore_crc = false;
 
 	BUG_ON(!card);
 	BUG_ON(!card->host);
@@ -445,14 +446,13 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		return 0;
 
 	/*
-	 * Must check status to be sure of no errors
-	 * If CMD13 is to check the busy completion of the timing change,
-	 * disable the check of CRC error.
+	 * CRC errors shall only be ignored in cases were CMD13 is used to poll
+	 * to detect busy completion.
 	 */
-	if (index == EXT_CSD_HS_TIMING &&
-	    !(card->host->caps & MMC_CAP_WAIT_WHILE_BUSY))
-		ignore_crc = true;
+	if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		ignore_crc = false;
 
+	/* Must check status to be sure of no errors. */
 	timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);
 	do {
 		if (send_status) {
@@ -501,7 +501,8 @@ EXPORT_SYMBOL_GPL(__mmc_switch);
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms)
 {
-	return __mmc_switch(card, set, index, value, timeout_ms, true, true);
+	return __mmc_switch(card, set, index, value, timeout_ms, true, true,
+				false);
 }
 EXPORT_SYMBOL_GPL(mmc_switch);
 
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index b276996..f206e29 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -152,7 +152,7 @@ extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
 	struct mmc_command *, int);
 extern void mmc_start_bkops(struct mmc_card *card, bool from_exception);
 extern int __mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int, bool,
-			bool);
+			bool, bool);
 extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
 extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
 
-- 
1.7.9.5

[PATCH 04/10] mmc: core: Fixup busy detection for mmc switch operations

[Ulf Hansson]

If the host controller supports busy detection in HW, we expect the
MMC_CAP_WAIT_WHILE_BUSY to be set. Likewise the corresponding
host->max_busy_timeout shall reflect the maximum busy detection timeout
supported by the host. A timeout set to zero, is interpreted as the
host supports whatever timeout the mmc core provides it with.

Previously we expected a host that supported MMC_CAP_WAIT_WHILE_BUSY to
cope with any timeout, which just isn't feasible due to HW limitations.

For most switch operations, R1B responses are expected and thus we need
to check for busy detection completion. To cope with cases where the
requested busy detection timeout is greater than what the host are able
to support, we fallback to use a R1 response instead. This will prevent
the host from doing HW busy detection.

In those cases busy detection completion is handled by polling the for
the card's status using CMD13, which is the same mechanism used when
the host doesn't support MMC_CAP_WAIT_WHILE_BUSY.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c |   53 ++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 5e1a2cb..2e0cccb 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -413,13 +413,31 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
 		bool ignore_crc)
 {
+	struct mmc_host *host;
 	int err;
 	struct mmc_command cmd = {0};
 	unsigned long timeout;
+	unsigned int max_busy_timeout;
 	u32 status = 0;
+	bool use_r1b_resp = true;
 
 	BUG_ON(!card);
 	BUG_ON(!card->host);
+	host = card->host;
+
+	/* Once all callers provides a timeout, remove this fallback. */
+	if (!timeout_ms)
+		timeout_ms = MMC_OPS_TIMEOUT_MS;
+
+	/* We interpret unspecified timeouts as the host can cope with all. */
+	max_busy_timeout = host->max_busy_timeout ?
+			host->max_busy_timeout : timeout_ms;
+
+	if (use_busy_signal && (host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
+		(timeout_ms > max_busy_timeout))
+			use_r1b_resp = false;
+	else if (!use_busy_signal)
+		use_r1b_resp = false;
 
 	cmd.opcode = MMC_SWITCH;
 	cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
@@ -427,17 +445,25 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		  (value << 8) |
 		  set;
 	cmd.flags = MMC_CMD_AC;
-	if (use_busy_signal)
+	if (use_r1b_resp)
 		cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
 	else
 		cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
 
+	if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) {
+		/* Tell the host what busy detection timeout to use. */
+		cmd.busy_timeout = timeout_ms;
+		/*
+		 * CRC errors shall only be ignored in cases were CMD13 is used
+		 * to poll to detect busy completion.
+		 */
+		ignore_crc = false;
+	}
 
-	cmd.busy_timeout = timeout_ms;
 	if (index == EXT_CSD_SANITIZE_START)
 		cmd.sanitize_busy = true;
 
-	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
+	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
 	if (err)
 		return err;
 
@@ -445,24 +471,17 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	if (!use_busy_signal)
 		return 0;
 
-	/*
-	 * CRC errors shall only be ignored in cases were CMD13 is used to poll
-	 * to detect busy completion.
-	 */
-	if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
-		ignore_crc = false;
-
 	/* Must check status to be sure of no errors. */
-	timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);
+	timeout = jiffies + msecs_to_jiffies(timeout_ms);
 	do {
 		if (send_status) {
 			err = __mmc_send_status(card, &status, ignore_crc);
 			if (err)
 				return err;
 		}
-		if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
 			break;
-		if (mmc_host_is_spi(card->host))
+		if (mmc_host_is_spi(host))
 			break;
 
 		/*
@@ -478,18 +497,18 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		/* Timeout if the device never leaves the program state. */
 		if (time_after(jiffies, timeout)) {
 			pr_err("%s: Card stuck in programming state! %s\n",
-				mmc_hostname(card->host), __func__);
+				mmc_hostname(host), __func__);
 			return -ETIMEDOUT;
 		}
 	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
 
-	if (mmc_host_is_spi(card->host)) {
+	if (mmc_host_is_spi(host)) {
 		if (status & R1_SPI_ILLEGAL_COMMAND)
 			return -EBADMSG;
 	} else {
 		if (status & 0xFDFFA000)
-			pr_warning("%s: unexpected status %#x after "
-			       "switch", mmc_hostname(card->host), status);
+			pr_warn("%s: unexpected status %#x after switch\n",
+				mmc_hostname(host), status);
 		if (status & R1_SWITCH_ERROR)
 			return -EBADMSG;
 	}
-- 
1.7.9.5

Re: [PATCH 04/10] mmc: core: Fixup busy detection for mmc switch operations

[Adrian Hunter]

On 22/01/14 17:00, Ulf Hansson wrote:
> If the host controller supports busy detection in HW, we expect the
> MMC_CAP_WAIT_WHILE_BUSY to be set. Likewise the corresponding
> host->max_busy_timeout shall reflect the maximum busy detection timeout
> supported by the host. A timeout set to zero, is interpreted as the
> host supports whatever timeout the mmc core provides it with.
> 
> Previously we expected a host that supported MMC_CAP_WAIT_WHILE_BUSY to
> cope with any timeout, which just isn't feasible due to HW limitations.
> 
> For most switch operations, R1B responses are expected and thus we need
> to check for busy detection completion. To cope with cases where the
> requested busy detection timeout is greater than what the host are able
> to support, we fallback to use a R1 response instead. This will prevent
> the host from doing HW busy detection.
> 
> In those cases busy detection completion is handled by polling the for
> the card's status using CMD13, which is the same mechanism used when
> the host doesn't support MMC_CAP_WAIT_WHILE_BUSY.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc_ops.c |   53 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 5e1a2cb..2e0cccb 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -413,13 +413,31 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
>  		bool ignore_crc)
>  {
> +	struct mmc_host *host;

It would be nicer if the addition of 'host' was a separate patch.  You
should remove the unnecessary BUG_ONs (it will oops anyway) at the same
time and then just do:

	struct mmc_host *host = card->host;

>  	int err;
>  	struct mmc_command cmd = {0};
>  	unsigned long timeout;
> +	unsigned int max_busy_timeout;
>  	u32 status = 0;
> +	bool use_r1b_resp = true;

This is a little confusing.  Why not:

	bool use_r1b_resp = use_busy_signal;

Although 'use_busy_signal' actually means 'wait_while_busy'.

>  
>  	BUG_ON(!card);
>  	BUG_ON(!card->host);
> +	host = card->host;
> +
> +	/* Once all callers provides a timeout, remove this fallback. */
> +	if (!timeout_ms)
> +		timeout_ms = MMC_OPS_TIMEOUT_MS;

A timeout of zero does not mean a very long timeout.  It means an unknown timeout.

> +
> +	/* We interpret unspecified timeouts as the host can cope with all. */
> +	max_busy_timeout = host->max_busy_timeout ?
> +			host->max_busy_timeout : timeout_ms;
> +
> +	if (use_busy_signal && (host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
> +		(timeout_ms > max_busy_timeout))
> +			use_r1b_resp = false;
> +	else if (!use_busy_signal)
> +		use_r1b_resp = false;

Why not just check what you know:

	if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
		use_r1b_resp = false;

>  
>  	cmd.opcode = MMC_SWITCH;
>  	cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
> @@ -427,17 +445,25 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  		  (value << 8) |
>  		  set;
>  	cmd.flags = MMC_CMD_AC;
> -	if (use_busy_signal)
> +	if (use_r1b_resp)
>  		cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
>  	else
>  		cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
>  
> +	if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) {
> +		/* Tell the host what busy detection timeout to use. */
> +		cmd.busy_timeout = timeout_ms;
> +		/*
> +		 * CRC errors shall only be ignored in cases were CMD13 is used
> +		 * to poll to detect busy completion.
> +		 */
> +		ignore_crc = false;
> +	}
>  
> -	cmd.busy_timeout = timeout_ms;

The busy_timeout should be provided for R1B i.e. this should be:

	if (use_r1b_resp)
		cmd.busy_timeout = timeout_ms;

>  	if (index == EXT_CSD_SANITIZE_START)
>  		cmd.sanitize_busy = true;
>  
> -	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
> +	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>  	if (err)
>  		return err;
>  
> @@ -445,24 +471,17 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  	if (!use_busy_signal)
>  		return 0;
>  
> -	/*
> -	 * CRC errors shall only be ignored in cases were CMD13 is used to poll
> -	 * to detect busy completion.
> -	 */
> -	if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
> -		ignore_crc = false;
> -
>  	/* Must check status to be sure of no errors. */
> -	timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);

This is the place to set the default timeout for the loop.

	if (!timeout_ms)
		timeout_ms = MMC_OPS_TIMEOUT_MS

> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
>  	do {
>  		if (send_status) {
>  			err = __mmc_send_status(card, &status, ignore_crc);
>  			if (err)
>  				return err;
>  		}
> -		if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
> +		if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>  			break;
> -		if (mmc_host_is_spi(card->host))
> +		if (mmc_host_is_spi(host))
>  			break;
>  
>  		/*
> @@ -478,18 +497,18 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  		/* Timeout if the device never leaves the program state. */
>  		if (time_after(jiffies, timeout)) {
>  			pr_err("%s: Card stuck in programming state! %s\n",
> -				mmc_hostname(card->host), __func__);
> +				mmc_hostname(host), __func__);
>  			return -ETIMEDOUT;
>  		}
>  	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>  
> -	if (mmc_host_is_spi(card->host)) {
> +	if (mmc_host_is_spi(host)) {
>  		if (status & R1_SPI_ILLEGAL_COMMAND)
>  			return -EBADMSG;
>  	} else {
>  		if (status & 0xFDFFA000)
> -			pr_warning("%s: unexpected status %#x after "
> -			       "switch", mmc_hostname(card->host), status);
> +			pr_warn("%s: unexpected status %#x after switch\n",
> +				mmc_hostname(host), status);
>  		if (status & R1_SWITCH_ERROR)
>  			return -EBADMSG;
>  	}
> 

Re: [PATCH 04/10] mmc: core: Fixup busy detection for mmc switch operations

[Ulf Hansson]

On 23 January 2014 11:10, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 22/01/14 17:00, Ulf Hansson wrote:
>> If the host controller supports busy detection in HW, we expect the
>> MMC_CAP_WAIT_WHILE_BUSY to be set. Likewise the corresponding
>> host->max_busy_timeout shall reflect the maximum busy detection timeout
>> supported by the host. A timeout set to zero, is interpreted as the
>> host supports whatever timeout the mmc core provides it with.
>>
>> Previously we expected a host that supported MMC_CAP_WAIT_WHILE_BUSY to
>> cope with any timeout, which just isn't feasible due to HW limitations.
>>
>> For most switch operations, R1B responses are expected and thus we need
>> to check for busy detection completion. To cope with cases where the
>> requested busy detection timeout is greater than what the host are able
>> to support, we fallback to use a R1 response instead. This will prevent
>> the host from doing HW busy detection.
>>
>> In those cases busy detection completion is handled by polling the for
>> the card's status using CMD13, which is the same mechanism used when
>> the host doesn't support MMC_CAP_WAIT_WHILE_BUSY.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc_ops.c |   53 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 36 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 5e1a2cb..2e0cccb 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -413,13 +413,31 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>               unsigned int timeout_ms, bool use_busy_signal, bool send_status,
>>               bool ignore_crc)
>>  {
>> +     struct mmc_host *host;
>
> It would be nicer if the addition of 'host' was a separate patch.  You
> should remove the unnecessary BUG_ONs (it will oops anyway) at the same
> time and then just do:
>
>         struct mmc_host *host = card->host;

Sure, make sense!

>
>>       int err;
>>       struct mmc_command cmd = {0};
>>       unsigned long timeout;
>> +     unsigned int max_busy_timeout;
>>       u32 status = 0;
>> +     bool use_r1b_resp = true;
>
> This is a little confusing.  Why not:
>
>         bool use_r1b_resp = use_busy_signal;
>
> Although 'use_busy_signal' actually means 'wait_while_busy'.

Right, that should simplify code a bit. I will update in a v2.

>
>>
>>       BUG_ON(!card);
>>       BUG_ON(!card->host);
>> +     host = card->host;
>> +
>> +     /* Once all callers provides a timeout, remove this fallback. */
>> +     if (!timeout_ms)
>> +             timeout_ms = MMC_OPS_TIMEOUT_MS;
>
> A timeout of zero does not mean a very long timeout.  It means an unknown timeout.

I guess this is a matter of definition.

For those hosts that don't have a hw timeout, but maybe implements a
software timeout, I thought this was more convenient. We likely then
also need to define a "MAX_BUSY_TIMEOUT", which host drivers could
use.

Additionally, since as of today only sdhci specifies the
max_discard_to (renamed to max_busy_timeout), I thought it make sense
to not force other hosts to specify the timeout to keep the existing
behaviour.

>
>> +
>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>> +     max_busy_timeout = host->max_busy_timeout ?
>> +                     host->max_busy_timeout : timeout_ms;
>> +
>> +     if (use_busy_signal && (host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>> +             (timeout_ms > max_busy_timeout))
>> +                     use_r1b_resp = false;
>> +     else if (!use_busy_signal)
>> +             use_r1b_resp = false;
>
> Why not just check what you know:
>
>         if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
>                 use_r1b_resp = false;
>

I wanted to maintain the R1B response for hosts that don't support
MMC_CAP_WAIT_WHILE_BUSY. With your proposal this will not be done.

Given this a second thought. I think it would make sense to adapt to
your proposal. I will update in v2.

>>
>>       cmd.opcode = MMC_SWITCH;
>>       cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
>> @@ -427,17 +445,25 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>                 (value << 8) |
>>                 set;
>>       cmd.flags = MMC_CMD_AC;
>> -     if (use_busy_signal)
>> +     if (use_r1b_resp)
>>               cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
>>       else
>>               cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
>>
>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) {
>> +             /* Tell the host what busy detection timeout to use. */
>> +             cmd.busy_timeout = timeout_ms;
>> +             /*
>> +              * CRC errors shall only be ignored in cases were CMD13 is used
>> +              * to poll to detect busy completion.
>> +              */
>> +             ignore_crc = false;
>> +     }
>>
>> -     cmd.busy_timeout = timeout_ms;
>
> The busy_timeout should be provided for R1B i.e. this should be:
>
>         if (use_r1b_resp)
>                 cmd.busy_timeout = timeout_ms;
>

Will fix in v2, given you still think this is good approach according
to my comment just above.

>>       if (index == EXT_CSD_SANITIZE_START)
>>               cmd.sanitize_busy = true;
>>
>> -     err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
>> +     err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>>       if (err)
>>               return err;
>>
>> @@ -445,24 +471,17 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>       if (!use_busy_signal)
>>               return 0;
>>
>> -     /*
>> -      * CRC errors shall only be ignored in cases were CMD13 is used to poll
>> -      * to detect busy completion.
>> -      */
>> -     if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>> -             ignore_crc = false;
>> -
>>       /* Must check status to be sure of no errors. */
>> -     timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);
>
> This is the place to set the default timeout for the loop.
>
>         if (!timeout_ms)
>                 timeout_ms = MMC_OPS_TIMEOUT_MS
>
>> +     timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>       do {
>>               if (send_status) {
>>                       err = __mmc_send_status(card, &status, ignore_crc);
>>                       if (err)
>>                               return err;
>>               }
>> -             if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>> +             if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>>                       break;
>> -             if (mmc_host_is_spi(card->host))
>> +             if (mmc_host_is_spi(host))
>>                       break;
>>
>>               /*
>> @@ -478,18 +497,18 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>               /* Timeout if the device never leaves the program state. */
>>               if (time_after(jiffies, timeout)) {
>>                       pr_err("%s: Card stuck in programming state! %s\n",
>> -                             mmc_hostname(card->host), __func__);
>> +                             mmc_hostname(host), __func__);
>>                       return -ETIMEDOUT;
>>               }
>>       } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>>
>> -     if (mmc_host_is_spi(card->host)) {
>> +     if (mmc_host_is_spi(host)) {
>>               if (status & R1_SPI_ILLEGAL_COMMAND)
>>                       return -EBADMSG;
>>       } else {
>>               if (status & 0xFDFFA000)
>> -                     pr_warning("%s: unexpected status %#x after "
>> -                            "switch", mmc_hostname(card->host), status);
>> +                     pr_warn("%s: unexpected status %#x after switch\n",
>> +                             mmc_hostname(host), status);
>>               if (status & R1_SWITCH_ERROR)
>>                       return -EBADMSG;
>>       }
>>
>

Adrian, thanks for reviewing!

Kind regards
Uffe

Re: [PATCH 04/10] mmc: core: Fixup busy detection for mmc switch operations

[Adrian Hunter]

On 23/01/14 16:11, Ulf Hansson wrote:
> On 23 January 2014 11:10, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 22/01/14 17:00, Ulf Hansson wrote:
>>> If the host controller supports busy detection in HW, we expect the
>>> MMC_CAP_WAIT_WHILE_BUSY to be set. Likewise the corresponding
>>> host->max_busy_timeout shall reflect the maximum busy detection timeout
>>> supported by the host. A timeout set to zero, is interpreted as the
>>> host supports whatever timeout the mmc core provides it with.
>>>
>>> Previously we expected a host that supported MMC_CAP_WAIT_WHILE_BUSY to
>>> cope with any timeout, which just isn't feasible due to HW limitations.
>>>
>>> For most switch operations, R1B responses are expected and thus we need
>>> to check for busy detection completion. To cope with cases where the
>>> requested busy detection timeout is greater than what the host are able
>>> to support, we fallback to use a R1 response instead. This will prevent
>>> the host from doing HW busy detection.
>>>
>>> In those cases busy detection completion is handled by polling the for
>>> the card's status using CMD13, which is the same mechanism used when
>>> the host doesn't support MMC_CAP_WAIT_WHILE_BUSY.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/core/mmc_ops.c |   53 ++++++++++++++++++++++++++++++--------------
>>>  1 file changed, 36 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>> index 5e1a2cb..2e0cccb 100644
>>> --- a/drivers/mmc/core/mmc_ops.c
>>> +++ b/drivers/mmc/core/mmc_ops.c
>>> @@ -413,13 +413,31 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>               unsigned int timeout_ms, bool use_busy_signal, bool send_status,
>>>               bool ignore_crc)
>>>  {
>>> +     struct mmc_host *host;
>>
>> It would be nicer if the addition of 'host' was a separate patch.  You
>> should remove the unnecessary BUG_ONs (it will oops anyway) at the same
>> time and then just do:
>>
>>         struct mmc_host *host = card->host;
> 
> Sure, make sense!
> 
>>
>>>       int err;
>>>       struct mmc_command cmd = {0};
>>>       unsigned long timeout;
>>> +     unsigned int max_busy_timeout;
>>>       u32 status = 0;
>>> +     bool use_r1b_resp = true;
>>
>> This is a little confusing.  Why not:
>>
>>         bool use_r1b_resp = use_busy_signal;
>>
>> Although 'use_busy_signal' actually means 'wait_while_busy'.
> 
> Right, that should simplify code a bit. I will update in a v2.
> 
>>
>>>
>>>       BUG_ON(!card);
>>>       BUG_ON(!card->host);
>>> +     host = card->host;
>>> +
>>> +     /* Once all callers provides a timeout, remove this fallback. */
>>> +     if (!timeout_ms)
>>> +             timeout_ms = MMC_OPS_TIMEOUT_MS;
>>
>> A timeout of zero does not mean a very long timeout.  It means an unknown timeout.
> 
> I guess this is a matter of definition.

JEDEC did not define GENERIC_CMD6_TIME until v4.5 so before that the timeout
is unknown.  It is reasonable for the host controller drivers to select a
value that suits them rather than constrain them to some arbitrarily large
timeout.

> 
> For those hosts that don't have a hw timeout, but maybe implements a
> software timeout, I thought this was more convenient. We likely then
> also need to define a "MAX_BUSY_TIMEOUT", which host drivers could
> use.
> 
> Additionally, since as of today only sdhci specifies the
> max_discard_to (renamed to max_busy_timeout), I thought it make sense
> to not force other hosts to specify the timeout to keep the existing
> behaviour.

Yes max_busy_timeout of zero again means unknown.

> 
>>
>>> +
>>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>>> +     max_busy_timeout = host->max_busy_timeout ?
>>> +                     host->max_busy_timeout : timeout_ms;
>>> +
>>> +     if (use_busy_signal && (host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>> +             (timeout_ms > max_busy_timeout))
>>> +                     use_r1b_resp = false;
>>> +     else if (!use_busy_signal)
>>> +             use_r1b_resp = false;
>>
>> Why not just check what you know:
>>
>>         if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
>>                 use_r1b_resp = false;
>>
> 
> I wanted to maintain the R1B response for hosts that don't support
> MMC_CAP_WAIT_WHILE_BUSY. With your proposal this will not be done.
> 
> Given this a second thought. I think it would make sense to adapt to
> your proposal. I will update in v2.
> 
>>>
>>>       cmd.opcode = MMC_SWITCH;
>>>       cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
>>> @@ -427,17 +445,25 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>                 (value << 8) |
>>>                 set;
>>>       cmd.flags = MMC_CMD_AC;
>>> -     if (use_busy_signal)
>>> +     if (use_r1b_resp)
>>>               cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
>>>       else
>>>               cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
>>>
>>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) {
>>> +             /* Tell the host what busy detection timeout to use. */
>>> +             cmd.busy_timeout = timeout_ms;
>>> +             /*
>>> +              * CRC errors shall only be ignored in cases were CMD13 is used
>>> +              * to poll to detect busy completion.
>>> +              */
>>> +             ignore_crc = false;
>>> +     }
>>>
>>> -     cmd.busy_timeout = timeout_ms;
>>
>> The busy_timeout should be provided for R1B i.e. this should be:
>>
>>         if (use_r1b_resp)
>>                 cmd.busy_timeout = timeout_ms;
>>
> 
> Will fix in v2, given you still think this is good approach according
> to my comment just above.
> 
>>>       if (index == EXT_CSD_SANITIZE_START)
>>>               cmd.sanitize_busy = true;
>>>
>>> -     err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
>>> +     err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>>>       if (err)
>>>               return err;
>>>
>>> @@ -445,24 +471,17 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>       if (!use_busy_signal)
>>>               return 0;
>>>
>>> -     /*
>>> -      * CRC errors shall only be ignored in cases were CMD13 is used to poll
>>> -      * to detect busy completion.
>>> -      */
>>> -     if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>>> -             ignore_crc = false;
>>> -
>>>       /* Must check status to be sure of no errors. */
>>> -     timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);
>>
>> This is the place to set the default timeout for the loop.
>>
>>         if (!timeout_ms)
>>                 timeout_ms = MMC_OPS_TIMEOUT_MS
>>
>>> +     timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>>       do {
>>>               if (send_status) {
>>>                       err = __mmc_send_status(card, &status, ignore_crc);
>>>                       if (err)
>>>                               return err;
>>>               }
>>> -             if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>>> +             if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>>>                       break;
>>> -             if (mmc_host_is_spi(card->host))
>>> +             if (mmc_host_is_spi(host))
>>>                       break;
>>>
>>>               /*
>>> @@ -478,18 +497,18 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>               /* Timeout if the device never leaves the program state. */
>>>               if (time_after(jiffies, timeout)) {
>>>                       pr_err("%s: Card stuck in programming state! %s\n",
>>> -                             mmc_hostname(card->host), __func__);
>>> +                             mmc_hostname(host), __func__);
>>>                       return -ETIMEDOUT;
>>>               }
>>>       } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>>>
>>> -     if (mmc_host_is_spi(card->host)) {
>>> +     if (mmc_host_is_spi(host)) {
>>>               if (status & R1_SPI_ILLEGAL_COMMAND)
>>>                       return -EBADMSG;
>>>       } else {
>>>               if (status & 0xFDFFA000)
>>> -                     pr_warning("%s: unexpected status %#x after "
>>> -                            "switch", mmc_hostname(card->host), status);
>>> +                     pr_warn("%s: unexpected status %#x after switch\n",
>>> +                             mmc_hostname(host), status);
>>>               if (status & R1_SWITCH_ERROR)
>>>                       return -EBADMSG;
>>>       }
>>>
>>
> 
> Adrian, thanks for reviewing!
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH 04/10] mmc: core: Fixup busy detection for mmc switch operations

[Ulf Hansson]

On 27 January 2014 11:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 23/01/14 16:11, Ulf Hansson wrote:
>> On 23 January 2014 11:10, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>> If the host controller supports busy detection in HW, we expect the
>>>> MMC_CAP_WAIT_WHILE_BUSY to be set. Likewise the corresponding
>>>> host->max_busy_timeout shall reflect the maximum busy detection timeout
>>>> supported by the host. A timeout set to zero, is interpreted as the
>>>> host supports whatever timeout the mmc core provides it with.
>>>>
>>>> Previously we expected a host that supported MMC_CAP_WAIT_WHILE_BUSY to
>>>> cope with any timeout, which just isn't feasible due to HW limitations.
>>>>
>>>> For most switch operations, R1B responses are expected and thus we need
>>>> to check for busy detection completion. To cope with cases where the
>>>> requested busy detection timeout is greater than what the host are able
>>>> to support, we fallback to use a R1 response instead. This will prevent
>>>> the host from doing HW busy detection.
>>>>
>>>> In those cases busy detection completion is handled by polling the for
>>>> the card's status using CMD13, which is the same mechanism used when
>>>> the host doesn't support MMC_CAP_WAIT_WHILE_BUSY.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/mmc/core/mmc_ops.c |   53 ++++++++++++++++++++++++++++++--------------
>>>>  1 file changed, 36 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>>> index 5e1a2cb..2e0cccb 100644
>>>> --- a/drivers/mmc/core/mmc_ops.c
>>>> +++ b/drivers/mmc/core/mmc_ops.c
>>>> @@ -413,13 +413,31 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>>               unsigned int timeout_ms, bool use_busy_signal, bool send_status,
>>>>               bool ignore_crc)
>>>>  {
>>>> +     struct mmc_host *host;
>>>
>>> It would be nicer if the addition of 'host' was a separate patch.  You
>>> should remove the unnecessary BUG_ONs (it will oops anyway) at the same
>>> time and then just do:
>>>
>>>         struct mmc_host *host = card->host;
>>
>> Sure, make sense!
>>
>>>
>>>>       int err;
>>>>       struct mmc_command cmd = {0};
>>>>       unsigned long timeout;
>>>> +     unsigned int max_busy_timeout;
>>>>       u32 status = 0;
>>>> +     bool use_r1b_resp = true;
>>>
>>> This is a little confusing.  Why not:
>>>
>>>         bool use_r1b_resp = use_busy_signal;
>>>
>>> Although 'use_busy_signal' actually means 'wait_while_busy'.
>>
>> Right, that should simplify code a bit. I will update in a v2.
>>
>>>
>>>>
>>>>       BUG_ON(!card);
>>>>       BUG_ON(!card->host);
>>>> +     host = card->host;
>>>> +
>>>> +     /* Once all callers provides a timeout, remove this fallback. */
>>>> +     if (!timeout_ms)
>>>> +             timeout_ms = MMC_OPS_TIMEOUT_MS;
>>>
>>> A timeout of zero does not mean a very long timeout.  It means an unknown timeout.
>>
>> I guess this is a matter of definition.
>
> JEDEC did not define GENERIC_CMD6_TIME until v4.5 so before that the timeout
> is unknown.  It is reasonable for the host controller drivers to select a
> value that suits them rather than constrain them to some arbitrarily large
> timeout.

You are right, did not think of this!

I suppose the MMC_OPS_TIMEOUT_MS, also should be decreased, it just
seems silly waiting for 10 minutes. :-) I guess somewhere around 10 -
20 s should be enough for those cases were we need to guess.

>
>>
>> For those hosts that don't have a hw timeout, but maybe implements a
>> software timeout, I thought this was more convenient. We likely then
>> also need to define a "MAX_BUSY_TIMEOUT", which host drivers could
>> use.
>>
>> Additionally, since as of today only sdhci specifies the
>> max_discard_to (renamed to max_busy_timeout), I thought it make sense
>> to not force other hosts to specify the timeout to keep the existing
>> behaviour.
>
> Yes max_busy_timeout of zero again means unknown.

Got, it. Thanks for your input!

>
>>
>>>
>>>> +
>>>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>>>> +     max_busy_timeout = host->max_busy_timeout ?
>>>> +                     host->max_busy_timeout : timeout_ms;
>>>> +
>>>> +     if (use_busy_signal && (host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>>> +             (timeout_ms > max_busy_timeout))
>>>> +                     use_r1b_resp = false;
>>>> +     else if (!use_busy_signal)
>>>> +             use_r1b_resp = false;
>>>
>>> Why not just check what you know:
>>>
>>>         if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
>>>                 use_r1b_resp = false;
>>>
>>
>> I wanted to maintain the R1B response for hosts that don't support
>> MMC_CAP_WAIT_WHILE_BUSY. With your proposal this will not be done.
>>
>> Given this a second thought. I think it would make sense to adapt to
>> your proposal. I will update in v2.
>>
>>>>
>>>>       cmd.opcode = MMC_SWITCH;
>>>>       cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
>>>> @@ -427,17 +445,25 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>>                 (value << 8) |
>>>>                 set;
>>>>       cmd.flags = MMC_CMD_AC;
>>>> -     if (use_busy_signal)
>>>> +     if (use_r1b_resp)
>>>>               cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
>>>>       else
>>>>               cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
>>>>
>>>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) {
>>>> +             /* Tell the host what busy detection timeout to use. */
>>>> +             cmd.busy_timeout = timeout_ms;
>>>> +             /*
>>>> +              * CRC errors shall only be ignored in cases were CMD13 is used
>>>> +              * to poll to detect busy completion.
>>>> +              */
>>>> +             ignore_crc = false;
>>>> +     }
>>>>
>>>> -     cmd.busy_timeout = timeout_ms;
>>>
>>> The busy_timeout should be provided for R1B i.e. this should be:
>>>
>>>         if (use_r1b_resp)
>>>                 cmd.busy_timeout = timeout_ms;
>>>
>>
>> Will fix in v2, given you still think this is good approach according
>> to my comment just above.
>>
>>>>       if (index == EXT_CSD_SANITIZE_START)
>>>>               cmd.sanitize_busy = true;
>>>>
>>>> -     err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
>>>> +     err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>>>>       if (err)
>>>>               return err;
>>>>
>>>> @@ -445,24 +471,17 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>>       if (!use_busy_signal)
>>>>               return 0;
>>>>
>>>> -     /*
>>>> -      * CRC errors shall only be ignored in cases were CMD13 is used to poll
>>>> -      * to detect busy completion.
>>>> -      */
>>>> -     if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>>>> -             ignore_crc = false;
>>>> -
>>>>       /* Must check status to be sure of no errors. */
>>>> -     timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);
>>>
>>> This is the place to set the default timeout for the loop.
>>>
>>>         if (!timeout_ms)
>>>                 timeout_ms = MMC_OPS_TIMEOUT_MS
>>>
>>>> +     timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>>>       do {
>>>>               if (send_status) {
>>>>                       err = __mmc_send_status(card, &status, ignore_crc);
>>>>                       if (err)
>>>>                               return err;
>>>>               }
>>>> -             if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>>>> +             if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>>>>                       break;
>>>> -             if (mmc_host_is_spi(card->host))
>>>> +             if (mmc_host_is_spi(host))
>>>>                       break;
>>>>
>>>>               /*
>>>> @@ -478,18 +497,18 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>>>               /* Timeout if the device never leaves the program state. */
>>>>               if (time_after(jiffies, timeout)) {
>>>>                       pr_err("%s: Card stuck in programming state! %s\n",
>>>> -                             mmc_hostname(card->host), __func__);
>>>> +                             mmc_hostname(host), __func__);
>>>>                       return -ETIMEDOUT;
>>>>               }
>>>>       } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>>>>
>>>> -     if (mmc_host_is_spi(card->host)) {
>>>> +     if (mmc_host_is_spi(host)) {
>>>>               if (status & R1_SPI_ILLEGAL_COMMAND)
>>>>                       return -EBADMSG;
>>>>       } else {
>>>>               if (status & 0xFDFFA000)
>>>> -                     pr_warning("%s: unexpected status %#x after "
>>>> -                            "switch", mmc_hostname(card->host), status);
>>>> +                     pr_warn("%s: unexpected status %#x after switch\n",
>>>> +                             mmc_hostname(host), status);
>>>>               if (status & R1_SWITCH_ERROR)
>>>>                       return -EBADMSG;
>>>>       }
>>>>
>>>
>>
>> Adrian, thanks for reviewing!
>>
>> Kind regards
>> Uffe
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] mmc: core: Use generic CMD6 time while switching to eMMC HS200 mode

[Ulf Hansson]

Conform to the eMMC spec and use the CMD6 generic timeout from the
EXT_CSD register, when switching to HS200 mode.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index ae3489a..897fdd1 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -857,7 +857,9 @@ static int mmc_select_hs200(struct mmc_card *card)
 	/* switch to HS200 mode if bus width set successfully */
 	if (!err)
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-				EXT_CSD_HS_TIMING, 2, 0, true, true, true);
+				EXT_CSD_HS_TIMING, 2,
+				card->ext_csd.generic_cmd6_time,
+				true, true, true);
 err:
 	return err;
 }
-- 
1.7.9.5

[PATCH 06/10] mmc: core: Respect host's max_busy_timeout when sending sleep cmd

[Ulf Hansson]

When sending the sleep command for host drivers supporting
MMC_CAP_WAIT_WHILE_BUSY, we need to confirm that max_busy_timeout is
big enough comparing to the sleep timeout specified from card's
EXT_CSD. If this isn't case, we use a R1 response instead of R1B and
fallback to use a delay instead.

Do note that a max_busy_timeout set to zero by the host, is interpreted
as it can cope with whatever timeout the mmc core provides it with.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 897fdd1..32e1546 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1359,6 +1359,8 @@ static int mmc_sleep(struct mmc_host *host)
 {
 	struct mmc_command cmd = {0};
 	struct mmc_card *card = host->card;
+	unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
+	unsigned int max_busy_timeout;
 	int err;
 
 	if (host->caps2 & MMC_CAP2_NO_SLEEP_CMD)
@@ -1372,7 +1374,18 @@ static int mmc_sleep(struct mmc_host *host)
 	cmd.arg = card->rca << 16;
 	cmd.arg |= 1 << 15;
 
-	cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
+	/* We interpret unspecified timeouts as the host can cope with all. */
+	max_busy_timeout = host->max_busy_timeout ?
+			host->max_busy_timeout : timeout_ms;
+
+	if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
+		(timeout_ms <= max_busy_timeout)) {
+		cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
+		cmd.busy_timeout = timeout_ms;
+	} else {
+		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	}
+
 	err = mmc_wait_for_cmd(host, &cmd, 0);
 	if (err)
 		return err;
@@ -1383,8 +1396,8 @@ static int mmc_sleep(struct mmc_host *host)
 	 * SEND_STATUS command to poll the status because that command (and most
 	 * others) is invalid while the card sleeps.
 	 */
-	if (!(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
-		mmc_delay(DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000));
+	if (!cmd.busy_timeout)
+		mmc_delay(timeout_ms);
 
 	return err;
 }
-- 
1.7.9.5

Re: [PATCH 06/10] mmc: core: Respect host's max_busy_timeout when sending sleep cmd

[Adrian Hunter]

On 22/01/14 17:00, Ulf Hansson wrote:
> When sending the sleep command for host drivers supporting
> MMC_CAP_WAIT_WHILE_BUSY, we need to confirm that max_busy_timeout is
> big enough comparing to the sleep timeout specified from card's
> EXT_CSD. If this isn't case, we use a R1 response instead of R1B and
> fallback to use a delay instead.
> 
> Do note that a max_busy_timeout set to zero by the host, is interpreted
> as it can cope with whatever timeout the mmc core provides it with.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c |   19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 897fdd1..32e1546 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1359,6 +1359,8 @@ static int mmc_sleep(struct mmc_host *host)
>  {
>  	struct mmc_command cmd = {0};
>  	struct mmc_card *card = host->card;
> +	unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
> +	unsigned int max_busy_timeout;
>  	int err;
>  
>  	if (host->caps2 & MMC_CAP2_NO_SLEEP_CMD)
> @@ -1372,7 +1374,18 @@ static int mmc_sleep(struct mmc_host *host)
>  	cmd.arg = card->rca << 16;
>  	cmd.arg |= 1 << 15;
>  
> -	cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
> +	/* We interpret unspecified timeouts as the host can cope with all. */
> +	max_busy_timeout = host->max_busy_timeout ?
> +			host->max_busy_timeout : timeout_ms;
> +
> +	if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
> +		(timeout_ms <= max_busy_timeout)) {
> +		cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
> +		cmd.busy_timeout = timeout_ms;
> +	} else {
> +		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +	}

I do not see why this is related to MMC_CAP_WAIT_WHILE_BUSY.
Why not just:

	if (host->max_busy_timeout && timeout_ms > host->max_busy_timeout) {
		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
	} else {
		cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
		cmd.busy_timeout = timeout_ms;
	}

> +
>  	err = mmc_wait_for_cmd(host, &cmd, 0);
>  	if (err)
>  		return err;
> @@ -1383,8 +1396,8 @@ static int mmc_sleep(struct mmc_host *host)
>  	 * SEND_STATUS command to poll the status because that command (and most
>  	 * others) is invalid while the card sleeps.
>  	 */
> -	if (!(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
> -		mmc_delay(DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000));
> +	if (!cmd.busy_timeout)
> +		mmc_delay(timeout_ms);

And this becomes:

	if (!cmd.busy_timeout || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
		mmc_delay(timeout_ms);

>  
>  	return err;
>  }
> 

Re: [PATCH 06/10] mmc: core: Respect host's max_busy_timeout when sending sleep cmd

[Ulf Hansson]

On 23 January 2014 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 22/01/14 17:00, Ulf Hansson wrote:
>> When sending the sleep command for host drivers supporting
>> MMC_CAP_WAIT_WHILE_BUSY, we need to confirm that max_busy_timeout is
>> big enough comparing to the sleep timeout specified from card's
>> EXT_CSD. If this isn't case, we use a R1 response instead of R1B and
>> fallback to use a delay instead.
>>
>> Do note that a max_busy_timeout set to zero by the host, is interpreted
>> as it can cope with whatever timeout the mmc core provides it with.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc.c |   19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 897fdd1..32e1546 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1359,6 +1359,8 @@ static int mmc_sleep(struct mmc_host *host)
>>  {
>>       struct mmc_command cmd = {0};
>>       struct mmc_card *card = host->card;
>> +     unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
>> +     unsigned int max_busy_timeout;
>>       int err;
>>
>>       if (host->caps2 & MMC_CAP2_NO_SLEEP_CMD)
>> @@ -1372,7 +1374,18 @@ static int mmc_sleep(struct mmc_host *host)
>>       cmd.arg = card->rca << 16;
>>       cmd.arg |= 1 << 15;
>>
>> -     cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>> +     max_busy_timeout = host->max_busy_timeout ?
>> +                     host->max_busy_timeout : timeout_ms;
>> +
>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>> +             (timeout_ms <= max_busy_timeout)) {
>> +             cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>> +             cmd.busy_timeout = timeout_ms;
>> +     } else {
>> +             cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +     }
>
> I do not see why this is related to MMC_CAP_WAIT_WHILE_BUSY.
> Why not just:

Before I do any update, we need to decide what host->max_busy_timeout
of zero means. Please see the response in the other patch in this
patchset.

I see that my patch for the mmc_switch function, maintain the R1B for
host not supporting MMC_CAP_WAIT_WHILE_BUSY, but this one for sleep
doesn't. :-) We should align the behaviour.


>
>         if (host->max_busy_timeout && timeout_ms > host->max_busy_timeout) {
>                 cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>         } else {
>                 cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>                 cmd.busy_timeout = timeout_ms;
>         }

So here your suggestion will mean you would like to keep R1B for hosts
not supporting MMC_CAP_WAIT_WHILE_BUSY. This opposite of what you
proposed for the mmc_switch. :-)

I suggest that we only use R1B when the host are able to handle busy
detection in hw. If you think that is bad idea, please let me know.

>
>> +
>>       err = mmc_wait_for_cmd(host, &cmd, 0);
>>       if (err)
>>               return err;
>> @@ -1383,8 +1396,8 @@ static int mmc_sleep(struct mmc_host *host)
>>        * SEND_STATUS command to poll the status because that command (and most
>>        * others) is invalid while the card sleeps.
>>        */
>> -     if (!(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>> -             mmc_delay(DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000));
>> +     if (!cmd.busy_timeout)
>> +             mmc_delay(timeout_ms);
>
> And this becomes:
>
>         if (!cmd.busy_timeout || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>                 mmc_delay(timeout_ms);
>
>>
>>       return err;
>>  }
>>
>

Kind regards
Uffe

Re: [PATCH 06/10] mmc: core: Respect host's max_busy_timeout when sending sleep cmd

[Adrian Hunter]

On 23/01/14 16:26, Ulf Hansson wrote:
> On 23 January 2014 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 22/01/14 17:00, Ulf Hansson wrote:
>>> When sending the sleep command for host drivers supporting
>>> MMC_CAP_WAIT_WHILE_BUSY, we need to confirm that max_busy_timeout is
>>> big enough comparing to the sleep timeout specified from card's
>>> EXT_CSD. If this isn't case, we use a R1 response instead of R1B and
>>> fallback to use a delay instead.
>>>
>>> Do note that a max_busy_timeout set to zero by the host, is interpreted
>>> as it can cope with whatever timeout the mmc core provides it with.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/core/mmc.c |   19 ++++++++++++++++---
>>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 897fdd1..32e1546 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1359,6 +1359,8 @@ static int mmc_sleep(struct mmc_host *host)
>>>  {
>>>       struct mmc_command cmd = {0};
>>>       struct mmc_card *card = host->card;
>>> +     unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
>>> +     unsigned int max_busy_timeout;
>>>       int err;
>>>
>>>       if (host->caps2 & MMC_CAP2_NO_SLEEP_CMD)
>>> @@ -1372,7 +1374,18 @@ static int mmc_sleep(struct mmc_host *host)
>>>       cmd.arg = card->rca << 16;
>>>       cmd.arg |= 1 << 15;
>>>
>>> -     cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>>> +     max_busy_timeout = host->max_busy_timeout ?
>>> +                     host->max_busy_timeout : timeout_ms;
>>> +
>>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>> +             (timeout_ms <= max_busy_timeout)) {
>>> +             cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>> +             cmd.busy_timeout = timeout_ms;
>>> +     } else {
>>> +             cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>> +     }
>>
>> I do not see why this is related to MMC_CAP_WAIT_WHILE_BUSY.
>> Why not just:
> 
> Before I do any update, we need to decide what host->max_busy_timeout
> of zero means. Please see the response in the other patch in this
> patchset.

Unless you want to change all the host controller drivers, zero means
don't know.

> 
> I see that my patch for the mmc_switch function, maintain the R1B for
> host not supporting MMC_CAP_WAIT_WHILE_BUSY, but this one for sleep
> doesn't. :-) We should align the behaviour.
> 
> 
>>
>>         if (host->max_busy_timeout && timeout_ms > host->max_busy_timeout) {
>>                 cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>         } else {
>>                 cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>                 cmd.busy_timeout = timeout_ms;
>>         }
> 
> So here your suggestion will mean you would like to keep R1B for hosts
> not supporting MMC_CAP_WAIT_WHILE_BUSY. This opposite of what you
> proposed for the mmc_switch. :-)

I suggested:

	if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
                use_r1b_resp = false;

(without modifying timeout_ms) which wasn't related to MMC_CAP_WAIT_WHILE_BUSY
i.e. keeps R1B for hosts not supporting MMC_CAP_WAIT_WHILE_BUSY

> 
> I suggest that we only use R1B when the host are able to handle busy
> detection in hw. If you think that is bad idea, please let me know.
> 
>>
>>> +
>>>       err = mmc_wait_for_cmd(host, &cmd, 0);
>>>       if (err)
>>>               return err;
>>> @@ -1383,8 +1396,8 @@ static int mmc_sleep(struct mmc_host *host)
>>>        * SEND_STATUS command to poll the status because that command (and most
>>>        * others) is invalid while the card sleeps.
>>>        */
>>> -     if (!(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>>> -             mmc_delay(DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000));
>>> +     if (!cmd.busy_timeout)
>>> +             mmc_delay(timeout_ms);
>>
>> And this becomes:
>>
>>         if (!cmd.busy_timeout || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>>                 mmc_delay(timeout_ms);
>>
>>>
>>>       return err;
>>>  }
>>>
>>
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH 06/10] mmc: core: Respect host's max_busy_timeout when sending sleep cmd

[Ulf Hansson]

On 27 January 2014 11:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 23/01/14 16:26, Ulf Hansson wrote:
>> On 23 January 2014 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>> When sending the sleep command for host drivers supporting
>>>> MMC_CAP_WAIT_WHILE_BUSY, we need to confirm that max_busy_timeout is
>>>> big enough comparing to the sleep timeout specified from card's
>>>> EXT_CSD. If this isn't case, we use a R1 response instead of R1B and
>>>> fallback to use a delay instead.
>>>>
>>>> Do note that a max_busy_timeout set to zero by the host, is interpreted
>>>> as it can cope with whatever timeout the mmc core provides it with.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/mmc/core/mmc.c |   19 ++++++++++++++++---
>>>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 897fdd1..32e1546 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1359,6 +1359,8 @@ static int mmc_sleep(struct mmc_host *host)
>>>>  {
>>>>       struct mmc_command cmd = {0};
>>>>       struct mmc_card *card = host->card;
>>>> +     unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
>>>> +     unsigned int max_busy_timeout;
>>>>       int err;
>>>>
>>>>       if (host->caps2 & MMC_CAP2_NO_SLEEP_CMD)
>>>> @@ -1372,7 +1374,18 @@ static int mmc_sleep(struct mmc_host *host)
>>>>       cmd.arg = card->rca << 16;
>>>>       cmd.arg |= 1 << 15;
>>>>
>>>> -     cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>>>> +     max_busy_timeout = host->max_busy_timeout ?
>>>> +                     host->max_busy_timeout : timeout_ms;
>>>> +
>>>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>>> +             (timeout_ms <= max_busy_timeout)) {
>>>> +             cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>>> +             cmd.busy_timeout = timeout_ms;
>>>> +     } else {
>>>> +             cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>>> +     }
>>>
>>> I do not see why this is related to MMC_CAP_WAIT_WHILE_BUSY.
>>> Why not just:
>>
>> Before I do any update, we need to decide what host->max_busy_timeout
>> of zero means. Please see the response in the other patch in this
>> patchset.
>
> Unless you want to change all the host controller drivers, zero means
> don't know.
>

Agree!

>>
>> I see that my patch for the mmc_switch function, maintain the R1B for
>> host not supporting MMC_CAP_WAIT_WHILE_BUSY, but this one for sleep
>> doesn't. :-) We should align the behaviour.
>>
>>
>>>
>>>         if (host->max_busy_timeout && timeout_ms > host->max_busy_timeout) {
>>>                 cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>>         } else {
>>>                 cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>>                 cmd.busy_timeout = timeout_ms;
>>>         }
>>
>> So here your suggestion will mean you would like to keep R1B for hosts
>> not supporting MMC_CAP_WAIT_WHILE_BUSY. This opposite of what you
>> proposed for the mmc_switch. :-)
>
> I suggested:
>
>         if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
>                 use_r1b_resp = false;
>
> (without modifying timeout_ms) which wasn't related to MMC_CAP_WAIT_WHILE_BUSY
> i.e. keeps R1B for hosts not supporting MMC_CAP_WAIT_WHILE_BUSY

Will fix in v2, thanks!


>
>>
>> I suggest that we only use R1B when the host are able to handle busy
>> detection in hw. If you think that is bad idea, please let me know.
>>
>>>
>>>> +
>>>>       err = mmc_wait_for_cmd(host, &cmd, 0);
>>>>       if (err)
>>>>               return err;
>>>> @@ -1383,8 +1396,8 @@ static int mmc_sleep(struct mmc_host *host)
>>>>        * SEND_STATUS command to poll the status because that command (and most
>>>>        * others) is invalid while the card sleeps.
>>>>        */
>>>> -     if (!(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>>>> -             mmc_delay(DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000));
>>>> +     if (!cmd.busy_timeout)
>>>> +             mmc_delay(timeout_ms);
>>>
>>> And this becomes:
>>>
>>>         if (!cmd.busy_timeout || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>>>                 mmc_delay(timeout_ms);
>>>
>>>>
>>>>       return err;
>>>>  }
>>>>
>>>
>>
>> Kind regards
>> Uffe
>>
>>
>

[PATCH 07/10] mmc: card: Use R1 responses for stop cmds for read requests

[Ulf Hansson]

While using open ended transmission and thus ending the transfer by
sending a stop command, we shall use R1B only for writes and R1 shall
be used for reads. Previously R1B were used in both cases.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/card/block.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 7b5424f..87cd2b0 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1335,7 +1335,6 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	brq->data.blksz = 512;
 	brq->stop.opcode = MMC_STOP_TRANSMISSION;
 	brq->stop.arg = 0;
-	brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 	brq->data.blocks = blk_rq_sectors(req);
 
 	/*
@@ -1378,9 +1377,15 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	if (rq_data_dir(req) == READ) {
 		brq->cmd.opcode = readcmd;
 		brq->data.flags |= MMC_DATA_READ;
+		if (brq->mrq.stop)
+			brq->stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
+					MMC_CMD_AC;
 	} else {
 		brq->cmd.opcode = writecmd;
 		brq->data.flags |= MMC_DATA_WRITE;
+		if (brq->mrq.stop)
+			brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
+					MMC_CMD_AC;
 	}
 
 	if (do_rel_wr)
-- 
1.7.9.5

[PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Ulf Hansson]

Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
detection completion in the recovery path, which were the case when
using R1B response.

Start using R1 as response instead to align behavior, no matter if
MMC_CAP_WAIT_WHILE_BUSY is supported or not.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/card/block.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 87cd2b0..74169fa 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
 	int err;
 
 	cmd.opcode = MMC_STOP_TRANSMISSION;
-	cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
 	err = mmc_wait_for_cmd(card->host, &cmd, 5);
 	if (err == 0)
 		*status = cmd.resp[0];
-- 
1.7.9.5

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Adrian Hunter]

On 22/01/14 17:00, Ulf Hansson wrote:
> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
> detection completion in the recovery path, which were the case when
> using R1B response.
> 
> Start using R1 as response instead to align behavior, no matter if
> MMC_CAP_WAIT_WHILE_BUSY is supported or not.

This does not make sense to me.  If you are sending a STOP command you
should use the correct response type.  R1B should be OK here because the
card should release the busy signal in any case except failure.

> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/card/block.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 87cd2b0..74169fa 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>  	int err;
>  
>  	cmd.opcode = MMC_STOP_TRANSMISSION;
> -	cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>  	err = mmc_wait_for_cmd(card->host, &cmd, 5);
>  	if (err == 0)
>  		*status = cmd.resp[0];
> 

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Ulf Hansson]

On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 22/01/14 17:00, Ulf Hansson wrote:
>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>> detection completion in the recovery path, which were the case when
>> using R1B response.
>>
>> Start using R1 as response instead to align behavior, no matter if
>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>
> This does not make sense to me.  If you are sending a STOP command you
> should use the correct response type.  R1B should be OK here because the
> card should release the busy signal in any case except failure.

For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
assumed to be treated same as an R1, which means there are no busy
detection handled in the host.

mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
Additionally it does not care about to handle busy detection with
CDM13 polling.

Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
means there no busy detection done, I wanted to align to this
behaviour - no matter if the host can do HW busy detection or not.

I am not saying this is how it must be done, just trying to provide
you with some more reasons to why I wanted to change.

If we instead decide keep the R1B for send_stop(), we should implement
CMD 13 polling to meet the same behaviour for hosts not supporting
MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
busy timeout, do you have any suggestion of what would be a reasonable
value for it?

Kind regards
Ulf Hansson

>
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/card/block.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 87cd2b0..74169fa 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>       int err;
>>
>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>       if (err == 0)
>>               *status = cmd.resp[0];
>>
>

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Adrian Hunter]

On 23/01/14 15:21, Ulf Hansson wrote:
> On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 22/01/14 17:00, Ulf Hansson wrote:
>>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>>> detection completion in the recovery path, which were the case when
>>> using R1B response.
>>>
>>> Start using R1 as response instead to align behavior, no matter if
>>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>>
>> This does not make sense to me.  If you are sending a STOP command you
>> should use the correct response type.  R1B should be OK here because the
>> card should release the busy signal in any case except failure.
> 
> For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
> assumed to be treated same as an R1, which means there are no busy
> detection handled in the host.

That is not entirely true.  For hosts that do not set
MMC_CAP_WAIT_WHILE_BUSY we don't know if they wait or not.  I imagine most
do because it is more efficient, but the kernel has always been programmed
to poll the status anyway so you can't tell from the code.

MMC_CAP_WAIT_WHILE_BUSY was one of my inventions I am afraid.  If I recall
correctly it was mainly due to the SLEEP command because you can't poll in
that case and you don't want to delay the system from sleeping - if you are
certain that the controller has waited for busy to de-assert (i.e.
MMC_CAP_WAIT_WHILE_BUSY) then you can exit immediately.

> 
> mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
> Additionally it does not care about to handle busy detection with
> CDM13 polling.
> 
> Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
> means there no busy detection done, I wanted to align to this
> behaviour - no matter if the host can do HW busy detection or not.
> 
> I am not saying this is how it must be done, just trying to provide
> you with some more reasons to why I wanted to change.
> 
> If we instead decide keep the R1B for send_stop(), we should implement
> CMD 13 polling to meet the same behaviour for hosts not supporting
> MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
> busy timeout, do you have any suggestion of what would be a reasonable
> value for it?

It is hard to tell if waiting is ever going to help more than hinder, so I
would not change this.

> 
> Kind regards
> Ulf Hansson
> 
>>
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/card/block.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index 87cd2b0..74169fa 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>>       int err;
>>>
>>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>>       if (err == 0)
>>>               *status = cmd.resp[0];
>>>
>>
> 
> 

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Ulf Hansson]

On 23 January 2014 15:29, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 23/01/14 15:21, Ulf Hansson wrote:
>> On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>>>> detection completion in the recovery path, which were the case when
>>>> using R1B response.
>>>>
>>>> Start using R1 as response instead to align behavior, no matter if
>>>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>>>
>>> This does not make sense to me.  If you are sending a STOP command you
>>> should use the correct response type.  R1B should be OK here because the
>>> card should release the busy signal in any case except failure.
>>
>> For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
>> assumed to be treated same as an R1, which means there are no busy
>> detection handled in the host.
>
> That is not entirely true.  For hosts that do not set
> MMC_CAP_WAIT_WHILE_BUSY we don't know if they wait or not.  I imagine most
> do because it is more efficient, but the kernel has always been programmed
> to poll the status anyway so you can't tell from the code.

You are right, we can't know - unless we dive in into each host driver
and check.

Surely there could be more than omap_hsmmc and sdhci that support
this. Still I think we need to conclude on how to go forward with
MMC_CAP_WAIT_WHILE_BUSY, since at the moment it seems a bit of a mess.
Obviously we need to be careful to not break anything.

>
> MMC_CAP_WAIT_WHILE_BUSY was one of my inventions I am afraid.  If I recall
> correctly it was mainly due to the SLEEP command because you can't poll in
> that case and you don't want to delay the system from sleeping - if you are
> certain that the controller has waited for busy to de-assert (i.e.
> MMC_CAP_WAIT_WHILE_BUSY) then you can exit immediately.

I think MMC_CAP_WAIT_WHILE_BUSY was a needed feature, now we only have
to make it more mature. :-)

>
>>
>> mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
>> Additionally it does not care about to handle busy detection with
>> CDM13 polling.
>>
>> Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
>> means there no busy detection done, I wanted to align to this
>> behaviour - no matter if the host can do HW busy detection or not.
>>
>> I am not saying this is how it must be done, just trying to provide
>> you with some more reasons to why I wanted to change.
>>
>> If we instead decide keep the R1B for send_stop(), we should implement
>> CMD 13 polling to meet the same behaviour for hosts not supporting
>> MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
>> busy timeout, do you have any suggestion of what would be a reasonable
>> value for it?
>
> It is hard to tell if waiting is ever going to help more than hinder, so I
> would not change this.

Fair enough, but certainly we should implement a CMD13 polling
mechanism - to align behaviour.

Are you then also indirectly suggesting that not specficing
"cmd.busy_timeout" should be interpreted by the host as "use whatever
timeout you want"?

Do note, there are another scenario, which also don't specify a busy
timeout, which is when we have used an open ended WRITE transmission
and using CMD12 to finalize it.
But, in this scenario we do polling with CMD13, also without a
timeout. So at least the behaviour are aligned here, but still no
timeout specified.

>
>>
>> Kind regards
>> Ulf Hansson
>>
>>>
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/mmc/card/block.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index 87cd2b0..74169fa 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>>>       int err;
>>>>
>>>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>>>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>>>       if (err == 0)
>>>>               *status = cmd.resp[0];
>>>>
>>>
>>
>>
>

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Adrian Hunter]

On 23/01/14 16:59, Ulf Hansson wrote:
> On 23 January 2014 15:29, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 23/01/14 15:21, Ulf Hansson wrote:
>>> On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>>>>> detection completion in the recovery path, which were the case when
>>>>> using R1B response.
>>>>>
>>>>> Start using R1 as response instead to align behavior, no matter if
>>>>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>>>>
>>>> This does not make sense to me.  If you are sending a STOP command you
>>>> should use the correct response type.  R1B should be OK here because the
>>>> card should release the busy signal in any case except failure.
>>>
>>> For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
>>> assumed to be treated same as an R1, which means there are no busy
>>> detection handled in the host.
>>
>> That is not entirely true.  For hosts that do not set
>> MMC_CAP_WAIT_WHILE_BUSY we don't know if they wait or not.  I imagine most
>> do because it is more efficient, but the kernel has always been programmed
>> to poll the status anyway so you can't tell from the code.
> 
> You are right, we can't know - unless we dive in into each host driver
> and check.
> 
> Surely there could be more than omap_hsmmc and sdhci that support
> this. Still I think we need to conclude on how to go forward with
> MMC_CAP_WAIT_WHILE_BUSY, since at the moment it seems a bit of a mess.
> Obviously we need to be careful to not break anything.
> 
>>
>> MMC_CAP_WAIT_WHILE_BUSY was one of my inventions I am afraid.  If I recall
>> correctly it was mainly due to the SLEEP command because you can't poll in
>> that case and you don't want to delay the system from sleeping - if you are
>> certain that the controller has waited for busy to de-assert (i.e.
>> MMC_CAP_WAIT_WHILE_BUSY) then you can exit immediately.
> 
> I think MMC_CAP_WAIT_WHILE_BUSY was a needed feature, now we only have
> to make it more mature. :-)
> 
>>
>>>
>>> mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
>>> Additionally it does not care about to handle busy detection with
>>> CDM13 polling.
>>>
>>> Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
>>> means there no busy detection done, I wanted to align to this
>>> behaviour - no matter if the host can do HW busy detection or not.
>>>
>>> I am not saying this is how it must be done, just trying to provide
>>> you with some more reasons to why I wanted to change.
>>>
>>> If we instead decide keep the R1B for send_stop(), we should implement
>>> CMD 13 polling to meet the same behaviour for hosts not supporting
>>> MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
>>> busy timeout, do you have any suggestion of what would be a reasonable
>>> value for it?
>>
>> It is hard to tell if waiting is ever going to help more than hinder, so I
>> would not change this.
> 
> Fair enough, but certainly we should implement a CMD13 polling
> mechanism - to align behaviour.

Recovery probably isn't possible.  The block driver heroically has a go
at it.  For some people it much more important to fail fast than to
recover.  Consequently, unless you has a specific use-case, I wouldn't
add anything that would slow down that path.

> 
> Are you then also indirectly suggesting that not specficing
> "cmd.busy_timeout" should be interpreted by the host as "use whatever
> timeout you want"?

That is how it is now.  The problem with trying to so something better is
that sometimes the timeout really is undefined.

> 
> Do note, there are another scenario, which also don't specify a busy
> timeout, which is when we have used an open ended WRITE transmission
> and using CMD12 to finalize it.
> But, in this scenario we do polling with CMD13, also without a
> timeout. So at least the behaviour are aligned here, but still no
> timeout specified.

I don't think that is right.  The data timeout applies in that case too.

> 
>>
>>>
>>> Kind regards
>>> Ulf Hansson
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> ---
>>>>>  drivers/mmc/card/block.c |    2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>> index 87cd2b0..74169fa 100644
>>>>> --- a/drivers/mmc/card/block.c
>>>>> +++ b/drivers/mmc/card/block.c
>>>>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>>>>       int err;
>>>>>
>>>>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>>>>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>>>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>>>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>>>>       if (err == 0)
>>>>>               *status = cmd.resp[0];
>>>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Ulf Hansson]

On 27 January 2014 11:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 23/01/14 16:59, Ulf Hansson wrote:
>> On 23 January 2014 15:29, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 23/01/14 15:21, Ulf Hansson wrote:
>>>> On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>>>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>>>>>> detection completion in the recovery path, which were the case when
>>>>>> using R1B response.
>>>>>>
>>>>>> Start using R1 as response instead to align behavior, no matter if
>>>>>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>>>>>
>>>>> This does not make sense to me.  If you are sending a STOP command you
>>>>> should use the correct response type.  R1B should be OK here because the
>>>>> card should release the busy signal in any case except failure.
>>>>
>>>> For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
>>>> assumed to be treated same as an R1, which means there are no busy
>>>> detection handled in the host.
>>>
>>> That is not entirely true.  For hosts that do not set
>>> MMC_CAP_WAIT_WHILE_BUSY we don't know if they wait or not.  I imagine most
>>> do because it is more efficient, but the kernel has always been programmed
>>> to poll the status anyway so you can't tell from the code.
>>
>> You are right, we can't know - unless we dive in into each host driver
>> and check.
>>
>> Surely there could be more than omap_hsmmc and sdhci that support
>> this. Still I think we need to conclude on how to go forward with
>> MMC_CAP_WAIT_WHILE_BUSY, since at the moment it seems a bit of a mess.
>> Obviously we need to be careful to not break anything.
>>
>>>
>>> MMC_CAP_WAIT_WHILE_BUSY was one of my inventions I am afraid.  If I recall
>>> correctly it was mainly due to the SLEEP command because you can't poll in
>>> that case and you don't want to delay the system from sleeping - if you are
>>> certain that the controller has waited for busy to de-assert (i.e.
>>> MMC_CAP_WAIT_WHILE_BUSY) then you can exit immediately.
>>
>> I think MMC_CAP_WAIT_WHILE_BUSY was a needed feature, now we only have
>> to make it more mature. :-)
>>
>>>
>>>>
>>>> mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
>>>> Additionally it does not care about to handle busy detection with
>>>> CDM13 polling.
>>>>
>>>> Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
>>>> means there no busy detection done, I wanted to align to this
>>>> behaviour - no matter if the host can do HW busy detection or not.
>>>>
>>>> I am not saying this is how it must be done, just trying to provide
>>>> you with some more reasons to why I wanted to change.
>>>>
>>>> If we instead decide keep the R1B for send_stop(), we should implement
>>>> CMD 13 polling to meet the same behaviour for hosts not supporting
>>>> MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
>>>> busy timeout, do you have any suggestion of what would be a reasonable
>>>> value for it?
>>>
>>> It is hard to tell if waiting is ever going to help more than hinder, so I
>>> would not change this.
>>
>> Fair enough, but certainly we should implement a CMD13 polling
>> mechanism - to align behaviour.
>
> Recovery probably isn't possible.  The block driver heroically has a go
> at it.  For some people it much more important to fail fast than to
> recover.  Consequently, unless you has a specific use-case, I wouldn't
> add anything that would slow down that path.

I agree that it's hard to tell what's the best approach. :-) Though I
still think we can do a bit better than what we do today, and I really
don't like that we don't have an aligned behaviour - between hosts
supporting and not supporting MMC_CAP_WAIT_WHILE_BUSY for this
particular case.

I understand that you want to prevent us from breaking something. But
currently, if we consider all hosts, either it all works like a charm,
or some are broken, because the behaviour is different.

In this case, I don't see the benefit of using hw busy detection at
all, since the performance to gain is not relevant. We could then
convert to R1 instead of R1B and for all cases do polling with CMD13
for a small period of let's say 200 ms.

Would that be acceptable for you?

>
>>
>> Are you then also indirectly suggesting that not specficing
>> "cmd.busy_timeout" should be interpreted by the host as "use whatever
>> timeout you want"?
>
> That is how it is now.  The problem with trying to so something better is
> that sometimes the timeout really is undefined.

That makes sense!

>
>>
>> Do note, there are another scenario, which also don't specify a busy
>> timeout, which is when we have used an open ended WRITE transmission
>> and using CMD12 to finalize it.
>> But, in this scenario we do polling with CMD13, also without a
>> timeout. So at least the behaviour are aligned here, but still no
>> timeout specified.
>
> I don't think that is right.  The data timeout applies in that case too.

No it shouldn't. The data timeout applies only to the ongoing data
transfer. In other words, if the card stops sending/receiving data in
the middle of the transfer.

>
>>
>>>
>>>>
>>>> Kind regards
>>>> Ulf Hansson
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>> ---
>>>>>>  drivers/mmc/card/block.c |    2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>>> index 87cd2b0..74169fa 100644
>>>>>> --- a/drivers/mmc/card/block.c
>>>>>> +++ b/drivers/mmc/card/block.c
>>>>>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>>>>>       int err;
>>>>>>
>>>>>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>>>>>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>>>>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>>>>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>>>>>       if (err == 0)
>>>>>>               *status = cmd.resp[0];
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Adrian Hunter]

On 28/01/14 14:39, Ulf Hansson wrote:
> On 27 January 2014 11:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 23/01/14 16:59, Ulf Hansson wrote:
>>> On 23 January 2014 15:29, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 23/01/14 15:21, Ulf Hansson wrote:
>>>>> On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>>>>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>>>>>>> detection completion in the recovery path, which were the case when
>>>>>>> using R1B response.
>>>>>>>
>>>>>>> Start using R1 as response instead to align behavior, no matter if
>>>>>>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>>>>>>
>>>>>> This does not make sense to me.  If you are sending a STOP command you
>>>>>> should use the correct response type.  R1B should be OK here because the
>>>>>> card should release the busy signal in any case except failure.
>>>>>
>>>>> For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
>>>>> assumed to be treated same as an R1, which means there are no busy
>>>>> detection handled in the host.
>>>>
>>>> That is not entirely true.  For hosts that do not set
>>>> MMC_CAP_WAIT_WHILE_BUSY we don't know if they wait or not.  I imagine most
>>>> do because it is more efficient, but the kernel has always been programmed
>>>> to poll the status anyway so you can't tell from the code.
>>>
>>> You are right, we can't know - unless we dive in into each host driver
>>> and check.
>>>
>>> Surely there could be more than omap_hsmmc and sdhci that support
>>> this. Still I think we need to conclude on how to go forward with
>>> MMC_CAP_WAIT_WHILE_BUSY, since at the moment it seems a bit of a mess.
>>> Obviously we need to be careful to not break anything.
>>>
>>>>
>>>> MMC_CAP_WAIT_WHILE_BUSY was one of my inventions I am afraid.  If I recall
>>>> correctly it was mainly due to the SLEEP command because you can't poll in
>>>> that case and you don't want to delay the system from sleeping - if you are
>>>> certain that the controller has waited for busy to de-assert (i.e.
>>>> MMC_CAP_WAIT_WHILE_BUSY) then you can exit immediately.
>>>
>>> I think MMC_CAP_WAIT_WHILE_BUSY was a needed feature, now we only have
>>> to make it more mature. :-)
>>>
>>>>
>>>>>
>>>>> mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
>>>>> Additionally it does not care about to handle busy detection with
>>>>> CDM13 polling.
>>>>>
>>>>> Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
>>>>> means there no busy detection done, I wanted to align to this
>>>>> behaviour - no matter if the host can do HW busy detection or not.
>>>>>
>>>>> I am not saying this is how it must be done, just trying to provide
>>>>> you with some more reasons to why I wanted to change.
>>>>>
>>>>> If we instead decide keep the R1B for send_stop(), we should implement
>>>>> CMD 13 polling to meet the same behaviour for hosts not supporting
>>>>> MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
>>>>> busy timeout, do you have any suggestion of what would be a reasonable
>>>>> value for it?
>>>>
>>>> It is hard to tell if waiting is ever going to help more than hinder, so I
>>>> would not change this.
>>>
>>> Fair enough, but certainly we should implement a CMD13 polling
>>> mechanism - to align behaviour.
>>
>> Recovery probably isn't possible.  The block driver heroically has a go
>> at it.  For some people it much more important to fail fast than to
>> recover.  Consequently, unless you has a specific use-case, I wouldn't
>> add anything that would slow down that path.
> 
> I agree that it's hard to tell what's the best approach. :-) Though I
> still think we can do a bit better than what we do today, and I really
> don't like that we don't have an aligned behaviour - between hosts
> supporting and not supporting MMC_CAP_WAIT_WHILE_BUSY for this
> particular case.
> 
> I understand that you want to prevent us from breaking something. But
> currently, if we consider all hosts, either it all works like a charm,
> or some are broken, because the behaviour is different.
> 
> In this case, I don't see the benefit of using hw busy detection at
> all, since the performance to gain is not relevant. We could then
> convert to R1 instead of R1B and for all cases do polling with CMD13
> for a small period of let's say 200 ms.
> 
> Would that be acceptable for you?

How about R1 for the read case, R1B for write and poll only if
!MMC_CAP_WAIT_WHILE_BUSY.

I notice send_stop() has no timeout - perhaps fish out the data timeout and
use that for both R1B and polling.

> 
>>
>>>
>>> Are you then also indirectly suggesting that not specficing
>>> "cmd.busy_timeout" should be interpreted by the host as "use whatever
>>> timeout you want"?
>>
>> That is how it is now.  The problem with trying to so something better is
>> that sometimes the timeout really is undefined.
> 
> That makes sense!
> 
>>
>>>
>>> Do note, there are another scenario, which also don't specify a busy
>>> timeout, which is when we have used an open ended WRITE transmission
>>> and using CMD12 to finalize it.
>>> But, in this scenario we do polling with CMD13, also without a
>>> timeout. So at least the behaviour are aligned here, but still no
>>> timeout specified.
>>
>> I don't think that is right.  The data timeout applies in that case too.
> 
> No it shouldn't. The data timeout applies only to the ongoing data
> transfer. In other words, if the card stops sending/receiving data in
> the middle of the transfer.

I couldn't find anything in the JEDEC spec. but the SD spec. says:

	There are two types of busies in a multiple block write operation.
	(1) Write busy at block gap (without CMD12) is maximum 250ms
	(2) Write busy after CMD12 is maximum 250ms (500ms for SDXC)

> 
>>
>>>
>>>>
>>>>>
>>>>> Kind regards
>>>>> Ulf Hansson
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>> ---
>>>>>>>  drivers/mmc/card/block.c |    2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>>>> index 87cd2b0..74169fa 100644
>>>>>>> --- a/drivers/mmc/card/block.c
>>>>>>> +++ b/drivers/mmc/card/block.c
>>>>>>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>>>>>>       int err;
>>>>>>>
>>>>>>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>>>>>>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>>>>>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>>>>>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>>>>>>       if (err == 0)
>>>>>>>               *status = cmd.resp[0];
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 08/10] mmc: card: Use R1 response for the stop cmd at recovery path

[Ulf Hansson]

On 28 January 2014 15:45, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 28/01/14 14:39, Ulf Hansson wrote:
>> On 27 January 2014 11:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 23/01/14 16:59, Ulf Hansson wrote:
>>>> On 23 January 2014 15:29, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 23/01/14 15:21, Ulf Hansson wrote:
>>>>>> On 23 January 2014 11:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>>>>>> Hosts supporting MMC_CAP_WAIT_WHILE_BUSY shall not be waiting for busy
>>>>>>>> detection completion in the recovery path, which were the case when
>>>>>>>> using R1B response.
>>>>>>>>
>>>>>>>> Start using R1 as response instead to align behavior, no matter if
>>>>>>>> MMC_CAP_WAIT_WHILE_BUSY is supported or not.
>>>>>>>
>>>>>>> This does not make sense to me.  If you are sending a STOP command you
>>>>>>> should use the correct response type.  R1B should be OK here because the
>>>>>>> card should release the busy signal in any case except failure.
>>>>>>
>>>>>> For those hosts not supporting MMC_CAP_WAIT_WHILE_BUSY a R1B is
>>>>>> assumed to be treated same as an R1, which means there are no busy
>>>>>> detection handled in the host.
>>>>>
>>>>> That is not entirely true.  For hosts that do not set
>>>>> MMC_CAP_WAIT_WHILE_BUSY we don't know if they wait or not.  I imagine most
>>>>> do because it is more efficient, but the kernel has always been programmed
>>>>> to poll the status anyway so you can't tell from the code.
>>>>
>>>> You are right, we can't know - unless we dive in into each host driver
>>>> and check.
>>>>
>>>> Surely there could be more than omap_hsmmc and sdhci that support
>>>> this. Still I think we need to conclude on how to go forward with
>>>> MMC_CAP_WAIT_WHILE_BUSY, since at the moment it seems a bit of a mess.
>>>> Obviously we need to be careful to not break anything.
>>>>
>>>>>
>>>>> MMC_CAP_WAIT_WHILE_BUSY was one of my inventions I am afraid.  If I recall
>>>>> correctly it was mainly due to the SLEEP command because you can't poll in
>>>>> that case and you don't want to delay the system from sleeping - if you are
>>>>> certain that the controller has waited for busy to de-assert (i.e.
>>>>> MMC_CAP_WAIT_WHILE_BUSY) then you can exit immediately.
>>>>
>>>> I think MMC_CAP_WAIT_WHILE_BUSY was a needed feature, now we only have
>>>> to make it more mature. :-)
>>>>
>>>>>
>>>>>>
>>>>>> mmc_blk_cmd_recovery() is the only caller of the send_stop() function.
>>>>>> Additionally it does not care about to handle busy detection with
>>>>>> CDM13 polling.
>>>>>>
>>>>>> Now, since most hosts don't support MMC_CAP_WAIT_WHILE_BUSY which
>>>>>> means there no busy detection done, I wanted to align to this
>>>>>> behaviour - no matter if the host can do HW busy detection or not.
>>>>>>
>>>>>> I am not saying this is how it must be done, just trying to provide
>>>>>> you with some more reasons to why I wanted to change.
>>>>>>
>>>>>> If we instead decide keep the R1B for send_stop(), we should implement
>>>>>> CMD 13 polling to meet the same behaviour for hosts not supporting
>>>>>> MMC_CAP_WAIT_WHILE_BUSY. In this scenario, we need to set a select a
>>>>>> busy timeout, do you have any suggestion of what would be a reasonable
>>>>>> value for it?
>>>>>
>>>>> It is hard to tell if waiting is ever going to help more than hinder, so I
>>>>> would not change this.
>>>>
>>>> Fair enough, but certainly we should implement a CMD13 polling
>>>> mechanism - to align behaviour.
>>>
>>> Recovery probably isn't possible.  The block driver heroically has a go
>>> at it.  For some people it much more important to fail fast than to
>>> recover.  Consequently, unless you has a specific use-case, I wouldn't
>>> add anything that would slow down that path.
>>
>> I agree that it's hard to tell what's the best approach. :-) Though I
>> still think we can do a bit better than what we do today, and I really
>> don't like that we don't have an aligned behaviour - between hosts
>> supporting and not supporting MMC_CAP_WAIT_WHILE_BUSY for this
>> particular case.
>>
>> I understand that you want to prevent us from breaking something. But
>> currently, if we consider all hosts, either it all works like a charm,
>> or some are broken, because the behaviour is different.
>>
>> In this case, I don't see the benefit of using hw busy detection at
>> all, since the performance to gain is not relevant. We could then
>> convert to R1 instead of R1B and for all cases do polling with CMD13
>> for a small period of let's say 200 ms.
>>
>> Would that be acceptable for you?
>
> How about R1 for the read case, R1B for write and poll only if
> !MMC_CAP_WAIT_WHILE_BUSY.
>
> I notice send_stop() has no timeout - perhaps fish out the data timeout and
> use that for both R1B and polling.

That could work! I will give it go and send a new v2 patch.

>
>>
>>>
>>>>
>>>> Are you then also indirectly suggesting that not specficing
>>>> "cmd.busy_timeout" should be interpreted by the host as "use whatever
>>>> timeout you want"?
>>>
>>> That is how it is now.  The problem with trying to so something better is
>>> that sometimes the timeout really is undefined.
>>
>> That makes sense!
>>
>>>
>>>>
>>>> Do note, there are another scenario, which also don't specify a busy
>>>> timeout, which is when we have used an open ended WRITE transmission
>>>> and using CMD12 to finalize it.
>>>> But, in this scenario we do polling with CMD13, also without a
>>>> timeout. So at least the behaviour are aligned here, but still no
>>>> timeout specified.
>>>
>>> I don't think that is right.  The data timeout applies in that case too.
>>
>> No it shouldn't. The data timeout applies only to the ongoing data
>> transfer. In other words, if the card stops sending/receiving data in
>> the middle of the transfer.
>
> I couldn't find anything in the JEDEC spec. but the SD spec. says:
>
>         There are two types of busies in a multiple block write operation.
>         (1) Write busy at block gap (without CMD12) is maximum 250ms
>         (2) Write busy after CMD12 is maximum 250ms (500ms for SDXC)

The SD spec mentions NAC for read and NWR for writes, similar bus
timings exists for the eMMC spec.

For the SD spec, it seems a bit more clear on what a host could expect
as a maximum busy timeout. According to what you also found above.

Maybe it is a really good option of re-using the data timeout as the
busy timeout, as you suggested above.

Still, the eMMC spec is not so clear. From the "Data Write" chapter
you will find the following statement:

"Some Devices may require long and unpredictable times to write a
block of data.". :-)

Kind regards
Uffe

>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Kind regards
>>>>>> Ulf Hansson
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>>> ---
>>>>>>>>  drivers/mmc/card/block.c |    2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>>>>> index 87cd2b0..74169fa 100644
>>>>>>>> --- a/drivers/mmc/card/block.c
>>>>>>>> +++ b/drivers/mmc/card/block.c
>>>>>>>> @@ -728,7 +728,7 @@ static int send_stop(struct mmc_card *card, u32 *status)
>>>>>>>>       int err;
>>>>>>>>
>>>>>>>>       cmd.opcode = MMC_STOP_TRANSMISSION;
>>>>>>>> -     cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>>>>>>> +     cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>>>>>>>>       err = mmc_wait_for_cmd(card->host, &cmd, 5);
>>>>>>>>       if (err == 0)
>>>>>>>>               *status = cmd.resp[0];
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>

[PATCH 09/10] mmc: mmci: Handle CMD irq before DATA irq

[Ulf Hansson]

In case of a read operation both MCI_CMDRESPEND and MCI_DATAEND can be
set in the status register when entering the interrupt handler. This is
due to that the card start sending data before the host has
acknowledged the command response.

To resolve the issue for this scenario, we must start by handling the
CMD irq instead of the DATA irq. The reason is beacuse the completion
of the DATA irq will not respect the current command and then causing
it to be garbled.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Johan Rudholm <jrudholm@gmail.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index f320579..1a4b153 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1144,16 +1144,17 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 
 		dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
 
+		cmd = host->cmd;
+		if (status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|
+			      MCI_CMDRESPEND) && cmd)
+			mmci_cmd_irq(host, cmd, status);
+
 		data = host->data;
 		if (status & (MCI_DATACRCFAIL|MCI_DATATIMEOUT|MCI_STARTBITERR|
 			      MCI_TXUNDERRUN|MCI_RXOVERRUN|MCI_DATAEND|
 			      MCI_DATABLOCKEND) && data)
 			mmci_data_irq(host, data, status);
 
-		cmd = host->cmd;
-		if (status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND) && cmd)
-			mmci_cmd_irq(host, cmd, status);
-
 		ret = 1;
 	} while (status);
 
-- 
1.7.9.5

Re: [PATCH 09/10] mmc: mmci: Handle CMD irq before DATA irq

[Russell King - ARM Linux]

On Wed, Jan 22, 2014 at 04:00:23PM +0100, Ulf Hansson wrote:
> In case of a read operation both MCI_CMDRESPEND and MCI_DATAEND can be
> set in the status register when entering the interrupt handler. This is
> due to that the card start sending data before the host has
> acknowledged the command response.
> 
> To resolve the issue for this scenario, we must start by handling the
> CMD irq instead of the DATA irq. The reason is beacuse the completion
> of the DATA irq will not respect the current command and then causing
> it to be garbled.

And that's because the driver isn't written to be able to handle a
command being issued concurrently with a data transfer, because it
only has one place to store the current command.

Consider what happens in this case:

- Command issued to start a transfer.
- Command completes, data transfer starts.
- New command comes along to be issued, and is in progress
- Data transfer finishes, we now issue the stop command to halt the
  transfer
- We overwrite the new command with the stop command

All hell breaks loose.  No, this isn't a fix I want to see until the
driver can properly handle concurrent commands.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH 09/10] mmc: mmci: Handle CMD irq before DATA irq

[Ulf Hansson]

On 22 January 2014 16:19, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jan 22, 2014 at 04:00:23PM +0100, Ulf Hansson wrote:
>> In case of a read operation both MCI_CMDRESPEND and MCI_DATAEND can be
>> set in the status register when entering the interrupt handler. This is
>> due to that the card start sending data before the host has
>> acknowledged the command response.
>>
>> To resolve the issue for this scenario, we must start by handling the
>> CMD irq instead of the DATA irq. The reason is beacuse the completion
>> of the DATA irq will not respect the current command and then causing
>> it to be garbled.
>
> And that's because the driver isn't written to be able to handle a
> command being issued concurrently with a data transfer, because it
> only has one place to store the current command.

For a READ request, we are starting the data state machine by invoking
mmci_start_data before we send the READ command. That is not like we
handle concurrent requests, it is still only one request we are
operating on.

Now, suppose you have a high interface speed and a well performing
card, that will mean you can actually get the DATAEND irq at the same
time you get the CMD response. It is only because of this scenario we
need to reverse the order of handling the IRQs. Sorry if the commit
message was to vague.

>
> Consider what happens in this case:
>
> - Command issued to start a transfer.
> - Command completes, data transfer starts.
> - New command comes along to be issued, and is in progress
> - Data transfer finishes, we now issue the stop command to halt the
>   transfer
> - We overwrite the new command with the stop command
>
> All hell breaks loose.  No, this isn't a fix I want to see until the
> driver can properly handle concurrent commands.

This is not the scenario I want or need to solve. The host must only
handle one request at time. When we are done, we do call
mmc_request_done, which tells the mmc core it's fine to start a new
one. So the above sequence is not even possible.

Kind regards
Ulf Hansson

>
> --
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".

[PATCH 10/10] mmc: mmci: Enable support for busy detection for ux500 variant

[Ulf Hansson]

The ux500 variants have HW busy detection support, which is indicated
by the busy_detect flag. For these variants let's enable the
MMC_CAP_WAIT_WHILE_BUSY flag and add the support for it.

The mmc core will provide the RSP_BUSY command flag for those requests
we should care about busy detection. Regarding the max_busy_timeout,
the HW don't support busy detection timeouts so at this initial step
let's make it simple and set it to zero to indicate we are able to
support any timeout.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Johan Rudholm <jrudholm@gmail.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |   51 +++++++++++++++++++++++++++++++++++++++--------
 drivers/mmc/host/mmci.h |    2 ++
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 1a4b153..9976a90 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -921,6 +921,29 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 {
 	void __iomem *base = host->base;
 	bool sbc = (cmd == host->mrq->sbc);
+	bool busy_resp = host->variant->busy_detect &&
+			(cmd->flags & MMC_RSP_BUSY);
+
+	/* Check if we need to wait for busy completion. */
+	if (host->busy_status && (status & MCI_ST_CARDBUSY))
+		return;
+
+	/* Enable busy completion if needed and supported. */
+	if (!host->busy_status && busy_resp &&
+		!(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
+		(readl(base + MMCISTATUS) & MCI_ST_CARDBUSY)) {
+		writel(readl(base + MMCIMASK0) | MCI_ST_BUSYEND,
+			base + MMCIMASK0);
+		host->busy_status = status & (MCI_CMDSENT|MCI_CMDRESPEND);
+		return;
+	}
+
+	/* At busy completion, mask the IRQ and complete the request. */
+	if (host->busy_status) {
+		writel(readl(base + MMCIMASK0) & ~MCI_ST_BUSYEND,
+			base + MMCIMASK0);
+		host->busy_status = 0;
+	}
 
 	host->cmd = NULL;
 
@@ -1139,14 +1162,19 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 			status &= ~MCI_IRQ1MASK;
 		}
 
+		/*
+		 * We intentionally clear the MCI_ST_CARDBUSY IRQ here (if it's
+		 * enabled) since the HW seems to be triggering the IRQ on both
+		 * edges while monitoring DAT0 for busy completion.
+		 */
 		status &= readl(host->base + MMCIMASK0);
 		writel(status, host->base + MMCICLEAR);
 
 		dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
 
 		cmd = host->cmd;
-		if (status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|
-			      MCI_CMDRESPEND) && cmd)
+		if ((status|host->busy_status) & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|
+			MCI_CMDSENT|MCI_CMDRESPEND) && cmd)
 			mmci_cmd_irq(host, cmd, status);
 
 		data = host->data;
@@ -1155,6 +1183,10 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 			      MCI_DATABLOCKEND) && data)
 			mmci_data_irq(host, data, status);
 
+		/* Don't poll for busy completion in irq context. */
+		if (host->busy_status)
+			status &= ~MCI_ST_CARDBUSY;
+
 		ret = 1;
 	} while (status);
 
@@ -1504,12 +1536,6 @@ static int mmci_probe(struct amba_device *dev,
 		goto clk_disable;
 	}
 
-	if (variant->busy_detect) {
-		mmci_ops.card_busy = mmci_card_busy;
-		mmci_write_datactrlreg(host, MCI_ST_DPSM_BUSYMODE);
-	}
-
-	mmc->ops = &mmci_ops;
 	/*
 	 * The ARM and ST versions of the block have slightly different
 	 * clock divider equations which means that the minimum divider
@@ -1543,6 +1569,15 @@ static int mmci_probe(struct amba_device *dev,
 	mmc->caps = plat->capabilities;
 	mmc->caps2 = plat->capabilities2;
 
+	if (variant->busy_detect) {
+		mmci_ops.card_busy = mmci_card_busy;
+		mmci_write_datactrlreg(host, MCI_ST_DPSM_BUSYMODE);
+		mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
+		mmc->max_busy_timeout = 0;
+	}
+
+	mmc->ops = &mmci_ops;
+
 	/* We support these PM capabilities. */
 	mmc->pm_caps = MMC_PM_KEEP_POWER;
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 168bc72..b008ace 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -139,6 +139,7 @@
 /* Extended status bits for the ST Micro variants */
 #define MCI_ST_SDIOITMASK	(1 << 22)
 #define MCI_ST_CEATAENDMASK	(1 << 23)
+#define MCI_ST_BUSYEND		(1 << 24)
 
 #define MMCIMASK1		0x040
 #define MMCIFIFOCNT		0x048
@@ -186,6 +187,7 @@ struct mmci_host {
 	u32			pwr_reg;
 	u32			clk_reg;
 	u32			datactrl_reg;
+	u32			busy_status;
 	bool			vqmmc_enabled;
 	struct mmci_platform_data *plat;
 	struct variant_data	*variant;
-- 
1.7.9.5