| Message ID | 1443559488-2416-2-git-send-email-hamzahfrq.sub@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Sep 29, 2015 at 10:44:43PM +0200, 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 Hmmm, why is threading broken in this series? Also why is this PATCH 1/6 whereas the rest of series is v3! Right way is to let git do this for you and use git format-patch --subject-prefix="PATCH v3"... You don't do these things manually, that would be terrible waste of time Also use git send-email to keep threading > 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"); And user has no way of knowing this on production system > + } while (++i < NR_READS_TO_WAIT); > > -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) > + return -EBUSY; > +} > + > +/* Called with chan lock held */ > +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) > { > - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); > + u32 chcr; > + int ret; > > + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); > chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | > RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); > rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); > + ret = rcar_dmac_wait_stop(chan); > + > + WARN_ON(ret < 0); What if we have broken board and will see dmesg storm for this, perhaps WARN_ON_ONCE and also would be helpful to use the verbose version and print some meaningful message
Hello Muhammad, Thank you for the patch. 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. > + } while (++i < NR_READS_TO_WAIT); How long does the DMA engine typically need to stop ? Is there a safe upper bound ? > -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) > + return -EBUSY; > +} > + > +/* Called with chan lock held */ > +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) > { > - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); > + u32 chcr; > + int ret; > > + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); > chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | > RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); > rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); > + ret = rcar_dmac_wait_stop(chan); As the rcar_dmac_wait_stop() function is used here only I'd inline the code directly. > + > + WARN_ON(ret < 0); If you use dev_err() instead of dev_dbg() above you could remove the WARN_on. > + > + return ret; > } > > static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)
Hi Vinod, > Hmmm, why is threading broken in this series? > Also why is this PATCH 1/6 whereas the rest of series is v3! > > Right way is to let git do this for you and use git format-patch > --subject-prefix="PATCH v3"... > > You don't do these things manually, that would be terrible waste of time > > Also use git send-email to keep threading I will keep that in mind >> +#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"); > > And user has no way of knowing this on production system What do you suggest here instead? >> +/* Called with chan lock held */ >> +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> { >> - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> + u32 chcr; >> + int ret; >> >> + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | >> RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); >> rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); >> + ret = rcar_dmac_wait_stop(chan); >> + >> + WARN_ON(ret < 0); > > What if we have broken board and will see dmesg storm for this, perhaps > WARN_ON_ONCE and also would be helpful to use the verbose version and print > some meaningful message Yes that does make sense. I shall update it in next version Thanks for the comments Best, Hamza On Mon, Oct 12, 2015 at 4:59 PM, Vinod Koul <vinod.koul@intel.com> wrote: > On Tue, Sep 29, 2015 at 10:44:43PM +0200, 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 > > Hmmm, why is threading broken in this series? > Also why is this PATCH 1/6 whereas the rest of series is v3! > > Right way is to let git do this for you and use git format-patch > --subject-prefix="PATCH v3"... > > You don't do these things manually, that would be terrible waste of time > > Also use git send-email to keep threading > >> 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"); > > And user has no way of knowing this on production system > >> + } while (++i < NR_READS_TO_WAIT); >> >> -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> + return -EBUSY; >> +} >> + >> +/* Called with chan lock held */ >> +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> { >> - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> + u32 chcr; >> + int ret; >> >> + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | >> RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); >> rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); >> + ret = rcar_dmac_wait_stop(chan); >> + >> + WARN_ON(ret < 0); > > What if we have broken board and will see dmesg storm for this, perhaps > WARN_ON_ONCE and also would be helpful to use the verbose version and print > some meaningful message > > -- > ~Vinod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 Best regards, Hamza On Thu, Oct 15, 2015 at 5:58 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hello Muhammad, > > Thank you for the patch. > > 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 > >> + } while (++i < NR_READS_TO_WAIT); > > How long does the DMA engine typically need to stop ? Is there a safe upper > bound ? I have tested it many times. The highest number of retries was 3 which occurred not very frequently. Normally it is 1-2 > >> -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> + return -EBUSY; >> +} >> + >> +/* Called with chan lock held */ >> +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> { >> - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> + u32 chcr; >> + int ret; >> >> + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | >> RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); >> rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); >> + ret = rcar_dmac_wait_stop(chan); > > As the rcar_dmac_wait_stop() function is used here only I'd inline the code > directly. It is used in DMA_PAUSE operation as well > >> + >> + WARN_ON(ret < 0); > > If you use dev_err() instead of dev_dbg() above you could remove the WARN_on. see comment about dev_err above > >> + >> + return ret; >> } >> >> static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan) > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Hamza, (With a question for Morimoto-san below) 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. > >> + } while (++i < NR_READS_TO_WAIT); > > > > How long does the DMA engine typically need to stop ? Is there a safe > > upper bound ? > > I have tested it many times. The highest number of retries was 3 which > occurred not very frequently. Normally it is 1-2 Morimoto-san, could you check with the hardware team if there's a worst case guarantee regarding how long the channel could take to stop ? > >> -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) > >> + return -EBUSY; > >> +} > >> + > >> +/* Called with chan lock held */ > >> +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) > >> { > >> - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); > >> + u32 chcr; > >> + int ret; > >> > >> + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); > >> chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | > >> RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); > >> rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); > >> + ret = rcar_dmac_wait_stop(chan); > > > > As the rcar_dmac_wait_stop() function is used here only I'd inline the > > code directly. > > It is used in DMA_PAUSE operation as well Let's resolve the DMA pause issue and then decide :-) > >> + > >> + WARN_ON(ret < 0); > > > > If you use dev_err() instead of dev_dbg() above you could remove the > > WARN_on. > > see comment about dev_err above > > >> + > >> + return ret; > >> } > >> > >> static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)
Hi Laurent, On Thu, Oct 15, 2015 at 10:42 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hello Hamza, > > (With a question for Morimoto-san below) > > 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) > >> >> + } while (++i < NR_READS_TO_WAIT); >> > >> > How long does the DMA engine typically need to stop ? Is there a safe >> > upper bound ? >> >> I have tested it many times. The highest number of retries was 3 which >> occurred not very frequently. Normally it is 1-2 > > Morimoto-san, could you check with the hardware team if there's a worst case > guarantee regarding how long the channel could take to stop ? > >> >> -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> >> + return -EBUSY; >> >> +} >> >> + >> >> +/* Called with chan lock held */ >> >> +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) >> >> { >> >> - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> >> + u32 chcr; >> >> + int ret; >> >> >> >> + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); >> >> chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | >> >> RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); >> >> rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); >> >> + ret = rcar_dmac_wait_stop(chan); >> > >> > As the rcar_dmac_wait_stop() function is used here only I'd inline the >> > code directly. >> >> It is used in DMA_PAUSE operation as well > > Let's resolve the DMA pause issue and then decide :-) > >> >> + >> >> + WARN_ON(ret < 0); >> > >> > If you use dev_err() instead of dev_dbg() above you could remove the >> > WARN_on. >> >> see comment about dev_err above >> >> >> + >> >> + return ret; >> >> } >> >> >> >> static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan) > > -- > Regards, > > Laurent Pinchart > Best, Hamza -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
Hi Laurent, On Mon, Oct 19, 2015 at 11:06 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > 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; > } ah ok, I understood it as displaying error every time it does not stop. Yea this makes sense. As a DMA engine user, I would like to see the information in at least debug output though, that DMA did not stop in first attempt. So what do you say about keeping the dev_dbg message in the loop and adding your line (dev_err) outside? > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent # add Niklas Sorry for my suport late response. Thanks Niklas for reminder > (With a question for Morimoto-san below) (snip) > > >> + } while (++i < NR_READS_TO_WAIT); > > > > > > How long does the DMA engine typically need to stop ? Is there a safe > > > upper bound ? > > > > I have tested it many times. The highest number of retries was 3 which > > occurred not very frequently. Normally it is 1-2 > > Morimoto-san, could you check with the hardware team if there's a worst case > guarantee regarding how long the channel could take to stop ? You need to wait maximum 0.7ms for it. both for Gen2/Gen3. In worst case, it needs 8 request times. If 1 request is 20,000 cycle 20,000cycle x 8request x [4ns/cylce] = 640,000ns > 700us -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Morimoto-san, On Wednesday 08 Jun 2016 10:05:09 Kuninori Morimoto wrote: > Hi Laurent > # add Niklas > > Sorry for my suport late response. > Thanks Niklas for reminder > > > (With a question for Morimoto-san below) > > (snip) > > > > >> + } while (++i < NR_READS_TO_WAIT); > > > > > > > > How long does the DMA engine typically need to stop ? Is there a safe > > > > upper bound ? > > > > > > I have tested it many times. The highest number of retries was 3 which > > > occurred not very frequently. Normally it is 1-2 > > > > Morimoto-san, could you check with the hardware team if there's a worst > > case guarantee regarding how long the channel could take to stop ? > > You need to wait maximum 0.7ms for it. > both for Gen2/Gen3. > In worst case, it needs 8 request times. > If 1 request is 20,000 cycle > > 20,000cycle x 8request x [4ns/cylce] = 640,000ns > 700us And we're calling the wait function both from interrupt context and from user context with a spinlock held. I'm afraid that's not acceptable, we can't busy- loop for so long with interrupts disabled.
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"); + } while (++i < NR_READS_TO_WAIT); -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) + return -EBUSY; +} + +/* Called with chan lock held */ +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan) { - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); + u32 chcr; + int ret; + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE | RCAR_DMACHCR_TE | RCAR_DMACHCR_DE); rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr); + ret = rcar_dmac_wait_stop(chan); + + WARN_ON(ret < 0); + + return ret; } static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)