[PATCH] ASoC: pandora: switch clock back to internal on stop

[PATCH] ASoC: pandora: switch clock back to internal on stop

[Grazvydas Ignotas]

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 */
-- 
1.7.0.4

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 435 bytes --]

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?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Grazvydas Ignotas]

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.

-- 
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Peter Ujfalusi]

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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Grazvydas Ignotas]

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=commitdiff;h=7717278f6e

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=commitdiff;h=d3ef3adfa

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.


-- 
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Peter Ujfalusi]

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=commitdiff;h=7717278f6e

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=commitdiff;h=d3ef3adfa

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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Grazvydas Ignotas]

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=commitdiff;h=7717278f6e
>
> 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=commitdiff;h=d3ef3adfa
>
> 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.

-- 
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: pandora: switch clock back to internal on stop

[Jarkko Nikula]

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.

-- 
jarkko