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

[Lukasz Majewski]

Hi Marek,

> On Friday, September 11, 2015 at 11:45:28 PM, Lukasz Majewski wrote:
> > Hi Alexey,
> 
> Hi,
> 
> > > Hi Marek, Lukasz,
> > > 
> > > > On Wednesday, September 09, 2015 at 09:01:30 AM, Lukasz Majewski
> > > > 
> > > > wrote:
> > > > > Hi,
> > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> > > > > > Cc: Marek Vasut <marex at denx.de>
> > > > > > Cc: Pantelis Antoniou <panto at antoniou-consulting.com>
> > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > 
> > > > > Are there any more questions regarding this patch or is it
> > > > > ready for submission as fix for v2015.10?
> > > > 
> > > > No comments, just apply this.
> > > > 
> > > > But this should really be fixed properly in the next MW.
> > > > 
> > > > Best regards,
> > > > Marek Vasut
> > > 
> > > FWIW I faced similar problem even reading data.
> > > At least on one of my boards reading of ~8Mb file
> > > took ~1.7 seconds and so 1 second timeout was interrupting data
> > > exchange.
> > 
> > Was it SD card or eMMC device?
> > 
> > > So indeed we need to have some dirty hack for upcoming release
> > > like bumping timeout to something really huge but later we
> > > need to fix that problem properly.
> > > 
> > > I though proper solution would be to set timeout depending of
> > > amount of data to be exchanged... something like take slowest
> > > speed of SD/MMC that is allowed by spec and calculate delay based
> > > on how much time it might take for that slow device and for
> > > safety multiply it by say 2.
> > 
> > As fair as I remember, card provide this kind of information. We can
> > try to investigate this possibility.
> 
> Please do.
> 
> > > Now from this thread I see that there're other reasons that might
> > > affect length of at least write operation. In other words it
> > > could be complicated unfortunately.
> > 
> > My gut feeling is that proper handling of eMMC would require quite a
> > fair mmc subsystem rework.
> 
> Can you please elaborate ?

This is only my personal guess (when taking account my previous work
done with dw_mmc on Exynos HW)
- I think Pantelis would have more knowledge to elaborate here.

> 
> > > Still we need to fix regression first with virtually infinite
> > > timeout :) I would even thing that simple revert of Marek's patch
> > > may make sense for now.
> > 
> > +1 - unfortunately there were some other patches applied to this
> > particular patch. Simple revert might be a bit tricky here.
> 
> -1 - In case the card gets removed during the DMA transfer and the
> board doesn't have a watchdog, it will get stuck indefinitelly. 

I'm just wondering here - why the indefinite loop was working
previously? Was anybody complaining (on the ML) about the problem of
removing the SD card when some operation is ongoing?

The problem with potential removal of SD card (after booting the board)
is with us for quite long time. Even with indefinite loop (without your
patch) we also could "hang" the board if the SD card was removed
during a transfer.

> We
> absolutelly don't want this sort of behavior in U-Boot. I understand
> that this is the easiest way for everyone to achieve some sort of
> "working" solution, but it is definitelly not the correct one. While
> I do agree to increasing the timeout, I do not agree to unbounded
> loops, sorry.

We have agreed to not agree :-)

> 
> > > From both points of view for keeping history
> > > clean (compared to stacked fixes/workarounds) and from removal of
> > > regression root cause.
> > 
> > As I said before - +1 from me.
> 
> As I said before, -1 from me. Btw. did anything regress in here? To
> me, this seems like a newly discovered bug ...

Yes, this is a bug. We had similar problem with Samsung's SDHCI, before
we switched to dw_mmc. This issue is new at dw_mmc.

> 
> > > It's not that I like to have infinite loops but given previous
> > > implementation worked fine for people in the previous U-Boot
> > > release.
> > 
> > Good justification
> 
> It is never a justified to return to a potentially problematic version

IMHO revering the change (before the release) is from the software
development point of view better solution than adding some
heuristic delta to timeout.

> for the sake of getting some sort of crappy hardware operational.

Unfortunately this "crappy hardware" is pervasive and we cannot do
anything about it.

To sum up (my point of view):
1. The best would be to revert the patch - but if simple "git revert" is
not working then,
2. We should increase the timeout (with my patch) for v2015.10 release
3. After release we can devise some kind of solution
4. It is a good topic for U-boot's minisummit discussion at Dublin

Marek, Alexey, Tom, Pantelis what do you think?

> 
> Best regards,
> Marek Vasut

Best regards,
Lukasz 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/20150913/4d9ee1b9/attachment.sig>