[PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the eMMC is ready

[Jaehoon Chung]

On 5/19/20 12:37 PM, Peng Fan wrote:
>> Subject: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the eMMC
>> is ready
>>
>> From: Haibo Chen <haibo.chen at nxp.com>
>>
>> According to eMMC specification v5.1 section 6.4.3, we should issue
>> CMD1 repeatedly in the idle state until the eMMC is ready even if
>> mmc_send_op_cond() send CMD1 with argument = 0. Otherwise some
>> eMMC devices seems to enter the inactive mode after
>> mmc_complete_op_cond() issued CMD0 when the eMMC device is busy. This
>> patch also align with Linux 5.7.

Well, i think that it can't compare to linux driver.
is your issue fixed with this patch?

As i know, this sequence is not follow correct spec. 
To enhance boot, it was skipping the polling time.
If there is a problem, could you explain in more detail?
Then I will also check on my targets.

>>
>> Signed-off-by: Haibo Chen <haibo.chen at nxp.com>
>> ---
>>  drivers/mmc/mmc.c | 41 +++++++++++++++++++++++++++++++++--------
>>  1 file changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
>> 523c055967..509549756b 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -664,21 +664,46 @@ static int mmc_send_op_cond_iter(struct mmc
>> *mmc, int use_arg)
>>
>>  static int mmc_send_op_cond(struct mmc *mmc)  {
>> +	struct mmc_cmd cmd;
>>  	int err, i;
>> +	int timeout = 1000;
>> +	ulong start;
>>
>>  	/* Some cards seem to need this */
>>  	mmc_go_idle(mmc);
>>
>> - 	/* Asking to the card its capabilities */
>> -	for (i = 0; i < 2; i++) {
>> -		err = mmc_send_op_cond_iter(mmc, i != 0);
>> +	cmd.cmdidx = MMC_CMD_SEND_OP_COND;
>> +	cmd.resp_type = MMC_RSP_R3;
>> +	cmd.cmdarg = 0;
>> +
>> +	start = get_timer(0);
>> +	/*
>> +	 * According to eMMC specification v5.1 section 6.4.3, we
>> +	 * should issue CMD1 repeatedly in the idle state until
>> +	 * the eMMC is ready. Otherwise some eMMC devices seem to enter
>> +	 * the inactive mode after mmc_complete_op_cond() issued CMD0
>> when
>> +	 * the eMMC device is busy.
>> +	 */
> 
> Could we just extend the previous for loop to fix the issue?
> Your piece code duplicate much of the code and changed
> the original behavior.
> 
>> +	while (1) {
>> +		err = mmc_send_cmd(mmc, &cmd, NULL);
>>  		if (err)
>>  			return err;
>> -
>> -		/* exit if not busy (flag seems to be inverted) */
>> -		if (mmc->ocr & OCR_BUSY)
>> -			break;
>> +		if (mmc_host_is_spi(mmc)) {
>> +			if (!(cmd.response[0] & (1 << 0)))
>> +				break;
>> +		} else {
>> +			mmc->ocr = cmd.response[0];
>> +			/* exit if not busy (flag seems to be inverted) */
>> +			if (mmc->ocr & OCR_BUSY)
>> +				break;
>> +		}
>> +		if (get_timer(start) > timeout)
>> +			return -EOPNOTSUPP;
> 
> TIMEOUT, please.
> 
>> +		udelay(100);
> 
> This will change the original behavior by adding a delay.
> 
>> +		if (!mmc_host_is_spi(mmc))
>> +			cmd.cmdarg = cmd.response[0] | (1 << 30);
> 
> Do we need "1 << 30" ?
> 
>>  	}
>> +
>>  	mmc->op_cond_pending = 1;
>>  	return 0;
>>  }
>> @@ -691,7 +716,7 @@ static int mmc_complete_op_cond(struct mmc
>> *mmc)
>>  	int err;
>>
>>  	mmc->op_cond_pending = 0;
>> -	if (!(mmc->ocr & OCR_BUSY)) {
>> +	if (mmc->ocr & OCR_BUSY) {
> 
> When card not go out busy, it means card not
> finish power up seq. Why drop !?

Right. It doesn't finish power-ups seq.
If it's working with this patch, it seems something wrong.

> 
>>  		/* Some cards seem to need this */
>>  		mmc_go_idle(mmc);
>>
>> --
> 
> Would the following patch help?

yes. it's more better. 
If need to increase polling time, i think it's correct place in mmc_complete_op_cond().

Best Regards,
Jaehoon Chung

> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 523c055967..d3a0538cdb 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -665,12 +665,15 @@ static int mmc_send_op_cond_iter(struct mmc *mmc, int use_arg)
>  static int mmc_send_op_cond(struct mmc *mmc)
>  {
>         int err, i;
> +       int timeout = 1000;
> +       ulong start;
> 
>         /* Some cards seem to need this */
>         mmc_go_idle(mmc);
> 
> +       start = get_timer(0);
>         /* Asking to the card its capabilities */
> -       for (i = 0; i < 2; i++) {
> +       for (i = 0; ; i++) {
>                 err = mmc_send_op_cond_iter(mmc, i != 0);
>                 if (err)
>                         return err;
> @@ -678,6 +681,9 @@ static int mmc_send_op_cond(struct mmc *mmc)
>                 /* exit if not busy (flag seems to be inverted) */
>                 if (mmc->ocr & OCR_BUSY)
>                         break;
> +
> +               if (get_timer(start) > timeout)
> +                       return -ETIMEDOUT;
>         }
>         mmc->op_cond_pending = 1;
>         return 0;
> 
> Thanks,
> Peng.
>> 2.17.1
> 
>