[PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

- hw_params enables both RPL and REC functions each time,
enable the appropriate function in pxa2xx_i2s_trigger.
- pxa2xx_i2s_shutdown disables i2s anytime one of RPL or REC
function is off, turn it off only when both functions are off.
- do not put the clk_i2s for no reason in pxa2xx_i2s_shutdown.

Signed-off-by: Karl Beldan <karl.beldan@mobile-devices.fr>
---
 sound/soc/pxa/pxa2xx-i2s.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
index 52862dc..0e53d07 100644
--- a/sound/soc/pxa/pxa2xx-i2s.c
+++ b/sound/soc/pxa/pxa2xx-i2s.c
@@ -178,9 +178,7 @@ 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;
 
@@ -226,6 +224,10 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			SACR1 &= ~SACR1_DRPL;
+		else
+			SACR1 &= ~SACR1_DREC;
 		SACR0 |= SACR0_ENB;
 		break;
 	case SNDRV_PCM_TRIGGER_RESUME:
@@ -252,13 +254,11 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
 		SAIMR &= ~SAIMR_RFS;
 	}
 
-	if (SACR1 & (SACR1_DREC | SACR1_DRPL)) {
+	if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
 		SACR0 &= ~SACR0_ENB;
 		pxa_i2s_wait();
 		clk_disable(clk_i2s);
 	}
-
-	clk_put(clk_i2s);
 }
 
 #ifdef CONFIG_PM
-- 
1.6.3.rc1.34.g0be9b

-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Fri, May 08, 2009 at 01:53:47AM +0200, Karl Beldan wrote:
> 
> - hw_params enables both RPL and REC functions each time,
> enable the appropriate function in pxa2xx_i2s_trigger.
> - pxa2xx_i2s_shutdown disables i2s anytime one of RPL or REC
> function is off, turn it off only when both functions are off.

Looks OK, but...

> - do not put the clk_i2s for no reason in pxa2xx_i2s_shutdown.

...this really ought to be a separate patch.  I'll wait until Monday
when I'm back in the office before applying so I can test.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

On Fri, May 8, 2009 at 12:42 PM, Mark Brown <broonie@sirena.org.uk> wrote:
> On Fri, May 08, 2009 at 01:53:47AM +0200, Karl Beldan wrote:
>>
>> - hw_params enables both RPL and REC functions each time,
>> enable the appropriate function in pxa2xx_i2s_trigger.
>> - pxa2xx_i2s_shutdown disables i2s anytime one of RPL or REC
>> function is off, turn it off only when both functions are off.
>
> Looks OK, but...
>
>> - do not put the clk_i2s for no reason in pxa2xx_i2s_shutdown.
>
> ...this really ought to be a separate patch.  I'll wait until Monday
> when I'm back in the office before applying so I can test.
>

I'll split it.

-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Fri, May 08, 2009 at 11:42:29AM +0100, Mark Brown wrote:

> ...this really ought to be a separate patch.  I'll wait until Monday
> when I'm back in the office before applying so I can test.

This patch is causing problems in my testing.  With the patch the clocks
are not stopped when closing the stream, causing the next playback to
fail.  However, the error handling from that causes the appropriate
cleanup and allows playback to proceed.

I've not had time to review properly what's going on here - I suspect
that the splits in the patch series need to be looked at but for now
I'll not apply this.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

Mark Brown wrote:
> On Fri, May 08, 2009 at 11:42:29AM +0100, Mark Brown wrote:
> 
>> ...this really ought to be a separate patch.  I'll wait until Monday
>> when I'm back in the office before applying so I can test.
> 
> This patch is causing problems in my testing.  With the patch the clocks
> are not stopped when closing the stream, causing the next playback to
> fail.  However, the error handling from that causes the appropriate
> cleanup and allows playback to proceed.
> 

To be sure:
With current tree you do not have this problem.
You applied 1/4 and 2/4 and you have this problem.
Right ?

> I've not had time to review properly what's going on here - I suspect
> that the splits in the patch series need to be looked at but for now
> I'll not apply this.

-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Mon, May 11, 2009 at 09:43:11PM +0200, Karl Beldan wrote:

> To be sure:
> With current tree you do not have this problem.
> You applied 1/4 and 2/4 and you have this problem.
> Right ?

Actually just patch 2; patches 1 needs reworking.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

Mark Brown wrote:
> On Mon, May 11, 2009 at 09:43:11PM +0200, Karl Beldan wrote:
> 
>> To be sure:
>> With current tree you do not have this problem.
>> You applied 1/4 and 2/4 and you have this problem.
>> Right ?
> 
> Actually just patch 2; patches 1 needs reworking.

Then it is perfectly normal since reset enables both REC and RPL, 2/4 needs 1/4.
The current tree disables the clocks anytime one function is disabled.

I resent 1/4 and will make it 1/5 with the whole serie once everything is Clear.


-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Tue, May 12, 2009 at 12:00:33AM +0200, Karl Beldan wrote:

> Then it is perfectly normal since reset enables both REC and RPL, 2/4 needs 1/4.
> The current tree disables the clocks anytime one function is disabled.

That doesn't seem to tie up - I can see the initialisation changing the
behaviour on first run but it seems surprising that this should happen
on subsequent runs too.  Alternatively, is your initialisation patch
safe to apply by itself?

> I resent 1/4 and will make it 1/5 with the whole serie once everything is Clear.

As previously discussed you need to rework the patch to not do the reset
on initial probe not when the module is loaded, you need to address this
rather than reposting.

I'll try to find time to re-review the series but I'm going to need to
sit down with the datasheet and check this in much more detail.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

Mark Brown wrote:
> On Tue, May 12, 2009 at 12:00:33AM +0200, Karl Beldan wrote:
> 
>> Then it is perfectly normal since reset enables both REC and RPL, 2/4 needs 1/4.
>> The current tree disables the clocks anytime one function is disabled.
> 
> That doesn't seem to tie up - I can see the initialisation changing the
> behaviour on first run but it seems surprising that this should happen
> on subsequent runs too.  Alternatively, is your initialisation patch
> safe to apply by itself?
> 
Well 2/4 stops the clocks only if both REC and RPL are disabled.
Without 1/4 you end up with REC enabled at startup.
In a scenario where you have never used REC you end up RPLing with REC always on.
REC being on at shutdown(),clocks won't stop.

>> I resent 1/4 and will make it 1/5 with the whole serie once everything is Clear.
> 
> As previously discussed you need to rework the patch to not do the reset
> on initial probe not when the module is loaded, you need to address this
> rather than reposting.
> 
The patch in question is moving the reset in probe rather than module init - with comment updated.
What is wrong ?

> I'll try to find time to re-review the series but I'm going to need to
> sit down with the datasheet and check this in much more detail.
For 1/4 and 2/4 there should not be great need, Really.

-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Tue, May 12, 2009 at 11:59:03AM +0200, Karl Beldan wrote:
> Mark Brown wrote:

> > That doesn't seem to tie up - I can see the initialisation changing the
> > behaviour on first run but it seems surprising that this should happen
> > on subsequent runs too.  Alternatively, is your initialisation patch
> > safe to apply by itself?

> Well 2/4 stops the clocks only if both REC and RPL are disabled.
> Without 1/4 you end up with REC enabled at startup.
> In a scenario where you have never used REC you end up RPLing with REC always on.
> REC being on at shutdown(),clocks won't stop.

Yet they are being stopped by something...

> > As previously discussed you need to rework the patch to not do the reset
> > on initial probe not when the module is loaded, you need to address this
> > rather than reposting.

> The patch in question is moving the reset in probe rather than module init - with comment updated.
> What is wrong ?

A repost is where you send exactly the same thing again.  When you say
you're reposting something it means you've not made any changes; if you
say that's what you're doing and your code has problems that need to be
fixed it's fairly obvious that all the previous comments are going to
continue to apply.

> > I'll try to find time to re-review the series but I'm going to need to
> > sit down with the datasheet and check this in much more detail.

> For 1/4 and 2/4 there should not be great need, Really.

There's been enough stuff with the series that I've got a few alarm
bells ringing, if only with obscure relationships between the patches.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

Mark Brown wrote:
> On Tue, May 12, 2009 at 11:59:03AM +0200, Karl Beldan wrote:
>> Mark Brown wrote:
> 
>>> That doesn't seem to tie up - I can see the initialisation changing the
>>> behaviour on first run but it seems surprising that this should happen
>>> on subsequent runs too.  Alternatively, is your initialisation patch
>>> safe to apply by itself?
> 
>> Well 2/4 stops the clocks only if both REC and RPL are disabled.
>> Without 1/4 you end up with REC enabled at startup.
>> In a scenario where you have never used REC you end up RPLing with REC always on.
>> REC being on at shutdown(),clocks won't stop.
> 
> Yet they are being stopped by something...
> 
You said that clocks are NOT stopped when applying patch 2/4 without 1/4.
I detailed a fairly likely code path.


>>> As previously discussed you need to rework the patch to not do the reset
>>> on initial probe not when the module is loaded, you need to address this
>>> rather than reposting.
> 
>> The patch in question is moving the reset in probe rather than module init - with comment updated.
>> What is wrong ?
> 
> A repost is where you send exactly the same thing again.  When you say
> you're reposting something it means you've not made any changes; if you
> say that's what you're doing and your code has problems that need to be
> fixed it's fairly obvious that all the previous comments are going to
> continue to apply.
> 
When I said I 'resent' it, my meaning was not to let you understand that I did not modify it obviously.

>>> I'll try to find time to re-review the series but I'm going to need to
>>> sit down with the datasheet and check this in much more detail.
> 
>> For 1/4 and 2/4 there should not be great need, Really.
> 
> There's been enough stuff with the series that I've got a few alarm
> bells ringing, if only with obscure relationships between the patches.
I could say the same about the current status and handling but I will ask you to point precise points instead, please.
As of the current status of the driver there is not even full duplex, maintainance is likewise.
The patches I am sending are quite easy to discuss.

Could you point what is wrong in the code or comments ?
Nitpicking about "A repost is where you send exactly the same thing again." and talking so much about
pb applying 2/4 without 1/4 really sucks.

I said and am saying my patches improve greatly the code quality/functionnality, if you think otherwise, give tangible reasons and I'll be really happy to discuss/rework/abandon the patches.

Since I have no feedback from Maintainers, I am pinging the original author hoping I won't disturb too much.


-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Tue, May 12, 2009 at 02:38:19PM +0200, Karl Beldan wrote:
> Mark Brown wrote:

> > Yet they are being stopped by something...

> You said that clocks are NOT stopped when applying patch 2/4 without 1/4.
> I detailed a fairly likely code path.

They're stopped after every other playback; I can't remember at exactly
what point things sorted themselves out but IIRC it was starting the
second playback that stopped them.  I didn't look too closely since the
behaviour was so obviously incorrect.

> > A repost is where you send exactly the same thing again.  When you say

> When I said I 'resent' it, my meaning was not to let you understand that I did not modify it obviously.

Unfortunately that was the effect; it's actually less uncommon than you
might think for people to do just that.

> > There's been enough stuff with the series that I've got a few alarm
> > bells ringing, if only with obscure relationships between the patches.

> I could say the same about the current status and handling but I will
> ask you to point precise points instead, please.

It really is a case of alarm bells ringing - it's not one specific thing
but rather a cluster of things that make me want to review in more
detail, including the fact that I actually have the hardware to hand and
use it fairly often.

> As of the current status of the driver there is not even full duplex,
> maintainance is likewise.

I've used the driver for full duplex in the past; now you've pointed out
these issues I suspect it only ever did so due to some race condition or
other but it has been functional.

> Could you point what is wrong in the code or comments ?
> Nitpicking about "A repost is where you send exactly the same thing again." and talking so much about

It's probably as well to just wait for me to review the current set of
patches - I suspect the changes are good, I just need to check them in
more detail to make sure that I understand what they do.

The main thing you could do in future is write changelogs that more
fully explain what your patches are doing - make sure all the bases are
covered and the motivation is explained.  For example, the description
of patch 3 talked only about stream startup so the changes to suspend
and resume were surprising.  Similarly, with patch 1 the fact that it
isn't just cleaning up after other software and restoring the hardware
to a known good state would've been useful to point out (remember that
the reset values aren't visible in the code).

> pb applying 2/4 without 1/4 really sucks.

At the time I tested with the second patch you hadn't provided a fixed
version of patch 1.  I generally try to apply as much as possible since
this cuts down on the amount of effort required when the needed fixes
are done.

> Since I have no feedback from Maintainers, I am pinging the original
> author hoping I won't disturb too much.

The maintainer here is in effect a combination of me and Eric Maio, me
as a result of it being part of ASoC so falling to me by default and
Eric due to it being PXA hardware.  I'd certainly not expect anyone else
to review them as a matter of course.

BTW, please fix your MUA to wrap at 80 characters or so - it makes
things easier to read.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Fri, May 08, 2009 at 01:53:47AM +0200, Karl Beldan wrote:

> - hw_params enables both RPL and REC functions each time,
> enable the appropriate function in pxa2xx_i2s_trigger.
> - pxa2xx_i2s_shutdown disables i2s anytime one of RPL or REC
> function is off, turn it off only when both functions are off.
> - do not put the clk_i2s for no reason in pxa2xx_i2s_shutdown.

> Signed-off-by: Karl Beldan <karl.beldan@mobile-devices.fr>

I'm still seeing the behaviour I was before now I've applied patch 1.
To reproduce I'm looking at the clocks with a scope.  After startup I'm
starting a playback (aplay and mplayer have the same effect).  Once the
playback is finished (either normally or by killing the application) the
clocks are still present.  Restarting playback causes the clock to stop
immediately and the generation of a DMA error.  Subsequent iterations
repeat the same behaviour, as does recording.

>  	case SNDRV_PCM_TRIGGER_RESUME:
> @@ -252,13 +254,11 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
>  		SAIMR &= ~SAIMR_RFS;
>  	}
>  
> -	if (SACR1 & (SACR1_DREC | SACR1_DRPL)) {
> +	if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
>  		SACR0 &= ~SACR0_ENB;
>  		pxa_i2s_wait();
>  		clk_disable(clk_i2s);
>  	}

The problem happens because this code no longer triggers - since the
port is still being reset on startup if the DAI is inactive the first
patch will have no effect on DREC and DRPL, they'll be reset to their
power on default of enabled when startup() is first called.  Applying
your third patch which removes the port reset avoids that issue.

Unfortunately there's still the outstanding issue with the third patch
removing the FIFO flushes - looking at the datasheet I think we do need
to clear the FIFOs, especially in the case where the PXA is running as
slave and might've had clocks removed.  Section 14.4.7.2 explicitly says
there are situations where the FIFO might not be drained and I'd really
not be surprised if there were others.

I'm out of time to look at this today - I expect that the fix is
probably to move some version of the port reset change into this patch.

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

Mark Brown wrote:
> On Fri, May 08, 2009 at 01:53:47AM +0200, Karl Beldan wrote:
> 
>> - hw_params enables both RPL and REC functions each time,
>> enable the appropriate function in pxa2xx_i2s_trigger.
>> - pxa2xx_i2s_shutdown disables i2s anytime one of RPL or REC
>> function is off, turn it off only when both functions are off.
>> - do not put the clk_i2s for no reason in pxa2xx_i2s_shutdown.
> 
>> Signed-off-by: Karl Beldan <karl.beldan@mobile-devices.fr>
> 
> I'm still seeing the behaviour I was before now I've applied patch 1.
> To reproduce I'm looking at the clocks with a scope.  After startup I'm
> starting a playback (aplay and mplayer have the same effect).  Once the
> playback is finished (either normally or by killing the application) the
> clocks are still present.  Restarting playback causes the clock to stop
> immediately and the generation of a DMA error.  Subsequent iterations
> repeat the same behaviour, as does recording.
> 
>>  	case SNDRV_PCM_TRIGGER_RESUME:
>> @@ -252,13 +254,11 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
>>  		SAIMR &= ~SAIMR_RFS;
>>  	}
>>  
>> -	if (SACR1 & (SACR1_DREC | SACR1_DRPL)) {
>> +	if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
>>  		SACR0 &= ~SACR0_ENB;
>>  		pxa_i2s_wait();
>>  		clk_disable(clk_i2s);
>>  	}
> 
> The problem happens because this code no longer triggers - since the
> port is still being reset on startup if the DAI is inactive the first
> patch will have no effect on DREC and DRPL, they'll be reset to their
> power on default of enabled when startup() is first called.  Applying
> your third patch which removes the port reset avoids that issue.
> 
Indeed, I should have put the port reset in 2/4 or in 1/4 rather than in
3/4 !

> Unfortunately there's still the outstanding issue with the third patch
> removing the FIFO flushes - looking at the datasheet I think we do need
> to clear the FIFOs, especially in the case where the PXA is running as
> slave and might've had clocks removed.  Section 14.4.7.2 explicitly says
> there are situations where the FIFO might not be drained and I'd really
> not be surprised if there were others.
> 
> I'm out of time to look at this today - I expect that the fix is
> probably to move some version of the port reset change into this patch.
I'd go for moving port reset into 2/4 (or 1/4 ?), 3/4 would just remove
clk_disable(clk_i2s) and leave the FIFO flushes as is, and keeping 4/4.
What do you say ?

-- 
Karl

Re: [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Mark Brown]

On Tue, May 12, 2009 at 07:43:04PM +0200, Karl Beldan wrote:
> Mark Brown wrote:

> > I'm out of time to look at this today - I expect that the fix is
> > probably to move some version of the port reset change into this patch.

> I'd go for moving port reset into 2/4 (or 1/4 ?), 3/4 would just remove
> clk_disable(clk_i2s) and leave the FIFO flushes as is, and keeping 4/4.
> What do you say ?

That sounds reasonable - I've already applied your first patch to reset
everything on startup so no need to resend that (I forgot to push before
leaving work).  Might be as well to just merge the removal of the reset
into the patch to handle playback and capture separately but either way
is OK.

The suspend/resume change is fine.

[PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately

[Karl Beldan]

- hw_params enables both RPL and REC functions each time : Enable the
	appropriate function in pxa2xx_i2s_trigger.
- pxa2xx_i2s_shutdown disables i2s anytime one of RPL or REC function is
	off : Turn it off only when both functions are off.

Signed-off-by: Karl Beldan <karl.beldan@mobile-devices.fr>
---
 sound/soc/pxa/pxa2xx-i2s.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
index 59cc4f5..a461c8d 100644
--- a/sound/soc/pxa/pxa2xx-i2s.c
+++ b/sound/soc/pxa/pxa2xx-i2s.c
@@ -176,9 +176,7 @@ 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;

@@ -224,6 +222,10 @@ static int pxa2xx_i2s_trigger(struct snd_pcm_substream
*substream, int cmd,

 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			SACR1 &= ~SACR1_DRPL;
+		else
+			SACR1 &= ~SACR1_DREC;
 		SACR0 |= SACR0_ENB;
 		break;
 	case SNDRV_PCM_TRIGGER_RESUME:
@@ -250,7 +252,7 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream
*substream,
 		SAIMR &= ~SAIMR_RFS;
 	}

-	if (SACR1 & (SACR1_DREC | SACR1_DRPL)) {
+	if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
 		SACR0 &= ~SACR0_ENB;
 		pxa_i2s_wait();
 		clk_disable(clk_i2s);
-- 
1.6.3.rc1.34.g0be9b

-- 
Karl