Date:   Thu, 6 Jun 2019 15:47:00 +0100
From:   Jon Hunter <jonathanh@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>,
        Peter Ujfalusi <peter.ujfalusi@...com>,
        Sameer Pujar <spujar@...dia.com>, Vinod Koul <vkoul@...nel.org>
CC:     <dan.j.williams@...el.com>, <tiwai@...e.com>,
        <dmaengine@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <sharadg@...dia.com>, <rlokhande@...dia.com>, <dramesh@...dia.com>,
        <mkumard@...dia.com>, linux-tegra <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] [RFC] dmaengine: add fifo_size member


On 06/06/2019 15:36, Dmitry Osipenko wrote:
> 06.06.2019 17:26, Jon Hunter пишет:
>>
>> On 06/06/2019 14:55, Dmitry Osipenko wrote:
>>> 06.06.2019 16:45, Dmitry Osipenko пишет:
>>>> 06.06.2019 15:37, Jon Hunter пишет:
>>>>>
>>>>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>>>>
>>>>>>
>>>>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>>>>> well.
>>>>>>>>>> Not exactly equal.
>>>>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>>>>> multiples of 16]
>>>>>>>>>
>>>>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>>>>> has register fields for both of these.
>>>>>>>>
>>>>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>>>>> values and what is the relation of these values to the peripheral side
>>>>>>>> (ADMAIF)?
>>>>>>>
>>>>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>>>>> determine how much space is available in the FIFO. I assume the burst
>>>>>>> size just limits how much data is transferred per transaction.
>>>>>>>
>>>>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>>>>
>>>>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>>>>> like to pass both.
>>>>>>>>>
>>>>>>>>> We are also open to other ideas.
>>>>>>>>
>>>>>>>> I can not find public documentation (I think they are walled off by
>>>>>>>> registration), but correct me if I'm wrong:
>>>>>>>
>>>>>>> No unfortunately, you are not wrong here :-(
>>>>>>>
>>>>>>>> ADMAIF - peripheral side
>>>>>>>>  - kind of a small DMA for audio preipheral(s)?
>>>>>>>
>>>>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>>>>> configurable depending on the use-case.
>>>>>>>
>>>>>>>>  - Variable FIFO size
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>>  - sends DMA request to ADMA per words
>>>>>>>
>>>>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>>>>> word, yes.
>>>>>>>
>>>>>>>> ADMA - system DMA
>>>>>>>>  - receives the DMA requests from ADMAIF
>>>>>>>>  - counts the requests
>>>>>>>>  - based on some threshold of the counter it will send/read from ADMAIF?
>>>>>>>>   - maxburst number of words probably?
>>>>>>>
>>>>>>> Sounds about right to me.
>>>>>>>
>>>>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>>>>> or underrun?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>> So currently, if you look at the ADMA driver
>>>>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>>>>> for the burst, but the FIFO size is hard-coded (see the
>>>>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>>>>> Ideally, we should not hard-code this but pass it.
>>>>>>
>>>>>> Sure, hardcoding is never good ;)
>>>>>>
>>>>>>> Given that there are no current users of the ADMA upstream, we could
>>>>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>>>>> size as well would be ideal.
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Jon
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> If I understood everything correctly, the FIFO buffer is shared among
>>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>>> manage the quotas of the clients. So if there is only one client that
>>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>>> once another client starts to use ADMA, then the ADMA driver will have
>>>> to reconfigure hardware to split the quotas.
>>>>
>>>
>>> You could also simply hardcode the quotas per client in the ADMA driver
>>> if the quotas are going to be static anyway.
>>
>> Essentially this is what we have done so far, but Sameer is looking for
>> a way to make this more programmable/flexible. We can always do that if
>> there is no other option indeed. However, seems like a good time to see
>> if there is a better way.
> 
> If the default values are good enough, then why bother? Otherwise it
> looks like you'll need some kind of "quotas manager", please try to
> figure out if it's really needed. It's always better to avoid
> over-engineering.

We are proposing to add one variable. Hardly seems like over-engineering
to me. Again we are just looking to see if this would be acceptable or not.

Jon


-- 
nvpublic

Powered by blists - more mailing lists