On Thu, Feb 16, 2017 at 09:28:24AM +0100, Ulf Hansson wrote: > On 15 February 2017 at 19:05, Wolfram Sang > <[email protected]> wrote: > > The current code assumes that DMA is finished before SD access end is > > flagged. Thus, it schedules the 'dma_complete' tasklet in the SD card > > interrupt routine when DATAEND is set. The assumption is not safe, > > though. Even by mounting an SD card, it can be seen that sometimes DMA > > complete is first, sometimes DATAEND. It seems they are usually close > > enough timewise to not cause problems. However, a customer reported that > > with CMD53 sometimes things really break apart. As a result, the BSP has > > a patch which introduces flags for both events and makes sure both flags > > are set before scheduling the tasklet. The customer accepted the patch, > > yet it doesn't seem a proper upstream solution to me. > > > > This patch refactors the code to replace the tasklet with already > > existing and more lightweight mechanisms. First of all, we set the > > callback in a DMA descriptor to automatically get notified when DMA is > > done. In the callback, we then use a completion to make sure the SD > > access has already ended. Then, we proceed as before. > > I have similar experience and a HW behaviour. Your reasoning seems > correct to me.
Great. Thanks for the review!
> > So, calling for early review here. And opinions how to proceed. I am not
> > sure
> > we want this in renesas-drivers until the SDIO functionality has been
> > verified?
> > Because it is the reason this patch exists in the first place :)
>
> If there are no regressions, we could consider this as a nice
> clean-up, instead of waiting for the dependencies to be resolved.
Fine with me. I feel much more comfortable with your experience backing
up ours.
>
> Did you run some performance test?
The throughput numbers of 'dd' are exactly the same. But maybe I could
run one or two more tests with specifically testing that.
> > index 2b349d48fb9a8a..891d400d2a7cf2 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -137,7 +137,7 @@ struct tmio_mmc_host {
> > bool force_pio;
> > struct dma_chan *chan_rx;
> > struct dma_chan *chan_tx;
> > - struct tasklet_struct dma_complete;
> > + struct completion dma_dataend;
> > struct tasklet_struct dma_issue;
>
> The next step would be to convert the driver to a use threaded IRQ
> handler in favour of the dma_issue tasklet. That should also work,
> right!? :-)
I'll check. I also identified this tasklet as the next target but I
haven't actually looked into it yet.
Thanks for looking into it right away,
Wolfram
signature.asc
Description: PGP signature
