[alsa-devel] [PATCH 2/4] pxa2xx-i2s: Handle SACR1_DRPL and SACR1_DREC separately
- 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
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 ?
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.
participants (3)
-
Karl Beldan -
Mark Brown -
Mark Brown