diff mbox

[v3,2/6] dma: rcar-dma: Added dma_pause operation to rcar_dma driver

Message ID 1443559488-2416-3-git-send-email-hamzahfrq.sub@gmail.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

hamzahfrq.sub@gmail.com Sept. 29, 2015, 8:44 p.m. UTC
From: Muhammad Hamza Farooq <mfarooq@visteon.com>

Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
---
 drivers/dma/sh/rcar-dmac.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Laurent Pinchart Oct. 15, 2015, 3:18 p.m. UTC | #1
Hi Muhammad,

Thank you for the patch.

On Tuesday 29 September 2015 22:44:44 hamzahfrq.sub@gmail.com wrote:
> From: Muhammad Hamza Farooq <mfarooq@visteon.com>
> 
> Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> ---
>  drivers/dma/sh/rcar-dmac.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2b28291..c7cd4ed 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -774,6 +774,26 @@ static void rcar_dmac_chan_reinit(struct rcar_dmac_chan
> *chan) }
>  }
> 
> +static int rcar_dmac_chan_pause(struct dma_chan *chan)
> +{
> +	u32 chcr;
> +	int ret;
> +	unsigned long flags;
> +	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
> +
> +	spin_lock_irqsave(&rchan->lock, flags);
> +
> +	chcr = rcar_dmac_chan_read(rchan, RCAR_DMACHCR);
> +	chcr &= ~RCAR_DMACHCR_DE;
> +	rcar_dmac_chan_write(rchan, RCAR_DMACHCR, chcr);
> +	ret = rcar_dmac_wait_stop(rchan);

The DMA engine API documents the pause operation as stopping DMA transfers in 
a resumable way without data loss. Doesn't stopping the transfer forcefully 
like this result in incomplete transfers and thus data loss ? Shouldn't we 
instead signal the pause request to the interrupt handler to avoid starting 
the next transfer and wait for the end of the current transfer ?

> +
> +	spin_unlock_irqrestore(&rchan->lock, flags);
> +
> +	WARN_ON(ret < 0);
> +	return ret;
> +}
> +
>  static void rcar_dmac_stop(struct rcar_dmac *dmac)
>  {
>  	rcar_dmac_write(dmac, RCAR_DMAOR, 0);
> @@ -1740,6 +1760,7 @@ static int rcar_dmac_probe(struct platform_device
> *pdev) engine->device_prep_slave_sg = rcar_dmac_prep_slave_sg;
>  	engine->device_prep_dma_cyclic = rcar_dmac_prep_dma_cyclic;
>  	engine->device_config = rcar_dmac_device_config;
> +	engine->device_pause = rcar_dmac_chan_pause;
>  	engine->device_terminate_all = rcar_dmac_chan_terminate_all;
>  	engine->device_tx_status = rcar_dmac_tx_status;
>  	engine->device_issue_pending = rcar_dmac_issue_pending;
hamzahfrq.sub@gmail.com Oct. 19, 2015, 9:16 p.m. UTC | #2
Hi Laurent, Vinod,


On Thu, Oct 15, 2015 at 5:18 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Muhammad,
>
> Thank you for the patch.
>
> On Tuesday 29 September 2015 22:44:44 hamzahfrq.sub@gmail.com wrote:
>> From: Muhammad Hamza Farooq <mfarooq@visteon.com>
>>
>> Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
>> ---
>>  drivers/dma/sh/rcar-dmac.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
>> index 2b28291..c7cd4ed 100644
>> --- a/drivers/dma/sh/rcar-dmac.c
>> +++ b/drivers/dma/sh/rcar-dmac.c
>> @@ -774,6 +774,26 @@ static void rcar_dmac_chan_reinit(struct rcar_dmac_chan
>> *chan) }
>>  }
>>
>> +static int rcar_dmac_chan_pause(struct dma_chan *chan)
>> +{
>> +     u32 chcr;
>> +     int ret;
>> +     unsigned long flags;
>> +     struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
>> +
>> +     spin_lock_irqsave(&rchan->lock, flags);
>> +
>> +     chcr = rcar_dmac_chan_read(rchan, RCAR_DMACHCR);
>> +     chcr &= ~RCAR_DMACHCR_DE;
>> +     rcar_dmac_chan_write(rchan, RCAR_DMACHCR, chcr);
>> +     ret = rcar_dmac_wait_stop(rchan);
>
> The DMA engine API documents the pause operation as stopping DMA transfers in
> a resumable way without data loss. Doesn't stopping the transfer forcefully
> like this result in incomplete transfers and thus data loss ? Shouldn't we
> instead signal the pause request to the interrupt handler to avoid starting
> the next transfer and wait for the end of the current transfer ?

This fix is invalid if pausing the DMA engine midway through a
transfer is not allowed. If it is allowed, I'm not sure if it would
cause data loss. Additionally, if we signal the interrupt to pause the
DMA transfer, we cannot return true status of dma_pause operation
through this function call as it is a non-blocking function.

I think more research is needed in this direction. Perhaps Vinod can help

>
>> +
>> +     spin_unlock_irqrestore(&rchan->lock, flags);
>> +
>> +     WARN_ON(ret < 0);
>> +     return ret;
>> +}
>> +
>>  static void rcar_dmac_stop(struct rcar_dmac *dmac)
>>  {
>>       rcar_dmac_write(dmac, RCAR_DMAOR, 0);
>> @@ -1740,6 +1760,7 @@ static int rcar_dmac_probe(struct platform_device
>> *pdev) engine->device_prep_slave_sg = rcar_dmac_prep_slave_sg;
>>       engine->device_prep_dma_cyclic = rcar_dmac_prep_dma_cyclic;
>>       engine->device_config = rcar_dmac_device_config;
>> +     engine->device_pause = rcar_dmac_chan_pause;
>>       engine->device_terminate_all = rcar_dmac_chan_terminate_all;
>>       engine->device_tx_status = rcar_dmac_tx_status;
>>       engine->device_issue_pending = rcar_dmac_issue_pending;
>
> --
> 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
diff mbox

Patch

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 2b28291..c7cd4ed 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -774,6 +774,26 @@  static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)
 	}
 }
 
+static int rcar_dmac_chan_pause(struct dma_chan *chan)
+{
+	u32 chcr;
+	int ret;
+	unsigned long flags;
+	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
+
+	spin_lock_irqsave(&rchan->lock, flags);
+
+	chcr = rcar_dmac_chan_read(rchan, RCAR_DMACHCR);
+	chcr &= ~RCAR_DMACHCR_DE;
+	rcar_dmac_chan_write(rchan, RCAR_DMACHCR, chcr);
+	ret = rcar_dmac_wait_stop(rchan);
+
+	spin_unlock_irqrestore(&rchan->lock, flags);
+
+	WARN_ON(ret < 0);
+	return ret;
+}
+
 static void rcar_dmac_stop(struct rcar_dmac *dmac)
 {
 	rcar_dmac_write(dmac, RCAR_DMAOR, 0);
@@ -1740,6 +1760,7 @@  static int rcar_dmac_probe(struct platform_device *pdev)
 	engine->device_prep_slave_sg = rcar_dmac_prep_slave_sg;
 	engine->device_prep_dma_cyclic = rcar_dmac_prep_dma_cyclic;
 	engine->device_config = rcar_dmac_device_config;
+	engine->device_pause = rcar_dmac_chan_pause;
 	engine->device_terminate_all = rcar_dmac_chan_terminate_all;
 	engine->device_tx_status = rcar_dmac_tx_status;
 	engine->device_issue_pending = rcar_dmac_issue_pending;