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

[Jon Hunter]

On 07/06/2019 10:18, Jon Hunter wrote:
> 
> 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.

Actually, we don't even need to do that. We only use src_maxburst for
DEV_TO_MEM and dst_maxburst for MEM_TO_DEV. I don't see any reason why
we could not use both the src_maxburst for dst_maxburst for both
DEV_TO_MEM and MEM_TO_DEV, where one represents the FIFO size and one
represents that DMA burst size.

Sorry should have thought of that before. Any objections to using these
this way? Obviously we would document is clearly in the driver.

Cheers
Jon

-- 
nvpublic