ASoC: imx dma tx burst size set to 6 and to 4 ?

ASoC: imx dma tx burst size set to 6 and to 4 ?

[Lambrecht Jürgen]

Hello,

There is an inconsistency in this code:

/sound/soc/imx/imx-ssi.c:         ssi->dma_params_tx.burstsize = 4;
/sound/soc/imx/imx-pcm-dma-mx2.c: ssi->dma_params_tx.burstsize = 6;

Datasheet:
This sets the field TFWM0 in register SFCSR (SSI_SFCSR_TFWM0): the 
number of data words needed to set the empty flag (TFE0). Can be set 
from 1 to 8; FIFO size is 8 words (of 24b). And this is also the DMA 
burst size.

Which is best depends on the application (the higher the more efficient, 
but the more risk for a FIFO underrun). I would take 4.

But I guess it should only be set at 1 place, and what is then the best 
place?

Regards,
J?rgen

details:

/sound/soc/imx/imx-ssi.c: ssi->dma_params_tx.burstsize = 4;
Set in commit
commit 0a93421b6adf8ba127b3eafc4c16e3a14017e2ae
Author: Javier Martin <javier.martin@vista-silicon.com>
Date:   Tue Mar 1 15:02:06 2011 +0100

     ASoC: Fix burstsize and DSP_B format problems in imx-ssi.

     When choosing IMX_DMA flag, burtsizes are set to its default
     value (0) which leads to driver malfunction. Change them to 4.

     DSP_B interface needs additional flag to match DSP_B formats
     as described in several codecs as wm8741 and aic3205.

     Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
     Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
     Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Index in diff:
diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c
index 30894ea..bc92ec6 100644

/sound/soc/imx/imx-pcm-dma-mx2.c: ssi->dma_params_tx.burstsize = 6;
Set in commit
commit 2c4cf17a52f04fbe929977252d5b8ab81d2c6e9b
Author: Wolfram Sang <w.sang@pengutronix.de>
Date:   Fri Mar 25 16:51:44 2011 +0100

     ASoC: imx: set watermarks for mx2-dma

     They got accidently removed by f0fba2a (ASoC: multi-component - ASoC
     Multi-Component Support). Reintroduce them and get rid of the
     superfluous defines because the fiq-driver has its own hardcoded 
values.

     Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
     Acked-by: Liam Girdwood <lrg@ti.com>
     Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
     Cc: stable at kernel.org

Index in diff:
diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c 
b/sound/soc/imx/imx-pcm-dma-mx2.c
index 671ef8d..b2ed764 100644
-- 

J?rgen Lambrecht
R&D Associate
Tel: +32 (0)51 303045    Fax: +32 (0)51 310670
http://www.televic-rail.com
Televic Rail NV - Leo Bekaertlaan 1 - 8870 Izegem - Belgium
Company number 0825.539.581 - RPR Kortrijk

ASoC: imx dma tx burst size set to 6 and to 4 ?

[Sascha Hauer]

(Added Javier to Cc)

On Thu, Jun 30, 2011 at 08:25:29AM +0200, Lambrecht J?rgen wrote:
> Hello,
> 
> There is an inconsistency in this code:
> 
> /sound/soc/imx/imx-ssi.c:         ssi->dma_params_tx.burstsize = 4;
> /sound/soc/imx/imx-pcm-dma-mx2.c: ssi->dma_params_tx.burstsize = 6;
> 
> Datasheet:
> This sets the field TFWM0 in register SFCSR (SSI_SFCSR_TFWM0): the 
> number of data words needed to set the empty flag (TFE0). Can be set 
> from 1 to 8; FIFO size is 8 words (of 24b). And this is also the DMA 
> burst size.
> 
> Which is best depends on the application (the higher the more efficient, 
> but the more risk for a FIFO underrun). I would take 4.
> 
> But I guess it should only be set at 1 place, and what is then the best 
> place?

We need different burstsize settings for the FIQ driver and for the DMA
driver. For this reason the original intention was to set these values
in sound/soc/imx/imx-pcm-dma-mx2.c and in sound/soc/imx/imx-pcm-fiq.c.


> commit 0a93421b6adf8ba127b3eafc4c16e3a14017e2ae
> Author: Javier Martin <javier.martin@vista-silicon.com>
> Date:   Tue Mar 1 15:02:06 2011 +0100
> 
>     ASoC: Fix burstsize and DSP_B format problems in imx-ssi.
>     
>     When choosing IMX_DMA flag, burtsizes are set to its default
>     value (0) which leads to driver malfunction. Change them to 4.
>     
>     DSP_B interface needs additional flag to match DSP_B formats
>     as described in several codecs as wm8741 and aic3205.
>     
>     Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>     Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
>     Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c
> index 30894ea..bc92ec6 100644
> --- a/sound/soc/imx/imx-ssi.c
> +++ b/sound/soc/imx/imx-ssi.c
> @@ -108,7 +108,7 @@ static int imx_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
>  		break;
>  	case SND_SOC_DAIFMT_DSP_B:
>  		/* data on rising edge of bclk, frame high with data */
> -		strcr |= SSI_STCR_TFSL;
> +		strcr |= SSI_STCR_TFSL | SSI_STCR_TXBIT0;
>  		break;
>  	case SND_SOC_DAIFMT_DSP_A:
>  		/* data on rising edge of bclk, frame high 1clk before data */
> @@ -656,6 +656,9 @@ static int imx_ssi_probe(struct platform_device *pdev)
>  	ssi->dma_params_rx.dma_addr = res->start + SSI_SRX0;
>  	ssi->dma_params_tx.dma_addr = res->start + SSI_STX0;
>  
> +	ssi->dma_params_tx.burstsize = 4;
> +	ssi->dma_params_rx.burstsize = 4;
> +

Javier, is this hunk necessary? From what I see these values should be
overwritten later anyway.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

ASoC: imx dma tx burst size set to 6 and to 4 ?

[javier Martin]

2011/7/4 Sascha Hauer <s.hauer@pengutronix.de>:
> (Added Javier to Cc)
>
> On Thu, Jun 30, 2011 at 08:25:29AM +0200, Lambrecht J?rgen wrote:
>> Hello,
>>
>> There is an inconsistency in this code:
>>
>> /sound/soc/imx/imx-ssi.c: ? ? ? ? ssi->dma_params_tx.burstsize = 4;
>> /sound/soc/imx/imx-pcm-dma-mx2.c: ssi->dma_params_tx.burstsize = 6;
>>
>> Datasheet:
>> This sets the field TFWM0 in register SFCSR (SSI_SFCSR_TFWM0): the
>> number of data words needed to set the empty flag (TFE0). Can be set
>> from 1 to 8; FIFO size is 8 words (of 24b). And this is also the DMA
>> burst size.
>>
>> Which is best depends on the application (the higher the more efficient,
>> but the more risk for a FIFO underrun). I would take 4.
>>
>> But I guess it should only be set at 1 place, and what is then the best
>> place?
>
> We need different burstsize settings for the FIQ driver and for the DMA
> driver. For this reason the original intention was to set these values
> in sound/soc/imx/imx-pcm-dma-mx2.c and in sound/soc/imx/imx-pcm-fiq.c.
>
>
>> commit 0a93421b6adf8ba127b3eafc4c16e3a14017e2ae
>> Author: Javier Martin <javier.martin@vista-silicon.com>
>> Date: ? Tue Mar 1 15:02:06 2011 +0100
>>
>> ? ? ASoC: Fix burstsize and DSP_B format problems in imx-ssi.
>>
>> ? ? When choosing IMX_DMA flag, burtsizes are set to its default
>> ? ? value (0) which leads to driver malfunction. Change them to 4.
>>
>> ? ? DSP_B interface needs additional flag to match DSP_B formats
>> ? ? as described in several codecs as wm8741 and aic3205.
>>
>> ? ? Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ? ? Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
>> ? ? Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
>>
>> diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c
>> index 30894ea..bc92ec6 100644
>> --- a/sound/soc/imx/imx-ssi.c
>> +++ b/sound/soc/imx/imx-ssi.c
>> @@ -108,7 +108,7 @@ static int imx_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
>> ? ? ? ? ? ? ? break;
>> ? ? ? case SND_SOC_DAIFMT_DSP_B:
>> ? ? ? ? ? ? ? /* data on rising edge of bclk, frame high with data */
>> - ? ? ? ? ? ? strcr |= SSI_STCR_TFSL;
>> + ? ? ? ? ? ? strcr |= SSI_STCR_TFSL | SSI_STCR_TXBIT0;
>> ? ? ? ? ? ? ? break;
>> ? ? ? case SND_SOC_DAIFMT_DSP_A:
>> ? ? ? ? ? ? ? /* data on rising edge of bclk, frame high 1clk before data */
>> @@ -656,6 +656,9 @@ static int imx_ssi_probe(struct platform_device *pdev)
>> ? ? ? ssi->dma_params_rx.dma_addr = res->start + SSI_SRX0;
>> ? ? ? ssi->dma_params_tx.dma_addr = res->start + SSI_STX0;
>>
>> + ? ? ssi->dma_params_tx.burstsize = 4;
>> + ? ? ssi->dma_params_rx.burstsize = 4;
>> +
>
> Javier, is this hunk necessary? From what I see these values should be
> overwritten later anyway.
>
> Sascha

Hi,
without this hunk, both burstsizes where set to 0. This was checked
experimentally:

- Audio didn't work properly.
- Some runtime printk's showed that burstsizes where set to 0.

It seems assignments in imx-pcm-dma-mx2.c file are done at a place
they do not take effect.


-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

ASoC: imx dma tx burst size set to 6 and to 4 ?

[Sascha Hauer]

On Mon, Jul 04, 2011 at 10:00:34AM +0200, javier Martin wrote:
> 2011/7/4 Sascha Hauer <s.hauer@pengutronix.de>:
> > (Added Javier to Cc)
> >
> > On Thu, Jun 30, 2011 at 08:25:29AM +0200, Lambrecht J?rgen wrote:
> >> Hello,
> >>
> >> There is an inconsistency in this code:
> >>
> >> /sound/soc/imx/imx-ssi.c: ? ? ? ? ssi->dma_params_tx.burstsize = 4;
> >> /sound/soc/imx/imx-pcm-dma-mx2.c: ssi->dma_params_tx.burstsize = 6;
> >>
> >> Datasheet:
> >> This sets the field TFWM0 in register SFCSR (SSI_SFCSR_TFWM0): the
> >> number of data words needed to set the empty flag (TFE0). Can be set
> >> from 1 to 8; FIFO size is 8 words (of 24b). And this is also the DMA
> >> burst size.
> >>
> >> Which is best depends on the application (the higher the more efficient,
> >> but the more risk for a FIFO underrun). I would take 4.
> >>
> >> But I guess it should only be set at 1 place, and what is then the best
> >> place?
> >
> > We need different burstsize settings for the FIQ driver and for the DMA
> > driver. For this reason the original intention was to set these values
> > in sound/soc/imx/imx-pcm-dma-mx2.c and in sound/soc/imx/imx-pcm-fiq.c.
> >
> >
> >> commit 0a93421b6adf8ba127b3eafc4c16e3a14017e2ae
> >> Author: Javier Martin <javier.martin@vista-silicon.com>
> >> Date: ? Tue Mar 1 15:02:06 2011 +0100
> >>
> >> ? ? ASoC: Fix burstsize and DSP_B format problems in imx-ssi.
> >>
> >> ? ? When choosing IMX_DMA flag, burtsizes are set to its default
> >> ? ? value (0) which leads to driver malfunction. Change them to 4.
> >>
> >> ? ? DSP_B interface needs additional flag to match DSP_B formats
> >> ? ? as described in several codecs as wm8741 and aic3205.
> >>
> >> ? ? Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >> ? ? Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
> >> ? ? Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> >>
> >> diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c
> >> index 30894ea..bc92ec6 100644
> >> --- a/sound/soc/imx/imx-ssi.c
> >> +++ b/sound/soc/imx/imx-ssi.c
> >> @@ -108,7 +108,7 @@ static int imx_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> >> ? ? ? ? ? ? ? break;
> >> ? ? ? case SND_SOC_DAIFMT_DSP_B:
> >> ? ? ? ? ? ? ? /* data on rising edge of bclk, frame high with data */
> >> - ? ? ? ? ? ? strcr |= SSI_STCR_TFSL;
> >> + ? ? ? ? ? ? strcr |= SSI_STCR_TFSL | SSI_STCR_TXBIT0;
> >> ? ? ? ? ? ? ? break;
> >> ? ? ? case SND_SOC_DAIFMT_DSP_A:
> >> ? ? ? ? ? ? ? /* data on rising edge of bclk, frame high 1clk before data */
> >> @@ -656,6 +656,9 @@ static int imx_ssi_probe(struct platform_device *pdev)
> >> ? ? ? ssi->dma_params_rx.dma_addr = res->start + SSI_SRX0;
> >> ? ? ? ssi->dma_params_tx.dma_addr = res->start + SSI_STX0;
> >>
> >> + ? ? ssi->dma_params_tx.burstsize = 4;
> >> + ? ? ssi->dma_params_rx.burstsize = 4;
> >> +
> >
> > Javier, is this hunk necessary? From what I see these values should be
> > overwritten later anyway.
> >
> > Sascha
> 
> Hi,
> without this hunk, both burstsizes where set to 0. This was checked
> experimentally:
> 
> - Audio didn't work properly.
> - Some runtime printk's showed that burstsizes where set to 0.
> 
> It seems assignments in imx-pcm-dma-mx2.c file are done at a place
> they do not take effect.

I just did a quick test. In imx_ssi_dai_probe where the values are
actually used they are correctly initialized.
Anyway, the current way to initialize these values seems a bit strange.
Maybe we should pass the burstsize parameters in imx_pcm_new instead.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |