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

[Marek Vasut]

On 3/5/19 8:10 AM, Ang, Chee Hong wrote:
[...]
>>>>> 4) If MMC controller response with error continue to step 5
>>>>> else go
>>>>> to
>>>>> step 7
>>>>> 5) If is less than 4 attempts go to step 3 else continue to
>>>>> step 6
>>>>> 6) Indicate to user there was an error loading uboot as SSBL
>>>>> from
>>>>> MMC
>>>>> and stop here
>>>>> 7) Send command to read blocks of data from MMC
>>>>> 8) uboot successfully loaded to DDR memory
>>>>> 9) SPL jumps to uboot and continue the boot flow.
>>>>>
>>>>> So this patch actually introduce a small delay at step 4.
>>>>> Without
>>>>> this
>>>>> small delay, step 4 might sometime fail in all 4 attempts to
>>>>> issue
>>>>> the
>>>>> 'SET_BLOCKEN' command to MMC controller.
>>>> Isn't what you observe here some sort of timeout which you need
>>>> to
>>>> respect when the card rejects a command ? If so, such timeout
>>>> should
>>>> be
>>>> described in either of the SD or MMC specification and it should
>>>> be
>>>> in
>>>> the MMC framework core instead of a driver.
>>> I don't have chance to test other MMC drivers and I am not sure
>>> other
>>> MMC drivers need such small delay for next retry if the card reject
>>> this command.
>> Does the SD or MMC JEDEC spec say something about such delay?
> I checked the SD/MMC JEDEC. Nothing about this delay is mentioned in
> the spec.

OK, neither of the specs say anything, hmmmm

>>> This DW MMC driver already enforce a small delay even the
>>> command is success, so this makes me think that this 'time out'
>>> issue
>>> is only specific to this DW MMC driver.
>> So why is there that 100 uS delay ? Does the Linux DWMMC driver have
>> such a delay too?
> Linux DWMMC has only the delay for card detection and I think is quite
> common because this is needed to make sure the memory card is properly
> inserted before any operations can be started.

So why doesn't Linux need this special 100 uS delay , but U-Boot does ?

>>> Is it necessary to 'enforce' this small delay in common MMC
>>> framework
>>> (affecting all other MMC drivers as well) as shown below:
>>>
>>> drivers/mmc/mmc.c
>>>
>>> #ifdef CONFIG_MMC_QUIRKS
>>> 	if (err && (mmc->quirks & MMC_QUIRK_RETRY_SET_BLOCKLEN)) {
>>> 		int retries = 4;
>>> 		/*
>>> 		 * It has been seen that SET_BLOCKLEN may fail on the
>>> first
>>> 		 * attempt, let's try a few more time
>>> 		 */
>>> 		do {
>>> 			err = mmc_send_cmd(mmc, &cmd, NULL);
>>> 			if (!err)
>>> 				break;
>>> +			/* Enforce small delay before retry */
>>> +			udelay(100);
>>> 		} while (retries--);
>>> 	}
>>> #endif
>>
>> Expanding the CC to other DWMMC users, maybe their datasheets contain
>> something about the 100 uS delay.
>>
>>>
>>>>
>>>>>
>>>>> Note that this failure is
>>>>> intermittent. If it fails at step 3 and there is no small delay
>>>>> in
>>>>> step
>>>>> 4, it will most likely fail in subsequent attempts.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>> Unfortunately, I can't find any registers I can poll for the
>>>>> status
>>>>> of
>>>>> the readiness of the controller from DesignWare storage
>>>>> controller
>>>>> databook.
>>>>>>
>>>>>>
>>>>>>
>>>>>> btw do you have the same problem if you disable caches ?
>>>>> The error happen while SPL trying to load uboot as SSBL from
>>>>> MMC
>>>>> into
>>>>> DDR memory.At this point, the caches are disabled.
>>>> I-Cache is enabled in SPL, D-cache _might_ be too.
>>>>
>>>> Do you get this error after cold boot too ?
>>> Yes. It happened after cold boot too.
>> Hmmmm, try applying
>>
>> [PATCH V2] ARM: socfpga: Clear PL310 early in SPL
>>
>> I wonder if this might be what you're hitting.
> Nope. This patch only apply to Aaria 10 and this problem is found on
> Stratix 10 platform.

The patch I referenced applies to Gen5, but OK.

-- 
Best regards,
Marek Vasut