[U-Boot] [PATCH v1] mmc: dwmmc: Enable small delay before returning error

[Marek Vasut]

On 2/18/19 3:51 PM, Ang, Chee Hong wrote:
> On Mon, 2019-02-18 at 12:57 +0100, Marek Vasut wrote:
>> On 2/18/19 5:16 AM, chee.hong.ang at intel.com wrote:
>>>
>>> From: "Ang, Chee Hong" <chee.hong.ang at intel.com>
>>>
>>> 'SET_BLOCKLEN' may occasionally fail on first attempt.
>> Why ?
> This is part of the workaround of mmc driver which is enabled with
> 'CONFIG_MMC_QUIRKS' config.
> https://github.com/u-boot/u-boot/blob/dbe70c7d4e3d5c705a98d82952e05a591
> efd0683/drivers/mmc/mmc.c#L272

OK, let's take a step back. What problem do you observe, that you're
trying to fix ?

>>> This patch enable a small delay in dwmci_send_cmd() on
>>> busy, I/O or CRC error to allow the MMC controller recovers
>>> from the failure/error on subsequent retries.
>> Why 100 uS ?
> This is a good question. Perhaps the driver's authors can explain this.
> The driver delay 100us after dwmci_send_cmd() success with the command
> sent. But it never delay 100us whenever mmc controller response to the
> command sent with I/O or CRC errors. MMC drivers expect the first
> 'SET_BLOCKEN' command issued by dwmci_send_cmd() may fail
> intermittently so it will retry up to 4 times before it gave up and
> return error. Without this 100us delay after error response,
> 'SET_BLOCKEN' may sometime fail in 4 attempts in a row. Therefore, we
> encountered intermittent failure in loading u-boot (SSBL) from MMC.

Can you be more specific about the failure you saw ?

>> Can we rather detect whether the controller recovered by polling some
>> bit?
> Hmmm...I can take a look at the databook of this controller and dig
> further. Probably this is the limitation of the controller itself. The
> mmc driver code which introduce 100us delay after every command sent in
> dwmci_send_cmd() looks iffy.

If you have access to it, please do.

btw do you have the same problem if you disable caches ?

>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang at intel.com>
>>> ---
>>>  drivers/mmc/dw_mmc.c | 14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>> index 7544b84..8dcc518 100644
>>> --- a/drivers/mmc/dw_mmc.c
>>> +++ b/drivers/mmc/dw_mmc.c
>>> @@ -266,8 +266,11 @@ static int dwmci_send_cmd(struct mmc *mmc,
>>> struct mmc_cmd *cmd,
>>>  	if (data)
>>>  		flags = dwmci_set_transfer_mode(host, data);
>>>  
>>> -	if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type &
>>> MMC_RSP_BUSY))
>>> -		return -1;
>>> +	if ((cmd->resp_type & MMC_RSP_136) &&
>>> +	    (cmd->resp_type & MMC_RSP_BUSY)) {
>>> +		ret = -1;
>>> +		goto delay_ret;
>>> +	}
>>>  
>>>  	if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
>>>  		flags |= DWMCI_CMD_ABORT_STOP;
>>> @@ -316,11 +319,13 @@ static int dwmci_send_cmd(struct mmc *mmc,
>>> struct mmc_cmd *cmd,
>>>  		return -ETIMEDOUT;
>>>  	} else if (mask & DWMCI_INTMSK_RE) {
>>>  		debug("%s: Response Error.\n", __func__);
>>> -		return -EIO;
>>> +		ret = -EIO;
>>> +		goto delay_ret;
>>>  	} else if ((cmd->resp_type & MMC_RSP_CRC) &&
>>>  		   (mask & DWMCI_INTMSK_RCRC)) {
>>>  		debug("%s: Response CRC Error.\n", __func__);
>>> -		return -EIO;
>>> +		ret = -EIO;
>>> +		goto delay_ret;
>>>  	}
>>>  
>>>  
>>> @@ -347,6 +352,7 @@ static int dwmci_send_cmd(struct mmc *mmc,
>>> struct mmc_cmd *cmd,
>>>  		}
>>>  	}
>>>  
>>> +delay_ret:
>>>  	udelay(100);
>>>  
>>>  	return ret;
>>>


-- 
Best regards,
Marek Vasut