Re: [RFC] mmc: host: tmio: ensure end of DMA and SD access are in sync

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]

On Thu, Feb 16, 2017 at 09:28:24AM +0100, Ulf Hansson wrote:
> On 15 February 2017 at 19:05, Wolfram Sang
> <wsa+renesas@sang-engineering.com> 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]