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

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

[hamzahfrq.sub]

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 | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 7820d07..b5b5e89 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_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)
-- 
1.9.1

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

[Sergei Shtylyov]

Hello.

On 9/20/2015 10:10 PM, 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 | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 7820d07..b5b5e89 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 */

    Parens not needed. (I'm seeing this patchset for the first time.)

> +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);

    Hm, this can be a *for* loop, no?

[...]
> +/* 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_DMACHCR_TE | RCAR_DMACHCR_DE);

    Please don't change the existing indentation, it doesn't seem like you're 
changing anything else here.

[...]

MBR, Sergei

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

[Hamza Farooq]

Hi,

> > +#define NR_READS_TO_WAIT (5) /* number of times to check if DE = 0 */
>
>    Parens not needed. (I'm seeing this patchset for the first time.)
They prevent funny things from happening so I use them all the time.
First time when I sent the patch-set, I had missed linux-sh mailing
list.
Here's the link from dmaengine mailing list in case you're interested
http://www.spinics.net/lists/dmaengine/msg06103.html

> > +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);
>
>    Hm, this can be a *for* loop, no?
Could you explain why would you prefer a for loop here? do..while
makes sure even if NR_READS_TO_WAIT is set to 0, the stop action takes
place

>  Please don't change the existing indentation, it doesn't seem like you're changing anything else here
ok. How should I send the updates patch though, through replying to
this email or a new thread?

Thanks!
Hamza

On Mon, Sep 21, 2015 at 10:59 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 9/20/2015 10:10 PM, 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 | 30 +++++++++++++++++++++++++++---
>>   1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
>> index 7820d07..b5b5e89 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 */
>
>
>    Parens not needed. (I'm seeing this patchset for the first time.)
>
>> +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);
>
>
>    Hm, this can be a *for* loop, no?
>
> [...]
>>
>> +/* 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_DMACHCR_TE | RCAR_DMACHCR_DE);
>
>
>    Please don't change the existing indentation, it doesn't seem like you're
> changing anything else here.
>
> [...]
>
> MBR, Sergei
>

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

[Vinod Koul]

On Mon, Sep 21, 2015 at 01:30:05PM +0200, Hamza Farooq wrote:
> ok. How should I send the updates patch though, through replying to
> this email or a new thread?
V2 patch series addressing all comments, sent using git send email would be
preferred

-- 
~Vinod

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

[Sergei Shtylyov]

Hello.

On 09/21/2015 02:30 PM, Hamza Farooq wrote:

>>> +#define NR_READS_TO_WAIT (5) /* number of times to check if DE = 0 */
>>
>>     Parens not needed. (I'm seeing this patchset for the first time.)

> They prevent funny things from happening so I use them all the time.

    No, they don't in this case.

> First time when I sent the patch-set, I had missed linux-sh mailing
> list.
> Here's the link from dmaengine mailing list in case you're interested
> http://www.spinics.net/lists/dmaengine/msg06103.html

    Thanks for the link.

>>> +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);
>>
>>     Hm, this can be a *for* loop, no?

> Could you explain why would you prefer a for loop here? do..while
> makes sure even if NR_READS_TO_WAIT is set to 0, the stop action takes
> place

    Hm, this doesn't make sense to me. If # of reads is 0, we shouldn't read, no?

>>   Please don't change the existing indentation, it doesn't seem like you're changing anything else here

> ok. How should I send the updates patch though, through replying to
> this email or a new thread?

    New thread I think.

> Thanks!
> Hamza

MBR, Sergei

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

[Hamza Farooq]

Hello,

From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]

>>>> +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);
>>>
>>>     Hm, this can be a *for* loop, no?
>
>> Could you explain why would you prefer a for loop here? do..while
>> makes sure even if NR_READS_TO_WAIT is set to 0, the stop action takes
>> place
>
>    Hm, this doesn't make sense to me. If # of reads is 0, we shouldn't read, no?
The datasheet states that we have to wait after a write. I have used
do..while to impose this check

Best,
Hamza



On Mon, Sep 21, 2015 at 7:39 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 09/21/2015 02:30 PM, Hamza Farooq wrote:
>
>>>> +#define NR_READS_TO_WAIT (5) /* number of times to check if DE = 0 */
>>>
>>>
>>>     Parens not needed. (I'm seeing this patchset for the first time.)
>
>
>> They prevent funny things from happening so I use them all the time.
>
>
>    No, they don't in this case.
>
>> First time when I sent the patch-set, I had missed linux-sh mailing
>> list.
>> Here's the link from dmaengine mailing list in case you're interested
>> http://www.spinics.net/lists/dmaengine/msg06103.html
>
>
>    Thanks for the link.
>
>>>> +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);
>>>
>>>
>>>     Hm, this can be a *for* loop, no?
>
>
>> Could you explain why would you prefer a for loop here? do..while
>> makes sure even if NR_READS_TO_WAIT is set to 0, the stop action takes
>> place
>
>
>    Hm, this doesn't make sense to me. If # of reads is 0, we shouldn't read,
> no?
>
>>>   Please don't change the existing indentation, it doesn't seem like
>>> you're changing anything else here
>
>
>> ok. How should I send the updates patch though, through replying to
>> this email or a new thread?
>
>
>    New thread I think.
>
>> Thanks!
>> Hamza
>
>
> MBR, Sergei
>