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

[Jaehoon Chung]

Hi Bin,

On 2/1/21 1:20 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng at 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 at 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;
>