* Re: [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-20 19:10 [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
@ 2015-09-21 8:59 ` Sergei Shtylyov
2015-09-21 11:30 ` Hamza Farooq
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2015-09-21 8:59 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-20 19:10 [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
2015-09-21 8:59 ` Sergei Shtylyov
@ 2015-09-21 11:30 ` Hamza Farooq
2015-09-21 15:24 ` Vinod Koul
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Hamza Farooq @ 2015-09-21 11:30 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-20 19:10 [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
2015-09-21 8:59 ` Sergei Shtylyov
2015-09-21 11:30 ` Hamza Farooq
@ 2015-09-21 15:24 ` Vinod Koul
2015-09-21 17:39 ` Sergei Shtylyov
2015-09-22 7:41 ` Hamza Farooq
4 siblings, 0 replies; 6+ messages in thread
From: Vinod Koul @ 2015-09-21 15:24 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-20 19:10 [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (2 preceding siblings ...)
2015-09-21 15:24 ` Vinod Koul
@ 2015-09-21 17:39 ` Sergei Shtylyov
2015-09-22 7:41 ` Hamza Farooq
4 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2015-09-21 17:39 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine
2015-09-20 19:10 [RESEND PATCH 1/6] dma: rcar-dma: add wait after stopping dma engine hamzahfrq.sub
` (3 preceding siblings ...)
2015-09-21 17:39 ` Sergei Shtylyov
@ 2015-09-22 7:41 ` Hamza Farooq
4 siblings, 0 replies; 6+ messages in thread
From: Hamza Farooq @ 2015-09-22 7:41 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread