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

[Lukasz Majewski <l.majewski@majess.pl>]

Hi Marek,

> 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@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.

> Is this behavior
> specific to Samsung SD cards ?

I've experienced the problem with Kingston (brand new one) and Adata
MicroSD HC (4GiB) cards.

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

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

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

> 
> Best regards,
> Marek Vasut

Best regards,

?ukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150829/2b634bd5/attachment.sig>