ASoC: pxa2xx-i2s

ASoC: pxa2xx-i2s

[Brian Rhodes]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

i2s replay is not being enabled properly on occasion on my pxa270 
board.  Current code is touching SACR1 in hw_params which I believe is 
not correct.  That should be configured in trigger before enabling i2s.  
Record is not disabled for playback mode.  Any invalid data in the 
transmit fifo causes deadlock and replay is being disabled.

I have been able to reproduce it in kernel 2.6.26 as well as Linus' git 
tree using speaker-test.  (repeatedly starting and stopping).  I've 
verified that this fix prevents (at least) this condition.

Please review this patch and let me know if there is anything I have 
overlooked.

Thanks

[-- Attachment #2: pxa2xx-i2s-clean-startup.patch --]
[-- Type: text/plain, Size: 906 bytes --]

diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
index 517991f..6cda7f1 100644
--- a/sound/soc/pxa/pxa2xx-i2s.c
+++ b/sound/soc/pxa/pxa2xx-i2s.c
@@ -211,12 +211,10 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
 	if (!(SACR0 & SACR0_ENB)) {
 
 		SACR0 = 0;
-		SACR1 = 0;
 		if (pxa_i2s.master)
 			SACR0 |= SACR0_BCKD;
 
 		SACR0 |= SACR0_RFTH(14) | SACR0_TFTH(1);
-		SACR1 |= pxa_i2s.fmt;
 	}
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		SAIMR |= SAIMR_TFS;
@@ -257,6 +255,12 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		SACR1 = pxa_i2s.fmt | SACR1_DRPL | SACR1_DREC;
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			SACR1 &= ~SACR1_DRPL;
+		} else {
+			SACR1 &= ~SACR1_DREC;
+		}
 		SACR0 |= SACR0_ENB;
 		break;
 	case SNDRV_PCM_TRIGGER_RESUME:

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: pxa2xx-i2s

[Karl Beldan]

Brian Rhodes wrote:
> i2s replay is not being enabled properly on occasion on my pxa270 
> board.  Current code is touching SACR1 in hw_params which I believe is 
> not correct.  That should be configured in trigger before enabling i2s.  
> Record is not disabled for playback mode.  Any invalid data in the 
> transmit fifo causes deadlock and replay is being disabled.
> 
> I have been able to reproduce it in kernel 2.6.26 as well as Linus' git 
> tree using speaker-test.  (repeatedly starting and stopping).  I've 
> verified that this fix prevents (at least) this condition.
> 
> Please review this patch and let me know if there is anything I have 
> overlooked.
> 
> Thanks
> 
> diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
> index 517991f..6cda7f1 100644
> --- a/sound/soc/pxa/pxa2xx-i2s.c
> +++ b/sound/soc/pxa/pxa2xx-i2s.c
> @@ -211,12 +211,10 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
>  	if (!(SACR0 & SACR0_ENB)) {
>  
>  		SACR0 = 0;
> -		SACR1 = 0;
>  		if (pxa_i2s.master)
>  			SACR0 |= SACR0_BCKD;
>  
>  		SACR0 |= SACR0_RFTH(14) | SACR0_TFTH(1);
> -		SACR1 |= pxa_i2s.fmt;
>  	}
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  		SAIMR |= SAIMR_TFS;
> @@ -257,6 +255,12 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> +		SACR1 = pxa_i2s.fmt | SACR1_DRPL | SACR1_DREC;

Here you are stopping any ongoing stream - removing SACR1 reset in
hw_params should be enough.

> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			SACR1 &= ~SACR1_DRPL;
> +		} else {
> +			SACR1 &= ~SACR1_DREC;
> +		}
>  		SACR0 |= SACR0_ENB;
>  		break;
>  	case SNDRV_PCM_TRIGGER_RESUME:
> 

If - I can - I will post my changes, unless you are finished by then ;)

-- 
Karl

Re: ASoC: pxa2xx-i2s

[Brian Rhodes]

Karl Beldan wrote:
> Brian Rhodes wrote:
>   
>> i2s replay is not being enabled properly on occasion on my pxa270 
>> board.  Current code is touching SACR1 in hw_params which I believe is 
>> not correct.  That should be configured in trigger before enabling i2s.  
>> Record is not disabled for playback mode.  Any invalid data in the 
>> transmit fifo causes deadlock and replay is being disabled.
>>
>> I have been able to reproduce it in kernel 2.6.26 as well as Linus' git 
>> tree using speaker-test.  (repeatedly starting and stopping).  I've 
>> verified that this fix prevents (at least) this condition.
>>
>> Please review this patch and let me know if there is anything I have 
>> overlooked.
>>
>> Thanks
>>
>> diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
>> index 517991f..6cda7f1 100644
>> --- a/sound/soc/pxa/pxa2xx-i2s.c
>> +++ b/sound/soc/pxa/pxa2xx-i2s.c
>> @@ -211,12 +211,10 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
>>  	if (!(SACR0 & SACR0_ENB)) {
>>  
>>  		SACR0 = 0;
>> -		SACR1 = 0;
>>  		if (pxa_i2s.master)
>>  			SACR0 |= SACR0_BCKD;
>>  
>>  		SACR0 |= SACR0_RFTH(14) | SACR0_TFTH(1);
>> -		SACR1 |= pxa_i2s.fmt;
>>  	}
>>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>  		SAIMR |= SAIMR_TFS;
>> @@ -257,6 +255,12 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>>  
>>  	switch (cmd) {
>>  	case SNDRV_PCM_TRIGGER_START:
>> +		SACR1 = pxa_i2s.fmt | SACR1_DRPL | SACR1_DREC;
>>     
>
> Here you are stopping any ongoing stream - removing SACR1 reset in
> hw_params should be enough.
>
>   
In that case, replay would still be disabled is i2s was shutdown.

Value at address 0x40400004 (0x4001e004): 0x10
Value at address 0x40400000 (0x4001e000): 0xE104

I am using normal i2s mode, so in the original code it is basically 
enabling replay twice in hw_params, then enabling i2s in trigger.  Then 
disabling replay and i2s in shutdown.  I'm trying to understand what the 
real problem is here.  Is something failing that is causing replay to be 
disabled by the i2s controller?  The condition is intermittent.  The 
only difference could be timing, or the data in the transmit FIFO.  When 
it gets in this state I can clear DRPL using devmem2 and it audio will 
start playing.

>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +			SACR1 &= ~SACR1_DRPL;
>> +		} else {
>> +			SACR1 &= ~SACR1_DREC;
>> +		}
>>  		SACR0 |= SACR0_ENB;
>>  		break;
>>  	case SNDRV_PCM_TRIGGER_RESUME:
>>
>>     
>
> If - I can - I will post my changes, unless you are finished by then ;)
>
>   

Re: ASoC: pxa2xx-i2s

[Brian Rhodes]

[-- Attachment #1: Type: text/plain, Size: 3684 bytes --]

Brian Rhodes wrote:
> Karl Beldan wrote:
>   
>> Brian Rhodes wrote:
>>   
>>     
>>> i2s replay is not being enabled properly on occasion on my pxa270 
>>> board.  Current code is touching SACR1 in hw_params which I believe is 
>>> not correct.  That should be configured in trigger before enabling i2s.  
>>> Record is not disabled for playback mode.  Any invalid data in the 
>>> transmit fifo causes deadlock and replay is being disabled.
>>>
>>> I have been able to reproduce it in kernel 2.6.26 as well as Linus' git 
>>> tree using speaker-test.  (repeatedly starting and stopping).  I've 
>>> verified that this fix prevents (at least) this condition.
>>>
>>> Please review this patch and let me know if there is anything I have 
>>> overlooked.
>>>
>>> Thanks
>>>
>>> diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
>>> index 517991f..6cda7f1 100644
>>> --- a/sound/soc/pxa/pxa2xx-i2s.c
>>> +++ b/sound/soc/pxa/pxa2xx-i2s.c
>>> @@ -211,12 +211,10 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
>>>  	if (!(SACR0 & SACR0_ENB)) {
>>>  
>>>  		SACR0 = 0;
>>> -		SACR1 = 0;
>>>  		if (pxa_i2s.master)
>>>  			SACR0 |= SACR0_BCKD;
>>>  
>>>  		SACR0 |= SACR0_RFTH(14) | SACR0_TFTH(1);
>>> -		SACR1 |= pxa_i2s.fmt;
>>>  	}
>>>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>  		SAIMR |= SAIMR_TFS;
>>> @@ -257,6 +255,12 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>>>  
>>>  	switch (cmd) {
>>>  	case SNDRV_PCM_TRIGGER_START:
>>> +		SACR1 = pxa_i2s.fmt | SACR1_DRPL | SACR1_DREC;
>>>     
>>>       
>> Here you are stopping any ongoing stream - removing SACR1 reset in
>> hw_params should be enough.
>>
>>   
>>     
> In that case, replay would still be disabled is i2s was shutdown.
>
> Value at address 0x40400004 (0x4001e004): 0x10
> Value at address 0x40400000 (0x4001e000): 0xE104
>
> I am using normal i2s mode, so in the original code it is basically 
> enabling replay twice in hw_params, then enabling i2s in trigger.  Then 
> disabling replay and i2s in shutdown.  I'm trying to understand what the 
> real problem is here.  Is something failing that is causing replay to be 
> disabled by the i2s controller?  The condition is intermittent.  The 
> only difference could be timing, or the data in the transmit FIFO.  When 
> it gets in this state I can clear DRPL using devmem2 and it audio will 
> start playing.
>
>   
>>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>> +			SACR1 &= ~SACR1_DRPL;
>>> +		} else {
>>> +			SACR1 &= ~SACR1_DREC;
>>> +		}
>>>  		SACR0 |= SACR0_ENB;
>>>  		break;
>>>  	case SNDRV_PCM_TRIGGER_RESUME:
>>>
>>>     
>>>       
>> If - I can - I will post my changes, unless you are finished by then ;)
>>
>>   
>>     

This method also works, and does not affect the the other stream.  The 
difference for me really seems to be that when DREC is set and DRPL is 
cleared when i2s is enabled it will occasionally never start.  I cannot 
recreate the issue by with the original code if I initialize SACR1 to 
set DREC in startup and do not reset it in hw_params.  It is possible 
the problem is with the codec.  TLV320dac2x.

I am simply testing this using.

while [ 1 ]; do ./speaker-test ; done
while [ 1 ]; do killall speaker-test ; sleep 1 ; done

Let it run for a few hours.  The code from 2.6.26 and the code from 
Linus' tree both fail within a minute on my pxa270 system.  When they 
are fail DRPL is set every time.  The attached code has run for a couple 
hours.  Nothing in the i2s controller documentation mentions the 
possibility of anything in the hardware disabling replay, so I am 
confused what is happening here.



[-- Attachment #2: pxa2xx-i2s-clean-startup.patch --]
[-- Type: text/plain, Size: 2091 bytes --]

diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
index 517991f..1d8b43f 100644
--- a/sound/soc/pxa/pxa2xx-i2s.c
+++ b/sound/soc/pxa/pxa2xx-i2s.c
@@ -133,6 +133,7 @@ static int pxa2xx_i2s_startup(struct snd_pcm_substream *substream,
 	if (!cpu_dai->active) {
 		SACR0 |= SACR0_RST;
 		SACR0 = 0;
+		SACR1 = (pxa_i2s.fmt | SACR1_DRPL | SACR1_DREC);
 	}
 
 	return 0;
@@ -209,20 +210,13 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
 
 	/* is port used by another stream */
 	if (!(SACR0 & SACR0_ENB)) {
-
 		SACR0 = 0;
-		SACR1 = 0;
 		if (pxa_i2s.master)
 			SACR0 |= SACR0_BCKD;
 
 		SACR0 |= SACR0_RFTH(14) | SACR0_TFTH(1);
-		SACR1 |= pxa_i2s.fmt;
 	}
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		SAIMR |= SAIMR_TFS;
-	else
-		SAIMR |= SAIMR_RFS;
-
+	
 	switch (params_rate(params)) {
 	case 8000:
 		SADIV = 0x48;
@@ -257,13 +251,23 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		SACR0 |= SACR0_ENB;
-		break;
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			SAIMR |= SAIMR_TFS; SACR1 &= ~SACR1_DRPL;
+		} else {
+			SAIMR |= SAIMR_RFS; SACR1 &= ~SACR1_DREC;
+		}
+		SACR0 |= SACR0_ENB;
+		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			SACR1 |= SACR1_DRPL; SAIMR &= ~SAIMR_TFS;
+		} else {
+			SACR1 |= SACR1_DREC; SAIMR &= ~SAIMR_RFS;
+		}
 		break;
 	default:
 		ret = -EINVAL;
@@ -275,14 +279,6 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
 				struct snd_soc_dai *dai)
 {
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		SACR1 |= SACR1_DRPL;
-		SAIMR &= ~SAIMR_TFS;
-	} else {
-		SACR1 |= SACR1_DREC;
-		SAIMR &= ~SAIMR_RFS;
-	}
-
 	if (SACR1 & (SACR1_DREC | SACR1_DRPL)) {
 		SACR0 &= ~SACR0_ENB;
 		pxa_i2s_wait();

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: pxa2xx-i2s

[Mark Brown]

On Tue, Jan 20, 2009 at 04:24:21PM -0600, Brian Rhodes wrote:

> This method also works, and does not affect the the other stream.  The 
> difference for me really seems to be that when DREC is set and DRPL is 
> cleared when i2s is enabled it will occasionally never start.  I cannot 

Without checking the datasheet for the chip this looks broadly
reasonable to me.

> recreate the issue by with the original code if I initialize SACR1 to 
> set DREC in startup and do not reset it in hw_params.  It is possible 
> the problem is with the codec.  TLV320dac2x.

What is the clock master in your system?

> hours.  Nothing in the i2s controller documentation mentions the 
> possibility of anything in the hardware disabling replay, so I am 
> confused what is happening here.

If chip documentation were always complete and accurate the world would
be a much better place :)

Re: ASoC: pxa2xx-i2s

[Mark Brown]

On Wed, Jan 21, 2009 at 10:46:17AM -0600, Brian G Rhodes wrote:
> Mark Brown wrote:

> >What is the clock master in your system?

> The master is the i2s controller sysclk.  The dac2x is in slave mode.  
> Though the i2s controller isn't directly supplying the bit clock to the 
> codec in this case.  We're using the PLL on the dac2x.

Just to confirm, the codec is the clock master on the I2S bus itself?

Re: ASoC: pxa2xx-i2s

[Brian Rhodes]

Mark Brown wrote:
> On Wed, Jan 21, 2009 at 10:46:17AM -0600, Brian G Rhodes wrote:
>   
>> Mark Brown wrote:
>>     
>
>   
>>> What is the clock master in your system?
>>>       
>
>   
>> The master is the i2s controller sysclk.  The dac2x is in slave mode.  
>> Though the i2s controller isn't directly supplying the bit clock to the 
>> codec in this case.  We're using the PLL on the dac2x.
>>     
>
> Just to confirm, the codec is the clock master on the I2S bus itself?
>
>   
No, the 270 is the clock master on the I2S bus.  The codec is sourcing 
the clock input using the PLL.  I apologize for being unclear.

Re: ASoC: pxa2xx-i2s

[Mark Brown]

On Wed, Jan 21, 2009 at 02:13:12PM -0600, Brian Rhodes wrote:

> No, the 270 is the clock master on the I2S bus.  The codec is sourcing 
> the clock input using the PLL.  I apologize for being unclear.

I think it's me that's not being clear, sorry - I'm trying to find out
is if the LRCLK and BCLK signals on the I2S bus itself are being driven
by the codec or by the I2S controller.  If the codec driver is the clock
master then your earlier speculation that your problems with the I2S
controller may be due to some interaction with the codec driver are more
likely to be true.

Re: ASoC: pxa2xx-i2s

[Brian Rhodes]

Mark Brown wrote:
> On Wed, Jan 21, 2009 at 02:13:12PM -0600, Brian Rhodes wrote:
>
>   
>> No, the 270 is the clock master on the I2S bus.  The codec is sourcing 
>> the clock input using the PLL.  I apologize for being unclear.
>>     
>
> I think it's me that's not being clear, sorry - I'm trying to find out
> is if the LRCLK and BCLK signals on the I2S bus itself are being driven
> by the codec or by the I2S controller.  If the codec driver is the clock
> master then your earlier speculation that your problems with the I2S
> controller may be due to some interaction with the codec driver are more
> likely to be true.
LCRK and BCLK are configured as inputs.  The Codec is not the bus master.

Re: ASoC: pxa2xx-i2s

[Brian Rhodes]

Mark Brown wrote:
> On Wed, Jan 21, 2009 at 02:13:12PM -0600, Brian Rhodes wrote:
>
>   
>> No, the 270 is the clock master on the I2S bus.  The codec is sourcing 
>> the clock input using the PLL.  I apologize for being unclear.
>>     
>
> I think it's me that's not being clear, sorry - I'm trying to find out
> is if the LRCLK and BCLK signals on the I2S bus itself are being driven
> by the codec or by the I2S controller.  If the codec driver is the clock
> master then your earlier speculation that your problems with the I2S
> controller may be due to some interaction with the codec driver are more
> likely to be true.
>
>   
I'm curious if you think this problem may be arising from the toggling 
of the direction of BCLK on the PXA while the I2S clock is enabled.  I'm 
sure there are people who have a reason for switching the bit clock 
master, but I'm thinking that this should not be explicitly done if it 
is not changing.

Re: ASoC: pxa2xx-i2s

[Mark Brown]

On Fri, Jan 23, 2009 at 10:50:39AM -0600, Brian Rhodes wrote:

> I'm curious if you think this problem may be arising from the toggling 
> of the direction of BCLK on the PXA while the I2S clock is enabled.  I'm 
> sure there are people who have a reason for switching the bit clock 
> master, but I'm thinking that this should not be explicitly done if it 
> is not changing.

Looking at the way the clock is enabled it looks like the hardware
requires the clock to be enabled to write to the registers, though again
I've not actually looked at the reference manual to check so this is
idle speculation.

Avoiding changing the register settings if they are already correct does
seem like a reasonable thing to try, though.  The ordering used when
configuring may be worth looking at too.

Re: ASoC: pxa2xx-i2s

[Karl Beldan]

Maybe you still have some device at hand for testing.

http://mailman.alsa-project.org/pipermail/alsa-devel/2009-May/017038.html

-- 
Karl