Messages in this thread | | | Date | Mon, 3 Jun 2013 14:54:59 +0300 | Subject | Re: [PATCH] dma: mmp_pdma: support for getting residual bytes | From | 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
| |