[U-Boot] [PATCH 1/2] mmc: dw_mmc: Increase timeout to 20 seconds

[Pantelis Antoniou]

Hi Marek,

> On Aug 29, 2015, at 22:19 , Marek Vasut <marex at denx.de> wrote:
> 
> On Saturday, August 29, 2015 at 06:38:48 PM, Lukasz Majewski wrote:
>> Hi Marek,
> 
> Hi Lukasz,
> 
>>> On Saturday, August 29, 2015 at 01:55:36 PM, Lukasz Majewski wrote:
>>>> On Fri, 28 Aug 2015 23:55:17 +0200
>>> 
>>> Hi!
>>> 
>>>> Marek Vasut <marex at denx.de> wrote:
>>>>> On Friday, August 28, 2015 at 03:50:20 PM, Lukasz Majewski wrote:
>>>>>> The commit: d9dbb97be0e4a550457aec5f11afefb446169c90
>>>>>> "mmc: dw_mmc: Zap endless timeout" removed endless loop waiting
>>>>>> for end of dw mmc transfer.
>>>>>> 
>>>>>> For some workloads - dfu test @ Odroid XU3 (sending 8MiB file) -
>>>>>> and SD cards (e.g. MicroSD Kingston 4GiB, Adata 4GiB)
>>>>>> the default timeout is to short.
>>>>>> 
>>>>>> The new value - 20 seconds - takes into account the situation
>>>>>> when SD card triggers internal clean up. Such process may take
>>>>>> more than 10 seconds on some cards.
>>>>> 
>>>>> What happens if you pull the SD card out of the slot during such a
>>>>> process?
>>>> 
>>>> Then we would wait 20 seconds :-) to proceed.
>>> 
>>> Oops, I think I was not clear here. I was wondering what happens to
>>> the card if you yank it out of the slot whole it's performing it's
>>> internal cleanup or whatever. Is it possible that the card suffers
>>> data corruption, effectively trashing user data ?
>> 
>> I think that only the card manufacturer may reveal what can happen with
>> the card (what policy have been implemented in their FW).
>> 
>> I guess that you may lost data in such case.
> 
> Uuuurgh, that's seriously shitty.
> 

Welcome to the world of managed flash. Manufacturers tend not to follow the
spec to the letter. What matters is standard consumer usage benchmarks.

>>> Is this behavior
>>> specific to Samsung SD cards ?
>> 
>> I've experienced the problem with Kingston (brand new one) and Adata
>> MicroSD HC (4GiB) cards.
> 
> I had bad previous experience with Kingston too.
> 
>>>> To be clear - the mentioned patch introduced regression.
>>> 
>>> That's a bug, not a regression, but anyway,
>>> that's not the point. I do
>>> agree with you that we do have a problem and I'm inclined to Ack this
>>> patch, I'd like to understand what the real implications of such a
>>> behavior of these cards are.
>>> 
>>>> It works for
>>>> small files on a commonly available SD cards (like 4 GiB
>>>> Kingston/Adata).
>>>> 
>>>> When I ran DFU tests I've discovered that there is a problem with
>>>> storing 8MiB file (dat_8M.img).
>>>> 
>>>> Even worse - when one wants to store Image.itb file (which might be
>>>> 4-6 MiB) it sometimes works and sometimes not. Nightmare for
>>>> debugging.
>>> 
>>> Ew, that's one crappy card you have there. I'm reading the SD card
>>> "Physical Layer Simplified Specification Version 4.10" (part1_410.pdf)
>>> section 4.6.2.2 and it states that for SDHC cards, the write operation
>>> should take at most 250mS, for SDXC it's 500mS. Could it be that your
>>> card is violating the spec ?
>> 
>> I'll look into the spec and then comment :-).
>> 
>> For my boards the time was 1.2 seconds for storing 8 MiB file.
> 
> OK, but that's weird. The transfer should always be up to 512B long and not
> any longer, unless it's a multiblock transfer.
> 
>>>> Please correct me if I'm wrong - but is seems like we are now using
>>>> 1 second timeout for detection if SD card has been removed?
>>>> 
>>>> Shouldn't we use polling on the card detect IO pin instead? We
>>>> could add such polling in several places in the MMC subsystem (like
>>>> we do it with watchdog).
>>>> 
>>>> Marek, Pantelis, what do you think about this?
>>> 
>>> If you implement board_mmc_getcd(), you can check if the card is
>>> present this way instead of waiting for command to time out. The
>>> infrastructure for that is already in place. Right ?
>> 
>> So you suggest adding board_mmc_getcd() in several places in the mmc
>> subsystem driver to detect removal of the SD card?
> 
> Hmmmm, I'm not sure about this one. Panto ?
> 

I’m fine with this. Perhaps we can avoid spamming this all over the place,
and that would be better.

>>> It'd be cool if the MMC subsystem could pull the wp-gpios and cd-gpios
>>> from DT though :)
>> 
>> +1
>> 

Indeed. I would expect this to be a per-board thing. I would not expect
all platforms to provide that capability.

>>>>> Also, where did you find out there is such "cleanup" mechanism
>>>>> please ?
>>>> 
>>>> Internally we did some tests with several SD cards. We were stunned
>>>> when it turned out that for some workloads it took up to 15 seconds
>>>> to end write operation for small data.
>>>> 
>>>> The culprit is the SD Card embedded controller responsible for FTL -
>>>> flash translation layer.
>>>> It allows NAND memory on the card to be visible as the block device.
>>>> More importantly it also takes care of wear leveling and bad block
>>>> management.
>>>> 
>>>> Hence, we don't know when it would start housekeeping operations.
>>>> We can only poll/wait until this controller finishes it work.
>>>> The code as it was (with the indefinite loop) was taking this
>>>> situation into account.
>>>> 
>>>> The 1 second timeout is apparently too short and makes using SD card
>>>> non-deterministic and error prone in u-boot.
>>>> 
>>>> Even worse, many devices use SD card as the only storage device.
>>> 
>>> Yes, horrible.
>> 
>> Good that we have agreed.
> 
> Heh :)
> 

Horrible closed source flash management algorithm is horrible, film at 11 :)

> Best regards,
> Marek Vasut

Regards

— Pantelis