[PATCH v2 0/3] mmc: mmc_spi: Fix potential spec violation in receiving card response
After command is sent and before card response shows up on the line, there is a variable number of clock cycles in between called Ncr. The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.
Current logic in mmc_spi_sendcmd() has a flaw that it could only work with certain SD cards with their Ncr being just 1 byte.
When resp_match is false, the codes try to receive only 1 byte from the SD card. On the other hand when resp_match is true, the logic happens to be no problem as it loops until timeout to receive as many bytes as possible to see a match of the expected resp_match_value. However not every call to mmc_spi_sendcmd() is made with resp_match being true hence this exposes a potential issue with SD cards that have a larger Ncr value.
Given no issue was reported as of today, we can reasonably conclude that all cards being used on the supported boards happen to have a 1 byte Ncr timing requirement. But a broken case can be triggered by utilizing QEMU to emulate a larger value of Ncr (by default 1 byte Ncr is used on QEMU). This series fixes such potential spec violation to improve the card compatibility.
[1] "Physical Layer Specification Version 8.00" chapter 7.5.1: Command / Response chapter 7.5.4: Timing Values
Changes in v2: - move the check before the debug output
Bin Meng (3): mmc: mmc_spi: Move argument check to the beginning of mmc_spi_sendcmd() mmc: mmc_spi: Fix potential spec violation in receiving card response mmc: mmc_spi: Document the 3 local functions
drivers/mmc/mmc_spi.c | 74 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 19 deletions(-)
From: Bin Meng bin.meng@windriver.com
The argument check should happen before any transfer on the SPI lines.
Signed-off-by: Bin Meng bin.meng@windriver.com Reviewed-by: Jaehoon Chung jh80.chung@samsung.com ---
Changes in v2: - move the check before the debug output
drivers/mmc/mmc_spi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c index 23b9073..a06862a 100644 --- a/drivers/mmc/mmc_spi.c +++ b/drivers/mmc/mmc_spi.c @@ -78,6 +78,9 @@ static int mmc_spi_sendcmd(struct udevice *dev, int i, rpos = 0, ret = 0; u8 cmdo[7], r;
+ if (!resp || !resp_size) + return 0; + debug("%s: cmd%d cmdarg=0x%x resp_type=0x%x " "resp_size=%d resp_match=%d resp_match_value=0x%x\n", __func__, cmdidx, cmdarg, resp_type, @@ -98,9 +101,6 @@ static int mmc_spi_sendcmd(struct udevice *dev, if (ret) return ret;
- if (!resp || !resp_size) - return 0; - debug("%s: cmd%d", __func__, cmdidx);
if (resp_match) {
From: Bin Meng bin.meng@windriver.com
After command is sent and before card response shows up on the line, there is a variable number of clock cycles in between called Ncr. The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.
Current logic in mmc_spi_sendcmd() has a flaw that it could only work with certain SD cards with their Ncr being just 1 byte.
When resp_match is false, the codes try to receive only 1 byte from the SD card. On the other hand when resp_match is true, the logic happens to be no problem as it loops until timeout to receive as many bytes as possible to see a match of the expected resp_match_value. However not every call to mmc_spi_sendcmd() is made with resp_match being true hence this exposes a potential issue with SD cards that have a larger Ncr value.
Given no issue was reported as of today, we can reasonably conclude that all cards being used on the supported boards happen to have a 1 byte Ncr timing requirement. But a broken case can be triggered by utilizing QEMU to emulate a larger value of Ncr (by default 1 byte Ncr is used on QEMU). This commit fixes such potential spec violation to improve the card compatibility.
[1] "Physical Layer Specification Version 8.00" chapter 7.5.1: Command / Response chapter 7.5.4: Timing Values
Signed-off-by: Bin Meng bin.meng@windriver.com Reviewed-by: Jaehoon Chung jh80.chung@samsung.com ---
(no changes since v1)
drivers/mmc/mmc_spi.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c index a06862a..fbdbcf7 100644 --- a/drivers/mmc/mmc_spi.c +++ b/drivers/mmc/mmc_spi.c @@ -103,29 +103,31 @@ static int mmc_spi_sendcmd(struct udevice *dev,
debug("%s: cmd%d", __func__, cmdidx);
- if (resp_match) { + if (resp_match) r = ~resp_match_value; - i = CMD_TIMEOUT; - while (i) { - ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0); - if (ret) - return ret; - debug(" resp%d=0x%x", rpos, r); - rpos++; - i--; + i = CMD_TIMEOUT; + while (i) { + ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0); + if (ret) + return ret; + debug(" resp%d=0x%x", rpos, r); + rpos++; + i--;
+ if (resp_match) { if (r == resp_match_value) break; + } else { + if (!(r & 0x80)) + break; } - if (!i && (r != resp_match_value)) + + if (!i) return -ETIMEDOUT; }
- for (i = 0; i < resp_size; i++) { - if (i == 0 && resp_match) { - resp[i] = resp_match_value; - continue; - } + resp[0] = r; + for (i = 1; i < resp_size; i++) { ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0); if (ret) return ret;
From: Bin Meng bin.meng@windriver.com
mmc_spi_sendcmd(), mmc_spi_readdata() and mmc_spi_writedata() are currently undocumented. Add comment blocks to explain the arguments and the return value.
Signed-off-by: Bin Meng bin.meng@windriver.com Reviewed-by: Jaehoon Chung jh80.chung@samsung.com ---
(no changes since v1)
drivers/mmc/mmc_spi.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c index fbdbcf7..e2d7879 100644 --- a/drivers/mmc/mmc_spi.c +++ b/drivers/mmc/mmc_spi.c @@ -37,7 +37,8 @@ #define SPI_RESPONSE_CRC_ERR ((5 << 1)|1) #define SPI_RESPONSE_WRITE_ERR ((6 << 1)|1)
-/* Read and write blocks start with these tokens and end with crc; +/* + * Read and write blocks start with these tokens and end with crc; * on error, read tokens act like a subset of R2_SPI_* values. */ /* single block write multiblock read */ @@ -70,6 +71,20 @@ struct mmc_spi_priv { struct spi_slave *spi; };
+/** + * mmc_spi_sendcmd() - send a command to the SD card + * + * @dev: mmc_spi device + * @cmdidx: command index + * @cmdarg: command argument + * @resp_type: card response type + * @resp: buffer to store the card response + * @resp_size: size of the card response + * @resp_match: if true, compare each of received bytes with @resp_match_value + * @resp_match_value: a value to be compared with each of received bytes + * @r1b: if true, receive additional bytes for busy signal token + * @return 0 if OK, -ETIMEDOUT if no card response is received, -ve on error + */ static int mmc_spi_sendcmd(struct udevice *dev, ushort cmdidx, u32 cmdarg, u32 resp_type, u8 *resp, u32 resp_size, @@ -159,6 +174,15 @@ static int mmc_spi_sendcmd(struct udevice *dev, return 0; }
+/** + * mmc_spi_readdata() - read data block(s) from the SD card + * + * @dev: mmc_spi device + * @xbuf: buffer of the actual data (excluding token and crc) to read + * @bcnt: number of data blocks to transfer + * @bsize: size of the actual data (excluding token and crc) in bytes + * @return 0 if OK, -ECOMM if crc error, -ETIMEDOUT on other errors + */ static int mmc_spi_readdata(struct udevice *dev, void *xbuf, u32 bcnt, u32 bsize) { @@ -207,6 +231,16 @@ static int mmc_spi_readdata(struct udevice *dev, return ret; }
+/** + * mmc_spi_writedata() - write data block(s) to the SD card + * + * @dev: mmc_spi device + * @xbuf: buffer of the actual data (excluding token and crc) to write + * @bcnt: number of data blocks to transfer + * @bsize: size of actual data (excluding token and crc) in bytes + * @multi: indicate a transfer by multiple block write command (CMD25) + * @return 0 if OK, -ECOMM if crc error, -ETIMEDOUT on other errors + */ static int mmc_spi_writedata(struct udevice *dev, const void *xbuf, u32 bcnt, u32 bsize, int multi) {
On Tue, Feb 2, 2021 at 10:48 AM Bin Meng bmeng.cn@gmail.com wrote:
After command is sent and before card response shows up on the line, there is a variable number of clock cycles in between called Ncr. The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.
Current logic in mmc_spi_sendcmd() has a flaw that it could only work with certain SD cards with their Ncr being just 1 byte.
When resp_match is false, the codes try to receive only 1 byte from the SD card. On the other hand when resp_match is true, the logic happens to be no problem as it loops until timeout to receive as many bytes as possible to see a match of the expected resp_match_value. However not every call to mmc_spi_sendcmd() is made with resp_match being true hence this exposes a potential issue with SD cards that have a larger Ncr value.
Given no issue was reported as of today, we can reasonably conclude that all cards being used on the supported boards happen to have a 1 byte Ncr timing requirement. But a broken case can be triggered by utilizing QEMU to emulate a larger value of Ncr (by default 1 byte Ncr is used on QEMU). This series fixes such potential spec violation to improve the card compatibility.
[1] "Physical Layer Specification Version 8.00" chapter 7.5.1: Command / Response chapter 7.5.4: Timing Values
Changes in v2:
- move the check before the debug output
Bin Meng (3): mmc: mmc_spi: Move argument check to the beginning of mmc_spi_sendcmd() mmc: mmc_spi: Fix potential spec violation in receiving card response mmc: mmc_spi: Document the 3 local functions
drivers/mmc/mmc_spi.c | 74 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 19 deletions(-)
Peng, ping?
participants (1)
-
Bin Meng