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