[PATCH] dma: mmp_pdma: support for getting residual bytes

[PATCH] dma: mmp_pdma: support for getting residual bytes

[Xiang Wang]

From: Xiang Wang <wangx@marvell.com>

In some of our drivers (e.g. UART) we may stop a running DMA
before it finishes. So we need to know how many bytes have
been transferred.

Signed-off-by: Xiang Wang <wangx@marvell.com>
---
 drivers/dma/mmp_pdma.c |   77 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 76a8dcf..0c623cc 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -104,7 +104,8 @@ struct mmp_pdma_chan {
 	spinlock_t desc_lock;		/* Descriptor list lock */
 	struct list_head chain_pending;	/* Link descriptors queue for pending */
 	struct list_head chain_running;	/* Link descriptors queue for running */
-	bool idle;			/* channel statue machine */
+	enum dma_status status; /* channel status machine */
+	u32 bytes_residue;
 
 	struct dma_pool *desc_pool;	/* Descriptors pool */
 };
@@ -270,7 +271,7 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
 	struct mmp_pdma_desc_sw *desc;
 
 	/* still in running, irq will start the pending list */
-	if (!chan->idle) {
+	if (chan->status == DMA_IN_PROGRESS) {
 		dev_dbg(chan->dev, "DMA controller still busy\n");
 		return;
 	}
@@ -307,7 +308,55 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
 	 */
 	set_desc(chan->phy, desc->async_tx.phys);
 	enable_chan(chan->phy);
-	chan->idle = false;
+	chan->status = DMA_IN_PROGRESS;
+	chan->bytes_residue = 0;
+}
+
+/*
+ * Get the number of bytes untransferred by DMA.
+ */
+static u32 mmp_pdma_get_bytes_residue(struct mmp_pdma_chan *chan)
+{
+	u32 reg, orig_pos, cur_pos, residue = 0;
+	struct mmp_pdma_desc_sw *desc_sw;
+	struct list_head *list_chain = NULL;
+
+	/*
+	 * When a phy channel is unavailable, maybe its destroied, return last
+	 * stored value for safe.
+	 */
+	if (!chan || !chan->phy)
+		return chan->bytes_residue;
+
+	if (!list_empty(&chan->chain_running))
+		list_chain = &chan->chain_running;
+	else
+		return 0;
+
+	desc_sw = list_first_entry(list_chain, struct mmp_pdma_desc_sw, node);
+
+	switch (chan->dir) {
+	case DMA_DEV_TO_MEM:
+		reg = (chan->phy->idx << 4) + DTADR;
+		cur_pos = readl_relaxed(chan->phy->base + reg);
+		orig_pos = desc_sw->desc.dtadr;
+		break;
+	case DMA_MEM_TO_DEV:
+		reg = (chan->phy->idx << 4) + DSADR;
+		cur_pos = readl_relaxed(chan->phy->base + reg);
+		orig_pos = desc_sw->desc.dsadr;
+		break;
+	case DMA_MEM_TO_MEM:
+		reg = (chan->phy->idx << 4) + DTADR;
+		cur_pos = readl_relaxed(chan->phy->base + reg);
+		orig_pos = desc_sw->desc.dtadr;
+		break;
+	default:
+		return 0;
+	}
+
+	residue = (u32)(desc_sw->desc.dcmd & DCMD_LENGTH) + orig_pos - cur_pos;
+	return residue;
 }
 
 
@@ -381,7 +430,7 @@ static int mmp_pdma_alloc_chan_resources(struct dma_chan *dchan)
 		chan->phy->vchan = NULL;
 		chan->phy = NULL;
 	}
-	chan->idle = true;
+	chan->status = DMA_SUCCESS;
 	chan->dev_addr = 0;
 	return 1;
 }
@@ -409,7 +458,7 @@ static void mmp_pdma_free_chan_resources(struct dma_chan *dchan)
 
 	dma_pool_destroy(chan->desc_pool);
 	chan->desc_pool = NULL;
-	chan->idle = true;
+	chan->status = DMA_SUCCESS;
 	chan->dev_addr = 0;
 	if (chan->phy) {
 		chan->phy->vchan = NULL;
@@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
 		mmp_pdma_free_desc_list(chan, &chan->chain_pending);
 		mmp_pdma_free_desc_list(chan, &chan->chain_running);
 		spin_unlock_irqrestore(&chan->desc_lock, flags);
-		chan->idle = true;
+		chan->status = DMA_SUCCESS;
+		chan->bytes_residue = 0;
+		break;
+	case DMA_PAUSE:
+		disable_chan(chan->phy);
+		chan->status = DMA_PAUSED;
+		chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
 		break;
 	case DMA_SLAVE_CONFIG:
 		if (cfg->direction == DMA_DEV_TO_MEM) {
@@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
 	unsigned long flags;
 
 	spin_lock_irqsave(&chan->desc_lock, flags);
-	ret = dma_cookie_status(dchan, cookie, txstate);
+	ret = chan->status;
+	dma_set_residue(txstate, chan->bytes_residue);
 	spin_unlock_irqrestore(&chan->desc_lock, flags);
 
 	return ret;
@@ -684,6 +740,8 @@ static void dma_do_tasklet(unsigned long data)
 		dev_dbg(chan->dev, "completed_cookie=%d\n", cookie);
 	}
 
+	/* update bytes_residue so that the user can query later on */
+	chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
 	/*
 	 * move the descriptors to a temporary list so we can drop the lock
 	 * during the entire cleanup operation
@@ -691,7 +749,7 @@ static void dma_do_tasklet(unsigned long data)
 	list_splice_tail_init(&chan->chain_running, &chain_cleanup);
 
 	/* the hardware is now idle and ready for more */
-	chan->idle = true;
+	chan->status = DMA_SUCCESS;
 
 	/* Start any pending transactions automatically */
 	start_pending_queue(chan);
@@ -750,6 +808,9 @@ static int mmp_pdma_chan_init(struct mmp_pdma_device *pdev,
 	INIT_LIST_HEAD(&chan->chain_pending);
 	INIT_LIST_HEAD(&chan->chain_running);
 
+	chan->status = DMA_SUCCESS;
+	chan->bytes_residue = 0;
+
 	/* register virt channel to dma engine */
 	list_add_tail(&chan->chan.device_node,
 			&pdev->device.channels);
-- 
1.7.5.4

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Andy Shevchenko]

On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
> In some of our drivers (e.g. UART) we may stop a running DMA
> before it finishes. So we need to know how many bytes have
> been transferred.

Couple of comments below.

> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c

> @@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
>                 mmp_pdma_free_desc_list(chan, &chan->chain_pending);
>                 mmp_pdma_free_desc_list(chan, &chan->chain_running);
>                 spin_unlock_irqrestore(&chan->desc_lock, flags);
> -               chan->idle = true;
> +               chan->status = DMA_SUCCESS;
> +               chan->bytes_residue = 0;
> +               break;
> +       case DMA_PAUSE:
> +               disable_chan(chan->phy);
> +               chan->status = DMA_PAUSED;
> +               chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);

Does it mean user has to do DMA_PAUSE first to get more or less
accurate residue?
Logically that sound correct, but in general we may allow user to get
approximate residue value of on going transfer.

>                 break;
>         case DMA_SLAVE_CONFIG:
>                 if (cfg->direction == DMA_DEV_TO_MEM) {
> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>         unsigned long flags;
>
>         spin_lock_irqsave(&chan->desc_lock, flags);
> -       ret = dma_cookie_status(dchan, cookie, txstate);
> +       ret = chan->status;
> +       dma_set_residue(txstate, chan->bytes_residue);
>         spin_unlock_irqrestore(&chan->desc_lock, flags);

Besides my patch which removes this spinlock I think the workflow
should be something like

status = dma_cookie_status()
if status == DMA_SUCCESS or !txstate:
return status

dma_set_residue()
return status

Because there is no reason to return residue of successfully finished
transfer. It should be 0.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Xiang Wang]

2013/5/31 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>> In some of our drivers (e.g. UART) we may stop a running DMA
>> before it finishes. So we need to know how many bytes have
>> been transferred.
>
> Couple of comments below.
>
>> --- a/drivers/dma/mmp_pdma.c
>> +++ b/drivers/dma/mmp_pdma.c
>
>> @@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
>>                 mmp_pdma_free_desc_list(chan, &chan->chain_pending);
>>                 mmp_pdma_free_desc_list(chan, &chan->chain_running);
>>                 spin_unlock_irqrestore(&chan->desc_lock, flags);
>> -               chan->idle = true;
>> +               chan->status = DMA_SUCCESS;
>> +               chan->bytes_residue = 0;
>> +               break;
>> +       case DMA_PAUSE:
>> +               disable_chan(chan->phy);
>> +               chan->status = DMA_PAUSED;
>> +               chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>
> Does it mean user has to do DMA_PAUSE first to get more or less
> accurate residue?
> Logically that sound correct, but in general we may allow user to get
> approximate residue value of on going transfer.
Hi, Andy
Your comment makes sense. But if the user is allowed to query the
residue value in real-time, we cannot just return a saved value to
him.
Why I use a saved value (chan->bytes_residue)?
In current mmp pdma driver, a phy channel will be freed after the
transmission finishes (chan->phy is set to NULL). So we cannot get the
physical channel information after we call DMA_TERMINATE_ALL or DMA
finishes itself.
That is to say, when the use queries the channel information at these
points, the chan->phy is usually NULL.
>
>>                 break;
>>         case DMA_SLAVE_CONFIG:
>>                 if (cfg->direction == DMA_DEV_TO_MEM) {
>> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>>         unsigned long flags;
>>
>>         spin_lock_irqsave(&chan->desc_lock, flags);
>> -       ret = dma_cookie_status(dchan, cookie, txstate);
>> +       ret = chan->status;
>> +       dma_set_residue(txstate, chan->bytes_residue);
>>         spin_unlock_irqrestore(&chan->desc_lock, flags);
>
> Besides my patch which removes this spinlock I think the workflow
> should be something like
>
> status = dma_cookie_status()
> if status == DMA_SUCCESS or !txstate:
> return status
>
> dma_set_residue()
> return status
>
> Because there is no reason to return residue of successfully finished
> transfer. It should be 0.
Hi, Andy
There is one exception from my point of view. When we are receiving
data from peripheral devices, we usually start a DMA transaction with
a target length of 4K for example. When a timed-out event occurs in
peripheral device, it will notify DMA controller and DMA controller
will send out a End of Receive interrupt (Marvell specific?).
In such situation, DMA status is also DMA_SUCCESS. But the residual
bytes is not 0 and the user must query it.
Thanks!
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Andy Shevchenko]

On Mon, Jun 3, 2013 at 6:22 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
> 2013/5/31 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>>> In some of our drivers (e.g. UART) we may stop a running DMA
>>> before it finishes. So we need to know how many bytes have
>>> been transferred.
>>
>> Couple of comments below.
>>
>>> --- a/drivers/dma/mmp_pdma.c
>>> +++ b/drivers/dma/mmp_pdma.c
>>
>>> @@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
>>>                 mmp_pdma_free_desc_list(chan, &chan->chain_pending);
>>>                 mmp_pdma_free_desc_list(chan, &chan->chain_running);
>>>                 spin_unlock_irqrestore(&chan->desc_lock, flags);
>>> -               chan->idle = true;
>>> +               chan->status = DMA_SUCCESS;
>>> +               chan->bytes_residue = 0;
>>> +               break;
>>> +       case DMA_PAUSE:
>>> +               disable_chan(chan->phy);
>>> +               chan->status = DMA_PAUSED;
>>> +               chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>>
>> Does it mean user has to do DMA_PAUSE first to get more or less
>> accurate residue?
>> Logically that sound correct, but in general we may allow user to get
>> approximate residue value of on going transfer.

> Your comment makes sense. But if the user is allowed to query the
> residue value in real-time, we cannot just return a saved value to
> him.

Right.

> Why I use a saved value (chan->bytes_residue)?
> In current mmp pdma driver, a phy channel will be freed after the
> transmission finishes (chan->phy is set to NULL). So we cannot get the
> physical channel information after we call DMA_TERMINATE_ALL or DMA
> finishes itself.

I don't see any contradiction to workflow.
So, If you call device_tx_status() when transfer is completed or
aborted you will get 0 as a residue, which is correct.

> That is to say, when the use queries the channel information at these
> points, the chan->phy is usually NULL.

>>> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>>>         unsigned long flags;
>>>
>>>         spin_lock_irqsave(&chan->desc_lock, flags);
>>> -       ret = dma_cookie_status(dchan, cookie, txstate);
>>> +       ret = chan->status;
>>> +       dma_set_residue(txstate, chan->bytes_residue);
>>>         spin_unlock_irqrestore(&chan->desc_lock, flags);
>>
>> Besides my patch which removes this spinlock I think the workflow
>> should be something like
>>
>> status = dma_cookie_status()
>> if status == DMA_SUCCESS or !txstate:
>> return status
>>
>> dma_set_residue()
>> return status
>>
>> Because there is no reason to return residue of successfully finished
>> transfer. It should be 0.

> There is one exception from my point of view. When we are receiving
> data from peripheral devices, we usually start a DMA transaction with
> a target length of 4K for example. When a timed-out event occurs in
> peripheral device, it will notify DMA controller and DMA controller
> will send out a End of Receive interrupt (Marvell specific?).

Might be your hardware specifics, in our case we have got a timeout
interrupt from uart controller.

> In such situation, DMA status is also DMA_SUCCESS. But the residual
> bytes is not 0 and the user must query it.

Which sounds wrong approach.

P.S. take a look at  drivers/tty/serial/8250/8250_dma.c

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Xiang Wang]

2013/6/3 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Mon, Jun 3, 2013 at 6:22 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>> 2013/5/31 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>> On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>>>> In some of our drivers (e.g. UART) we may stop a running DMA
>>>> before it finishes. So we need to know how many bytes have
>>>> been transferred.
>>>
>>> Couple of comments below.
>>>
>>>> --- a/drivers/dma/mmp_pdma.c
>>>> +++ b/drivers/dma/mmp_pdma.c
>>>
>>>> @@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
>>>>                 mmp_pdma_free_desc_list(chan, &chan->chain_pending);
>>>>                 mmp_pdma_free_desc_list(chan, &chan->chain_running);
>>>>                 spin_unlock_irqrestore(&chan->desc_lock, flags);
>>>> -               chan->idle = true;
>>>> +               chan->status = DMA_SUCCESS;
>>>> +               chan->bytes_residue = 0;
>>>> +               break;
>>>> +       case DMA_PAUSE:
>>>> +               disable_chan(chan->phy);
>>>> +               chan->status = DMA_PAUSED;
>>>> +               chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>>>
>>> Does it mean user has to do DMA_PAUSE first to get more or less
>>> accurate residue?
>>> Logically that sound correct, but in general we may allow user to get
>>> approximate residue value of on going transfer.
>
>> Your comment makes sense. But if the user is allowed to query the
>> residue value in real-time, we cannot just return a saved value to
>> him.
>
> Right.
>
>> Why I use a saved value (chan->bytes_residue)?
>> In current mmp pdma driver, a phy channel will be freed after the
>> transmission finishes (chan->phy is set to NULL). So we cannot get the
>> physical channel information after we call DMA_TERMINATE_ALL or DMA
>> finishes itself.
>
> I don't see any contradiction to workflow.
> So, If you call device_tx_status() when transfer is completed or
> aborted you will get 0 as a residue, which is correct.
>
>> That is to say, when the use queries the channel information at these
>> points, the chan->phy is usually NULL.
>
>>>> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>>>>         unsigned long flags;
>>>>
>>>>         spin_lock_irqsave(&chan->desc_lock, flags);
>>>> -       ret = dma_cookie_status(dchan, cookie, txstate);
>>>> +       ret = chan->status;
>>>> +       dma_set_residue(txstate, chan->bytes_residue);
>>>>         spin_unlock_irqrestore(&chan->desc_lock, flags);
>>>
>>> Besides my patch which removes this spinlock I think the workflow
>>> should be something like
>>>
>>> status = dma_cookie_status()
>>> if status == DMA_SUCCESS or !txstate:
>>> return status
>>>
>>> dma_set_residue()
>>> return status
>>>
>>> Because there is no reason to return residue of successfully finished
>>> transfer. It should be 0.
>
>> There is one exception from my point of view. When we are receiving
>> data from peripheral devices, we usually start a DMA transaction with
>> a target length of 4K for example. When a timed-out event occurs in
>> peripheral device, it will notify DMA controller and DMA controller
>> will send out a End of Receive interrupt (Marvell specific?).
>
> Might be your hardware specifics, in our case we have got a timeout
> interrupt from uart controller.
>
>> In such situation, DMA status is also DMA_SUCCESS. But the residual
>> bytes is not 0 and the user must query it.
>
> Which sounds wrong approach.
>
> P.S. take a look at  drivers/tty/serial/8250/8250_dma.c
Hi, Andy
When peripheral device (e.g. UART) is using DMA controller, we should
have 2 solutions to deal with trailing bytes:
1. Timeout interrupt from UART controller.
In this case, we usually pause DMA and read out data from DMA buffer
in uart irq handler.
2. DMA controller handles trailing bytes for us.
This is the case I mentioned in my previous email. "When a timed-out
event occurs in peripheral device, it will notify DMA controller and
DMA controller will send out a End of Receive interrupt"
I think we should know how many residual bytes in this case even the
DMA status is DMA_SUCCESS.

Thanks!
>
> --
> With Best Regards,
> Andy Shevchenko



--
Regards,
Xiang

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Andy Shevchenko]

On Thu, Jun 6, 2013 at 6:09 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
> 2013/6/3 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Mon, Jun 3, 2013 at 6:22 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>>> 2013/5/31 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>>> On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>>>>> In some of our drivers (e.g. UART) we may stop a running DMA
>>>>> before it finishes. So we need to know how many bytes have
>>>>> been transferred.

[]

>>> Why I use a saved value (chan->bytes_residue)?
>>> In current mmp pdma driver, a phy channel will be freed after the
>>> transmission finishes (chan->phy is set to NULL). So we cannot get the
>>> physical channel information after we call DMA_TERMINATE_ALL or DMA
>>> finishes itself.
>>
>> I don't see any contradiction to workflow.
>> So, If you call device_tx_status() when transfer is completed or
>> aborted you will get 0 as a residue, which is correct.

>>>>> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>>>>>         unsigned long flags;
>>>>>
>>>>>         spin_lock_irqsave(&chan->desc_lock, flags);
>>>>> -       ret = dma_cookie_status(dchan, cookie, txstate);
>>>>> +       ret = chan->status;
>>>>> +       dma_set_residue(txstate, chan->bytes_residue);
>>>>>         spin_unlock_irqrestore(&chan->desc_lock, flags);
>>>>
>>>> Besides my patch which removes this spinlock I think the workflow
>>>> should be something like
>>>>
>>>> status = dma_cookie_status()
>>>> if status == DMA_SUCCESS or !txstate:
>>>> return status
>>>>
>>>> dma_set_residue()
>>>> return status
>>>>
>>>> Because there is no reason to return residue of successfully finished
>>>> transfer. It should be 0.
>>
>>> There is one exception from my point of view. When we are receiving
>>> data from peripheral devices, we usually start a DMA transaction with
>>> a target length of 4K for example. When a timed-out event occurs in
>>> peripheral device, it will notify DMA controller and DMA controller
>>> will send out a End of Receive interrupt (Marvell specific?).
>>
>> Might be your hardware specifics, in our case we have got a timeout
>> interrupt from uart controller.
>>
>>> In such situation, DMA status is also DMA_SUCCESS. But the residual
>>> bytes is not 0 and the user must query it.
>>
>> Which sounds wrong approach.
>>
>> P.S. take a look at  drivers/tty/serial/8250/8250_dma.c

> When peripheral device (e.g. UART) is using DMA controller, we should
> have 2 solutions to deal with trailing bytes:

> 1. Timeout interrupt from UART controller.
> In this case, we usually pause DMA and read out data from DMA buffer
> in uart irq handler.

Hmm... When UART timeout occurs you have trailing bytes in the UART
FIFO. Correct? What do you mean under "DMA buffer" ?
I think you have to read bytes from UART FIFO.

> 2. DMA controller handles trailing bytes for us.
> This is the case I mentioned in my previous email. "When a timed-out
> event occurs in peripheral device, it will notify DMA controller and
> DMA controller will send out a End of Receive interrupt"
> I think we should know how many residual bytes in this case even the
> DMA status is DMA_SUCCESS.

I'm still not convinced that you have implement such algorithm. Anyway,
if I got it correctly you have something like "Receive FIFO Occupancy
Register (FOR)" in UART. When you got timeout interrupt, you may get
residue and value from FOR, sum them, program DMA to transfer the
trailing bytes.
What did I miss?

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Xiang Wang]

2013/6/6 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Thu, Jun 6, 2013 at 6:09 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>> 2013/6/3 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>> On Mon, Jun 3, 2013 at 6:22 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>>>> 2013/5/31 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>>>> On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
>>>>>> In some of our drivers (e.g. UART) we may stop a running DMA
>>>>>> before it finishes. So we need to know how many bytes have
>>>>>> been transferred.
>
> []
>
>>>> Why I use a saved value (chan->bytes_residue)?
>>>> In current mmp pdma driver, a phy channel will be freed after the
>>>> transmission finishes (chan->phy is set to NULL). So we cannot get the
>>>> physical channel information after we call DMA_TERMINATE_ALL or DMA
>>>> finishes itself.
>>>
>>> I don't see any contradiction to workflow.
>>> So, If you call device_tx_status() when transfer is completed or
>>> aborted you will get 0 as a residue, which is correct.
>
>>>>>> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>>>>>>         unsigned long flags;
>>>>>>
>>>>>>         spin_lock_irqsave(&chan->desc_lock, flags);
>>>>>> -       ret = dma_cookie_status(dchan, cookie, txstate);
>>>>>> +       ret = chan->status;
>>>>>> +       dma_set_residue(txstate, chan->bytes_residue);
>>>>>>         spin_unlock_irqrestore(&chan->desc_lock, flags);
>>>>>
>>>>> Besides my patch which removes this spinlock I think the workflow
>>>>> should be something like
>>>>>
>>>>> status = dma_cookie_status()
>>>>> if status == DMA_SUCCESS or !txstate:
>>>>> return status
>>>>>
>>>>> dma_set_residue()
>>>>> return status
>>>>>
>>>>> Because there is no reason to return residue of successfully finished
>>>>> transfer. It should be 0.
>>>
>>>> There is one exception from my point of view. When we are receiving
>>>> data from peripheral devices, we usually start a DMA transaction with
>>>> a target length of 4K for example. When a timed-out event occurs in
>>>> peripheral device, it will notify DMA controller and DMA controller
>>>> will send out a End of Receive interrupt (Marvell specific?).
>>>
>>> Might be your hardware specifics, in our case we have got a timeout
>>> interrupt from uart controller.
>>>
>>>> In such situation, DMA status is also DMA_SUCCESS. But the residual
>>>> bytes is not 0 and the user must query it.
>>>
>>> Which sounds wrong approach.
>>>
>>> P.S. take a look at  drivers/tty/serial/8250/8250_dma.c
>
>> When peripheral device (e.g. UART) is using DMA controller, we should
>> have 2 solutions to deal with trailing bytes:
>
>> 1. Timeout interrupt from UART controller.
>> In this case, we usually pause DMA and read out data from DMA buffer
>> in uart irq handler.
>
> Hmm... When UART timeout occurs you have trailing bytes in the UART
> FIFO. Correct? What do you mean under "DMA buffer" ?
> I think you have to read bytes from UART FIFO.
I didn't described clearly. We have to deal with 2 kinds of data in
this situation: 1) The data that DMA has moved but not reached the
target length (So DMA interrupt is not sent out). This is what I mean
in "DMA buffer". When we receive Timeout Interrupt from UART
controller, we pause DMA and read out the data  2) The data in UART
FIFO.

>
>> 2. DMA controller handles trailing bytes for us.
>> This is the case I mentioned in my previous email. "When a timed-out
>> event occurs in peripheral device, it will notify DMA controller and
>> DMA controller will send out a End of Receive interrupt"
>> I think we should know how many residual bytes in this case even the
>> DMA status is DMA_SUCCESS.
>
> I'm still not convinced that you have implement such algorithm. Anyway,
> if I got it correctly you have something like "Receive FIFO Occupancy
> Register (FOR)" in UART. When you got timeout interrupt, you may get
> residue and value from FOR, sum them, program DMA to transfer the
> trailing bytes.
> What did I miss?
This case is another working mode of peripheral device (e.g. UART). If
we configure UART to work in this mode, the hardware(DMA controller
and UART controller) will deal with the trailing bytes for us.
The workflow is likely to be:
a. When timeout occurs in UART, it notifies DMA controller to move the
trailing bytes in UART FIFO. (software transparent)
b. After DMA finishes moving all bytes in UART FIFO, it will send out
an End Of Receive(EOR) interrupt.
So in this case, software sees an EOR interrupt from DMA controller
instead of an Timeout Interrupt from UART controller.
Actually Marvell PXA988 UART controller supports this mode and I've tested. :)
>
> --
> With Best Regards,
> Andy Shevchenko



--
Regards,
Xiang

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Andy Shevchenko]

On Fri, Jun 7, 2013 at 6:02 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
> 2013/6/6 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Thu, Jun 6, 2013 at 6:09 AM, Xiang Wang <wangxfdu@gmail.com> wrote:

[]

>>> 2. DMA controller handles trailing bytes for us.
>>> This is the case I mentioned in my previous email. "When a timed-out
>>> event occurs in peripheral device, it will notify DMA controller and
>>> DMA controller will send out a End of Receive interrupt"
>>> I think we should know how many residual bytes in this case even the
>>> DMA status is DMA_SUCCESS.
>>
>> I'm still not convinced that you have implement such algorithm. Anyway,
>> if I got it correctly you have something like "Receive FIFO Occupancy
>> Register (FOR)" in UART. When you got timeout interrupt, you may get
>> residue and value from FOR, sum them, program DMA to transfer the
>> trailing bytes.
>> What did I miss?
> This case is another working mode of peripheral device (e.g. UART). If
> we configure UART to work in this mode, the hardware(DMA controller
> and UART controller) will deal with the trailing bytes for us.
> The workflow is likely to be:
> a. When timeout occurs in UART, it notifies DMA controller to move the
> trailing bytes in UART FIFO. (software transparent)
> b. After DMA finishes moving all bytes in UART FIFO, it will send out
> an End Of Receive(EOR) interrupt.
> So in this case, software sees an EOR interrupt from DMA controller
> instead of an Timeout Interrupt from UART controller.

Thanks for explanation.

It's quite good your hw could manage such cases, though I thing it's
wrong to have residue great than 0 and DMA_SUCCESS as a status in the
driver.

So, if Vinod, Dan, or any experienced developer is okay with it, I'm okay too.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Vinod Koul]

On Fri, May 31, 2013 at 04:21:25PM +0800, Xiang Wang wrote:
> From: Xiang Wang <wangx@marvell.com>
> 
> In some of our drivers (e.g. UART) we may stop a running DMA
> before it finishes. So we need to know how many bytes have
> been transferred.
> 
> Signed-off-by: Xiang Wang <wangx@marvell.com>
> ---
>  drivers/dma/mmp_pdma.c |   77 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 69 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 76a8dcf..0c623cc 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -104,7 +104,8 @@ struct mmp_pdma_chan {
>  	spinlock_t desc_lock;		/* Descriptor list lock */
>  	struct list_head chain_pending;	/* Link descriptors queue for pending */
>  	struct list_head chain_running;	/* Link descriptors queue for running */
> -	bool idle;			/* channel statue machine */
> +	enum dma_status status; /* channel status machine */
> +	u32 bytes_residue;
>  
>  	struct dma_pool *desc_pool;	/* Descriptors pool */
>  };
> @@ -270,7 +271,7 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
>  	struct mmp_pdma_desc_sw *desc;
>  
>  	/* still in running, irq will start the pending list */
> -	if (!chan->idle) {
> +	if (chan->status == DMA_IN_PROGRESS) {
>  		dev_dbg(chan->dev, "DMA controller still busy\n");
why are you not reading the residue and filling it here as well

>  		return;
>  	}
> @@ -307,7 +308,55 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
>  	 */
>  	set_desc(chan->phy, desc->async_tx.phys);
>  	enable_chan(chan->phy);
> -	chan->idle = false;
> +	chan->status = DMA_IN_PROGRESS;
> +	chan->bytes_residue = 0;
> +}
> +
> +/*
> + * Get the number of bytes untransferred by DMA.
			      ^^^^^^^^^^^^^
The word which might fit better is pending
> + */
> +static u32 mmp_pdma_get_bytes_residue(struct mmp_pdma_chan *chan)
> +{
> +	u32 reg, orig_pos, cur_pos, residue = 0;
> +	struct mmp_pdma_desc_sw *desc_sw;
> +	struct list_head *list_chain = NULL;
> +
> +	/*
> +	 * When a phy channel is unavailable, maybe its destroied, return last
typo							^^^^^^^^^
> +	 * stored value for safe.
> +	 */
> +	if (!chan || !chan->phy)
> +		return chan->bytes_residue;
> +
> +	if (!list_empty(&chan->chain_running))
> +		list_chain = &chan->chain_running;
> +	else
> +		return 0;
> +
> +	desc_sw = list_first_entry(list_chain, struct mmp_pdma_desc_sw, node);
> +
> +	switch (chan->dir) {
> +	case DMA_DEV_TO_MEM:
> +		reg = (chan->phy->idx << 4) + DTADR;
> +		cur_pos = readl_relaxed(chan->phy->base + reg);
> +		orig_pos = desc_sw->desc.dtadr;
> +		break;
empty line here and below pls

> +	case DMA_MEM_TO_DEV:
> +		reg = (chan->phy->idx << 4) + DSADR;
> +		cur_pos = readl_relaxed(chan->phy->base + reg);
> +		orig_pos = desc_sw->desc.dsadr;
> +		break;
> +	case DMA_MEM_TO_MEM:
> +		reg = (chan->phy->idx << 4) + DTADR;
> +		cur_pos = readl_relaxed(chan->phy->base + reg);
> +		orig_pos = desc_sw->desc.dtadr;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	residue = (u32)(desc_sw->desc.dcmd & DCMD_LENGTH) + orig_pos - cur_pos;
> +	return residue;
>  }
>  
>  
> @@ -381,7 +430,7 @@ static int mmp_pdma_alloc_chan_resources(struct dma_chan *dchan)
>  		chan->phy->vchan = NULL;
>  		chan->phy = NULL;
>  	}
> -	chan->idle = true;
> +	chan->status = DMA_SUCCESS;
>  	chan->dev_addr = 0;
>  	return 1;
>  }
> @@ -409,7 +458,7 @@ static void mmp_pdma_free_chan_resources(struct dma_chan *dchan)
>  
>  	dma_pool_destroy(chan->desc_pool);
>  	chan->desc_pool = NULL;
> -	chan->idle = true;
> +	chan->status = DMA_SUCCESS;
>  	chan->dev_addr = 0;
>  	if (chan->phy) {
>  		chan->phy->vchan = NULL;
> @@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
>  		mmp_pdma_free_desc_list(chan, &chan->chain_pending);
>  		mmp_pdma_free_desc_list(chan, &chan->chain_running);
>  		spin_unlock_irqrestore(&chan->desc_lock, flags);
> -		chan->idle = true;
> +		chan->status = DMA_SUCCESS;
> +		chan->bytes_residue = 0;
> +		break;
ditto
> +	case DMA_PAUSE:
> +		disable_chan(chan->phy);
> +		chan->status = DMA_PAUSED;
> +		chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>  		break;
>  	case DMA_SLAVE_CONFIG:
>  		if (cfg->direction == DMA_DEV_TO_MEM) {
> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&chan->desc_lock, flags);
> -	ret = dma_cookie_status(dchan, cookie, txstate);
> +	ret = chan->status;
> +	dma_set_residue(txstate, chan->bytes_residue);
>  	spin_unlock_irqrestore(&chan->desc_lock, flags);
>  
>  	return ret;
> @@ -684,6 +740,8 @@ static void dma_do_tasklet(unsigned long data)
>  		dev_dbg(chan->dev, "completed_cookie=%d\n", cookie);
>  	}
>  
> +	/* update bytes_residue so that the user can query later on */
> +	chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>  	/*
>  	 * move the descriptors to a temporary list so we can drop the lock
>  	 * during the entire cleanup operation
> @@ -691,7 +749,7 @@ static void dma_do_tasklet(unsigned long data)
>  	list_splice_tail_init(&chan->chain_running, &chain_cleanup);
>  
>  	/* the hardware is now idle and ready for more */
> -	chan->idle = true;
> +	chan->status = DMA_SUCCESS;
>  
>  	/* Start any pending transactions automatically */
>  	start_pending_queue(chan);
> @@ -750,6 +808,9 @@ static int mmp_pdma_chan_init(struct mmp_pdma_device *pdev,
>  	INIT_LIST_HEAD(&chan->chain_pending);
>  	INIT_LIST_HEAD(&chan->chain_running);
>  
> +	chan->status = DMA_SUCCESS;
> +	chan->bytes_residue = 0;
> +
>  	/* register virt channel to dma engine */
>  	list_add_tail(&chan->chan.device_node,
>  			&pdev->device.channels);
Okay I went thru the thread briefly. I understand why you are returning the
reside and nice usage model you have here.
What i odnt understand is that the reason for limiting the dma driver to a
certain usage model. Yes this is what you need a nd what you have done. But
nothing prevents you from reading the reside and returning on the fly. All the
basic elements are alreayd in this patch. SO lets do the right thing now :)

--
~Vinod

Re: [PATCH] dma: mmp_pdma: support for getting residual bytes

[Xiang Wang]

2013/6/17 Vinod Koul <vinod.koul@intel.com>:
> On Fri, May 31, 2013 at 04:21:25PM +0800, Xiang Wang wrote:
>> From: Xiang Wang <wangx@marvell.com>
>>
>> In some of our drivers (e.g. UART) we may stop a running DMA
>> before it finishes. So we need to know how many bytes have
>> been transferred.
>>
>> Signed-off-by: Xiang Wang <wangx@marvell.com>
>> ---
>>  drivers/dma/mmp_pdma.c |   77 +++++++++++++++++++++++++++++++++++++++++++-----
>>  1 files changed, 69 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
>> index 76a8dcf..0c623cc 100644
>> --- a/drivers/dma/mmp_pdma.c
>> +++ b/drivers/dma/mmp_pdma.c
>> @@ -104,7 +104,8 @@ struct mmp_pdma_chan {
>>       spinlock_t desc_lock;           /* Descriptor list lock */
>>       struct list_head chain_pending; /* Link descriptors queue for pending */
>>       struct list_head chain_running; /* Link descriptors queue for running */
>> -     bool idle;                      /* channel statue machine */
>> +     enum dma_status status; /* channel status machine */
>> +     u32 bytes_residue;
>>
>>       struct dma_pool *desc_pool;     /* Descriptors pool */
>>  };
>> @@ -270,7 +271,7 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
>>       struct mmp_pdma_desc_sw *desc;
>>
>>       /* still in running, irq will start the pending list */
>> -     if (!chan->idle) {
>> +     if (chan->status == DMA_IN_PROGRESS) {
>>               dev_dbg(chan->dev, "DMA controller still busy\n");
> why are you not reading the residue and filling it here as well
In patch V2, the user can query the residue bytes on the fly. We only
need to read and save the value in some key points where phy channel
are released (the registers will not be accessible).
>
>>               return;
>>       }
>> @@ -307,7 +308,55 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
>>        */
>>       set_desc(chan->phy, desc->async_tx.phys);
>>       enable_chan(chan->phy);
>> -     chan->idle = false;
>> +     chan->status = DMA_IN_PROGRESS;
>> +     chan->bytes_residue = 0;
>> +}
>> +
>> +/*
>> + * Get the number of bytes untransferred by DMA.
>                               ^^^^^^^^^^^^^
> The word which might fit better is pending
Updated in patch V2. Thanks!
>> + */
>> +static u32 mmp_pdma_get_bytes_residue(struct mmp_pdma_chan *chan)
>> +{
>> +     u32 reg, orig_pos, cur_pos, residue = 0;
>> +     struct mmp_pdma_desc_sw *desc_sw;
>> +     struct list_head *list_chain = NULL;
>> +
>> +     /*
>> +      * When a phy channel is unavailable, maybe its destroied, return last
> typo                                                    ^^^^^^^^^
Updated in patch V2. Thanks!
>> +      * stored value for safe.
>> +      */
>> +     if (!chan || !chan->phy)
>> +             return chan->bytes_residue;
>> +
>> +     if (!list_empty(&chan->chain_running))
>> +             list_chain = &chan->chain_running;
>> +     else
>> +             return 0;
>> +
>> +     desc_sw = list_first_entry(list_chain, struct mmp_pdma_desc_sw, node);
>> +
>> +     switch (chan->dir) {
>> +     case DMA_DEV_TO_MEM:
>> +             reg = (chan->phy->idx << 4) + DTADR;
>> +             cur_pos = readl_relaxed(chan->phy->base + reg);
>> +             orig_pos = desc_sw->desc.dtadr;
>> +             break;
> empty line here and below pls
Updated in patch V2. Thanks!
>
>> +     case DMA_MEM_TO_DEV:
>> +             reg = (chan->phy->idx << 4) + DSADR;
>> +             cur_pos = readl_relaxed(chan->phy->base + reg);
>> +             orig_pos = desc_sw->desc.dsadr;
>> +             break;
>> +     case DMA_MEM_TO_MEM:
>> +             reg = (chan->phy->idx << 4) + DTADR;
>> +             cur_pos = readl_relaxed(chan->phy->base + reg);
>> +             orig_pos = desc_sw->desc.dtadr;
>> +             break;
>> +     default:
>> +             return 0;
>> +     }
>> +
>> +     residue = (u32)(desc_sw->desc.dcmd & DCMD_LENGTH) + orig_pos - cur_pos;
>> +     return residue;
>>  }
>>
>>
>> @@ -381,7 +430,7 @@ static int mmp_pdma_alloc_chan_resources(struct dma_chan *dchan)
>>               chan->phy->vchan = NULL;
>>               chan->phy = NULL;
>>       }
>> -     chan->idle = true;
>> +     chan->status = DMA_SUCCESS;
>>       chan->dev_addr = 0;
>>       return 1;
>>  }
>> @@ -409,7 +458,7 @@ static void mmp_pdma_free_chan_resources(struct dma_chan *dchan)
>>
>>       dma_pool_destroy(chan->desc_pool);
>>       chan->desc_pool = NULL;
>> -     chan->idle = true;
>> +     chan->status = DMA_SUCCESS;
>>       chan->dev_addr = 0;
>>       if (chan->phy) {
>>               chan->phy->vchan = NULL;
>> @@ -589,7 +638,13 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
>>               mmp_pdma_free_desc_list(chan, &chan->chain_pending);
>>               mmp_pdma_free_desc_list(chan, &chan->chain_running);
>>               spin_unlock_irqrestore(&chan->desc_lock, flags);
>> -             chan->idle = true;
>> +             chan->status = DMA_SUCCESS;
>> +             chan->bytes_residue = 0;
>> +             break;
> ditto
Also updated
>> +     case DMA_PAUSE:
>> +             disable_chan(chan->phy);
>> +             chan->status = DMA_PAUSED;
>> +             chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>>               break;
>>       case DMA_SLAVE_CONFIG:
>>               if (cfg->direction == DMA_DEV_TO_MEM) {
>> @@ -637,7 +692,8 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
>>       unsigned long flags;
>>
>>       spin_lock_irqsave(&chan->desc_lock, flags);
>> -     ret = dma_cookie_status(dchan, cookie, txstate);
>> +     ret = chan->status;
>> +     dma_set_residue(txstate, chan->bytes_residue);
>>       spin_unlock_irqrestore(&chan->desc_lock, flags);
>>
>>       return ret;
>> @@ -684,6 +740,8 @@ static void dma_do_tasklet(unsigned long data)
>>               dev_dbg(chan->dev, "completed_cookie=%d\n", cookie);
>>       }
>>
>> +     /* update bytes_residue so that the user can query later on */
>> +     chan->bytes_residue = mmp_pdma_get_bytes_residue(chan);
>>       /*
>>        * move the descriptors to a temporary list so we can drop the lock
>>        * during the entire cleanup operation
>> @@ -691,7 +749,7 @@ static void dma_do_tasklet(unsigned long data)
>>       list_splice_tail_init(&chan->chain_running, &chain_cleanup);
>>
>>       /* the hardware is now idle and ready for more */
>> -     chan->idle = true;
>> +     chan->status = DMA_SUCCESS;
>>
>>       /* Start any pending transactions automatically */
>>       start_pending_queue(chan);
>> @@ -750,6 +808,9 @@ static int mmp_pdma_chan_init(struct mmp_pdma_device *pdev,
>>       INIT_LIST_HEAD(&chan->chain_pending);
>>       INIT_LIST_HEAD(&chan->chain_running);
>>
>> +     chan->status = DMA_SUCCESS;
>> +     chan->bytes_residue = 0;
>> +
>>       /* register virt channel to dma engine */
>>       list_add_tail(&chan->chan.device_node,
>>                       &pdev->device.channels);
> Okay I went thru the thread briefly. I understand why you are returning the
> reside and nice usage model you have here.
> What i odnt understand is that the reason for limiting the dma driver to a
> certain usage model. Yes this is what you need a nd what you have done. But
> nothing prevents you from reading the reside and returning on the fly. All the
> basic elements are alreayd in this patch. SO lets do the right thing now :)
In patch V2, the user can query residue bytes on the fly by modifying
mmp_pdma_tx_status().
>
> --
> ~Vinod



--
Regards,
Xiang