From: Lukasz Majewski <l.majewski@samsung.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 1/2] mmc: dw_mmc: Increase timeout to 20 seconds
Date: Tue, 01 Sep 2015 13:19:09 +0200 [thread overview]
Message-ID: <20150901131909.59df773d@amdc2363> (raw)
In-Reply-To: <201508292119.11094.marex@denx.de>
Hi Marek,
> 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@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 ?
The "timeout" error is for situation when you issue write command
(either single or multiple block) and you don't receive any response
from the card.
In our case we use multiblock transfer (CMD25) with either set number
of block to transfer (CMD23) or explicit end of transmission (CMD12).
Let's consider the second case.
We setup data and issue CMD25. Then we check the CMD25 status (if we
don't receive reply in 250 ms we would get timeout error).
Afterwards data from buffer is transmitted to the card and flashed.
This operation may take long time. During this process you can issue
CMD13 (SEND_STATUS) to receive information about card state ([*] 4.10. -
page 85).
Two notable fields of [*] to check are READY_FOR_DATA and CURRENT_STATE.
Those two state what is the SD Card controller situation.
Then, you end the transfer with CMD12, which also provides some status
information from the SDCard (like "prg"|"rcv", etc).
If you want to issue next command you must check READY_FOR_DATA and
CURRENT_STATE. In the case of internal SD card controller operation you
would not get READY_FOR_DATA until it ends.
> >
> > 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.
It is a multi block 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
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
next prev parent reply other threads:[~2015-09-01 11:19 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-28 13:50 [U-Boot] [PATCH 1/2] mmc: dw_mmc: Increase timeout to 20 seconds Lukasz Majewski
2015-08-28 13:50 ` [U-Boot] [PATCH 2/2] mmc: dw_mmc: Make timeout error visible to u-boot console Lukasz Majewski
2015-08-28 23:21 ` Simon Glass
2015-08-29 12:09 ` Lukasz Majewski
2015-08-29 15:07 ` Simon Glass
2015-09-03 12:33 ` Lukasz Majewski
2015-09-03 12:21 ` [U-Boot] [PATCH] FIX: fat: Provide correct return code from disk_{read|write} to upper layers Lukasz Majewski
2015-09-03 12:44 ` Tom Rini
2015-09-03 13:40 ` Lukasz Majewski
2015-09-03 14:18 ` Lukasz Majewski
2015-09-23 3:17 ` Stephen Warren
2015-09-23 8:40 ` Lukasz Majewski
2015-09-25 5:47 ` Stephen Warren
2015-09-09 7:02 ` Lukasz Majewski
2015-09-17 14:44 ` Lukasz Majewski
2015-09-12 12:51 ` [U-Boot] " Tom Rini
2015-08-28 21:55 ` [U-Boot] [PATCH 1/2] mmc: dw_mmc: Increase timeout to 20 seconds Marek Vasut
2015-08-29 11:55 ` Lukasz Majewski
2015-08-29 13:52 ` Marek Vasut
2015-08-29 16:38 ` Lukasz Majewski
2015-08-29 19:19 ` Marek Vasut
2015-09-01 11:19 ` Lukasz Majewski [this message]
2015-09-01 11:33 ` Marek Vasut
2015-09-01 15:25 ` Lukasz Majewski
2015-09-01 15:35 ` Marek Vasut
2015-09-01 16:22 ` Pantelis Antoniou
2015-09-02 8:06 ` Marek Vasut
2015-09-09 7:01 ` Lukasz Majewski
2015-09-09 11:34 ` Marek Vasut
2015-09-11 17:20 ` Alexey Brodkin
2015-09-11 21:45 ` Lukasz Majewski
2015-09-12 16:13 ` Marek Vasut
2015-09-13 10:03 ` Lukasz Majewski
2015-09-13 14:00 ` Marek Vasut
2015-09-14 10:15 ` Alexey Brodkin
2015-09-14 11:22 ` Lukasz Majewski
2015-09-14 13:36 ` Marek Vasut
2015-09-17 14:43 ` Lukasz Majewski
2015-09-18 0:31 ` Tom Rini
2015-09-18 7:32 ` Lukasz Majewski
2015-09-18 8:07 ` Przemyslaw Marczak
2015-09-18 19:27 ` Tom Rini
2015-09-21 15:32 ` Pantelis Antoniou
2015-09-14 10:30 ` Alexey Brodkin
2015-09-14 11:15 ` Przemyslaw Marczak
2015-09-14 10:33 ` Przemyslaw Marczak
2015-09-25 16:25 ` [U-Boot] [PATCH] mmc: dw_mmc: Increase timeout to 4 minutes (as in Linux kernel) Lukasz Majewski
2015-09-28 13:43 ` Przemyslaw Marczak
2015-09-28 21:08 ` Tom Rini
2015-09-28 21:08 ` [U-Boot] " Tom Rini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150901131909.59df773d@amdc2363 \
--to=l.majewski@samsung.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.