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

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

[Jon Hunter]

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.

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.

Cheers
Jon

-- 
nvpublic

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

[Peter Ujfalusi]

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?).
Both threshold is set to one, so I assume currently ADMA is
pushing/pulling data word by word.

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?

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.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[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

-- 
nvpublic

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

[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.

-- 
Dmitry

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

[Dmitry Osipenko]

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.

-- 
Dmitry

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

[Jon Hunter]

On 06/06/2019 14:45, Dmitry Osipenko wrote:
> 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.

The FIFO quotas are managed by the ADMAIF driver (does not exist in
mainline currently but we are working to upstream this) because it is
this device that owns and needs to configure the FIFOs. So it is really
a means to pass the information from the ADMAIF to the ADMA.

Jon

-- 
nvpublic

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

[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.

Jon

-- 
nvpublic

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

[Jon Hunter]

On 06/06/2019 15:26, Jon Hunter wrote:

...

>>> 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.

My thoughts on resolving this, in order of preference, would be ...

1. Add a new 'fifo_size' variable as Sameer is proposing.
2. Update the ADMA driver to use src/dst_maxburst as the fifo size and
   then have the ADMA driver set a suitable burst size for its burst
   size.
3. Resort to a static configuration.

I can see that #1 only makes sense if others would find it useful,
otherwise #2, may give us enough flexibility for now.

Cheers
Jon

-- 
nvpublic

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

[Dmitry Osipenko]

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.

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

[Jon Hunter]

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

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

[Dmitry Osipenko]

06.06.2019 17:25, Jon Hunter пишет:
> 
> 
> On 06/06/2019 14:45, Dmitry Osipenko wrote:
>> 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.
> 
> The FIFO quotas are managed by the ADMAIF driver (does not exist in
> mainline currently but we are working to upstream this) because it is
> this device that owns and needs to configure the FIFOs. So it is really
> a means to pass the information from the ADMAIF to the ADMA.

So you'd want to reserve a larger FIFO for an audio channel that has a
higher audio rate since it will perform reads more often. You could also
prioritize one channel over the others, like in a case of audio call for
example.

Is the shared buffer smaller than may be needed by clients in a worst
case scenario? If you could split the quotas statically such that each
client won't ever starve, then seems there is no much need in the
dynamic configuration.

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

[Jon Hunter]

On 06/06/2019 16:18, Dmitry Osipenko wrote:

...

>>> 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.
>>
>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>> mainline currently but we are working to upstream this) because it is
>> this device that owns and needs to configure the FIFOs. So it is really
>> a means to pass the information from the ADMAIF to the ADMA.
> 
> So you'd want to reserve a larger FIFO for an audio channel that has a
> higher audio rate since it will perform reads more often. You could also
> prioritize one channel over the others, like in a case of audio call for
> example.
> 
> Is the shared buffer smaller than may be needed by clients in a worst
> case scenario? If you could split the quotas statically such that each
> client won't ever starve, then seems there is no much need in the
> dynamic configuration.

Actually, this is still very much relevant for the static case. Even if
we defined a static configuration of the FIFO mapping in the ADMAIF
driver we still need to pass this information to the ADMA. I don't
really like the idea of having it statically defined in two different
drivers.

Jon

-- 
nvpublic

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

[Dmitry Osipenko]

06.06.2019 19:32, Jon Hunter пишет:
> 
> On 06/06/2019 16:18, Dmitry Osipenko wrote:
> 
> ...
> 
>>>> 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.
>>>
>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>> mainline currently but we are working to upstream this) because it is
>>> this device that owns and needs to configure the FIFOs. So it is really
>>> a means to pass the information from the ADMAIF to the ADMA.
>>
>> So you'd want to reserve a larger FIFO for an audio channel that has a
>> higher audio rate since it will perform reads more often. You could also
>> prioritize one channel over the others, like in a case of audio call for
>> example.
>>
>> Is the shared buffer smaller than may be needed by clients in a worst
>> case scenario? If you could split the quotas statically such that each
>> client won't ever starve, then seems there is no much need in the
>> dynamic configuration.
> 
> Actually, this is still very much relevant for the static case. Even if
> we defined a static configuration of the FIFO mapping in the ADMAIF
> driver we still need to pass this information to the ADMA. I don't
> really like the idea of having it statically defined in two different
> drivers.

Ah, so you need to apply the same configuration in two places. Correct?

Are ADMAIF and ADMA really two different hardware blocks? Or you
artificially decoupled the ADMA driver?

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

[Jon Hunter]

On 06/06/2019 17:44, Dmitry Osipenko wrote:
> 06.06.2019 19:32, Jon Hunter пишет:
>>
>> On 06/06/2019 16:18, Dmitry Osipenko wrote:
>>
>> ...
>>
>>>>> 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.
>>>>
>>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>>> mainline currently but we are working to upstream this) because it is
>>>> this device that owns and needs to configure the FIFOs. So it is really
>>>> a means to pass the information from the ADMAIF to the ADMA.
>>>
>>> So you'd want to reserve a larger FIFO for an audio channel that has a
>>> higher audio rate since it will perform reads more often. You could also
>>> prioritize one channel over the others, like in a case of audio call for
>>> example.
>>>
>>> Is the shared buffer smaller than may be needed by clients in a worst
>>> case scenario? If you could split the quotas statically such that each
>>> client won't ever starve, then seems there is no much need in the
>>> dynamic configuration.
>>
>> Actually, this is still very much relevant for the static case. Even if
>> we defined a static configuration of the FIFO mapping in the ADMAIF
>> driver we still need to pass this information to the ADMA. I don't
>> really like the idea of having it statically defined in two different
>> drivers.
> 
> Ah, so you need to apply the same configuration in two places. Correct?
> 
> Are ADMAIF and ADMA really two different hardware blocks? Or you
> artificially decoupled the ADMA driver?

These are two different hardware modules with their own register sets.
Yes otherwise, it would be a lot simpler!

Jon

-- 
nvpublic

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

[Dmitry Osipenko]

06.06.2019 19:53, Jon Hunter пишет:
> 
> On 06/06/2019 17:44, Dmitry Osipenko wrote:
>> 06.06.2019 19:32, Jon Hunter пишет:
>>>
>>> On 06/06/2019 16:18, Dmitry Osipenko wrote:
>>>
>>> ...
>>>
>>>>>> 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.
>>>>>
>>>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>>>> mainline currently but we are working to upstream this) because it is
>>>>> this device that owns and needs to configure the FIFOs. So it is really
>>>>> a means to pass the information from the ADMAIF to the ADMA.
>>>>
>>>> So you'd want to reserve a larger FIFO for an audio channel that has a
>>>> higher audio rate since it will perform reads more often. You could also
>>>> prioritize one channel over the others, like in a case of audio call for
>>>> example.
>>>>
>>>> Is the shared buffer smaller than may be needed by clients in a worst
>>>> case scenario? If you could split the quotas statically such that each
>>>> client won't ever starve, then seems there is no much need in the
>>>> dynamic configuration.
>>>
>>> Actually, this is still very much relevant for the static case. Even if
>>> we defined a static configuration of the FIFO mapping in the ADMAIF
>>> driver we still need to pass this information to the ADMA. I don't
>>> really like the idea of having it statically defined in two different
>>> drivers.
>>
>> Ah, so you need to apply the same configuration in two places. Correct?
>>
>> Are ADMAIF and ADMA really two different hardware blocks? Or you
>> artificially decoupled the ADMA driver?
> 
> These are two different hardware modules with their own register sets.
> Yes otherwise, it would be a lot simpler!

The register sets are indeed separated, but it looks like that ADMAIF is
really a part of ADMA that is facing to Audio Crossbar. No? What is the
purpose of ADMAIF? Maybe you could amend the ADMA hardware description
with the ADMAIF addition until it's too late.

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

[Dmitry Osipenko]

06.06.2019 20:25, Dmitry Osipenko пишет:
> 06.06.2019 19:53, Jon Hunter пишет:
>>
>> On 06/06/2019 17:44, Dmitry Osipenko wrote:
>>> 06.06.2019 19:32, Jon Hunter пишет:
>>>>
>>>> On 06/06/2019 16:18, Dmitry Osipenko wrote:
>>>>
>>>> ...
>>>>
>>>>>>> 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.
>>>>>>
>>>>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>>>>> mainline currently but we are working to upstream this) because it is
>>>>>> this device that owns and needs to configure the FIFOs. So it is really
>>>>>> a means to pass the information from the ADMAIF to the ADMA.
>>>>>
>>>>> So you'd want to reserve a larger FIFO for an audio channel that has a
>>>>> higher audio rate since it will perform reads more often. You could also
>>>>> prioritize one channel over the others, like in a case of audio call for
>>>>> example.
>>>>>
>>>>> Is the shared buffer smaller than may be needed by clients in a worst
>>>>> case scenario? If you could split the quotas statically such that each
>>>>> client won't ever starve, then seems there is no much need in the
>>>>> dynamic configuration.
>>>>
>>>> Actually, this is still very much relevant for the static case. Even if
>>>> we defined a static configuration of the FIFO mapping in the ADMAIF
>>>> driver we still need to pass this information to the ADMA. I don't
>>>> really like the idea of having it statically defined in two different
>>>> drivers.
>>>
>>> Ah, so you need to apply the same configuration in two places. Correct?
>>>
>>> Are ADMAIF and ADMA really two different hardware blocks? Or you
>>> artificially decoupled the ADMA driver?
>>
>> These are two different hardware modules with their own register sets.
>> Yes otherwise, it would be a lot simpler!
> 
> The register sets are indeed separated, but it looks like that ADMAIF is
> really a part of ADMA that is facing to Audio Crossbar. No? What is the
> purpose of ADMAIF? Maybe you could amend the ADMA hardware description
> with the ADMAIF addition until it's too late.
> 

Ugh.. I now regret looking at the TRM. That Audio Processor Engine is a
horrifying beast, it even has FPGA :)

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

[Peter Ujfalusi]

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?

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.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[Jon Hunter]

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

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

[Jon Hunter]

On 06/06/2019 18:25, Dmitry Osipenko wrote:
> 06.06.2019 19:53, Jon Hunter пишет:
>>
>> On 06/06/2019 17:44, Dmitry Osipenko wrote:
>>> 06.06.2019 19:32, Jon Hunter пишет:
>>>>
>>>> On 06/06/2019 16:18, Dmitry Osipenko wrote:
>>>>
>>>> ...
>>>>
>>>>>>> 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.
>>>>>>
>>>>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>>>>> mainline currently but we are working to upstream this) because it is
>>>>>> this device that owns and needs to configure the FIFOs. So it is really
>>>>>> a means to pass the information from the ADMAIF to the ADMA.
>>>>>
>>>>> So you'd want to reserve a larger FIFO for an audio channel that has a
>>>>> higher audio rate since it will perform reads more often. You could also
>>>>> prioritize one channel over the others, like in a case of audio call for
>>>>> example.
>>>>>
>>>>> Is the shared buffer smaller than may be needed by clients in a worst
>>>>> case scenario? If you could split the quotas statically such that each
>>>>> client won't ever starve, then seems there is no much need in the
>>>>> dynamic configuration.
>>>>
>>>> Actually, this is still very much relevant for the static case. Even if
>>>> we defined a static configuration of the FIFO mapping in the ADMAIF
>>>> driver we still need to pass this information to the ADMA. I don't
>>>> really like the idea of having it statically defined in two different
>>>> drivers.
>>>
>>> Ah, so you need to apply the same configuration in two places. Correct?
>>>
>>> Are ADMAIF and ADMA really two different hardware blocks? Or you
>>> artificially decoupled the ADMA driver?
>>
>> These are two different hardware modules with their own register sets.
>> Yes otherwise, it would be a lot simpler!
> 
> The register sets are indeed separated, but it looks like that ADMAIF is
> really a part of ADMA that is facing to Audio Crossbar. No? What is the
> purpose of ADMAIF? Maybe you could amend the ADMA hardware description
> with the ADMAIF addition until it's too late.

The ADMA can perform the following transfers (per the CH_CONFIG
register) ...

MEMORY_TO_MEMORY
AHUB_TO_MEMORY
MEMORY_TO_AHUB
AHUB_TO_AHUB

Hence it is possible to use the ADMA to do memory-to-memory transfers
that do not involve the ADMAIF.

So no the ADMAIF is not part of the ADMA. It is purely the interface to
the crossbar (AHUB/APE), but from a hardware standpoint they are
separate. And so no we will not amend the hardware description.

Jon

-- 
nvpublic

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

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

[Peter Ujfalusi]

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

Imho if you can explain it without using 'HACK' in the sentences it
might be OK, but it does not feel right.

However since your ADMA and ADMIF is highly coupled and it does needs
special maxburst information (burst and allocated FIFO depth) I would
rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:

ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)

So lower 1 byte is the burst value you want from ADMA
the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.

Sure, you need a header for this to make sure there is no
misunderstanding between the two sides.

Or pass the allocated FIFO size via maxburst and then the ADMA driver
will pick a 'good/safe' burst value for it.

Or new member, but do you need two of them for src/dst? Probably
fifo_depth is better word for it, or allocated_fifo_depth.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[Jon Hunter]

On 07/06/2019 13:17, Peter Ujfalusi wrote:
> 
> 
> On 07/06/2019 13.27, Jon Hunter wrote:
>>>> 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.
> 
> Imho if you can explain it without using 'HACK' in the sentences it
> might be OK, but it does not feel right.

I don't perceive this as a hack. Although from looking at the
description of the src/dst_maxburst these are burst size with regard to
the device, so maybe it is a stretch.

> However since your ADMA and ADMIF is highly coupled and it does needs
> special maxburst information (burst and allocated FIFO depth) I would
> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
> 
> ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
> ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)
> 
> So lower 1 byte is the burst value you want from ADMA
> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
> 
> Sure, you need a header for this to make sure there is no
> misunderstanding between the two sides.

I don't like this because as I mentioned to Dmitry, the ADMA can perform
memory-to-memory transfers where such encoding would not be applicable.

That does not align with the description in the
include/linux/dmaengine.h either.

> Or pass the allocated FIFO size via maxburst and then the ADMA driver
> will pick a 'good/safe' burst value for it.
> 
> Or new member, but do you need two of them for src/dst? Probably
> fifo_depth is better word for it, or allocated_fifo_depth.

Right, so looking at the struct dma_slave_config we have ...

u32 src_maxburst;
u32 dst_maxburst;
u32 src_port_window_size;
u32 dst_port_window_size;

Now if we could make these window sizes a union like the following this
could work ...

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8fcdee1c0cf9..851251263527 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -360,8 +360,14 @@ struct dma_slave_config {
        enum dma_slave_buswidth dst_addr_width;
        u32 src_maxburst;
        u32 dst_maxburst;
-       u32 src_port_window_size;
-       u32 dst_port_window_size;
+       union {
+               u32 port_window_size;
+               u32 port_fifo_size;
+       } src;
+       union {
+               u32 port_window_size;
+               u32 port_fifo_size;
+       } dst;

Cheers,
Jon

-- 
nvpublic

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

[Peter Ujfalusi]

On 07/06/2019 15.58, Jon Hunter wrote:
>> Imho if you can explain it without using 'HACK' in the sentences it
>> might be OK, but it does not feel right.
> 
> I don't perceive this as a hack. Although from looking at the
> description of the src/dst_maxburst these are burst size with regard to
> the device, so maybe it is a stretch.
> 
>> However since your ADMA and ADMIF is highly coupled and it does needs
>> special maxburst information (burst and allocated FIFO depth) I would
>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>
>> ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
>> ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)
>>
>> So lower 1 byte is the burst value you want from ADMA
>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>
>> Sure, you need a header for this to make sure there is no
>> misunderstanding between the two sides.
> 
> I don't like this because as I mentioned to Dmitry, the ADMA can perform
> memory-to-memory transfers where such encoding would not be applicable.

mem2mem does not really use dma_slave_config, it is for used by
is_slave_direction() == true type of transfers.

But true, if you use ADMA against anything other than ADMAIF then this
might be not right for non cyclic transfers.

> That does not align with the description in the
> include/linux/dmaengine.h either.

True.

>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>> will pick a 'good/safe' burst value for it.
>>
>> Or new member, but do you need two of them for src/dst? Probably
>> fifo_depth is better word for it, or allocated_fifo_depth.
> 
> Right, so looking at the struct dma_slave_config we have ...
> 
> u32 src_maxburst;
> u32 dst_maxburst;
> u32 src_port_window_size;
> u32 dst_port_window_size;
> 
> Now if we could make these window sizes a union like the following this
> could work ...
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8fcdee1c0cf9..851251263527 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -360,8 +360,14 @@ struct dma_slave_config {
>         enum dma_slave_buswidth dst_addr_width;
>         u32 src_maxburst;
>         u32 dst_maxburst;
> -       u32 src_port_window_size;
> -       u32 dst_port_window_size;
> +       union {
> +               u32 port_window_size;
> +               u32 port_fifo_size;
> +       } src;
> +       union {
> +               u32 port_window_size;
> +               u32 port_fifo_size;
> +       } dst;

What if in the future someone will have a setup where they would need both?

So not sure. Your problems are coming from a split DMA setup where the
two are highly coupled, but sits in a different place and need to be
configured as one device.

I think xilinx_dma is facing with similar issues and they have a custom
API to set parameters which does not fit or is peripheral specific:
include/linux/dma/xilinx_dma.h

Not sure if that is an acceptable solution.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[Dmitry Osipenko]

07.06.2019 16:35, Peter Ujfalusi пишет:
> 
> 
> On 07/06/2019 15.58, Jon Hunter wrote:
>>> Imho if you can explain it without using 'HACK' in the sentences it
>>> might be OK, but it does not feel right.
>>
>> I don't perceive this as a hack. Although from looking at the
>> description of the src/dst_maxburst these are burst size with regard to
>> the device, so maybe it is a stretch.
>>
>>> However since your ADMA and ADMIF is highly coupled and it does needs
>>> special maxburst information (burst and allocated FIFO depth) I would
>>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>>
>>> ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
>>> ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)
>>>
>>> So lower 1 byte is the burst value you want from ADMA
>>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>>
>>> Sure, you need a header for this to make sure there is no
>>> misunderstanding between the two sides.
>>
>> I don't like this because as I mentioned to Dmitry, the ADMA can perform
>> memory-to-memory transfers where such encoding would not be applicable.
> 
> mem2mem does not really use dma_slave_config, it is for used by
> is_slave_direction() == true type of transfers.
> 
> But true, if you use ADMA against anything other than ADMAIF then this
> might be not right for non cyclic transfers.
> 
>> That does not align with the description in the
>> include/linux/dmaengine.h either.
> 
> True.
> 
>>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>>> will pick a 'good/safe' burst value for it.
>>>
>>> Or new member, but do you need two of them for src/dst? Probably
>>> fifo_depth is better word for it, or allocated_fifo_depth.
>>
>> Right, so looking at the struct dma_slave_config we have ...
>>
>> u32 src_maxburst;
>> u32 dst_maxburst;
>> u32 src_port_window_size;
>> u32 dst_port_window_size;
>>
>> Now if we could make these window sizes a union like the following this
>> could work ...
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 8fcdee1c0cf9..851251263527 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -360,8 +360,14 @@ struct dma_slave_config {
>>         enum dma_slave_buswidth dst_addr_width;
>>         u32 src_maxburst;
>>         u32 dst_maxburst;
>> -       u32 src_port_window_size;
>> -       u32 dst_port_window_size;
>> +       union {
>> +               u32 port_window_size;
>> +               u32 port_fifo_size;
>> +       } src;
>> +       union {
>> +               u32 port_window_size;
>> +               u32 port_fifo_size;
>> +       } dst;
> 
> What if in the future someone will have a setup where they would need both?
> 
> So not sure. Your problems are coming from a split DMA setup where the
> two are highly coupled, but sits in a different place and need to be
> configured as one device.
> 
> I think xilinx_dma is facing with similar issues and they have a custom
> API to set parameters which does not fit or is peripheral specific:
> include/linux/dma/xilinx_dma.h
> 
> Not sure if that is an acceptable solution.

If there are no other drivers with the exactly same requirement, then
the custom API is an a good variant given that there is a precedent
already. It is always possible to convert to a common thing later on
since that's all internal to kernel.

Jon / Sameer, you should check all the other drivers thoroughly to find
anyone who is doing the same thing as you need in order to achieve
something that is really common. I'm also wondering if it will be
possible to make dma_slave_config more flexible in order to start
accepting vendor specific properties in a somewhat common fashion, maybe
Vinod and Dan already have some thoughts on it? Apparently there is
already a need for the customization and people are just starting to
invent their own thing, but maybe that's fine too. That's really up to
subsys maintainer to decide in what direction to steer.

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

[Jon Hunter]

On 07/06/2019 14:35, Peter Ujfalusi wrote:
> 
> 
> On 07/06/2019 15.58, Jon Hunter wrote:
>>> Imho if you can explain it without using 'HACK' in the sentences it
>>> might be OK, but it does not feel right.
>>
>> I don't perceive this as a hack. Although from looking at the
>> description of the src/dst_maxburst these are burst size with regard to
>> the device, so maybe it is a stretch.
>>
>>> However since your ADMA and ADMIF is highly coupled and it does needs
>>> special maxburst information (burst and allocated FIFO depth) I would
>>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>>
>>> ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
>>> ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)
>>>
>>> So lower 1 byte is the burst value you want from ADMA
>>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>>
>>> Sure, you need a header for this to make sure there is no
>>> misunderstanding between the two sides.
>>
>> I don't like this because as I mentioned to Dmitry, the ADMA can perform
>> memory-to-memory transfers where such encoding would not be applicable.
> 
> mem2mem does not really use dma_slave_config, it is for used by
> is_slave_direction() == true type of transfers.
> 
> But true, if you use ADMA against anything other than ADMAIF then this
> might be not right for non cyclic transfers.
> 
>> That does not align with the description in the
>> include/linux/dmaengine.h either.
> 
> True.
> 
>>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>>> will pick a 'good/safe' burst value for it.
>>>
>>> Or new member, but do you need two of them for src/dst? Probably
>>> fifo_depth is better word for it, or allocated_fifo_depth.
>>
>> Right, so looking at the struct dma_slave_config we have ...
>>
>> u32 src_maxburst;
>> u32 dst_maxburst;
>> u32 src_port_window_size;
>> u32 dst_port_window_size;
>>
>> Now if we could make these window sizes a union like the following this
>> could work ...
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 8fcdee1c0cf9..851251263527 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -360,8 +360,14 @@ struct dma_slave_config {
>>         enum dma_slave_buswidth dst_addr_width;
>>         u32 src_maxburst;
>>         u32 dst_maxburst;
>> -       u32 src_port_window_size;
>> -       u32 dst_port_window_size;
>> +       union {
>> +               u32 port_window_size;
>> +               u32 port_fifo_size;
>> +       } src;
>> +       union {
>> +               u32 port_window_size;
>> +               u32 port_fifo_size;
>> +       } dst;
> 
> What if in the future someone will have a setup where they would need both?

I think if you look at the description for the port_window_size you will
see that this is not applicable for FIFOs and so these would be mutually
exclusive AFAICT. However, if there was an even weirder DMA out there in
the future it could always be patched :-)

> So not sure. Your problems are coming from a split DMA setup where the
> two are highly coupled, but sits in a different place and need to be
> configured as one device.
> 
> I think xilinx_dma is facing with similar issues and they have a custom
> API to set parameters which does not fit or is peripheral specific:
> include/linux/dma/xilinx_dma.h
> 
> Not sure if that is an acceptable solution.

I am not a fan of that but it could work.

Cheers
Jon

-- 
nvpublic

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

[Jon Hunter]

On 07/06/2019 21:53, Dmitry Osipenko wrote:
> 07.06.2019 16:35, Peter Ujfalusi пишет:
>>
>>
>> On 07/06/2019 15.58, Jon Hunter wrote:
>>>> Imho if you can explain it without using 'HACK' in the sentences it
>>>> might be OK, but it does not feel right.
>>>
>>> I don't perceive this as a hack. Although from looking at the
>>> description of the src/dst_maxburst these are burst size with regard to
>>> the device, so maybe it is a stretch.
>>>
>>>> However since your ADMA and ADMIF is highly coupled and it does needs
>>>> special maxburst information (burst and allocated FIFO depth) I would
>>>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>>>
>>>> ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
>>>> ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)
>>>>
>>>> So lower 1 byte is the burst value you want from ADMA
>>>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>>>
>>>> Sure, you need a header for this to make sure there is no
>>>> misunderstanding between the two sides.
>>>
>>> I don't like this because as I mentioned to Dmitry, the ADMA can perform
>>> memory-to-memory transfers where such encoding would not be applicable.
>>
>> mem2mem does not really use dma_slave_config, it is for used by
>> is_slave_direction() == true type of transfers.
>>
>> But true, if you use ADMA against anything other than ADMAIF then this
>> might be not right for non cyclic transfers.
>>
>>> That does not align with the description in the
>>> include/linux/dmaengine.h either.
>>
>> True.
>>
>>>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>>>> will pick a 'good/safe' burst value for it.
>>>>
>>>> Or new member, but do you need two of them for src/dst? Probably
>>>> fifo_depth is better word for it, or allocated_fifo_depth.
>>>
>>> Right, so looking at the struct dma_slave_config we have ...
>>>
>>> u32 src_maxburst;
>>> u32 dst_maxburst;
>>> u32 src_port_window_size;
>>> u32 dst_port_window_size;
>>>
>>> Now if we could make these window sizes a union like the following this
>>> could work ...
>>>
>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>> index 8fcdee1c0cf9..851251263527 100644
>>> --- a/include/linux/dmaengine.h
>>> +++ b/include/linux/dmaengine.h
>>> @@ -360,8 +360,14 @@ struct dma_slave_config {
>>>         enum dma_slave_buswidth dst_addr_width;
>>>         u32 src_maxburst;
>>>         u32 dst_maxburst;
>>> -       u32 src_port_window_size;
>>> -       u32 dst_port_window_size;
>>> +       union {
>>> +               u32 port_window_size;
>>> +               u32 port_fifo_size;
>>> +       } src;
>>> +       union {
>>> +               u32 port_window_size;
>>> +               u32 port_fifo_size;
>>> +       } dst;
>>
>> What if in the future someone will have a setup where they would need both?
>>
>> So not sure. Your problems are coming from a split DMA setup where the
>> two are highly coupled, but sits in a different place and need to be
>> configured as one device.
>>
>> I think xilinx_dma is facing with similar issues and they have a custom
>> API to set parameters which does not fit or is peripheral specific:
>> include/linux/dma/xilinx_dma.h
>>
>> Not sure if that is an acceptable solution.
> 
> If there are no other drivers with the exactly same requirement, then
> the custom API is an a good variant given that there is a precedent
> already. It is always possible to convert to a common thing later on
> since that's all internal to kernel.
> 
> Jon / Sameer, you should check all the other drivers thoroughly to find
> anyone who is doing the same thing as you need in order to achieve
> something that is really common. I'm also wondering if it will be
> possible to make dma_slave_config more flexible in order to start
> accepting vendor specific properties in a somewhat common fashion, maybe
> Vinod and Dan already have some thoughts on it? Apparently there is
> already a need for the customization and people are just starting to
> invent their own thing, but maybe that's fine too. That's really up to
> subsys maintainer to decide in what direction to steer.

I am not a fan of having custom APIs, however, I would agree that
extending the dma_slave_config to allow a DMA specific structure to be
passed with additional configuration would be useful in this case as
well as the Xilinx case.

Cheers
Jon

-- 
nvpublic