Re: [PATCH 2/3] mmc: mmc_spi: Fix potential spec violation in receiving card response

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;