* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
@ 2015-10-12 14:56 ` Vinod Koul
2015-10-15 15:58 ` Laurent Pinchart
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2015-10-12 14:56 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
2015-10-12 14:56 ` Vinod Koul
@ 2015-10-15 15:58 ` Laurent Pinchart
2015-10-15 16:02 ` Hamza Farooq
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2015-10-15 15:58 UTC (permalink / raw)
To: linux-sh
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)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
2015-10-12 14:56 ` Vinod Koul
2015-10-15 15:58 ` Laurent Pinchart
@ 2015-10-15 16:02 ` Hamza Farooq
2015-10-15 16:08 ` Hamza Farooq
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Hamza Farooq @ 2015-10-15 16:02 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (2 preceding siblings ...)
2015-10-15 16:02 ` Hamza Farooq
@ 2015-10-15 16:08 ` Hamza Farooq
2015-10-15 20:42 ` Laurent Pinchart
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Hamza Farooq @ 2015-10-15 16:08 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (3 preceding siblings ...)
2015-10-15 16:08 ` Hamza Farooq
@ 2015-10-15 20:42 ` Laurent Pinchart
2015-10-19 20:54 ` Hamza Farooq
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2015-10-15 20:42 UTC (permalink / raw)
To: linux-sh
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)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (4 preceding siblings ...)
2015-10-15 20:42 ` Laurent Pinchart
@ 2015-10-19 20:54 ` Hamza Farooq
2015-10-19 21:06 ` Laurent Pinchart
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Hamza Farooq @ 2015-10-19 20:54 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (5 preceding siblings ...)
2015-10-19 20:54 ` Hamza Farooq
@ 2015-10-19 21:06 ` Laurent Pinchart
2015-10-20 12:30 ` Hamza Farooq
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2015-10-19 21:06 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (6 preceding siblings ...)
2015-10-19 21:06 ` Laurent Pinchart
@ 2015-10-20 12:30 ` Hamza Farooq
2016-06-08 10:05 ` Kuninori Morimoto
2016-06-08 14:23 ` Laurent Pinchart
9 siblings, 0 replies; 11+ messages in thread
From: Hamza Farooq @ 2015-10-20 12:30 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (7 preceding siblings ...)
2015-10-20 12:30 ` Hamza Farooq
@ 2016-06-08 10:05 ` Kuninori Morimoto
2016-06-08 14:23 ` Laurent Pinchart
9 siblings, 0 replies; 11+ messages in thread
From: Kuninori Morimoto @ 2016-06-08 10:05 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-29 20:44 [PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (8 preceding siblings ...)
2016-06-08 10:05 ` Kuninori Morimoto
@ 2016-06-08 14:23 ` Laurent Pinchart
9 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2016-06-08 14:23 UTC (permalink / raw)
To: linux-sh
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.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread