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

[Marek Vasut]

On Sunday, September 13, 2015 at 12:03:18 PM, Lukasz Majewski wrote:
> Hi Marek,

Hi,

[...]

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

OK

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

It worked for me for all the workloads I used. Noone was complaining.

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

Which is why we should weed out the unbounded loops.

> > 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 :-)

Yes :-)

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

Let's do this for the sake of crappy cards.

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

I think yes.