Re: [PATCH] [RFC] dmaengine: add fifo_size member

From: Jon Hunter <jonathanh@nvidia.com>
To: Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Sameer Pujar <spujar@nvidia.com>, Vinod Koul <vkoul@kernel.org>
Cc: <dan.j.williams@intel.com>, <tiwai@suse.com>,
	<dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<sharadg@nvidia.com>, <rlokhande@nvidia.com>,
	<dramesh@nvidia.com>, <mkumard@nvidia.com>,
	linux-tegra <linux-tegra@vger.kernel.org>
Subject: Re: [PATCH] [RFC] dmaengine: add fifo_size member
Date: Fri, 7 Jun 2019 10:18:09 +0100	[thread overview]
Message-ID: <5208a50a-9ca0-8f24-9ad0-d7503ec53f1c@nvidia.com> (raw)
In-Reply-To: <d0db90e3-3d05-dfba-8768-28511d9ee3ac@ti.com>

On 07/06/2019 06:50, Peter Ujfalusi wrote:
> Jon,
> 
> On 06/06/2019 15.37, Jon Hunter wrote:
>>> Looking at the drivers/dma/tegra210-adma.c for the
>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>> remote FIFO size would fit.
>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>> size (assuming word length, 3 == 32bits?).
>>
>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>> bytes.
>>
>>> Both threshold is set to one, so I assume currently ADMA is
>>> pushing/pulling data word by word.
>>
>> That's different. That indicates thresholds when transfers start.
>>
>>> Not sure what the burst size is used for, my guess would be that it is
>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>
>> That is the actual burst size.
>>
>>> My guess is that the threshold values are the counter limits, if the DMA
>>> request counter reaches it then ADMA would do a threshold limit worth of
>>> push/pull to ADMAIF.
>>> Or there is another register where the remote FIFO size can be written
>>> and ADMA is counting back from there until it reaches the threshold (and
>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>> filled with at least threshold amount of data?
>>>
>>> I think in both cases the threshold would be the maxburst.
>>>
>>> I suppose you have the patch for adma on how to use the fifo_size
>>> parameter? That would help understand what you are trying to achieve better.
>>
>> Its quite simple, we would just use the FIFO size to set the fields
>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
> 
> Hrm, it is still not clear how all of these fits together.
> 
> What happens if you configure ADMA side:
> BURST = 10
> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */
> *THRES = 5
> 
> And if you change the *THRES to 10?
> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?)
> And if you change the BURST to 5?
> 
> In other words what is the relation between all of these?

So the THRES values are only applicable when the FETCHING_POLICY (bit 31
of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes;
a threshold based transfer mode or a burst based transfer mode. The
burst mode transfer data as and when there is room for a burst in the FIFO.

We use the burst mode and so we really should not be setting the THRES
fields as they are not applicable. Oh well something else to correct,
but this is side issue.

> There must be a rule and constraints around these and if we do really
> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a
> generic way so others could benefit without 'misusing' a fifo_size
> parameter for similar, but not quite fifo_size information.

Yes I see what you are saying. One option would be to define both a
src/dst_maxburst and src/dst_minburst size. Then we could use max for
the FIFO size and min for the actual burst size.

Cheers
Jon

-- 
nvpublic