[PATCH 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
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 ---
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 b1edb6a..85a2818 100644 --- a/drivers/mmc/mmc_spi.c +++ b/drivers/mmc/mmc_spi.c @@ -83,6 +83,9 @@ static int mmc_spi_sendcmd(struct udevice *dev, __func__, cmdidx, cmdarg, resp_type, resp_size, resp_match, resp_match_value);
+ if (!resp || !resp_size) + return 0; + cmdo[0] = 0xff; cmdo[1] = MMC_SPI_CMD(cmdidx); cmdo[2] = cmdarg >> 24; @@ -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) {
On 2/1/21 1:20 PM, Bin Meng wrote:
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
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 b1edb6a..85a2818 100644 --- a/drivers/mmc/mmc_spi.c +++ b/drivers/mmc/mmc_spi.c @@ -83,6 +83,9 @@ static int mmc_spi_sendcmd(struct udevice *dev, __func__, cmdidx, cmdarg, resp_type, resp_size, resp_match, resp_match_value);
- if (!resp || !resp_size)
return 0;
I'm not sure..does it needs to locate more above? resp_size is referred in debug message. Other thing looks good to me.
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
- cmdo[0] = 0xff; cmdo[1] = MMC_SPI_CMD(cmdidx); cmdo[2] = cmdarg >> 24;
@@ -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 ---
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 85a2818..5ec6b0f 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;
Hi Bin,
On 2/1/21 1:20 PM, Bin Meng wrote:
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
After checking spec, i will reply for this patch. :)
Best Regards, Jaehoon Chung
Signed-off-by: Bin Meng bin.meng@windriver.com
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 85a2818..5ec6b0f 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;
On 2/1/21 1:20 PM, Bin Meng wrote:
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
Best Regards, Jaehoon Chung
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 85a2818..5ec6b0f 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 ---
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 5ec6b0f..6d4f205 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 2/1/21 1:20 PM, Bin Meng wrote:
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
Best Regards, Jaehoon Chung
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 5ec6b0f..6d4f205 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) {
participants (2)
-
Bin Meng -
Jaehoon Chung