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

[Andy Shevchenko]

On Mon, Jun 3, 2013 at 6:22 AM, Xiang Wang <wangxfdu@xxxxxxxxx> wrote:
> 2013/5/31 Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
>> On Fri, May 31, 2013 at 11:21 AM, Xiang Wang <wangxfdu@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/