Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hamza,

On Monday 19 October 2015 22:54:53 Hamza Farooq wrote:
> On Thu, Oct 15, 2015 at 10:42 PM, Laurent Pinchart wrote:
> > On Thursday 15 October 2015 18:08:56 Hamza Farooq wrote:
> >> Hi Laurent,
> >> 
> >> Thank you for the review. Please see my comments below. Also please let
> >> me know if this code needs to be fixed on somewhat urgent basis since I
> >> am a little occupied with few other tasks. I'd try to prioritize it then
> > 
> > I don't think this is urgent, so no worries.
> > 
> >> On Thu, Oct 15, 2015 at 5:58 PM, Laurent Pinchart wrote:
> >> > On Tuesday 29 September 2015 22:44:43 hamzahfrq.sub@gmail.com wrote:
> >> >> From: Muhammad Hamza Farooq <mfarooq@visteon.com>
> >> >> 
> >> >> DMA engine does not stop instantaneously when transaction is going on
> >> >> (see datasheet). Wait has been added
> >> >> 
> >> >> Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> >> >> ---
> >> >> 
> >> >>  drivers/dma/sh/rcar-dmac.c | 28 ++++++++++++++++++++++++++--
> >> >>  1 file changed, 26 insertions(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> >> >> index 7820d07..2b28291 100644
> >> >> --- a/drivers/dma/sh/rcar-dmac.c
> >> >> +++ b/drivers/dma/sh/rcar-dmac.c
> >> >> @@ -716,14 +716,38 @@ static int rcar_dmac_fill_hwdesc(struct
> >> >> rcar_dmac_chan *chan,
> >> >> /* -------------------------------------------------------------------
> >> >>  * Stop and reset
> >> >>  */
> >> >> 
> >> >> +#define NR_READS_TO_WAIT 5 /* number of times to check if DE = 0 */
> >> >> +static inline int rcar_dmac_wait_stop(struct rcar_dmac_chan *chan)
> >> >> +{
> >> >> +     unsigned int i = 0;
> >> >> +
> >> >> +     do {
> >> >> +             u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> >> >> +
> >> >> +             if (!(chcr & RCAR_DMACHCR_DE))
> >> >> +                     return 0;
> >> >> +             cpu_relax();
> >> >> +             dev_dbg(chan->chan.device->dev, "DMA xfer couldn't be
> >> >> stopped");
> >> > 
> >> > Would it make sense to move the message out of the loop and use
> >> > dev_err() ? It seems like a pretty serious error.
> >> 
> >> It happens quite often so dev_err might be too noisy. After couple
> >> attempts (1-3) it stops so I think it is better to not make it dev_err
> > 
> > That's why I mentioned moving it outside of the loop and only printing it
> > when the DMA channel doesn't stop.
> 
> I don't think it is a good idea to notify the user again and again
> about it, specially when it is handled by this patch. It occurs quite
> a lot (for me, a lot means around 10 times for a data transfer of 7 MB
> over sh-sci at 150 kbps)

I think there's some miscommunication going on. What I meant is

#define NR_READS_TO_WAIT 5 /* number of times to check if DE = 0 */

static inline int rcar_dmac_wait_stop(struct rcar_dmac_chan *chan)
{
        unsigned int i;

        for (i = 0; i < NR_READS_TO_WAIT; ++i) {
                u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);

                if (!(chcr & RCAR_DMACHCR_DE))
                        return 0;
                cpu_relax();
        }

        dev_err(chan->chan.device->dev, "DMA xfer couldn't be stopped");
        return -EBUSY;
}

-- 
Regards,

Laurent Pinchart