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

[Marek Vasut]

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.

> > 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 ?

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

Best regards,
Marek Vasut