[RFC] dmaengine: add fifo_size member

[RFC] dmaengine: add fifo_size member

[Sameer Pujar]

During the DMA transfers from memory to I/O, it was observed that transfers
were inconsistent and resulted in glitches for audio playback. It happened
because fifo size on DMA did not match with slave channel configuration.

currently 'dma_slave_config' structure does not have a field for fifo size.
Hence the platform pcm driver cannot pass the fifo size as a slave_config.
Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
cannot be used to pass the size info. This patch introduces fifo_size field
and the same can be populated on slave side. Users can set required size
for slave peripheral (multiple channels can be independently running with
different fifo sizes) and the corresponding sizes are programmed through
dma_slave_config on DMA side.

Request for feedback/suggestions.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 include/linux/dmaengine.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index d49ec5c..9ec198b 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -351,6 +351,8 @@ enum dma_slave_buswidth {
  * @slave_id: Slave requester id. Only valid for slave channels. The dma
  * slave peripheral will have unique id as dma requester which need to be
  * pass as slave config.
+ * @fifo_size: Fifo size value. The dma slave peripheral can configure required
+ * fifo size and the same needs to be passed as slave config.
  *
  * This struct is passed in as configuration data to a DMA engine
  * in order to set up a certain channel for DMA transport at runtime.
@@ -376,6 +378,7 @@ struct dma_slave_config {
 	u32 dst_port_window_size;
 	bool device_fc;
 	unsigned int slave_id;
+	u32 fifo_size;
 };
 
 /**

[PATCH] [RFC] dmaengine: add fifo_size member

[Sameer Pujar]

During the DMA transfers from memory to I/O, it was observed that transfers
were inconsistent and resulted in glitches for audio playback. It happened
because fifo size on DMA did not match with slave channel configuration.

currently 'dma_slave_config' structure does not have a field for fifo size.
Hence the platform pcm driver cannot pass the fifo size as a slave_config.
Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
cannot be used to pass the size info. This patch introduces fifo_size field
and the same can be populated on slave side. Users can set required size
for slave peripheral (multiple channels can be independently running with
different fifo sizes) and the corresponding sizes are programmed through
dma_slave_config on DMA side.

Request for feedback/suggestions.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 include/linux/dmaengine.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index d49ec5c..9ec198b 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -351,6 +351,8 @@ enum dma_slave_buswidth {
  * @slave_id: Slave requester id. Only valid for slave channels. The dma
  * slave peripheral will have unique id as dma requester which need to be
  * pass as slave config.
+ * @fifo_size: Fifo size value. The dma slave peripheral can configure required
+ * fifo size and the same needs to be passed as slave config.
  *
  * This struct is passed in as configuration data to a DMA engine
  * in order to set up a certain channel for DMA transport at runtime.
@@ -376,6 +378,7 @@ struct dma_slave_config {
 	u32 dst_port_window_size;
 	bool device_fc;
 	unsigned int slave_id;
+	u32 fifo_size;
 };
 
 /**
-- 
2.7.4

[RFC] dmaengine: add fifo_size member

[Vinod Koul]

On 30-04-19, 17:00, Sameer Pujar wrote:
> During the DMA transfers from memory to I/O, it was observed that transfers
> were inconsistent and resulted in glitches for audio playback. It happened
> because fifo size on DMA did not match with slave channel configuration.
> 
> currently 'dma_slave_config' structure does not have a field for fifo size.
> Hence the platform pcm driver cannot pass the fifo size as a slave_config.
> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
> cannot be used to pass the size info. This patch introduces fifo_size field
> and the same can be populated on slave side. Users can set required size
> for slave peripheral (multiple channels can be independently running with
> different fifo sizes) and the corresponding sizes are programmed through
> dma_slave_config on DMA side.

FIFO size is a hardware property not sure why you would want an
interface to program that?

On mismatch, I guess you need to take care of src/dst_maxburst..

> 
> Request for feedback/suggestions.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> ---
>  include/linux/dmaengine.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index d49ec5c..9ec198b 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
>   * @slave_id: Slave requester id. Only valid for slave channels. The dma
>   * slave peripheral will have unique id as dma requester which need to be
>   * pass as slave config.
> + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
> + * fifo size and the same needs to be passed as slave config.
>   *
>   * This struct is passed in as configuration data to a DMA engine
>   * in order to set up a certain channel for DMA transport at runtime.
> @@ -376,6 +378,7 @@ struct dma_slave_config {
>  	u32 dst_port_window_size;
>  	bool device_fc;
>  	unsigned int slave_id;
> +	u32 fifo_size;
>  };
>  
>  /**
> -- 
> 2.7.4

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

[Vinod Koul]

On 30-04-19, 17:00, Sameer Pujar wrote:
> During the DMA transfers from memory to I/O, it was observed that transfers
> were inconsistent and resulted in glitches for audio playback. It happened
> because fifo size on DMA did not match with slave channel configuration.
> 
> currently 'dma_slave_config' structure does not have a field for fifo size.
> Hence the platform pcm driver cannot pass the fifo size as a slave_config.
> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
> cannot be used to pass the size info. This patch introduces fifo_size field
> and the same can be populated on slave side. Users can set required size
> for slave peripheral (multiple channels can be independently running with
> different fifo sizes) and the corresponding sizes are programmed through
> dma_slave_config on DMA side.

FIFO size is a hardware property not sure why you would want an
interface to program that?

On mismatch, I guess you need to take care of src/dst_maxburst..

> 
> Request for feedback/suggestions.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> ---
>  include/linux/dmaengine.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index d49ec5c..9ec198b 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
>   * @slave_id: Slave requester id. Only valid for slave channels. The dma
>   * slave peripheral will have unique id as dma requester which need to be
>   * pass as slave config.
> + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
> + * fifo size and the same needs to be passed as slave config.
>   *
>   * This struct is passed in as configuration data to a DMA engine
>   * in order to set up a certain channel for DMA transport at runtime.
> @@ -376,6 +378,7 @@ struct dma_slave_config {
>  	u32 dst_port_window_size;
>  	bool device_fc;
>  	unsigned int slave_id;
> +	u32 fifo_size;
>  };
>  
>  /**
> -- 
> 2.7.4

-- 
~Vinod

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

[Sameer Pujar]

On 5/2/2019 11:34 AM, Vinod Koul wrote:
> On 30-04-19, 17:00, Sameer Pujar wrote:
>> During the DMA transfers from memory to I/O, it was observed that transfers
>> were inconsistent and resulted in glitches for audio playback. It happened
>> because fifo size on DMA did not match with slave channel configuration.
>>
>> currently 'dma_slave_config' structure does not have a field for fifo size.
>> Hence the platform pcm driver cannot pass the fifo size as a slave_config.
>> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
>> cannot be used to pass the size info. This patch introduces fifo_size field
>> and the same can be populated on slave side. Users can set required size
>> for slave peripheral (multiple channels can be independently running with
>> different fifo sizes) and the corresponding sizes are programmed through
>> dma_slave_config on DMA side.
> FIFO size is a hardware property not sure why you would want an
> interface to program that?
>
> On mismatch, I guess you need to take care of src/dst_maxburst..
Yes, FIFO size is a HW property. But it is SW configurable(atleast in my 
case) on
slave side and can be set to different sizes. The src/dst_maxburst is 
programmed
for specific values, I think this depends on few factors related to 
bandwidth
needs of client, DMA needs of the system etc.,
In such cases how does DMA know the actual FIFO depth of slave peripheral?
>> Request for feedback/suggestions.
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> ---
>>   include/linux/dmaengine.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index d49ec5c..9ec198b 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
>>    * @slave_id: Slave requester id. Only valid for slave channels. The dma
>>    * slave peripheral will have unique id as dma requester which need to be
>>    * pass as slave config.
>> + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
>> + * fifo size and the same needs to be passed as slave config.
>>    *
>>    * This struct is passed in as configuration data to a DMA engine
>>    * in order to set up a certain channel for DMA transport at runtime.
>> @@ -376,6 +378,7 @@ struct dma_slave_config {
>>   	u32 dst_port_window_size;
>>   	bool device_fc;
>>   	unsigned int slave_id;
>> +	u32 fifo_size;
>>   };
>>   
>>   /**
>> -- 
>> 2.7.4

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

[Vinod Koul]

On 02-05-19, 16:23, Sameer Pujar wrote:
> 
> On 5/2/2019 11:34 AM, Vinod Koul wrote:
> > On 30-04-19, 17:00, Sameer Pujar wrote:
> > > During the DMA transfers from memory to I/O, it was observed that transfers
> > > were inconsistent and resulted in glitches for audio playback. It happened
> > > because fifo size on DMA did not match with slave channel configuration.
> > > 
> > > currently 'dma_slave_config' structure does not have a field for fifo size.
> > > Hence the platform pcm driver cannot pass the fifo size as a slave_config.
> > > Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
> > > cannot be used to pass the size info. This patch introduces fifo_size field
> > > and the same can be populated on slave side. Users can set required size
> > > for slave peripheral (multiple channels can be independently running with
> > > different fifo sizes) and the corresponding sizes are programmed through
> > > dma_slave_config on DMA side.
> > FIFO size is a hardware property not sure why you would want an
> > interface to program that?
> > 
> > On mismatch, I guess you need to take care of src/dst_maxburst..
> Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
> case) on
> slave side and can be set to different sizes. The src/dst_maxburst is

Are you sure, have you talked to HW folks on that? IIUC you are
programming the data to be used in FIFO not the FIFO length!

> programmed
> for specific values, I think this depends on few factors related to
> bandwidth
> needs of client, DMA needs of the system etc.,

Precisely

> In such cases how does DMA know the actual FIFO depth of slave peripheral?

Why should DMA know? Its job is to push/pull data as configured by
peripheral driver. The peripheral driver knows and configures DMA
accordingly.

 
> > > Request for feedback/suggestions.
> > > 
> > > Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> > > ---
> > >   include/linux/dmaengine.h | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > > index d49ec5c..9ec198b 100644
> > > --- a/include/linux/dmaengine.h
> > > +++ b/include/linux/dmaengine.h
> > > @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
> > >    * @slave_id: Slave requester id. Only valid for slave channels. The dma
> > >    * slave peripheral will have unique id as dma requester which need to be
> > >    * pass as slave config.
> > > + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
> > > + * fifo size and the same needs to be passed as slave config.
> > >    *
> > >    * This struct is passed in as configuration data to a DMA engine
> > >    * in order to set up a certain channel for DMA transport at runtime.
> > > @@ -376,6 +378,7 @@ struct dma_slave_config {
> > >   	u32 dst_port_window_size;
> > >   	bool device_fc;
> > >   	unsigned int slave_id;
> > > +	u32 fifo_size;
> > >   };
> > >   /**
> > > -- 
> > > 2.7.4

-- 
~Vinod

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

[Sameer Pujar]

On 5/2/2019 5:55 PM, Vinod Koul wrote:
> On 02-05-19, 16:23, Sameer Pujar wrote:
>> On 5/2/2019 11:34 AM, Vinod Koul wrote:
>>> On 30-04-19, 17:00, Sameer Pujar wrote:
>>>> During the DMA transfers from memory to I/O, it was observed that transfers
>>>> were inconsistent and resulted in glitches for audio playback. It happened
>>>> because fifo size on DMA did not match with slave channel configuration.
>>>>
>>>> currently 'dma_slave_config' structure does not have a field for fifo size.
>>>> Hence the platform pcm driver cannot pass the fifo size as a slave_config.
>>>> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
>>>> cannot be used to pass the size info. This patch introduces fifo_size field
>>>> and the same can be populated on slave side. Users can set required size
>>>> for slave peripheral (multiple channels can be independently running with
>>>> different fifo sizes) and the corresponding sizes are programmed through
>>>> dma_slave_config on DMA side.
>>> FIFO size is a hardware property not sure why you would want an
>>> interface to program that?
>>>
>>> On mismatch, I guess you need to take care of src/dst_maxburst..
>> Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
>> case) on
>> slave side and can be set to different sizes. The src/dst_maxburst is
> Are you sure, have you talked to HW folks on that? IIUC you are
> programming the data to be used in FIFO not the FIFO length!
Yes, I mentioned about FIFO length.

1. MAX FIFO size is fixed in HW. But there is a way to limit the usage 
per channel
    in multiples of 64 bytes.
2. Having a separate member would give independent control over MAX 
BURST SIZE and
    FIFO SIZE.
>
>> programmed
>> for specific values, I think this depends on few factors related to
>> bandwidth
>> needs of client, DMA needs of the system etc.,
> Precisely
>
>> In such cases how does DMA know the actual FIFO depth of slave peripheral?
> Why should DMA know? Its job is to push/pull data as configured by
> peripheral driver. The peripheral driver knows and configures DMA
> accordingly.
I am not sure if there is any HW logic that mandates DMA to know the size
of configured FIFO depth on slave side. I will speak to HW folks and
would update here.
>   
>>>> Request for feedback/suggestions.
>>>>
>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>> ---
>>>>    include/linux/dmaengine.h | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>> index d49ec5c..9ec198b 100644
>>>> --- a/include/linux/dmaengine.h
>>>> +++ b/include/linux/dmaengine.h
>>>> @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
>>>>     * @slave_id: Slave requester id. Only valid for slave channels. The dma
>>>>     * slave peripheral will have unique id as dma requester which need to be
>>>>     * pass as slave config.
>>>> + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
>>>> + * fifo size and the same needs to be passed as slave config.
>>>>     *
>>>>     * This struct is passed in as configuration data to a DMA engine
>>>>     * in order to set up a certain channel for DMA transport at runtime.
>>>> @@ -376,6 +378,7 @@ struct dma_slave_config {
>>>>    	u32 dst_port_window_size;
>>>>    	bool device_fc;
>>>>    	unsigned int slave_id;
>>>> +	u32 fifo_size;
>>>>    };
>>>>    /**
>>>> -- 
>>>> 2.7.4

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

[Peter Ujfalusi]

On 5/2/19 4:29 PM, Sameer Pujar wrote:
> 
> On 5/2/2019 5:55 PM, Vinod Koul wrote:
>> On 02-05-19, 16:23, Sameer Pujar wrote:
>>> On 5/2/2019 11:34 AM, Vinod Koul wrote:
>>>> On 30-04-19, 17:00, Sameer Pujar wrote:
>>>>> During the DMA transfers from memory to I/O, it was observed that
>>>>> transfers
>>>>> were inconsistent and resulted in glitches for audio playback. It
>>>>> happened
>>>>> because fifo size on DMA did not match with slave channel
>>>>> configuration.
>>>>>
>>>>> currently 'dma_slave_config' structure does not have a field for
>>>>> fifo size.
>>>>> Hence the platform pcm driver cannot pass the fifo size as a
>>>>> slave_config.
>>>>> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size
>>>>> field which
>>>>> cannot be used to pass the size info. This patch introduces
>>>>> fifo_size field
>>>>> and the same can be populated on slave side. Users can set required
>>>>> size
>>>>> for slave peripheral (multiple channels can be independently
>>>>> running with
>>>>> different fifo sizes) and the corresponding sizes are programmed
>>>>> through
>>>>> dma_slave_config on DMA side.
>>>> FIFO size is a hardware property not sure why you would want an
>>>> interface to program that?
>>>>
>>>> On mismatch, I guess you need to take care of src/dst_maxburst..
>>> Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
>>> case) on
>>> slave side and can be set to different sizes. The src/dst_maxburst is
>> Are you sure, have you talked to HW folks on that? IIUC you are
>> programming the data to be used in FIFO not the FIFO length!
> Yes, I mentioned about FIFO length.
> 
> 1. MAX FIFO size is fixed in HW. But there is a way to limit the usage
> per channel
>    in multiples of 64 bytes.
> 2. Having a separate member would give independent control over MAX
> BURST SIZE and
>    FIFO SIZE.

Why would the DMA care about the FIFO size of a peripheral?
All it should be concerned that how many bytes it should transfer per
DMA request from the peripheral.
It is the task of the peripheral driver to choose the maxburst to match
with the peripheral's configuration and the peripheral configuration
should not allow maxburst to overflow (or underflow) the peripheral FIFO.

>>
>>> programmed
>>> for specific values, I think this depends on few factors related to
>>> bandwidth
>>> needs of client, DMA needs of the system etc.,
>> Precisely
>>
>>> In such cases how does DMA know the actual FIFO depth of slave
>>> peripheral?
>> Why should DMA know? Its job is to push/pull data as configured by
>> peripheral driver. The peripheral driver knows and configures DMA
>> accordingly.
> I am not sure if there is any HW logic that mandates DMA to know the size
> of configured FIFO depth on slave side. I will speak to HW folks and
> would update here.
>>  
>>>>> Request for feedback/suggestions.
>>>>>
>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>> ---
>>>>>    include/linux/dmaengine.h | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>>> index d49ec5c..9ec198b 100644
>>>>> --- a/include/linux/dmaengine.h
>>>>> +++ b/include/linux/dmaengine.h
>>>>> @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
>>>>>     * @slave_id: Slave requester id. Only valid for slave channels.
>>>>> The dma
>>>>>     * slave peripheral will have unique id as dma requester which
>>>>> need to be
>>>>>     * pass as slave config.
>>>>> + * @fifo_size: Fifo size value. The dma slave peripheral can
>>>>> configure required
>>>>> + * fifo size and the same needs to be passed as slave config.
>>>>>     *
>>>>>     * This struct is passed in as configuration data to a DMA engine
>>>>>     * in order to set up a certain channel for DMA transport at
>>>>> runtime.
>>>>> @@ -376,6 +378,7 @@ struct dma_slave_config {
>>>>>        u32 dst_port_window_size;
>>>>>        bool device_fc;
>>>>>        unsigned int slave_id;
>>>>> +    u32 fifo_size;
>>>>>    };
>>>>>    /**
>>>>> -- 
>>>>> 2.7.4

- Peter

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

[Vinod Koul]

On 02-05-19, 18:59, Sameer Pujar wrote:
> 
> On 5/2/2019 5:55 PM, Vinod Koul wrote:
> > On 02-05-19, 16:23, Sameer Pujar wrote:
> > > On 5/2/2019 11:34 AM, Vinod Koul wrote:
> > > > On 30-04-19, 17:00, Sameer Pujar wrote:
> > > > > During the DMA transfers from memory to I/O, it was observed that transfers
> > > > > were inconsistent and resulted in glitches for audio playback. It happened
> > > > > because fifo size on DMA did not match with slave channel configuration.
> > > > > 
> > > > > currently 'dma_slave_config' structure does not have a field for fifo size.
> > > > > Hence the platform pcm driver cannot pass the fifo size as a slave_config.
> > > > > Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
> > > > > cannot be used to pass the size info. This patch introduces fifo_size field
> > > > > and the same can be populated on slave side. Users can set required size
> > > > > for slave peripheral (multiple channels can be independently running with
> > > > > different fifo sizes) and the corresponding sizes are programmed through
> > > > > dma_slave_config on DMA side.
> > > > FIFO size is a hardware property not sure why you would want an
> > > > interface to program that?
> > > > 
> > > > On mismatch, I guess you need to take care of src/dst_maxburst..
> > > Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
> > > case) on
> > > slave side and can be set to different sizes. The src/dst_maxburst is
> > Are you sure, have you talked to HW folks on that? IIUC you are
> > programming the data to be used in FIFO not the FIFO length!
> Yes, I mentioned about FIFO length.
> 
> 1. MAX FIFO size is fixed in HW. But there is a way to limit the usage per
> channel
>    in multiples of 64 bytes.
> 2. Having a separate member would give independent control over MAX BURST
> SIZE and
>    FIFO SIZE.
> > 
> > > programmed
> > > for specific values, I think this depends on few factors related to
> > > bandwidth
> > > needs of client, DMA needs of the system etc.,
> > Precisely
> > 
> > > In such cases how does DMA know the actual FIFO depth of slave peripheral?
> > Why should DMA know? Its job is to push/pull data as configured by
> > peripheral driver. The peripheral driver knows and configures DMA
> > accordingly.
> I am not sure if there is any HW logic that mandates DMA to know the size
> of configured FIFO depth on slave side. I will speak to HW folks and
> would update here.

I still do not comprehend why dma would care about slave side
configuration. In the absence of patch which uses this I am not sure
what you are trying to do...

> > > > > Request for feedback/suggestions.
> > > > > 
> > > > > Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> > > > > ---
> > > > >    include/linux/dmaengine.h | 3 +++
> > > > >    1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > > > > index d49ec5c..9ec198b 100644
> > > > > --- a/include/linux/dmaengine.h
> > > > > +++ b/include/linux/dmaengine.h
> > > > > @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
> > > > >     * @slave_id: Slave requester id. Only valid for slave channels. The dma
> > > > >     * slave peripheral will have unique id as dma requester which need to be
> > > > >     * pass as slave config.
> > > > > + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
> > > > > + * fifo size and the same needs to be passed as slave config.
> > > > >     *
> > > > >     * This struct is passed in as configuration data to a DMA engine
> > > > >     * in order to set up a certain channel for DMA transport at runtime.
> > > > > @@ -376,6 +378,7 @@ struct dma_slave_config {
> > > > >    	u32 dst_port_window_size;
> > > > >    	bool device_fc;
> > > > >    	unsigned int slave_id;
> > > > > +	u32 fifo_size;
> > > > >    };
> > > > >    /**
> > > > > -- 
> > > > > 2.7.4

-- 
~Vinod

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

[Sameer Pujar]

On 5/4/2019 3:53 PM, Vinod Koul wrote:
> On 02-05-19, 18:59, Sameer Pujar wrote:
>> On 5/2/2019 5:55 PM, Vinod Koul wrote:
>>> On 02-05-19, 16:23, Sameer Pujar wrote:
>>>> On 5/2/2019 11:34 AM, Vinod Koul wrote:
>>>>> On 30-04-19, 17:00, Sameer Pujar wrote:
>>>>>> During the DMA transfers from memory to I/O, it was observed that transfers
>>>>>> were inconsistent and resulted in glitches for audio playback. It happened
>>>>>> because fifo size on DMA did not match with slave channel configuration.
>>>>>>
>>>>>> currently 'dma_slave_config' structure does not have a field for fifo size.
>>>>>> Hence the platform pcm driver cannot pass the fifo size as a slave_config.
>>>>>> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
>>>>>> cannot be used to pass the size info. This patch introduces fifo_size field
>>>>>> and the same can be populated on slave side. Users can set required size
>>>>>> for slave peripheral (multiple channels can be independently running with
>>>>>> different fifo sizes) and the corresponding sizes are programmed through
>>>>>> dma_slave_config on DMA side.
>>>>> FIFO size is a hardware property not sure why you would want an
>>>>> interface to program that?
>>>>>
>>>>> On mismatch, I guess you need to take care of src/dst_maxburst..
>>>> Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
>>>> case) on
>>>> slave side and can be set to different sizes. The src/dst_maxburst is
>>> Are you sure, have you talked to HW folks on that? IIUC you are
>>> programming the data to be used in FIFO not the FIFO length!
>> Yes, I mentioned about FIFO length.
>>
>> 1. MAX FIFO size is fixed in HW. But there is a way to limit the usage per
>> channel
>>     in multiples of 64 bytes.
>> 2. Having a separate member would give independent control over MAX BURST
>> SIZE and
>>     FIFO SIZE.
>>>> programmed
>>>> for specific values, I think this depends on few factors related to
>>>> bandwidth
>>>> needs of client, DMA needs of the system etc.,
>>> Precisely
>>>
>>>> In such cases how does DMA know the actual FIFO depth of slave peripheral?
>>> Why should DMA know? Its job is to push/pull data as configured by
>>> peripheral driver. The peripheral driver knows and configures DMA
>>> accordingly.
>> I am not sure if there is any HW logic that mandates DMA to know the size
>> of configured FIFO depth on slave side. I will speak to HW folks and
>> would update here.
> I still do not comprehend why dma would care about slave side
> configuration. In the absence of patch which uses this I am not sure
> what you are trying to do...

I am using DMA HW in cyclic mode for data transfers to Audio sub-system.
In such cases flow control on DMA transfers is essential, since I/O is
consuming/producing the data at slower rate. The DMA tranfer is enabled/
disabled during start/stop of audio playback/capture sessions through ALSA
callbacks and DMA runs in cyclic mode. Hence DMA is the one which is doing
flow control and it is necessary for it to know the peripheral FIFO depth
to avoid overruns/underruns.

Also please note that, peripheral device has multiple channels and share
a fixed MAX FIFO buffer. But SW can program different FIFO sizes for
individual channels.

>>>>>> Request for feedback/suggestions.
>>>>>>
>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>>>> ---
>>>>>>     include/linux/dmaengine.h | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>>>> index d49ec5c..9ec198b 100644
>>>>>> --- a/include/linux/dmaengine.h
>>>>>> +++ b/include/linux/dmaengine.h
>>>>>> @@ -351,6 +351,8 @@ enum dma_slave_buswidth {
>>>>>>      * @slave_id: Slave requester id. Only valid for slave channels. The dma
>>>>>>      * slave peripheral will have unique id as dma requester which need to be
>>>>>>      * pass as slave config.
>>>>>> + * @fifo_size: Fifo size value. The dma slave peripheral can configure required
>>>>>> + * fifo size and the same needs to be passed as slave config.
>>>>>>      *
>>>>>>      * This struct is passed in as configuration data to a DMA engine
>>>>>>      * in order to set up a certain channel for DMA transport at runtime.
>>>>>> @@ -376,6 +378,7 @@ struct dma_slave_config {
>>>>>>     	u32 dst_port_window_size;
>>>>>>     	bool device_fc;
>>>>>>     	unsigned int slave_id;
>>>>>> +	u32 fifo_size;
>>>>>>     };
>>>>>>     /**
>>>>>> -- 
>>>>>> 2.7.4

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

[Vinod Koul]

On 06-05-19, 18:34, Sameer Pujar wrote:
> 
> On 5/4/2019 3:53 PM, Vinod Koul wrote:
> > On 02-05-19, 18:59, Sameer Pujar wrote:
> > > On 5/2/2019 5:55 PM, Vinod Koul wrote:
> > > > On 02-05-19, 16:23, Sameer Pujar wrote:
> > > > > On 5/2/2019 11:34 AM, Vinod Koul wrote:
> > > > > > On 30-04-19, 17:00, Sameer Pujar wrote:
> > > > > > > During the DMA transfers from memory to I/O, it was observed that transfers
> > > > > > > were inconsistent and resulted in glitches for audio playback. It happened
> > > > > > > because fifo size on DMA did not match with slave channel configuration.
> > > > > > > 
> > > > > > > currently 'dma_slave_config' structure does not have a field for fifo size.
> > > > > > > Hence the platform pcm driver cannot pass the fifo size as a slave_config.
> > > > > > > Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
> > > > > > > cannot be used to pass the size info. This patch introduces fifo_size field
> > > > > > > and the same can be populated on slave side. Users can set required size
> > > > > > > for slave peripheral (multiple channels can be independently running with
> > > > > > > different fifo sizes) and the corresponding sizes are programmed through
> > > > > > > dma_slave_config on DMA side.
> > > > > > FIFO size is a hardware property not sure why you would want an
> > > > > > interface to program that?
> > > > > > 
> > > > > > On mismatch, I guess you need to take care of src/dst_maxburst..
> > > > > Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
> > > > > case) on
> > > > > slave side and can be set to different sizes. The src/dst_maxburst is
> > > > Are you sure, have you talked to HW folks on that? IIUC you are
> > > > programming the data to be used in FIFO not the FIFO length!
> > > Yes, I mentioned about FIFO length.
> > > 
> > > 1. MAX FIFO size is fixed in HW. But there is a way to limit the usage per
> > > channel
> > >     in multiples of 64 bytes.
> > > 2. Having a separate member would give independent control over MAX BURST
> > > SIZE and
> > >     FIFO SIZE.
> > > > > programmed
> > > > > for specific values, I think this depends on few factors related to
> > > > > bandwidth
> > > > > needs of client, DMA needs of the system etc.,
> > > > Precisely
> > > > 
> > > > > In such cases how does DMA know the actual FIFO depth of slave peripheral?
> > > > Why should DMA know? Its job is to push/pull data as configured by
> > > > peripheral driver. The peripheral driver knows and configures DMA
> > > > accordingly.
> > > I am not sure if there is any HW logic that mandates DMA to know the size
> > > of configured FIFO depth on slave side. I will speak to HW folks and
> > > would update here.
> > I still do not comprehend why dma would care about slave side
> > configuration. In the absence of patch which uses this I am not sure
> > what you are trying to do...
> 
> I am using DMA HW in cyclic mode for data transfers to Audio sub-system.
> In such cases flow control on DMA transfers is essential, since I/O is

right and people use burst size for precisely that!

> consuming/producing the data at slower rate. The DMA tranfer is enabled/
> disabled during start/stop of audio playback/capture sessions through ALSA
> callbacks and DMA runs in cyclic mode. Hence DMA is the one which is doing
> flow control and it is necessary for it to know the peripheral FIFO depth
> to avoid overruns/underruns.

not really, knowing that doesnt help anyway you have described! DMA
pushes/pulls data and that is controlled by burst configured by slave
(so it know what to expect and porgrams things accordingly)

you are really going other way around about the whole picture. FWIW that
is how *other* folks do audio with dmaengine!

> Also please note that, peripheral device has multiple channels and share
> a fixed MAX FIFO buffer. But SW can program different FIFO sizes for
> individual channels.

yeah peripheral driver, yes. DMA driver nope!

-- 
~Vinod

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

[Sameer Pujar]

Sorry for late reply.
[Resending the reply since delivery failed for few recipients]

On 5/6/2019 9:20 PM, Vinod Koul wrote:
> On 06-05-19, 18:34, Sameer Pujar wrote:
>> On 5/4/2019 3:53 PM, Vinod Koul wrote:
>>> On 02-05-19, 18:59, Sameer Pujar wrote:
>>>> On 5/2/2019 5:55 PM, Vinod Koul wrote:
>>>>> On 02-05-19, 16:23, Sameer Pujar wrote:
>>>>>> On 5/2/2019 11:34 AM, Vinod Koul wrote:
>>>>>>> On 30-04-19, 17:00, Sameer Pujar wrote:
>>>>>>>> During the DMA transfers from memory to I/O, it was observed that transfers
>>>>>>>> were inconsistent and resulted in glitches for audio playback. It happened
>>>>>>>> because fifo size on DMA did not match with slave channel configuration.
>>>>>>>>
>>>>>>>> currently 'dma_slave_config' structure does not have a field for fifo size.
>>>>>>>> Hence the platform pcm driver cannot pass the fifo size as a slave_config.
>>>>>>>> Note that 'snd_dmaengine_dai_dma_data' structure has fifo_size field which
>>>>>>>> cannot be used to pass the size info. This patch introduces fifo_size field
>>>>>>>> and the same can be populated on slave side. Users can set required size
>>>>>>>> for slave peripheral (multiple channels can be independently running with
>>>>>>>> different fifo sizes) and the corresponding sizes are programmed through
>>>>>>>> dma_slave_config on DMA side.
>>>>>>> FIFO size is a hardware property not sure why you would want an
>>>>>>> interface to program that?
>>>>>>>
>>>>>>> On mismatch, I guess you need to take care of src/dst_maxburst..
>>>>>> Yes, FIFO size is a HW property. But it is SW configurable(atleast in my
>>>>>> case) on
>>>>>> slave side and can be set to different sizes. The src/dst_maxburst is
>>>>> Are you sure, have you talked to HW folks on that? IIUC you are
>>>>> programming the data to be used in FIFO not the FIFO length!
>>>> Yes, I mentioned about FIFO length.
>>>>
>>>> 1. MAX FIFO size is fixed in HW. But there is a way to limit the usage per
>>>> channel
>>>>      in multiples of 64 bytes.
>>>> 2. Having a separate member would give independent control over MAX BURST
>>>> SIZE and
>>>>      FIFO SIZE.
>>>>>> programmed
>>>>>> for specific values, I think this depends on few factors related to
>>>>>> bandwidth
>>>>>> needs of client, DMA needs of the system etc.,
>>>>> Precisely
>>>>>
>>>>>> In such cases how does DMA know the actual FIFO depth of slave peripheral?
>>>>> Why should DMA know? Its job is to push/pull data as configured by
>>>>> peripheral driver. The peripheral driver knows and configures DMA
>>>>> accordingly.
>>>> I am not sure if there is any HW logic that mandates DMA to know the size
>>>> of configured FIFO depth on slave side. I will speak to HW folks and
>>>> would update here.
>>> I still do not comprehend why dma would care about slave side
>>> configuration. In the absence of patch which uses this I am not sure
>>> what you are trying to do...
>> I am using DMA HW in cyclic mode for data transfers to Audio sub-system.
>> In such cases flow control on DMA transfers is essential, since I/O is
> right and people use burst size for precisely that!
>
>> consuming/producing the data at slower rate. The DMA tranfer is enabled/
>> disabled during start/stop of audio playback/capture sessions through ALSA
>> callbacks and DMA runs in cyclic mode. Hence DMA is the one which is doing
>> flow control and it is necessary for it to know the peripheral FIFO depth
>> to avoid overruns/underruns.
> not really, knowing that doesnt help anyway you have described! DMA
> pushes/pulls data and that is controlled by burst configured by slave
> (so it know what to expect and porgrams things accordingly)
>
> you are really going other way around about the whole picture. FWIW that
> is how *other* folks do audio with dmaengine!
I discussed this internally with HW folks and below is the reason why 
DMA needs
to know FIFO size.

- FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface 
to the audio sub-system.
- ADMAIF has multiple channels and share FIFO buffer for individual 
operations. There is a provision
   to allocate specific fifo size for each individual ADMAIF channel 
from the shared buffer.
- Tegra Audio DMA(ADMA) architecture is different from the usual DMA 
engines, which you described earlier.
- The flow control logic is placed inside ADMA. Slave peripheral 
device(ADMAIF) signals ADMA whenever a
   read or write happens on the FIFO(per WORD basis). Please note that 
the signaling is per channel. There is
   no other signaling present from ADMAIF to ADMA.
- ADMA keeps a counter related to above signaling. Whenever a sufficient 
space is available, it initiates a transfer.
   But the question is, how does it know when to transfer. This is the 
reason, why ADMA has to be aware of FIFO
   depth of ADMAIF channel. Depending on the counters and FIFO depth, it 
knows exactly when a free space is available
   in the context of a specific channel. On ADMA, FIFO_SIZE is just a 
value which should match to actual FIFO_DEPTH/SIZE
   of ADMAIF channel.
- Now consider two cases based on above logic,
   * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
     In this case, ADMA thinks that there is enough space available for 
transfer, when actually the FIFO data
     on slave is not consumed yet. It would result in OVERRUN.
   * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
     This is case where ADMA won’t transfer, even though sufficient 
space is available, resulting in UNDERRUN.
- The guideline is to program, DMA_FIFO_SIZE(on ADMA side) = 
SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
   way to communicate fifo size info to ADMA.

Thanks,
Sameer.
>> Also please note that, peripheral device has multiple channels and share
>> a fixed MAX FIFO buffer. But SW can program different FIFO sizes for
>> individual channels.
> yeah peripheral driver, yes. DMA driver nope!
>

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

[Peter Ujfalusi]

Hi Sameer,

On 06/06/2019 6.49, Sameer Pujar wrote:
> Sorry for late reply.
> [Resending the reply since delivery failed for few recipients]
> I discussed this internally with HW folks and below is the reason why
> DMA needs
> to know FIFO size.
> 
> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
> to the audio sub-system.
> - ADMAIF has multiple channels and share FIFO buffer for individual
> operations. There is a provision
>   to allocate specific fifo size for each individual ADMAIF channel from
> the shared buffer.
> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
> engines, which you described earlier.

It is not really different than what other DMAs are doing.

> - The flow control logic is placed inside ADMA. Slave peripheral
> device(ADMAIF) signals ADMA whenever a
>   read or write happens on the FIFO(per WORD basis). Please note that
> the signaling is per channel. There is
>   no other signaling present from ADMAIF to ADMA.
> - ADMA keeps a counter related to above signaling. Whenever a sufficient
> space is available, it initiates a transfer.
>   But the question is, how does it know when to transfer. This is the
> reason, why ADMA has to be aware of FIFO
>   depth of ADMAIF channel. Depending on the counters and FIFO depth, it
> knows exactly when a free space is available
>   in the context of a specific channel. On ADMA, FIFO_SIZE is just a
> value which should match to actual FIFO_DEPTH/SIZE
>   of ADMAIF channel.
> - Now consider two cases based on above logic,
>   * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>     In this case, ADMA thinks that there is enough space available for
> transfer, when actually the FIFO data
>     on slave is not consumed yet. It would result in OVERRUN.
>   * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>     This is case where ADMA won’t transfer, even though sufficient space
> is available, resulting in UNDERRUN.
> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>   way to communicate fifo size info to ADMA.

The src_maxburst / dst_maxburst is exactly for this reason. To
communicate how much data should be transferred per DMA request to/from
peripheral.

In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
which is dynamically configured (you can see the AFIFO of McASP as a
small DMA: on McASP side it services the peripheral, on the other side
it interacts with the given system DMA used by the SoC - EDMA, sDMA or
UDMAP). All DMAs needs a bit different configuration, but the AFIFO
depth on the McASP side is coming in via the src/dst_maxburst and the
drivers just need to interpret it correctly.

It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as well.

- 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

[Sameer Pujar]

On 6/6/2019 11:30 AM, Peter Ujfalusi wrote:
> Hi Sameer,
>
> On 06/06/2019 6.49, Sameer Pujar wrote:
>> Sorry for late reply.
>> [Resending the reply since delivery failed for few recipients]
>> I discussed this internally with HW folks and below is the reason why
>> DMA needs
>> to know FIFO size.
>>
>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
>> to the audio sub-system.
>> - ADMAIF has multiple channels and share FIFO buffer for individual
>> operations. There is a provision
>>    to allocate specific fifo size for each individual ADMAIF channel from
>> the shared buffer.
>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>> engines, which you described earlier.
> It is not really different than what other DMAs are doing.
>
>> - The flow control logic is placed inside ADMA. Slave peripheral
>> device(ADMAIF) signals ADMA whenever a
>>    read or write happens on the FIFO(per WORD basis). Please note that
>> the signaling is per channel. There is
>>    no other signaling present from ADMAIF to ADMA.
>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>> space is available, it initiates a transfer.
>>    But the question is, how does it know when to transfer. This is the
>> reason, why ADMA has to be aware of FIFO
>>    depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>> knows exactly when a free space is available
>>    in the context of a specific channel. On ADMA, FIFO_SIZE is just a
>> value which should match to actual FIFO_DEPTH/SIZE
>>    of ADMAIF channel.
>> - Now consider two cases based on above logic,
>>    * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>      In this case, ADMA thinks that there is enough space available for
>> transfer, when actually the FIFO data
>>      on slave is not consumed yet. It would result in OVERRUN.
>>    * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>      This is case where ADMA won’t transfer, even though sufficient space
>> is available, resulting in UNDERRUN.
>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>    way to communicate fifo size info to ADMA.
> The src_maxburst / dst_maxburst is exactly for this reason. To
> communicate how much data should be transferred per DMA request to/from
> peripheral.
>
> In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
> which is dynamically configured (you can see the AFIFO of McASP as a
> small DMA: on McASP side it services the peripheral, on the other side
> it interacts with the given system DMA used by the SoC - EDMA, sDMA or
> UDMAP). All DMAs needs a bit different configuration, but the AFIFO
> depth on the McASP side is coming in via the src/dst_maxburst and the
> drivers just need to interpret it correctly.
>
> 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 without this RFC patch, I need to do following.
- pass FIFO_SIZE via src/dst_maxburst (from peripheral ADMAIF driver)
- DMA driver can have following code,
   * Program DMA_FIFO_SIZE value from src/dst_maxburst
   * Add below logic
     if (src/dst_maxburst > 16)
         program ADMA burst_size = 16
     else
         program ADMA burst_size = 8 (1/2 of minimum FIFO depth of 
ADMAIF channel)

With above, the flexibility to program ADMA burst_size to any of the 
required values in the specified range is not there.
Hence ideally would have liked to have independent control over 
burst_size and fifo_size on ADMA side.
If there is no merit in this, I will update ADMA/ADMAIF driver as 
mentioned above.

Thanks,
Sameer.

>
> - 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 07:41, Sameer Pujar wrote:
> 
> On 6/6/2019 11:30 AM, Peter Ujfalusi wrote:
>> Hi Sameer,
>>
>> On 06/06/2019 6.49, Sameer Pujar wrote:
>>> Sorry for late reply.
>>> [Resending the reply since delivery failed for few recipients]
>>> I discussed this internally with HW folks and below is the reason why
>>> DMA needs
>>> to know FIFO size.
>>>
>>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
>>> to the audio sub-system.
>>> - ADMAIF has multiple channels and share FIFO buffer for individual
>>> operations. There is a provision
>>>    to allocate specific fifo size for each individual ADMAIF channel
>>> from
>>> the shared buffer.
>>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>>> engines, which you described earlier.
>> It is not really different than what other DMAs are doing.
>>
>>> - The flow control logic is placed inside ADMA. Slave peripheral
>>> device(ADMAIF) signals ADMA whenever a
>>>    read or write happens on the FIFO(per WORD basis). Please note that
>>> the signaling is per channel. There is
>>>    no other signaling present from ADMAIF to ADMA.
>>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>>> space is available, it initiates a transfer.
>>>    But the question is, how does it know when to transfer. This is the
>>> reason, why ADMA has to be aware of FIFO
>>>    depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>>> knows exactly when a free space is available
>>>    in the context of a specific channel. On ADMA, FIFO_SIZE is just a
>>> value which should match to actual FIFO_DEPTH/SIZE
>>>    of ADMAIF channel.
>>> - Now consider two cases based on above logic,
>>>    * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>>      In this case, ADMA thinks that there is enough space available for
>>> transfer, when actually the FIFO data
>>>      on slave is not consumed yet. It would result in OVERRUN.
>>>    * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>>      This is case where ADMA won’t transfer, even though sufficient
>>> space
>>> is available, resulting in UNDERRUN.
>>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>>    way to communicate fifo size info to ADMA.
>> The src_maxburst / dst_maxburst is exactly for this reason. To
>> communicate how much data should be transferred per DMA request to/from
>> peripheral.
>>
>> In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
>> which is dynamically configured (you can see the AFIFO of McASP as a
>> small DMA: on McASP side it services the peripheral, on the other side
>> it interacts with the given system DMA used by the SoC - EDMA, sDMA or
>> UDMAP). All DMAs needs a bit different configuration, but the AFIFO
>> depth on the McASP side is coming in via the src/dst_maxburst and the
>> drivers just need to interpret it correctly.
>>
>> 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.

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.

Cheers
Jon

-- 
nvpublic

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

[Peter Ujfalusi]

Hi Jon, Sameer,

On 06/06/2019 10.14, Jon Hunter wrote:
> 
> On 06/06/2019 07:41, Sameer Pujar wrote:
>>
>> On 6/6/2019 11:30 AM, Peter Ujfalusi wrote:
>>> Hi Sameer,
>>>
>>> On 06/06/2019 6.49, Sameer Pujar wrote:
>>>> Sorry for late reply.
>>>> [Resending the reply since delivery failed for few recipients]
>>>> I discussed this internally with HW folks and below is the reason why
>>>> DMA needs
>>>> to know FIFO size.
>>>>
>>>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
>>>> to the audio sub-system.
>>>> - ADMAIF has multiple channels and share FIFO buffer for individual
>>>> operations. There is a provision
>>>>    to allocate specific fifo size for each individual ADMAIF channel
>>>> from
>>>> the shared buffer.
>>>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>>>> engines, which you described earlier.
>>> It is not really different than what other DMAs are doing.
>>>
>>>> - The flow control logic is placed inside ADMA. Slave peripheral
>>>> device(ADMAIF) signals ADMA whenever a
>>>>    read or write happens on the FIFO(per WORD basis). Please note that
>>>> the signaling is per channel. There is
>>>>    no other signaling present from ADMAIF to ADMA.
>>>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>>>> space is available, it initiates a transfer.
>>>>    But the question is, how does it know when to transfer. This is the
>>>> reason, why ADMA has to be aware of FIFO
>>>>    depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>>>> knows exactly when a free space is available
>>>>    in the context of a specific channel. On ADMA, FIFO_SIZE is just a
>>>> value which should match to actual FIFO_DEPTH/SIZE
>>>>    of ADMAIF channel.
>>>> - Now consider two cases based on above logic,
>>>>    * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>>>      In this case, ADMA thinks that there is enough space available for
>>>> transfer, when actually the FIFO data
>>>>      on slave is not consumed yet. It would result in OVERRUN.
>>>>    * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>>>      This is case where ADMA won’t transfer, even though sufficient
>>>> space
>>>> is available, resulting in UNDERRUN.
>>>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>>>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>>>    way to communicate fifo size info to ADMA.
>>> The src_maxburst / dst_maxburst is exactly for this reason. To
>>> communicate how much data should be transferred per DMA request to/from
>>> peripheral.
>>>
>>> In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
>>> which is dynamically configured (you can see the AFIFO of McASP as a
>>> small DMA: on McASP side it services the peripheral, on the other side
>>> it interacts with the given system DMA used by the SoC - EDMA, sDMA or
>>> UDMAP). All DMAs needs a bit different configuration, but the AFIFO
>>> depth on the McASP side is coming in via the src/dst_maxburst and the
>>> drivers just need to interpret it correctly.
>>>
>>> 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)?

> 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:
ADMAIF - peripheral side
 - kind of a small DMA for audio preipheral(s)?
 - Variable FIFO size
 - sends DMA request to ADMA per words
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?

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

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

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

[Vinod Koul]

On 06-06-19, 09:19, Sameer Pujar wrote:

> > you are really going other way around about the whole picture. FWIW that
> > is how *other* folks do audio with dmaengine!
> I discussed this internally with HW folks and below is the reason why DMA
> needs
> to know FIFO size.
> 
> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface to
> the audio sub-system.
> - ADMAIF has multiple channels and share FIFO buffer for individual
> operations. There is a provision
>   to allocate specific fifo size for each individual ADMAIF channel from the
> shared buffer.
> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
> engines, which you described earlier.
> - The flow control logic is placed inside ADMA. Slave peripheral
> device(ADMAIF) signals ADMA whenever a
>   read or write happens on the FIFO(per WORD basis). Please note that the
> signaling is per channel. There is
>   no other signaling present from ADMAIF to ADMA.
> - ADMA keeps a counter related to above signaling. Whenever a sufficient

when is signal triggered? When there is space available or some
threshold of space is reached?

> space is available, it initiates a transfer.
>   But the question is, how does it know when to transfer. This is the
> reason, why ADMA has to be aware of FIFO
>   depth of ADMAIF channel. Depending on the counters and FIFO depth, it
> knows exactly when a free space is available
>   in the context of a specific channel. On ADMA, FIFO_SIZE is just a value
> which should match to actual FIFO_DEPTH/SIZE
>   of ADMAIF channel.

That doesn't sound too different from typical dmaengine. To give an
example of a platform (and general DMAengine principles as well) I worked
on the FIFO was 16 word deep. DMA didn't knew!

Peripheral driver would signal to DMA when a threshold is reached and
DMA would send a burst controlled by src/dst_burst_size. For example if
you have a FIFO with 16 words depth, typical burst_size would be 8 words
and peripheral will configure signalling for FIFO having 8 words, so
signal from peripheral will make dma transfer 8 words.

Here the peripheral driver FIFO is important, but the driver
configures it and sets burst_size accordingly.

So can you explain me what is the difference here that the peripheral
cannot configure and use burst size with passing fifo depth?

> - Now consider two cases based on above logic,
>   * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>     In this case, ADMA thinks that there is enough space available for
> transfer, when actually the FIFO data
>     on slave is not consumed yet. It would result in OVERRUN.
>   * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>     This is case where ADMA won’t transfer, even though sufficient space is
> available, resulting in UNDERRUN.
> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>   way to communicate fifo size info to ADMA.

-- 
~Vinod

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

[Sameer Pujar]

On 6/13/2019 10:13 AM, Vinod Koul wrote:
> On 06-06-19, 09:19, Sameer Pujar wrote:
>
>>> you are really going other way around about the whole picture. FWIW that
>>> is how *other* folks do audio with dmaengine!
>> I discussed this internally with HW folks and below is the reason why DMA
>> needs
>> to know FIFO size.
>>
>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface to
>> the audio sub-system.
>> - ADMAIF has multiple channels and share FIFO buffer for individual
>> operations. There is a provision
>>    to allocate specific fifo size for each individual ADMAIF channel from the
>> shared buffer.
>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>> engines, which you described earlier.
>> - The flow control logic is placed inside ADMA. Slave peripheral
>> device(ADMAIF) signals ADMA whenever a
>>    read or write happens on the FIFO(per WORD basis). Please note that the
>> signaling is per channel. There is
>>    no other signaling present from ADMAIF to ADMA.
>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
> when is signal triggered? When there is space available or some
> threshold of space is reached?
Signal is triggered when FIFO read/write happens on the peripheral side. 
In other words
this happens when data is pushed/popped out of ADMAIF from/to one of the 
AHUB modules (I2S
for example) This is on peripheral side and ADMAIF signals ADMA per WORD 
basis.
ADMA <---(1. DMA transfers)---> ADMAIF <------ (2. FIFO read/write) 
------> I2S
To be more clear ADMAIF signals ADMA when [2] happens.

FIFO_THRESHOLD field in ADMAIF is just to indicate when can ADMAIF do 
operation [2].
Also please note FIFO_THRESHOLD field is present only for 
memory---->AHUB path (playback path)
and there is no such threshold concept for AHUB----> memory path 
(capture path)
>> space is available, it initiates a transfer.
>>    But the question is, how does it know when to transfer. This is the
>> reason, why ADMA has to be aware of FIFO
>>    depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>> knows exactly when a free space is available
>>    in the context of a specific channel. On ADMA, FIFO_SIZE is just a value
>> which should match to actual FIFO_DEPTH/SIZE
>>    of ADMAIF channel.
> That doesn't sound too different from typical dmaengine. To give an
> example of a platform (and general DMAengine principles as well) I worked
> on the FIFO was 16 word deep. DMA didn't knew!
>
> Peripheral driver would signal to DMA when a threshold is reached and
No, In our case ADMAIF does not do any threshold based signalling to ADMA.
> DMA would send a burst controlled by src/dst_burst_size. For example if
> you have a FIFO with 16 words depth, typical burst_size would be 8 words
> and peripheral will configure signalling for FIFO having 8 words, so
> signal from peripheral will make dma transfer 8 words.
The scenario is different in ADMA case, as ADMAIF cannot configure the
signalling based on FIFO_THRESHOLD settings.
>
> Here the peripheral driver FIFO is important, but the driver
> configures it and sets burst_size accordingly.
>
> So can you explain me what is the difference here that the peripheral
> cannot configure and use burst size with passing fifo depth?
Say for example FIFO_THRESHOLD is programmed as 16 WORDS, BURST_SIZE as 
8 WORDS.
ADMAIF does not push data to AHUB(operation [2]) till threshold of 16 
WORDS is
reached in ADMAIF FIFO. Hence 2 burst transfers are needed to reach the 
threshold.
As mentioned earlier, threshold here is to just indicate when data 
transfer can happen
to AHUB modules.

Once the data is popped from ADMAIF FIFO, ADMAIF signals ADMA. ADMA is 
the master
and it keeps track of the buffer occupancy by knowing the FIFO_DEPTH and 
the signalling.
Then finally it decides when to do next burst transfer depending on the 
free space
available in ADMAIF.
>> - Now consider two cases based on above logic,
>>    * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>      In this case, ADMA thinks that there is enough space available for
>> transfer, when actually the FIFO data
>>      on slave is not consumed yet. It would result in OVERRUN.
>>    * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>      This is case where ADMA won’t transfer, even though sufficient space is
>> available, resulting in UNDERRUN.
>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>    way to communicate fifo size info to ADMA.

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

[Vinod Koul]

On 17-06-19, 12:37, Sameer Pujar wrote:
> 
> On 6/13/2019 10:13 AM, Vinod Koul wrote:
> > On 06-06-19, 09:19, Sameer Pujar wrote:
> > 
> > > > you are really going other way around about the whole picture. FWIW that
> > > > is how *other* folks do audio with dmaengine!
> > > I discussed this internally with HW folks and below is the reason why DMA
> > > needs
> > > to know FIFO size.
> > > 
> > > - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface to
> > > the audio sub-system.
> > > - ADMAIF has multiple channels and share FIFO buffer for individual
> > > operations. There is a provision
> > >    to allocate specific fifo size for each individual ADMAIF channel from the
> > > shared buffer.
> > > - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
> > > engines, which you described earlier.
> > > - The flow control logic is placed inside ADMA. Slave peripheral
> > > device(ADMAIF) signals ADMA whenever a
> > >    read or write happens on the FIFO(per WORD basis). Please note that the
> > > signaling is per channel. There is
> > >    no other signaling present from ADMAIF to ADMA.
> > > - ADMA keeps a counter related to above signaling. Whenever a sufficient
> > when is signal triggered? When there is space available or some
> > threshold of space is reached?
> Signal is triggered when FIFO read/write happens on the peripheral side. In
> other words
> this happens when data is pushed/popped out of ADMAIF from/to one of the
> AHUB modules (I2S
> for example) This is on peripheral side and ADMAIF signals ADMA per WORD
> basis.
> ADMA <---(1. DMA transfers)---> ADMAIF <------ (2. FIFO read/write) ------>
> I2S
> To be more clear ADMAIF signals ADMA when [2] happens.

That is on every word read/write?

> FIFO_THRESHOLD field in ADMAIF is just to indicate when can ADMAIF do
> operation [2].
> Also please note FIFO_THRESHOLD field is present only for memory---->AHUB
> path (playback path)
> and there is no such threshold concept for AHUB----> memory path (capture
> path)

That is sane and common. For memory you dont have a constraint so you
transfer at full throttle.

> > > space is available, it initiates a transfer.
> > >    But the question is, how does it know when to transfer. This is the
> > > reason, why ADMA has to be aware of FIFO
> > >    depth of ADMAIF channel. Depending on the counters and FIFO depth, it
> > > knows exactly when a free space is available
> > >    in the context of a specific channel. On ADMA, FIFO_SIZE is just a value
> > > which should match to actual FIFO_DEPTH/SIZE
> > >    of ADMAIF channel.
> > That doesn't sound too different from typical dmaengine. To give an
> > example of a platform (and general DMAengine principles as well) I worked
> > on the FIFO was 16 word deep. DMA didn't knew!
> > 
> > Peripheral driver would signal to DMA when a threshold is reached and
> No, In our case ADMAIF does not do any threshold based signalling to ADMA.
> > DMA would send a burst controlled by src/dst_burst_size. For example if
> > you have a FIFO with 16 words depth, typical burst_size would be 8 words
> > and peripheral will configure signalling for FIFO having 8 words, so
> > signal from peripheral will make dma transfer 8 words.
> The scenario is different in ADMA case, as ADMAIF cannot configure the
> signalling based on FIFO_THRESHOLD settings.
> > 
> > Here the peripheral driver FIFO is important, but the driver
> > configures it and sets burst_size accordingly.
> > 
> > So can you explain me what is the difference here that the peripheral
> > cannot configure and use burst size with passing fifo depth?
> Say for example FIFO_THRESHOLD is programmed as 16 WORDS, BURST_SIZE as 8
> WORDS.
> ADMAIF does not push data to AHUB(operation [2]) till threshold of 16 WORDS
> is
> reached in ADMAIF FIFO. Hence 2 burst transfers are needed to reach the
> threshold.
> As mentioned earlier, threshold here is to just indicate when data transfer
> can happen
> to AHUB modules.

So we have ADMA and AHUB and peripheral. You are talking to AHUB and that
is _not_ peripheral and if I have guess right the fifo depth is for AHUB
right?

> Once the data is popped from ADMAIF FIFO, ADMAIF signals ADMA. ADMA is the
> master
> and it keeps track of the buffer occupancy by knowing the FIFO_DEPTH and the
> signalling.
> Then finally it decides when to do next burst transfer depending on the free
> space
> available in ADMAIF.
> > > - Now consider two cases based on above logic,
> > >    * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
> > >      In this case, ADMA thinks that there is enough space available for
> > > transfer, when actually the FIFO data
> > >      on slave is not consumed yet. It would result in OVERRUN.
> > >    * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
> > >      This is case where ADMA won’t transfer, even though sufficient space is
> > > available, resulting in UNDERRUN.
> > > - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
> > > SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
> > >    way to communicate fifo size info to ADMA.

-- 
~Vinod

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

[Sameer Pujar]

On 6/18/2019 10:03 AM, Vinod Koul wrote:
> On 17-06-19, 12:37, Sameer Pujar wrote:
>> On 6/13/2019 10:13 AM, Vinod Koul wrote:
>>> On 06-06-19, 09:19, Sameer Pujar wrote:
>>>
>>>>> you are really going other way around about the whole picture. FWIW that
>>>>> is how *other* folks do audio with dmaengine!
>>>> I discussed this internally with HW folks and below is the reason why DMA
>>>> needs
>>>> to know FIFO size.
>>>>
>>>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface to
>>>> the audio sub-system.
>>>> - ADMAIF has multiple channels and share FIFO buffer for individual
>>>> operations. There is a provision
>>>>     to allocate specific fifo size for each individual ADMAIF channel from the
>>>> shared buffer.
>>>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>>>> engines, which you described earlier.
>>>> - The flow control logic is placed inside ADMA. Slave peripheral
>>>> device(ADMAIF) signals ADMA whenever a
>>>>     read or write happens on the FIFO(per WORD basis). Please note that the
>>>> signaling is per channel. There is
>>>>     no other signaling present from ADMAIF to ADMA.
>>>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>>> when is signal triggered? When there is space available or some
>>> threshold of space is reached?
>> Signal is triggered when FIFO read/write happens on the peripheral side. In
>> other words
>> this happens when data is pushed/popped out of ADMAIF from/to one of the
>> AHUB modules (I2S
>> for example) This is on peripheral side and ADMAIF signals ADMA per WORD
>> basis.
>> ADMA <---(1. DMA transfers)---> ADMAIF <------ (2. FIFO read/write) ------>
>> I2S
>> To be more clear ADMAIF signals ADMA when [2] happens.
> That is on every word read/write?
yes this per WORD read/write
>
>> FIFO_THRESHOLD field in ADMAIF is just to indicate when can ADMAIF do
>> operation [2].
>> Also please note FIFO_THRESHOLD field is present only for memory---->AHUB
>> path (playback path)
>> and there is no such threshold concept for AHUB----> memory path (capture
>> path)
> That is sane and common. For memory you dont have a constraint so you
> transfer at full throttle.
>
>>>> space is available, it initiates a transfer.
>>>>     But the question is, how does it know when to transfer. This is the
>>>> reason, why ADMA has to be aware of FIFO
>>>>     depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>>>> knows exactly when a free space is available
>>>>     in the context of a specific channel. On ADMA, FIFO_SIZE is just a value
>>>> which should match to actual FIFO_DEPTH/SIZE
>>>>     of ADMAIF channel.
>>> That doesn't sound too different from typical dmaengine. To give an
>>> example of a platform (and general DMAengine principles as well) I worked
>>> on the FIFO was 16 word deep. DMA didn't knew!
>>>
>>> Peripheral driver would signal to DMA when a threshold is reached and
>> No, In our case ADMAIF does not do any threshold based signalling to ADMA.
>>> DMA would send a burst controlled by src/dst_burst_size. For example if
>>> you have a FIFO with 16 words depth, typical burst_size would be 8 words
>>> and peripheral will configure signalling for FIFO having 8 words, so
>>> signal from peripheral will make dma transfer 8 words.
>> The scenario is different in ADMA case, as ADMAIF cannot configure the
>> signalling based on FIFO_THRESHOLD settings.
>>> Here the peripheral driver FIFO is important, but the driver
>>> configures it and sets burst_size accordingly.
>>>
>>> So can you explain me what is the difference here that the peripheral
>>> cannot configure and use burst size with passing fifo depth?
>> Say for example FIFO_THRESHOLD is programmed as 16 WORDS, BURST_SIZE as 8
>> WORDS.
>> ADMAIF does not push data to AHUB(operation [2]) till threshold of 16 WORDS
>> is
>> reached in ADMAIF FIFO. Hence 2 burst transfers are needed to reach the
>> threshold.
>> As mentioned earlier, threshold here is to just indicate when data transfer
>> can happen
>> to AHUB modules.
> So we have ADMA and AHUB and peripheral. You are talking to AHUB and that
> is _not_ peripheral and if I have guess right the fifo depth is for AHUB
> right?
Yes the communication is between ADMA and AHUB. ADMAIF is the interface 
between
ADMA and AHUB. ADMA channel sending data to AHUB pairs with ADMAIF TX 
channel.
Similary ADMA channel receiving data from AHUB pairs with ADMAIF RX channel.
FIFO DEPTH we are talking is about each ADMAIF TX/RX channel and it is 
configurable.
DMA transfers happen to/from ADMAIF FIFOs and whenever data(per WORD) is 
popped/pushed
out of ADMAIF to/from AHUB, asseration is made to ADMA. ADMA keeps 
counters based on
these assertions. By knowing FIFO DEPTH and these counters, ADMA knows 
when to wait or
when to transfer data.
>
>> Once the data is popped from ADMAIF FIFO, ADMAIF signals ADMA. ADMA is the
>> master
>> and it keeps track of the buffer occupancy by knowing the FIFO_DEPTH and the
>> signalling.
>> Then finally it decides when to do next burst transfer depending on the free
>> space
>> available in ADMAIF.
>>>> - Now consider two cases based on above logic,
>>>>     * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>>>       In this case, ADMA thinks that there is enough space available for
>>>> transfer, when actually the FIFO data
>>>>       on slave is not consumed yet. It would result in OVERRUN.
>>>>     * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>>>       This is case where ADMA won’t transfer, even though sufficient space is
>>>> available, resulting in UNDERRUN.
>>>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>>>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>>>     way to communicate fifo size info to ADMA.

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

[Vinod Koul]

On 20-06-19, 15:59, Sameer Pujar wrote:

> > > > So can you explain me what is the difference here that the peripheral
> > > > cannot configure and use burst size with passing fifo depth?
> > > Say for example FIFO_THRESHOLD is programmed as 16 WORDS, BURST_SIZE as 8
> > > WORDS.
> > > ADMAIF does not push data to AHUB(operation [2]) till threshold of 16 WORDS
> > > is
> > > reached in ADMAIF FIFO. Hence 2 burst transfers are needed to reach the
> > > threshold.
> > > As mentioned earlier, threshold here is to just indicate when data transfer
> > > can happen
> > > to AHUB modules.
> > So we have ADMA and AHUB and peripheral. You are talking to AHUB and that
> > is _not_ peripheral and if I have guess right the fifo depth is for AHUB
> > right?
> Yes the communication is between ADMA and AHUB. ADMAIF is the interface
> between
> ADMA and AHUB. ADMA channel sending data to AHUB pairs with ADMAIF TX
> channel.
> Similary ADMA channel receiving data from AHUB pairs with ADMAIF RX channel.
> FIFO DEPTH we are talking is about each ADMAIF TX/RX channel and it is
> configurable.
> DMA transfers happen to/from ADMAIF FIFOs and whenever data(per WORD) is
> popped/pushed
> out of ADMAIF to/from AHUB, asseration is made to ADMA. ADMA keeps counters
> based on
> these assertions. By knowing FIFO DEPTH and these counters, ADMA knows when
> to wait or
> when to transfer data.

Where does ADMAIF driver reside in kernel, who configures it for normal
dma txns..?

Also it wold have helped the long discussion if that part was made clear
rather than talking about peripheral all this time :(

-- 
~Vinod

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

[Sameer Pujar]

On 6/24/2019 11:56 AM, Vinod Koul wrote:
> On 20-06-19, 15:59, Sameer Pujar wrote:
>
>>>>> So can you explain me what is the difference here that the peripheral
>>>>> cannot configure and use burst size with passing fifo depth?
>>>> Say for example FIFO_THRESHOLD is programmed as 16 WORDS, BURST_SIZE as 8
>>>> WORDS.
>>>> ADMAIF does not push data to AHUB(operation [2]) till threshold of 16 WORDS
>>>> is
>>>> reached in ADMAIF FIFO. Hence 2 burst transfers are needed to reach the
>>>> threshold.
>>>> As mentioned earlier, threshold here is to just indicate when data transfer
>>>> can happen
>>>> to AHUB modules.
>>> So we have ADMA and AHUB and peripheral. You are talking to AHUB and that
>>> is _not_ peripheral and if I have guess right the fifo depth is for AHUB
>>> right?
>> Yes the communication is between ADMA and AHUB. ADMAIF is the interface
>> between
>> ADMA and AHUB. ADMA channel sending data to AHUB pairs with ADMAIF TX
>> channel.
>> Similary ADMA channel receiving data from AHUB pairs with ADMAIF RX channel.
>> FIFO DEPTH we are talking is about each ADMAIF TX/RX channel and it is
>> configurable.
>> DMA transfers happen to/from ADMAIF FIFOs and whenever data(per WORD) is
>> popped/pushed
>> out of ADMAIF to/from AHUB, asseration is made to ADMA. ADMA keeps counters
>> based on
>> these assertions. By knowing FIFO DEPTH and these counters, ADMA knows when
>> to wait or
>> when to transfer data.
> Where does ADMAIF driver reside in kernel, who configures it for normal
> dma txns..?
Not yet, we are in the process of upstreaming ADMAIF driver.
To describe briefly, audio subsystem is using ALSA SoC(ASoC) layer. 
ADMAIF is
registered as platform driver and exports DMA functionality. It 
registers PCM
devices for each Rx/Tx ADMAIF channel. During PCM playback/capture 
operations,
ALSA callbacks configure DMA channel using API dmaengine_slave_config().
RFC patch proposed, is to help populate FIFO_SIZE value as well during above
call, since ADMA requires it.
>
> Also it wold have helped the long discussion if that part was made clear
> rather than talking about peripheral all this time :(
Thought it was clear, though should have avoided using 'peripheral' in the
discussions. Sorry for the confusion.

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

[Sameer Pujar]

Hi Vinod,

What are your final thoughts regarding this?

Thanks,
Sameer.

>> Where does ADMAIF driver reside in kernel, who configures it for normal
>> dma txns..?
> Not yet, we are in the process of upstreaming ADMAIF driver.
> To describe briefly, audio subsystem is using ALSA SoC(ASoC) layer. 
> ADMAIF is
> registered as platform driver and exports DMA functionality. It 
> registers PCM
> devices for each Rx/Tx ADMAIF channel. During PCM playback/capture 
> operations,
> ALSA callbacks configure DMA channel using API dmaengine_slave_config().
> RFC patch proposed, is to help populate FIFO_SIZE value as well during 
> above
> call, since ADMA requires it.
>>
>> Also it wold have helped the long discussion if that part was made clear
>> rather than talking about peripheral all this time :(
> Thought it was clear, though should have avoided using 'peripheral' in 
> the
> discussions. Sorry for the confusion.

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

[Sameer Pujar]

Hi Vinod,

Sorry for writing again.
Can we please conclude on this?

Thanks,
Sameer.

On 7/5/2019 11:45 AM, Sameer Pujar wrote:
> Hi Vinod,
>
> What are your final thoughts regarding this?
>
> Thanks,
> Sameer.
>
>>> Where does ADMAIF driver reside in kernel, who configures it for normal
>>> dma txns..?
>> Not yet, we are in the process of upstreaming ADMAIF driver.
>> To describe briefly, audio subsystem is using ALSA SoC(ASoC) layer. 
>> ADMAIF is
>> registered as platform driver and exports DMA functionality. It 
>> registers PCM
>> devices for each Rx/Tx ADMAIF channel. During PCM playback/capture 
>> operations,
>> ALSA callbacks configure DMA channel using API dmaengine_slave_config().
>> RFC patch proposed, is to help populate FIFO_SIZE value as well 
>> during above
>> call, since ADMA requires it.
>>>
>>> Also it wold have helped the long discussion if that part was made 
>>> clear
>>> rather than talking about peripheral all this time :(
>> Thought it was clear, though should have avoided using 'peripheral' 
>> in the
>> discussions. Sorry for the confusion.
>

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

[Vinod Koul]

On 05-07-19, 11:45, Sameer Pujar wrote:
> Hi Vinod,
> 
> What are your final thoughts regarding this?

Hi sameer,

Sorry for the delay in replying

On this, I am inclined to think that dma driver should not be involved.
The ADMAIF needs this configuration and we should take the path of
dma_router for this piece and add features like this to it

> 
> Thanks,
> Sameer.
> 
> > > Where does ADMAIF driver reside in kernel, who configures it for normal
> > > dma txns..?
> > Not yet, we are in the process of upstreaming ADMAIF driver.
> > To describe briefly, audio subsystem is using ALSA SoC(ASoC) layer.
> > ADMAIF is
> > registered as platform driver and exports DMA functionality. It
> > registers PCM
> > devices for each Rx/Tx ADMAIF channel. During PCM playback/capture
> > operations,
> > ALSA callbacks configure DMA channel using API dmaengine_slave_config().
> > RFC patch proposed, is to help populate FIFO_SIZE value as well during
> > above
> > call, since ADMA requires it.
> > > 
> > > Also it wold have helped the long discussion if that part was made clear
> > > rather than talking about peripheral all this time :(
> > Thought it was clear, though should have avoided using 'peripheral' in
> > the
> > discussions. Sorry for the confusion.

-- 
~Vinod

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

[Sameer Pujar]

On 7/19/2019 10:34 AM, Vinod Koul wrote:
> On 05-07-19, 11:45, Sameer Pujar wrote:
>> Hi Vinod,
>>
>> What are your final thoughts regarding this?
> Hi sameer,
>
> Sorry for the delay in replying
>
> On this, I am inclined to think that dma driver should not be involved.
> The ADMAIF needs this configuration and we should take the path of
> dma_router for this piece and add features like this to it

Hi Vinod,

The configuration is needed by both ADMA and ADMAIF. The size is 
configurable
on ADMAIF side. ADMA needs to know this info and program accordingly.
Not sure if dma_router can help to achieve this.

I checked on dma_router. It would have been useful when a configuration 
exported
via ADMA, had to be applied to ADMAIF. Please correct me if I am wrong here.

>> Thanks,
>> Sameer.
>>
>>>> Where does ADMAIF driver reside in kernel, who configures it for normal
>>>> dma txns..?
>>> Not yet, we are in the process of upstreaming ADMAIF driver.
>>> To describe briefly, audio subsystem is using ALSA SoC(ASoC) layer.
>>> ADMAIF is
>>> registered as platform driver and exports DMA functionality. It
>>> registers PCM
>>> devices for each Rx/Tx ADMAIF channel. During PCM playback/capture
>>> operations,
>>> ALSA callbacks configure DMA channel using API dmaengine_slave_config().
>>> RFC patch proposed, is to help populate FIFO_SIZE value as well during
>>> above
>>> call, since ADMA requires it.
>>>> Also it wold have helped the long discussion if that part was made clear
>>>> rather than talking about peripheral all this time :(
>>> Thought it was clear, though should have avoided using 'peripheral' in
>>> the
>>> discussions. Sorry for the confusion.

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

[Vinod Koul]

On 23-07-19, 11:24, Sameer Pujar wrote:
> 
> On 7/19/2019 10:34 AM, Vinod Koul wrote:
> > On 05-07-19, 11:45, Sameer Pujar wrote:
> > > Hi Vinod,
> > > 
> > > What are your final thoughts regarding this?
> > Hi sameer,
> > 
> > Sorry for the delay in replying
> > 
> > On this, I am inclined to think that dma driver should not be involved.
> > The ADMAIF needs this configuration and we should take the path of
> > dma_router for this piece and add features like this to it
> 
> Hi Vinod,
> 
> The configuration is needed by both ADMA and ADMAIF. The size is
> configurable
> on ADMAIF side. ADMA needs to know this info and program accordingly.

Well I would say client decides the settings for both DMA, DMAIF and
sets the peripheral accordingly as well, so client communicates the two
sets of info to two set of drivers

> Not sure if dma_router can help to achieve this.
> 
> I checked on dma_router. It would have been useful when a configuration
> exported
> via ADMA, had to be applied to ADMAIF. Please correct me if I am wrong here.

router was added for such a senario, if you feel that it doesn't serve
your purpose, feel free to send up update for it, if that is the case.

Thanks

-- 
~Vinod

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

[Jon Hunter]

On 29/07/2019 07:10, Vinod Koul wrote:
> On 23-07-19, 11:24, Sameer Pujar wrote:
>>
>> On 7/19/2019 10:34 AM, Vinod Koul wrote:
>>> On 05-07-19, 11:45, Sameer Pujar wrote:
>>>> Hi Vinod,
>>>>
>>>> What are your final thoughts regarding this?
>>> Hi sameer,
>>>
>>> Sorry for the delay in replying
>>>
>>> On this, I am inclined to think that dma driver should not be involved.
>>> The ADMAIF needs this configuration and we should take the path of
>>> dma_router for this piece and add features like this to it
>>
>> Hi Vinod,
>>
>> The configuration is needed by both ADMA and ADMAIF. The size is
>> configurable
>> on ADMAIF side. ADMA needs to know this info and program accordingly.
> 
> Well I would say client decides the settings for both DMA, DMAIF and
> sets the peripheral accordingly as well, so client communicates the two
> sets of info to two set of drivers

That maybe, but I still don't see how the information is passed from the
client in the first place. The current problem is that there is no means
to pass both a max-burst size and fifo-size to the DMA driver from the
client.

IMO there needs to be a way to pass vendor specific DMA configuration
(if this information is not common) otherwise we just end up in a
scenario like there is for the xilinx DMA driver
(include/linux/dma/xilinx_dma.h) that has a custom API for passing this
information.

Cheers
Jon

-- 
nvpublic

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

[Vinod Koul]

On 31-07-19, 10:48, Jon Hunter wrote:
> 
> On 29/07/2019 07:10, Vinod Koul wrote:
> > On 23-07-19, 11:24, Sameer Pujar wrote:
> >>
> >> On 7/19/2019 10:34 AM, Vinod Koul wrote:
> >>> On 05-07-19, 11:45, Sameer Pujar wrote:
> >>>> Hi Vinod,
> >>>>
> >>>> What are your final thoughts regarding this?
> >>> Hi sameer,
> >>>
> >>> Sorry for the delay in replying
> >>>
> >>> On this, I am inclined to think that dma driver should not be involved.
> >>> The ADMAIF needs this configuration and we should take the path of
> >>> dma_router for this piece and add features like this to it
> >>
> >> Hi Vinod,
> >>
> >> The configuration is needed by both ADMA and ADMAIF. The size is
> >> configurable
> >> on ADMAIF side. ADMA needs to know this info and program accordingly.
> > 
> > Well I would say client decides the settings for both DMA, DMAIF and
> > sets the peripheral accordingly as well, so client communicates the two
> > sets of info to two set of drivers
> 
> That maybe, but I still don't see how the information is passed from the
> client in the first place. The current problem is that there is no means
> to pass both a max-burst size and fifo-size to the DMA driver from the
> client.

So one thing not clear to me is why ADMA needs fifo-size, I thought it
was to program ADMAIF and if we have client programme the max-burst
size to ADMA and fifo-size to ADMAIF we wont need that. Can you please
confirm if my assumption is valid?

> IMO there needs to be a way to pass vendor specific DMA configuration
> (if this information is not common) otherwise we just end up in a
> scenario like there is for the xilinx DMA driver
> (include/linux/dma/xilinx_dma.h) that has a custom API for passing this
> information.
> 
> Cheers
> Jon
> 
> -- 
> nvpublic

-- 
~Vinod

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

[Jon Hunter]

On 31/07/2019 16:16, Vinod Koul wrote:
> On 31-07-19, 10:48, Jon Hunter wrote:
>>
>> On 29/07/2019 07:10, Vinod Koul wrote:
>>> On 23-07-19, 11:24, Sameer Pujar wrote:
>>>>
>>>> On 7/19/2019 10:34 AM, Vinod Koul wrote:
>>>>> On 05-07-19, 11:45, Sameer Pujar wrote:
>>>>>> Hi Vinod,
>>>>>>
>>>>>> What are your final thoughts regarding this?
>>>>> Hi sameer,
>>>>>
>>>>> Sorry for the delay in replying
>>>>>
>>>>> On this, I am inclined to think that dma driver should not be involved.
>>>>> The ADMAIF needs this configuration and we should take the path of
>>>>> dma_router for this piece and add features like this to it
>>>>
>>>> Hi Vinod,
>>>>
>>>> The configuration is needed by both ADMA and ADMAIF. The size is
>>>> configurable
>>>> on ADMAIF side. ADMA needs to know this info and program accordingly.
>>>
>>> Well I would say client decides the settings for both DMA, DMAIF and
>>> sets the peripheral accordingly as well, so client communicates the two
>>> sets of info to two set of drivers
>>
>> That maybe, but I still don't see how the information is passed from the
>> client in the first place. The current problem is that there is no means
>> to pass both a max-burst size and fifo-size to the DMA driver from the
>> client.
> 
> So one thing not clear to me is why ADMA needs fifo-size, I thought it
> was to program ADMAIF and if we have client programme the max-burst
> size to ADMA and fifo-size to ADMAIF we wont need that. Can you please
> confirm if my assumption is valid?

Let me see if I can clarify ...

1. The FIFO we are discussing here resides in the ADMAIF module which is
   a separate hardware block the ADMA (although the naming make this
   unclear).

2. The size of FIFO in the ADMAIF is configurable and it this is
   configured via the ADMAIF registers. This allows different channels
   to use different FIFO sizes. Think of this as a shared memory that is
   divided into n FIFOs shared between all channels.

3. The ADMA, not the ADMAIF, manages the flow to the FIFO and this is
   because the ADMAIF only tells the ADMA when a word has been
   read/written (depending on direction), the ADMAIF does not indicate
   if the FIFO is full, empty, etc. Hence, the ADMA needs to know the
   total FIFO size.

So the ADMA needs to know the FIFO size so that it does not overrun the
FIFO and we can also set a burst size (less than the total FIFO size)
indicating how many words to transfer at a time. Hence, the two parameters.

Even if we were to use some sort of router between the ADMA and ADMAIF,
the client still needs to indicate to the ADMA what FIFO size and burst
size, if I am following you correctly.

Let me know if this is clearer.

Thanks
Jon

-- 
nvpublic

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

[Vinod Koul]

On 02-08-19, 09:51, Jon Hunter wrote:
> 
> On 31/07/2019 16:16, Vinod Koul wrote:
> > On 31-07-19, 10:48, Jon Hunter wrote:
> >>
> >> On 29/07/2019 07:10, Vinod Koul wrote:
> >>> On 23-07-19, 11:24, Sameer Pujar wrote:
> >>>>
> >>>> On 7/19/2019 10:34 AM, Vinod Koul wrote:
> >>>>> On 05-07-19, 11:45, Sameer Pujar wrote:
> >>>>>> Hi Vinod,
> >>>>>>
> >>>>>> What are your final thoughts regarding this?
> >>>>> Hi sameer,
> >>>>>
> >>>>> Sorry for the delay in replying
> >>>>>
> >>>>> On this, I am inclined to think that dma driver should not be involved.
> >>>>> The ADMAIF needs this configuration and we should take the path of
> >>>>> dma_router for this piece and add features like this to it
> >>>>
> >>>> Hi Vinod,
> >>>>
> >>>> The configuration is needed by both ADMA and ADMAIF. The size is
> >>>> configurable
> >>>> on ADMAIF side. ADMA needs to know this info and program accordingly.
> >>>
> >>> Well I would say client decides the settings for both DMA, DMAIF and
> >>> sets the peripheral accordingly as well, so client communicates the two
> >>> sets of info to two set of drivers
> >>
> >> That maybe, but I still don't see how the information is passed from the
> >> client in the first place. The current problem is that there is no means
> >> to pass both a max-burst size and fifo-size to the DMA driver from the
> >> client.
> > 
> > So one thing not clear to me is why ADMA needs fifo-size, I thought it
> > was to program ADMAIF and if we have client programme the max-burst
> > size to ADMA and fifo-size to ADMAIF we wont need that. Can you please
> > confirm if my assumption is valid?
> 
> Let me see if I can clarify ...
> 
> 1. The FIFO we are discussing here resides in the ADMAIF module which is
>    a separate hardware block the ADMA (although the naming make this
>    unclear).
> 
> 2. The size of FIFO in the ADMAIF is configurable and it this is
>    configured via the ADMAIF registers. This allows different channels
>    to use different FIFO sizes. Think of this as a shared memory that is
>    divided into n FIFOs shared between all channels.
> 
> 3. The ADMA, not the ADMAIF, manages the flow to the FIFO and this is
>    because the ADMAIF only tells the ADMA when a word has been
>    read/written (depending on direction), the ADMAIF does not indicate
>    if the FIFO is full, empty, etc. Hence, the ADMA needs to know the
>    total FIFO size.
> 
> So the ADMA needs to know the FIFO size so that it does not overrun the
> FIFO and we can also set a burst size (less than the total FIFO size)
> indicating how many words to transfer at a time. Hence, the two parameters.

Thanks, I confirm this is my understanding as well.

To compare to regular case for example SPI on DMA, SPI driver will
calculate fifo size & burst to be used and program dma (burst size) and
its own fifos accordingly

So, in your case why should the peripheral driver not calculate the fifo
size for both ADMA and ADMAIF and (if required it's own FIFO) and
program the two (ADMA and ADMAIF).

What is the limiting factor in this flow is not clear to me.

> Even if we were to use some sort of router between the ADMA and ADMAIF,
> the client still needs to indicate to the ADMA what FIFO size and burst
> size, if I am following you correctly.
> 
> Let me know if this is clearer.
> 
> Thanks
> Jon
> 
> -- 
> nvpublic

-- 
~Vinod

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

[Jon Hunter]

On 08/08/2019 13:38, Vinod Koul wrote:
> On 02-08-19, 09:51, Jon Hunter wrote:
>>
>> On 31/07/2019 16:16, Vinod Koul wrote:
>>> On 31-07-19, 10:48, Jon Hunter wrote:
>>>>
>>>> On 29/07/2019 07:10, Vinod Koul wrote:
>>>>> On 23-07-19, 11:24, Sameer Pujar wrote:
>>>>>>
>>>>>> On 7/19/2019 10:34 AM, Vinod Koul wrote:
>>>>>>> On 05-07-19, 11:45, Sameer Pujar wrote:
>>>>>>>> Hi Vinod,
>>>>>>>>
>>>>>>>> What are your final thoughts regarding this?
>>>>>>> Hi sameer,
>>>>>>>
>>>>>>> Sorry for the delay in replying
>>>>>>>
>>>>>>> On this, I am inclined to think that dma driver should not be involved.
>>>>>>> The ADMAIF needs this configuration and we should take the path of
>>>>>>> dma_router for this piece and add features like this to it
>>>>>>
>>>>>> Hi Vinod,
>>>>>>
>>>>>> The configuration is needed by both ADMA and ADMAIF. The size is
>>>>>> configurable
>>>>>> on ADMAIF side. ADMA needs to know this info and program accordingly.
>>>>>
>>>>> Well I would say client decides the settings for both DMA, DMAIF and
>>>>> sets the peripheral accordingly as well, so client communicates the two
>>>>> sets of info to two set of drivers
>>>>
>>>> That maybe, but I still don't see how the information is passed from the
>>>> client in the first place. The current problem is that there is no means
>>>> to pass both a max-burst size and fifo-size to the DMA driver from the
>>>> client.
>>>
>>> So one thing not clear to me is why ADMA needs fifo-size, I thought it
>>> was to program ADMAIF and if we have client programme the max-burst
>>> size to ADMA and fifo-size to ADMAIF we wont need that. Can you please
>>> confirm if my assumption is valid?
>>
>> Let me see if I can clarify ...
>>
>> 1. The FIFO we are discussing here resides in the ADMAIF module which is
>>    a separate hardware block the ADMA (although the naming make this
>>    unclear).
>>
>> 2. The size of FIFO in the ADMAIF is configurable and it this is
>>    configured via the ADMAIF registers. This allows different channels
>>    to use different FIFO sizes. Think of this as a shared memory that is
>>    divided into n FIFOs shared between all channels.
>>
>> 3. The ADMA, not the ADMAIF, manages the flow to the FIFO and this is
>>    because the ADMAIF only tells the ADMA when a word has been
>>    read/written (depending on direction), the ADMAIF does not indicate
>>    if the FIFO is full, empty, etc. Hence, the ADMA needs to know the
>>    total FIFO size.
>>
>> So the ADMA needs to know the FIFO size so that it does not overrun the
>> FIFO and we can also set a burst size (less than the total FIFO size)
>> indicating how many words to transfer at a time. Hence, the two parameters.
> 
> Thanks, I confirm this is my understanding as well.
> 
> To compare to regular case for example SPI on DMA, SPI driver will
> calculate fifo size & burst to be used and program dma (burst size) and
> its own fifos accordingly
> 
> So, in your case why should the peripheral driver not calculate the fifo
> size for both ADMA and ADMAIF and (if required it's own FIFO) and
> program the two (ADMA and ADMAIF).
> 
> What is the limiting factor in this flow is not clear to me.

The FIFO size that is configured by the ADMAIF driver needs to be given
to the ADMA driver so that it can program its registers accordingly. The
difference here is that both the ADMA and ADMAIF need the FIFO size.

Cheers
Jon

-- 
nvpublic

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

[Vinod Koul]

On 19-08-19, 16:56, Jon Hunter wrote:
> >>>>>>> On this, I am inclined to think that dma driver should not be involved.
> >>>>>>> The ADMAIF needs this configuration and we should take the path of
> >>>>>>> dma_router for this piece and add features like this to it
> >>>>>>
> >>>>>> Hi Vinod,
> >>>>>>
> >>>>>> The configuration is needed by both ADMA and ADMAIF. The size is
> >>>>>> configurable
> >>>>>> on ADMAIF side. ADMA needs to know this info and program accordingly.
> >>>>>
> >>>>> Well I would say client decides the settings for both DMA, DMAIF and
> >>>>> sets the peripheral accordingly as well, so client communicates the two
> >>>>> sets of info to two set of drivers
> >>>>
> >>>> That maybe, but I still don't see how the information is passed from the
> >>>> client in the first place. The current problem is that there is no means
> >>>> to pass both a max-burst size and fifo-size to the DMA driver from the
> >>>> client.
> >>>
> >>> So one thing not clear to me is why ADMA needs fifo-size, I thought it
> >>> was to program ADMAIF and if we have client programme the max-burst
> >>> size to ADMA and fifo-size to ADMAIF we wont need that. Can you please
> >>> confirm if my assumption is valid?
> >>
> >> Let me see if I can clarify ...
> >>
> >> 1. The FIFO we are discussing here resides in the ADMAIF module which is
> >>    a separate hardware block the ADMA (although the naming make this
> >>    unclear).
> >>
> >> 2. The size of FIFO in the ADMAIF is configurable and it this is
> >>    configured via the ADMAIF registers. This allows different channels
> >>    to use different FIFO sizes. Think of this as a shared memory that is
> >>    divided into n FIFOs shared between all channels.
> >>
> >> 3. The ADMA, not the ADMAIF, manages the flow to the FIFO and this is
> >>    because the ADMAIF only tells the ADMA when a word has been
> >>    read/written (depending on direction), the ADMAIF does not indicate
> >>    if the FIFO is full, empty, etc. Hence, the ADMA needs to know the
> >>    total FIFO size.
> >>
> >> So the ADMA needs to know the FIFO size so that it does not overrun the
> >> FIFO and we can also set a burst size (less than the total FIFO size)
> >> indicating how many words to transfer at a time. Hence, the two parameters.
> > 
> > Thanks, I confirm this is my understanding as well.
> > 
> > To compare to regular case for example SPI on DMA, SPI driver will
> > calculate fifo size & burst to be used and program dma (burst size) and
> > its own fifos accordingly
> > 
> > So, in your case why should the peripheral driver not calculate the fifo
> > size for both ADMA and ADMAIF and (if required it's own FIFO) and
> > program the two (ADMA and ADMAIF).
> > 
> > What is the limiting factor in this flow is not clear to me.
> 
> The FIFO size that is configured by the ADMAIF driver needs to be given
> to the ADMA driver so that it can program its registers accordingly. The
> difference here is that both the ADMA and ADMAIF need the FIFO size.

Can you please help describing what it is programming using the FIFO
size of ADMAIF?

Thanks
-- 
~Vinod

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

[Sameer Pujar]

Sorry for the delay in replying.

On 8/20/2019 4:35 PM, Vinod Koul wrote:
> On 19-08-19, 16:56, Jon Hunter wrote:
>>>>>>>>> On this, I am inclined to think that dma driver should not be involved.
>>>>>>>>> The ADMAIF needs this configuration and we should take the path of
>>>>>>>>> dma_router for this piece and add features like this to it
>>>>>>>> Hi Vinod,
>>>>>>>>
>>>>>>>> The configuration is needed by both ADMA and ADMAIF. The size is
>>>>>>>> configurable
>>>>>>>> on ADMAIF side. ADMA needs to know this info and program accordingly.
>>>>>>> Well I would say client decides the settings for both DMA, DMAIF and
>>>>>>> sets the peripheral accordingly as well, so client communicates the two
>>>>>>> sets of info to two set of drivers
>>>>>> That maybe, but I still don't see how the information is passed from the
>>>>>> client in the first place. The current problem is that there is no means
>>>>>> to pass both a max-burst size and fifo-size to the DMA driver from the
>>>>>> client.
>>>>> So one thing not clear to me is why ADMA needs fifo-size, I thought it
>>>>> was to program ADMAIF and if we have client programme the max-burst
>>>>> size to ADMA and fifo-size to ADMAIF we wont need that. Can you please
>>>>> confirm if my assumption is valid?
>>>> Let me see if I can clarify ...
>>>>
>>>> 1. The FIFO we are discussing here resides in the ADMAIF module which is
>>>>     a separate hardware block the ADMA (although the naming make this
>>>>     unclear).
>>>>
>>>> 2. The size of FIFO in the ADMAIF is configurable and it this is
>>>>     configured via the ADMAIF registers. This allows different channels
>>>>     to use different FIFO sizes. Think of this as a shared memory that is
>>>>     divided into n FIFOs shared between all channels.
>>>>
>>>> 3. The ADMA, not the ADMAIF, manages the flow to the FIFO and this is
>>>>     because the ADMAIF only tells the ADMA when a word has been
>>>>     read/written (depending on direction), the ADMAIF does not indicate
>>>>     if the FIFO is full, empty, etc. Hence, the ADMA needs to know the
>>>>     total FIFO size.
>>>>
>>>> So the ADMA needs to know the FIFO size so that it does not overrun the
>>>> FIFO and we can also set a burst size (less than the total FIFO size)
>>>> indicating how many words to transfer at a time. Hence, the two parameters.
>>> Thanks, I confirm this is my understanding as well.
>>>
>>> To compare to regular case for example SPI on DMA, SPI driver will
>>> calculate fifo size & burst to be used and program dma (burst size) and
>>> its own fifos accordingly
>>>
>>> So, in your case why should the peripheral driver not calculate the fifo
>>> size for both ADMA and ADMAIF and (if required it's own FIFO) and
>>> program the two (ADMA and ADMAIF).
>>>
>>> What is the limiting factor in this flow is not clear to me.
>> The FIFO size that is configured by the ADMAIF driver needs to be given
>> to the ADMA driver so that it can program its registers accordingly. The
>> difference here is that both the ADMA and ADMAIF need the FIFO size.
> Can you please help describing what it is programming using the FIFO
> size of ADMAIF?
**ADMA channel register is programmed with the same FIFO_SIZE as ADMAIF 
channel
to which it is mapped to.**
As previously mentioned, HW (on ADMA) uses this value to understand the 
FIFO depth
and comes to know when a space of BURST_SIZE is available. ADMAIF is an 
interface
to AHUB and when data moves forward to other clients via AHUB, it sends 
signal to
ADMA per WORD basis. ADMA calculates this to know available space and 
initiates a
transfer when sufficient space is available.
> Thanks