| Message ID | 1556623828-21577-1-git-send-email-spujar@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] dmaengine: add fifo_size member | expand |
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
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
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
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
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
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
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
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!
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! >
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
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
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
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
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
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
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
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.
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.
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
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
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
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.
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
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.
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
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?
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
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.
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 :)
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
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
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
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
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
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
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
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.
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
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
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.
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.
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.
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.
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 :(
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.
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.
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. >
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.
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.
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
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
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
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
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
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
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
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
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; }; /**
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(+)