[alsa-devel] [PATCH] ASoC: pandora: switch clock back to internal on stop
For some reason, OMAP doesn't enter lower power states with functional clock (CLKS) source set to external, so switch it back to internal when done playing.
Signed-off-by: Grazvydas Ignotas notasas@gmail.com --- sound/soc/omap/omap3pandora.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/sound/soc/omap/omap3pandora.c b/sound/soc/omap/omap3pandora.c index 07794bd..1be26df 100644 --- a/sound/soc/omap/omap3pandora.c +++ b/sound/soc/omap/omap3pandora.c @@ -77,6 +77,27 @@ static int omap3pandora_hw_params(struct snd_pcm_substream *substream, return 0; }
+static int omap3pandora_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + int ret; + + /* + * For some reason, if we don't set clock back to internal, OMAP power + * saving features don't work properly (per power domain doesn't enter + * lower power states). Switch it back until better solution is found. + */ + ret = snd_soc_dai_set_sysclk(cpu_dai, OMAP_MCBSP_SYSCLK_CLKS_FCLK, + 0, SND_SOC_CLOCK_IN); + if (ret < 0) { + pr_err(PREFIX "can't set cpu system clock\n"); + return ret; + } + + return 0; +} + static int omap3pandora_dac_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *k, int event) { @@ -201,6 +222,7 @@ static int omap3pandora_in_init(struct snd_soc_pcm_runtime *rtd)
static struct snd_soc_ops omap3pandora_ops = { .hw_params = omap3pandora_hw_params, + .hw_free = omap3pandora_hw_free, };
/* Digital audio interface glue - connects codec <--> CPU */
On Sun, Feb 19, 2012 at 12:39:55AM +0200, Grazvydas Ignotas wrote:
For some reason, OMAP doesn't enter lower power states with functional clock (CLKS) source set to external, so switch it back to internal when done playing.
Signed-off-by: Grazvydas Ignotas notasas@gmail.com
Might be an idea to do this in the McBSP driver - have it do the switch transparently, then flip back when the port is brought up again?
On Sun, Feb 19, 2012 at 8:30 AM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Sun, Feb 19, 2012 at 12:39:55AM +0200, Grazvydas Ignotas wrote:
For some reason, OMAP doesn't enter lower power states with functional clock (CLKS) source set to external, so switch it back to internal when done playing.
Signed-off-by: Grazvydas Ignotas notasas@gmail.com
Might be an idea to do this in the McBSP driver - have it do the switch transparently, then flip back when the port is brought up again?
Maybe, but I think pandora is the only one using external clock in mainline and mcbsp currently doesn't track this state, just does a register write. If you still prefer it on mcbsp side, I can do it after Peter's mcbsp merge work is finished I guess.
On 02/19/2012 05:52 PM, Grazvydas Ignotas wrote:
Might be an idea to do this in the McBSP driver - have it do the switch transparently, then flip back when the port is brought up again?
Maybe, but I think pandora is the only one using external clock in mainline
I have patch on top of the mcbsp merge series which allows users (developers) to switch between McBSP2 master/slave configuration on Beagle. It will have two PCM: 0 is the current configuration (twl4030 master, mcbsp2 slave) 1 is the same as with pandora (twl4030 slave, mcbsp2 master - CLKS pin is the source for the SRG). With this I can help to track down the suspend issue you see on Pandora. I hope.
and mcbsp currently doesn't track this state, just does a register write.
I was also thinking that this should be handled by the mcbsp driver. I'm really not sure why we need to do this - it might be clock/hwmod issue at the end. We could do this unconditionally when all streams has been stopped on the mcbsp port IMHO.
If you still prefer it on mcbsp side, I can do it after Peter's mcbsp merge work is finished I guess.
I'll wait for Janusz for OMAP1 results before I send the v2 series. So far so good: now the Pandora like configuration also works.
On Mon, Feb 20, 2012 at 11:52 AM, Peter Ujfalusi peter.ujfalusi@ti.com wrote:
On 02/19/2012 05:52 PM, Grazvydas Ignotas wrote:
Might be an idea to do this in the McBSP driver - have it do the switch transparently, then flip back when the port is brought up again?
Maybe, but I think pandora is the only one using external clock in mainline
I have patch on top of the mcbsp merge series which allows users (developers) to switch between McBSP2 master/slave configuration on Beagle. It will have two PCM: 0 is the current configuration (twl4030 master, mcbsp2 slave) 1 is the same as with pandora (twl4030 slave, mcbsp2 master - CLKS pin is the source for the SRG). With this I can help to track down the suspend issue you see on Pandora. I hope.
Sounds good, I can wait for this I guess.
Slightly off-topic: would you mind getting OSS emulation working on OMAP? We talked about it some years back.. I'm still carrying this hack in pandora tree: http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitd...
We have found in-kernel OSS emulation to have best compatibility and performance, and the later is important for a games machine with an aging SoC..
The other issue with pandora on mainline is frequent buffer underflows just after the stream starts. Games tend to use tiny buffers with a goal to have low audio latency, and this often ends up with endless loop of stream start and underflow events. We used this hack on earlier kernels (the comment is probably not entirely correct, and fifo_samples should be multiplied by channels I guess): http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitd...
This can of course be fixed in a program itself, but the idea was to reduce effort needed to port things to pandora, and now I'll have to be carrying this for compatibility, but I wonder how that looks from mainline point of view.
On 02/20/2012 12:50 PM, Grazvydas Ignotas wrote:
Sounds good, I can wait for this I guess.
I just wonder why not just switch to twl4030 master scenario for pandora? I know that there are products out there using this scenario (I mean real consumer devices), and AFAIK they work pretty well. What is the main reason to keep pandora in McBSP master mode? What is the benefit for the platform? Can you send the per domain to suspend during audio playback?
Slightly off-topic: would you mind getting OSS emulation working on OMAP? We talked about it some years back.. I'm still carrying this hack in pandora tree: http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitd...
Hrm, yes I remember. Was it so that the OSS emulation calls hw_params several times, each with different parameter to configure, or something?
If you do not have duplex operation this might be OK to do, but there are cases when we return -EINVAL if a parameter is not correct... To be honest I have not used OSS emulation for quite a long time, and systems tend to use PulseAudio or AudioFlinger nowdays. If it is really important for you we can take a look, it might bring enhancements for other users as well at the end.
We also have the same mechanism in place in omap-mcbsp.c (to not allow reconfiguration of the mcbsp port) so 'fixing' twl4030 might not be enough for you.
We have found in-kernel OSS emulation to have best compatibility and performance, and the later is important for a games machine with an aging SoC..
Is it still the case?
The other issue with pandora on mainline is frequent buffer underflows just after the stream starts. Games tend to use tiny buffers with a goal to have low audio latency, and this often ends up with endless loop of stream start and underflow events. We used this hack on earlier kernels (the comment is probably not entirely correct, and fifo_samples should be multiplied by channels I guess): http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitd...
So we want to make sure that application written at least FIFO amount of data into the buffer before we start the McBSP/DMA, right? Not sure if it is a valid thing to just override the start_threshold. But if we do something like this it might be better to have support in the core (ASoC, or even in ALSA)..
This can of course be fixed in a program itself, but the idea was to reduce effort needed to port things to pandora, and now I'll have to be carrying this for compatibility, but I wonder how that looks from mainline point of view.
We can try to figure out something, but I have a feeling that this would be useful for other platforms as well.
On Mon, Feb 20, 2012 at 3:26 PM, Peter Ujfalusi peter.ujfalusi@ti.com wrote:
On 02/20/2012 12:50 PM, Grazvydas Ignotas wrote:
Sounds good, I can wait for this I guess.
I just wonder why not just switch to twl4030 master scenario for pandora? I know that there are products out there using this scenario (I mean real consumer devices), and AFAIK they work pretty well. What is the main reason to keep pandora in McBSP master mode? What is the benefit for the platform?
Pandora uses 256*fs clock for it's external DAC, which was added for improved audio quality (mcbsp2 is connected there instead of twl). twl4030 can only provide 256*fs clock in slave mode. So twl4030 is not used for playback, but is still used for audio in.
Speaking of which, there is crazy amount of twl4030 audio controls available that are not relevant to pandora, since these things are not connected, confusing the users. I think there were plans to hide snd_soc_dapm_nc_pin() marked things, but that never materialized?
Can you send the per domain to suspend during audio playback?
I suppose not, pm_debug counters are not increasing while audio is playing. Is that supposed to be possible?
Slightly off-topic: would you mind getting OSS emulation working on OMAP? We talked about it some years back.. I'm still carrying this hack in pandora tree: http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitd...
Hrm, yes I remember. Was it so that the OSS emulation calls hw_params several times, each with different parameter to configure, or something?
Yes, OSS has separate ioctl's for setting rate, channels, etc, each of which translates separate hw_params call, and twl4030 only accepts first one, so most things end up unconfigured. What's worse the program isn't even told some parameters were not set because there is no error returned.
If you do not have duplex operation this might be OK to do, but there are cases when we return -EINVAL if a parameter is not correct... To be honest I have not used OSS emulation for quite a long time, and systems tend to use PulseAudio or AudioFlinger nowdays. If it is really important for you we can take a look, it might bring enhancements for other users as well at the end.
At least for now we are using kernel OSS emulation, so it would be nice to have it working. PulseAudio was problematic some time back, maybe it's good now, but it's hard to imagine adding another audio layer would not affect performance. Additional latency or CPU use is not good for a games console, even if it's small.
We also have the same mechanism in place in omap-mcbsp.c (to not allow reconfiguration of the mcbsp port) so 'fixing' twl4030 might not be enough for you.
True, it seems first hw_params matches all others in most cases so it was not an issue.
We have found in-kernel OSS emulation to have best compatibility and performance, and the later is important for a games machine with an aging SoC..
Is it still the case?
Well at least for performance I think it still is, and pandora is stuck with OMAP3s at least for a while more.
The other issue with pandora on mainline is frequent buffer underflows just after the stream starts. Games tend to use tiny buffers with a goal to have low audio latency, and this often ends up with endless loop of stream start and underflow events. We used this hack on earlier kernels (the comment is probably not entirely correct, and fifo_samples should be multiplied by channels I guess): http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitd...
So we want to make sure that application written at least FIFO amount of data into the buffer before we start the McBSP/DMA, right?
I think it should be at least 2x fifo as IIRC underflow condition is triggered when there is nothing left to DMA.
Some retro console emulators work like this: while (1) { emulate the 'guest' CPU for 1 video frame generate 1 video frame's worth of audio data and send to host hardware draw video frame, read controls, etc sleep if there is time left }
So in this case host audio hardware is always kept very close to underflow condition, but has very low audio latency. This doesn't work well with current mainline.
Not sure if it is a valid thing to just override the start_threshold. But if we do something like this it might be better to have support in the core (ASoC, or even in ALSA)..
Sure, that would work for us too.
This can of course be fixed in a program itself, but the idea was to reduce effort needed to port things to pandora, and now I'll have to be carrying this for compatibility, but I wonder how that looks from mainline point of view.
We can try to figure out something, but I have a feeling that this would be useful for other platforms as well.
Sounds promising.
On 02/20/2012 11:52 AM, Peter Ujfalusi wrote:
I have patch on top of the mcbsp merge series which allows users (developers) to switch between McBSP2 master/slave configuration on Beagle. It will have two PCM: 0 is the current configuration (twl4030 master, mcbsp2 slave) 1 is the same as with pandora (twl4030 slave, mcbsp2 master - CLKS pin is the source for the SRG).
Side note: Don't forget the FCLK as a SRG source for McBSP master case. It's quite useful when bringing up and testing new hw.
participants (4)
-
Grazvydas Ignotas -
Jarkko Nikula -
Mark Brown -
Peter Ujfalusi