Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller

[Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 09.10.2017 13:39, Vinod Koul wrote:
> On Mon, Oct 09, 2017 at 10:43:54AM +0100, Jon Hunter wrote:
>>
>>
>> On 06/10/17 20:11, Dmitry Osipenko wrote:
>>> On 04.10.2017 02:58, Dmitry Osipenko wrote:
>>>> AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers
>>>> memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver
>>>> doesn't yet implement transfers larger than 64K and scatter-gather
>>>> transfers that have NENT > 1, HW doesn't have native support for these
>>>> cases, mem-to-mem isn't implemented as well.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  drivers/dma/Kconfig           |  10 +
>>>>  drivers/dma/Makefile          |   1 +
>>>>  drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 641 insertions(+)
>>>>  create mode 100644 drivers/dma/tegra20-ahb-dma.c
>>
>>
>> ...
>>
>>>> +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx(
>>>> +						struct tegra_ahbdma_chan *chan)
>>>> +{
>>>> +	struct virt_dma_desc *vdesc = vchan_next_desc(&chan->vchan);
>>>> +
>>>> +	if (vdesc)
>>>> +		list_del(&vdesc->node);
>>>
>>> I just noticed that this is incorrect. Node must be deleted after TX completion,
>>> otherwise vchan_find_desc won't find TX and residual won't be reported by
>>> dmaengine_tx_status.
>>>
>>> Jon, I think you ADMA driver has the same issue, as well as several other DMA
>>> drivers that use virt-dma.
>>
>> Actually, I think that the above is correct. If vchan_find_desc() finds
>> the descriptor then this indicates that the transfer has not started yet
>> and so the residual is equal to the transfer size.
> 
> That is correct, so you can do a quick calculation and find the residue only
> for current one
> 

Yeah, the problem was that the current *active* one wasn't found.

>> If you look at the adma driver, you will see the if vchan_find_desc()
>> does not find the descriptor, then we check to see if the current
>> transfer is the one we are querying and if so return the bytes
>> remaining. Looking at this driver again, what you have in
>> tegra_ahbdma_tx_status() does not look correct. You should have
>> something like ...
>>
>> 	vdesc = vchan_find_desc(&ahbdma_chan->vchan, cookie);
>> 	if (vdesc) {
>> 		tx = to_ahbdma_tx_desc(vdesc);
>> 		residual = tx->csr & AHBDMA_CH_WCOUNT_MASK;
>> 	} else if (ahbdma_chan->tx_active && 	
>> 		   ahbdma_chan->tx_active->vd.tx.cookie == cookie) {
>> 		residual = tegra_ahbdma_residual(ahbdma_chan);
>> 	} else {
>> 		residual = 0;
>> 	}

I've moved list_del() to the TX completion handler, but I agree that it is not
entirely correct even though that solution works fine. Your variant also works
and does the right thing, thanks!

ADMA driver is correct. Sorry for the 'false alarm' :)