[PATCH v2] mci: only write blocks when card out of programming mode

[PATCH v2] mci: only write blocks when card out of programming mode

[Ahmad Fatoum]

eMMC Spec v5.1 (JEDEC Standard No. 84-B51) is quite clear that:

  Due to legacy reasons, a Device may still treat CMD24/25 during
  prg-state (while busy is active) as a legal or illegal command.
  A host should not send CMD24/25 while the Device is in the prg
  state and busy is active.

So far, we had never evaluated MMC_CMD_SEND_STATUS. This patch
corrects this and thereby resolves an issue of timed out writes
with the atmel_mci driver:

  barebox@SAMA5D4:/ saveenv
  ERROR: atmel_mci fc000000.mmc@fc000000.of: command/data timeout
  ERROR: atmel_mci fc000000.mmc@fc000000.of: command/data timeout

These issues are not new, but were first reported in 2018[0] along
with a workaround suggesting a delay at the end of atmci_request:

  if (cmdidx == MMC_CMD_STOP_TRANSMISSION)
	mdelay(5)

Just before the command timing out, we read 0x00c00_0025 from the
status register, which is

  CMDRDY | TXRDY | NOTBUSY | XFERDONE | FIFOEMPTY

which suggests the issue is not at the MCI host side, but rather at
the card side. With this patch applied and debugging enabled, this
seems to be confirmed:

  barebox@SAMA5D4:/ saveenv
  saving environment
  mmc1: Ready polling took 0ms
  mmc1: Ready polling took 4ms

I compared with AT91Bootstrap[1], U-Boot[2] and Trusted Firmware-A[3]
and all of them poll MCI status after block writes.

The sequence imported here is taken from U-Boot, but instead of doing
it after writes, we do it before them in hope that we need not always
incur the extra delay.

I don't have an answer why this was only necessary on the SAMA5D3/4 and
such issues weren't observed with other drivers. Card was operated
at 50MHz (SD HS) and it didn't help trying other cards or going down
to 400kHz. I tested this change also on a Beaglebone Black where an
environment is also stored into FAT on a SD-Card operated with 50 MHz:
Ready polling took 0ms for each of the two writes.

[0]: https://lore.barebox.org/barebox/20181102091156.26476-1-s.hauer@pengutronix.de/
[1]: https://github.com/linux4sam/at91bootstrap/blob/v4.0.5/driver/mci_media.c#L1187
[2]: https://github.com/trini/u-boot/blob/v2023.01-rc3/drivers/mmc/mmc_write.c#L181
[3]: https://github.com/ARM-software/arm-trusted-firmware/blob/v2.8.0/drivers/mmc/mmc.c#L718

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
For next, not master. I intend to test this change across more devices
in the coming weeks.

v1 -> v2:
  - skip mci_poll_until_ready for SPI mode (Sascha)
  - simplify timeout handling (Sascha)
  - turn dev_info into dev_dbg
  - remove comment about debugging message placement
  - increase timeout from 10ms to 1s

---
 drivers/mci/mci-core.c | 94 ++++++++++++++++++++++++++++++++++++++++++
 include/mci.h          | 15 +++++++
 2 files changed, 109 insertions(+)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 8cda07e71120..6c7293a21e47 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -74,6 +74,28 @@ static int mci_send_cmd(struct mci *mci, struct mci_cmd *cmd, struct mci_data *d
 	return host->send_cmd(mci->host, cmd, data);
 }
 
+/**
+ * mci_send_cmd_retry() - send a command to the mmc device, retrying on error
+ *
+ * @dev:	device to receive the command
+ * @cmd:	command to send
+ * @data:	additional data to send/receive
+ * @retries:	how many times to retry; mci_send_cmd is always called at least
+ *              once
+ * Return: 0 if ok, -ve on error
+ */
+static int mci_send_cmd_retry(struct mci *mci, struct mci_cmd *cmd,
+			      struct mci_data *data, unsigned retries)
+{
+	int ret;
+
+	do
+		ret = mci_send_cmd(mci, cmd, data);
+	while (ret && retries--);
+
+	return ret;
+}
+
 /**
  * @param p Command definition to setup
  * @param cmd Valid SD/MMC command (refer MMC_CMD_* / SD_CMD_*)
@@ -119,6 +141,67 @@ static int mci_set_blocklen(struct mci *mci, unsigned len)
 
 static void *sector_buf;
 
+static int mci_send_status(struct mci *mci, unsigned int *status)
+{
+	struct mci_host *host = mci->host;
+	struct mci_cmd cmd;
+	int ret;
+
+	/*
+	 * While CMD13 is defined for SPI mode, the reported status bits have
+	 * different layout that SD/MMC. We skip supporting this for now.
+	 */
+	if (mmc_host_is_spi(host))
+		return -ENOSYS;
+
+	cmd.cmdidx = MMC_CMD_SEND_STATUS;
+	cmd.resp_type = MMC_RSP_R1;
+	cmd.cmdarg = mci->rca << 16;
+
+	ret = mci_send_cmd_retry(mci, &cmd, NULL, 4);
+	if (!ret)
+		*status = cmd.response[0];
+
+	return ret;
+}
+
+static int mci_poll_until_ready(struct mci *mci, int timeout_ms)
+{
+	unsigned int status;
+	int err, retries = 0;
+
+	while (1) {
+		err = mci_send_status(mci, &status);
+		if (err)
+			return err;
+
+		/*
+		 * Some cards mishandle the status bits, so make sure to
+		 * check both the busy indication and the card state.
+		 */
+		if ((status & R1_READY_FOR_DATA) &&
+		    R1_CURRENT_STATE(status) != R1_STATE_PRG)
+			break;
+
+		if (status & R1_STATUS_MASK) {
+			dev_err(&mci->dev, "Status Error: 0x%08x\n", status);
+			return -EIO;
+		}
+
+		if (retries++ == timeout_ms) {
+			dev_err(&mci->dev, "Timeout awaiting card ready\n");
+			return -ETIMEDOUT;
+		}
+
+		udelay(1000);
+	}
+
+	dev_dbg(&mci->dev, "Ready polling took %ums\n", retries);
+
+	return 0;
+}
+
+
 /**
  * Write one or several blocks of data to the card
  * @param mci_dev MCI instance
@@ -136,6 +219,17 @@ static int mci_block_write(struct mci *mci, const void *src, int blocknum,
 	unsigned mmccmd;
 	int ret;
 
+	/*
+	 * Quoting eMMC Spec v5.1 (JEDEC Standard No. 84-B51):
+	 * Due to legacy reasons, a Device may still treat CMD24/25 during
+	 * prg-state (while busy is active) as a legal or illegal command.
+	 * A host should not send CMD24/25 while the Device is in the prg
+	 * state and busy is active.
+	 */
+	ret = mci_poll_until_ready(mci, 1000 /* ms */);
+	if (ret && ret != -ENOSYS)
+		return ret;
+
 	if (blocks > 1)
 		mmccmd = MMC_CMD_WRITE_MULTIPLE_BLOCK;
 	else
diff --git a/include/mci.h b/include/mci.h
index d949310fac30..1e53a09d71db 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -308,8 +308,23 @@
 #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
 
 #define R1_ILLEGAL_COMMAND		(1 << 22)
+#define R1_STATUS(x)			(x & 0xFFF9A000)
+#define R1_CURRENT_STATE(x)		((x & 0x00001E00) >> 9)	/* sx, b (4 bits) */
+#define R1_READY_FOR_DATA 		(1 << 8)		/* sx, a */
 #define R1_APP_CMD			(1 << 5)
 
+#define R1_STATUS_MASK			(~0x0206BF7F)
+
+#define	R1_STATE_IDLE	0
+#define	R1_STATE_READY	1
+#define	R1_STATE_IDENT	2
+#define	R1_STATE_STBY	3
+#define	R1_STATE_TRAN	4
+#define	R1_STATE_DATA	5
+#define	R1_STATE_RCV	6
+#define	R1_STATE_PRG	7
+#define	R1_STATE_DIS	8
+
 #define R1_SPI_IDLE		(1 << 0)
 #define R1_SPI_ERASE_RESET	(1 << 1)
 #define R1_SPI_ILLEGAL_COMMAND	(1 << 2)
-- 
2.30.2

Re: [PATCH v2] mci: only write blocks when card out of programming mode

[Sascha Hauer]

On Wed, Dec 14, 2022 at 11:50:50AM +0100, Ahmad Fatoum wrote:
> eMMC Spec v5.1 (JEDEC Standard No. 84-B51) is quite clear that:
> 
>   Due to legacy reasons, a Device may still treat CMD24/25 during
>   prg-state (while busy is active) as a legal or illegal command.
>   A host should not send CMD24/25 while the Device is in the prg
>   state and busy is active.
> 
> So far, we had never evaluated MMC_CMD_SEND_STATUS. This patch
> corrects this and thereby resolves an issue of timed out writes
> with the atmel_mci driver:
> 
>   barebox@SAMA5D4:/ saveenv
>   ERROR: atmel_mci fc000000.mmc@fc000000.of: command/data timeout
>   ERROR: atmel_mci fc000000.mmc@fc000000.of: command/data timeout
> 
> These issues are not new, but were first reported in 2018[0] along
> with a workaround suggesting a delay at the end of atmci_request:
> 
>   if (cmdidx == MMC_CMD_STOP_TRANSMISSION)
> 	mdelay(5)
> 
> Just before the command timing out, we read 0x00c00_0025 from the
> status register, which is
> 
>   CMDRDY | TXRDY | NOTBUSY | XFERDONE | FIFOEMPTY
> 
> which suggests the issue is not at the MCI host side, but rather at
> the card side. With this patch applied and debugging enabled, this
> seems to be confirmed:
> 
>   barebox@SAMA5D4:/ saveenv
>   saving environment
>   mmc1: Ready polling took 0ms
>   mmc1: Ready polling took 4ms
> 
> I compared with AT91Bootstrap[1], U-Boot[2] and Trusted Firmware-A[3]
> and all of them poll MCI status after block writes.
> 
> The sequence imported here is taken from U-Boot, but instead of doing
> it after writes, we do it before them in hope that we need not always
> incur the extra delay.
> 
> I don't have an answer why this was only necessary on the SAMA5D3/4 and
> such issues weren't observed with other drivers. Card was operated
> at 50MHz (SD HS) and it didn't help trying other cards or going down
> to 400kHz. I tested this change also on a Beaglebone Black where an
> environment is also stored into FAT on a SD-Card operated with 50 MHz:
> Ready polling took 0ms for each of the two writes.
> 
> [0]: https://lore.barebox.org/barebox/20181102091156.26476-1-s.hauer@pengutronix.de/
> [1]: https://github.com/linux4sam/at91bootstrap/blob/v4.0.5/driver/mci_media.c#L1187
> [2]: https://github.com/trini/u-boot/blob/v2023.01-rc3/drivers/mmc/mmc_write.c#L181
> [3]: https://github.com/ARM-software/arm-trusted-firmware/blob/v2.8.0/drivers/mmc/mmc.c#L718
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> For next, not master. I intend to test this change across more devices
> in the coming weeks.
> 
> v1 -> v2:
>   - skip mci_poll_until_ready for SPI mode (Sascha)
>   - simplify timeout handling (Sascha)
>   - turn dev_info into dev_dbg
>   - remove comment about debugging message placement
>   - increase timeout from 10ms to 1s

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |