[PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Peter Ujfalusi]

Hello,

The following patch should fix (or at least makes it less intrusive) the 'tuck'
phenomena on observed on the twl4030 Headset outputs when the codec is going
to standby mode.

I have debugged this with the scope and if we are not waiting RAMP_DELAY time,
the VMID is cut before it reaching 0, which generates the 'tuck'.

Note that I have not removed the control for the RAMP_DELAY configuration, the
code will use that to calculate the needed delay (also the sysfreq is needed,
which also stored).
However I do think that one should not touch the RAMP_DELAY...

It would be nice to hear from you guys, if it solves/softens the 'tuck'.

---
Peter Ujfalusi (1):
  ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

 sound/soc/codecs/twl4030.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

[PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

This patch fixes the - agressive - 'tuck' sound heard when
the codec goes to standby mode.

We need to wait for the ramp down completion in case of
headsetl_event:POST_PMD.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/twl4030.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index fd392c6..c22f8d6 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -130,6 +130,8 @@ struct twl4030_priv {
 	unsigned int rate;
 	unsigned int sample_bits;
 	unsigned int channels;
+
+	unsigned int sysclk;
 };
 
 /*
@@ -586,6 +588,11 @@ static int headsetl_event(struct snd_soc_dapm_widget *w,
 		struct snd_kcontrol *kcontrol, int event)
 {
 	unsigned char hs_gain, hs_pop;
+	struct twl4030_priv *twl4030 = w->codec->private_data;
+	/* Base values for ramp delay calculation: 2^19 - 2^26 */
+	unsigned int ramp_base[] = {524288, 1048576, 2097152, 4194304,
+				    8388608, 16777216, 33554432, 67108864};
+	unsigned int ramp_delay;
 
 	/* Save the current volume */
 	hs_gain = twl4030_read_reg_cache(w->codec, TWL4030_REG_HS_GAIN_SET);
@@ -609,6 +616,12 @@ static int headsetl_event(struct snd_soc_dapm_widget *w,
 		/* Do the anti-pop/bias ramp disable according to the TRM */
 		hs_pop &= ~TWL4030_RAMP_EN;
 		twl4030_write(w->codec, TWL4030_REG_HS_POPN_SET, hs_pop);
+
+		/* We need to wait RAMP_DELAY time, otherwise 'tuck' sound can
+		 * be heard on the output */
+		ramp_delay = (hs_pop & TWL4030_RAMP_DELAY) >> 2;
+		mdelay(ramp_base[ramp_delay] / twl4030->sysclk);
+
 		/* Bypass the reg_cache to mute the headset */
 		twl4030_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
 					hs_gain & (~0x0f),
@@ -1567,17 +1580,21 @@ static int twl4030_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 		int clk_id, unsigned int freq, int dir)
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
+	struct twl4030_priv *twl4030 = codec->private_data;
 	u8 infreq;
 
 	switch (freq) {
 	case 19200000:
 		infreq = TWL4030_APLL_INFREQ_19200KHZ;
+		twl4030->sysclk = 19200;
 		break;
 	case 26000000:
 		infreq = TWL4030_APLL_INFREQ_26000KHZ;
+		twl4030->sysclk = 26000;
 		break;
 	case 38400000:
 		infreq = TWL4030_APLL_INFREQ_38400KHZ;
+		twl4030->sysclk = 38400;
 		break;
 	default:
 		printk(KERN_ERR "TWL4030 set sysclk: unknown rate %d\n",
@@ -1929,6 +1946,9 @@ static int twl4030_probe(struct platform_device *pdev)
 		kfree(codec);
 		return -ENOMEM;
 	}
+	/* Set default sysclk (used by the headsetl_event callback for
+	 * pop-attenuation) */
+	twl4030->sysclk = 26000;
 
 	codec->private_data = twl4030;
 	socdev->card->codec = codec;
-- 
1.6.2.3

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Mon, 11 May 2009 14:14:25 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> This patch fixes the - agressive - 'tuck' sound heard when
> the codec goes to standby mode.
> 
> We need to wait for the ramp down completion in case of
> headsetl_event:POST_PMD.
> 
For my ears this sounds that the power-down pop is now even worse. I'm
listening with some 32 ohm headphones connected into Beagle.

Before it was kind of sharp snap but now it is snap + low-frequency
tuck/pop. Positive that this low-frequency pop sounds kind of same than
the power-up pop so there is now ramp in both events when coupling
capasitor is charging/discharging.

Do you know is the capacitor coupling configuration correct? TPS65950
is mentioning it but I don't find it at quick look where to configure it
for ac-coupled connection.


-- 
Jarkko

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Arun KS]

On Mon, May 11, 2009 at 4:44 PM, Peter Ujfalusi
<peter.ujfalusi@nokia.com> wrote:
> Hello,
>
> The following patch should fix (or at least makes it less intrusive) the 'tuck'
> phenomena on observed on the twl4030 Headset outputs when the codec is going
> to standby mode.
>
> I have debugged this with the scope and if we are not waiting RAMP_DELAY time,
> the VMID is cut before it reaching 0, which generates the 'tuck'.
>
> Note that I have not removed the control for the RAMP_DELAY configuration, the
> code will use that to calculate the needed delay (also the sysfreq is needed,
> which also stored).
> However I do think that one should not touch the RAMP_DELAY...
>
> It would be nice to hear from you guys, if it solves/softens the 'tuck'.

'Tuck' still present on omap3evm board.

>
> ---
> Peter Ujfalusi (1):
>  ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD
>
>  sound/soc/codecs/twl4030.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
>

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Anuj Aggarwal]

Moreover, I am hearing a sine-tonish kind of sound, always in the beginning.
It lasts only for less than a sec but is very prominent and occurs always.

Has anyone noticed this too?

Regards,
Anuj

On Mon, May 11, 2009 at 6:27 PM, Arun KS <getarunks@gmail.com> wrote:

> On Mon, May 11, 2009 at 4:44 PM, Peter Ujfalusi
> <peter.ujfalusi@nokia.com> wrote:
> > Hello,
> >
> > The following patch should fix (or at least makes it less intrusive) the
> 'tuck'
> > phenomena on observed on the twl4030 Headset outputs when the codec is
> going
> > to standby mode.
> >
> > I have debugged this with the scope and if we are not waiting RAMP_DELAY
> time,
> > the VMID is cut before it reaching 0, which generates the 'tuck'.
> >
> > Note that I have not removed the control for the RAMP_DELAY
> configuration, the
> > code will use that to calculate the needed delay (also the sysfreq is
> needed,
> > which also stored).
> > However I do think that one should not touch the RAMP_DELAY...
> >
> > It would be nice to hear from you guys, if it solves/softens the 'tuck'.
>
> 'Tuck' still present on omap3evm board.
>
> >
> > ---
> > Peter Ujfalusi (1):
> >  ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD
> >
> >  sound/soc/codecs/twl4030.c |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> >
> >
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



-- 
Best Regards,
Anuj Aggarwal

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Jarkko Nikula]

On Tue, 12 May 2009 09:29:55 +0530
Anuj Aggarwal <anuj.aggarwal@gmail.com> wrote:

> Moreover, I am hearing a sine-tonish kind of sound, always in the
> beginning. It lasts only for less than a sec but is very prominent
> and occurs always.
> 
> Has anyone noticed this too?
> 
I haven't noticed (yet) but can you post your use case here how to
reproduce it. It would help to hunt is the problem OMAP & TWL4030
specific or is it somewhere else.


-- 
Jarkko

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Aggarwal, Anuj]

> -----Original Message-----
> From: Jarkko Nikula [mailto:jhnikula@gmail.com]
> Sent: Tuesday, May 12, 2009 11:03 AM
> To: Anuj Aggarwal
> Cc: Arun KS; Peter Ujfalusi; sakoman@gmail.com; Aggarwal, Anuj; alsa-
> devel@alsa-project.org; broonie@opensource.wolfsonmicro.com; Aarti Dewan
> (Corp HR), Noida
> Subject: Re: [alsa-devel] [PATCH 0/1] ASoC: TWL4030: Headset ramp down
> fix
> 
> On Tue, 12 May 2009 09:29:55 +0530
> Anuj Aggarwal <anuj.aggarwal@gmail.com> wrote:
> 
> > Moreover, I am hearing a sine-tonish kind of sound, always in the
> > beginning. It lasts only for less than a sec but is very prominent
> > and occurs always.
> >
> > Has anyone noticed this too?
> >
> I haven't noticed (yet) but can you post your use case here how to
> reproduce it. It would help to hunt is the problem OMAP & TWL4030
> specific or is it somewhere else.
[Aggarwal, Anuj] There is nothing specific to my use case. I am just 
trying to playback an audio file (16bit, stereo, 44.1khz) and hearing 
this sine-tonish sound everytime in the beginning.
This artifact was not there when I was using ALSA; it started coming
only when I moved to the newer ASoC framework.
> 
> 
> --
> Jarkko

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Jarkko Nikula]

On Tue, 12 May 2009 11:44:21 +0530
"Aggarwal, Anuj" <anuj.aggarwal@ti.com> wrote:

> > I haven't noticed (yet) but can you post your use case here how to
> > reproduce it. It would help to hunt is the problem OMAP & TWL4030
> > specific or is it somewhere else.
> [Aggarwal, Anuj] There is nothing specific to my use case. I am just 
> trying to playback an audio file (16bit, stereo, 44.1khz) and hearing 
> this sine-tonish sound everytime in the beginning.
> This artifact was not there when I was using ALSA; it started coming
> only when I moved to the newer ASoC framework.

What HW do you use for testing and what was the previous kernel having
ALSA drivers for it? Can you reproduce the problem with standard aplay
from alsa-utils?

For instance I'm curious to know can we have some unknown issue with the
1 ksample FIFO inside OMAP3 McBSP2 which is not handled right now with
the ASoC drivers but if the another kernel has workaround for it.


-- 
Jarkko

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Peter Ujfalusi]

On Monday 11 May 2009 15:57:28 ext Arun KS wrote:
>
> 'Tuck' still present on omap3evm board.

I'm really puzzled over this...

On my scope I can see 10s of the HSOL line.
I can see when the playback (/dev/urandom) is terminated, than after 5s the 
HSOL line is ramping down (from 1.4V to 12mV) after that the HSOL line stays 
low.
The signal looks really fine, no glitches on the way at all (I have not seen a 
single glitch in any of my measurements).
Looking at the scope picture, I can not see anything which can cause the 
'tuck' anymore. Before the patch the HSOL jumped from 1.4V to 12mV, while now 
it gradually goes down...

Arun: can you try to increase the 'HS ramp delay' to for example 
'1748/1291/874'? Does it makes any difference?

Are you hearing the 'tuck' on the right, left or in both headset speaker?
Have you configured the Headset left routing correctly? For some reason (which 
I can not recall why I did it) the headsetl_event is only triggered when the 
HSOL routing is correctly configured.

For reference, this is what happens here after the playback is stopped:

twl4030_set_bias_level: SND_SOC_BIAS_PREPARE active: 0
twl4030_codec_mute: MUTE
headsetl_event: PRE_PMD /* Not used, just printout */
headsetl_event: POST_PMD
   delay: 27
twl4030_set_bias_level: SND_SOC_BIAS_STANDBY active: 0


-- 
Péter

Re: [PATCH 0/1] ASoC: TWL4030: Headset ramp down fix

[Aggarwal, Anuj]

> -----Original Message-----
> From: Jarkko Nikula [mailto:jhnikula@gmail.com]
> Sent: Tuesday, May 12, 2009 12:10 PM
> To: Aggarwal, Anuj
> Cc: Arun KS; Peter Ujfalusi; sakoman@gmail.com; alsa-devel@alsa-
> project.org; broonie@opensource.wolfsonmicro.com; Anuj Aggarwal
> Subject: Re: [alsa-devel] [PATCH 0/1] ASoC: TWL4030: Headset ramp down
> fix
> 
> On Tue, 12 May 2009 11:44:21 +0530
> "Aggarwal, Anuj" <anuj.aggarwal@ti.com> wrote:
> 
> > > I haven't noticed (yet) but can you post your use case here how to
> > > reproduce it. It would help to hunt is the problem OMAP & TWL4030
> > > specific or is it somewhere else.
> > [Aggarwal, Anuj] There is nothing specific to my use case. I am just
> > trying to playback an audio file (16bit, stereo, 44.1khz) and hearing
> > this sine-tonish sound everytime in the beginning.
> > This artifact was not there when I was using ALSA; it started coming
> > only when I moved to the newer ASoC framework.
> 
> What HW do you use for testing and what was the previous kernel having
> ALSA drivers for it? Can you reproduce the problem with standard aplay
> from alsa-utils?
> 
> For instance I'm curious to know can we have some unknown issue with the
> 1 ksample FIFO inside OMAP3 McBSP2 which is not handled right now with
> the ASoC drivers but if the another kernel has workaround for it.
[Aggarwal, Anuj] I am currently working on OMAP3 EVM, on 2.6.29-rc3 kernel. The previous kernel version where I was using ALSA was 2.6.22. I am using aplay only and observing this artifact on ASoC alone.
> 
> 
> --
> Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Monday 11 May 2009 15:03:20 ext Jarkko Nikula wrote:
> On Mon, 11 May 2009 14:14:25 +0300
>
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > This patch fixes the - agressive - 'tuck' sound heard when
> > the codec goes to standby mode.
> >
> > We need to wait for the ramp down completion in case of
> > headsetl_event:POST_PMD.
>
> For my ears this sounds that the power-down pop is now even worse. I'm
> listening with some 32 ohm headphones connected into Beagle.
>
> Before it was kind of sharp snap but now it is snap + low-frequency
> tuck/pop. Positive that this low-frequency pop sounds kind of same than
> the power-up pop so there is now ramp in both events when coupling
> capasitor is charging/discharging.
>
> Do you know is the capacitor coupling configuration correct? TPS65950
> is mentioning it but I don't find it at quick look where to configure it
> for ac-coupled connection.

Jarkko and others: can you test this on top of the my patch?
How does the 'tuck' sounds after this?


diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 7847f80..f930fbc 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -61,7 +61,7 @@ static int dapm_up_seq[] = {

 static int dapm_down_seq[] = {
        snd_soc_dapm_pre, snd_soc_dapm_adc, snd_soc_dapm_hp, snd_soc_dapm_spk,
-       snd_soc_dapm_pga, snd_soc_dapm_mixer_named_ctl, snd_soc_dapm_mixer,
+       snd_soc_dapm_mixer_named_ctl, snd_soc_dapm_mixer, snd_soc_dapm_pga,
        snd_soc_dapm_dac, snd_soc_dapm_mic, snd_soc_dapm_micbias,
        snd_soc_dapm_mux, snd_soc_dapm_value_mux, snd_soc_dapm_supply,
        snd_soc_dapm_post

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Tue, 12 May 2009 13:50:26 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> Jarkko and others: can you test this on top of the my patch?
> How does the 'tuck' sounds after this?
> 
Do difference IMO. I just noted that I'm able to get both
power-up/-down pops from both channels by toggling the control
"HeadsetL Mixer AudioL2" or "HeadsetL Mixer Voice" but only if "HeadsetL
Mixer AudioL1" is muted. But no pop from HeaderR controls.


-- 
Jarkko

> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index 7847f80..f930fbc 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -61,7 +61,7 @@ static int dapm_up_seq[] = {
> 
>  static int dapm_down_seq[] = {
>         snd_soc_dapm_pre, snd_soc_dapm_adc, snd_soc_dapm_hp,
> snd_soc_dapm_spk,
> -       snd_soc_dapm_pga, snd_soc_dapm_mixer_named_ctl,
> snd_soc_dapm_mixer,
> +       snd_soc_dapm_mixer_named_ctl, snd_soc_dapm_mixer,
> snd_soc_dapm_pga, snd_soc_dapm_dac, snd_soc_dapm_mic,
> snd_soc_dapm_micbias, snd_soc_dapm_mux, snd_soc_dapm_value_mux,
> snd_soc_dapm_supply, snd_soc_dapm_post
> 
> -- 
> Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Tuesday 12 May 2009 14:19:37 ext Jarkko Nikula wrote:
> On Tue, 12 May 2009 13:50:26 +0300
>
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > Jarkko and others: can you test this on top of the my patch?
> > How does the 'tuck' sounds after this?
>
> Do difference IMO.

Is it better or worst?
Can you try also with:

amixer sset 'HS ramp delay' '55/40/27 ms'
amixer sset 'HS ramp delay' '109/81/55 ms'
amixer sset 'HS ramp delay' '27/20/14 ms'


> I just noted that I'm able to get both
> power-up/-down pops from both channels by toggling the control
> "HeadsetL Mixer AudioL2" or "HeadsetL Mixer Voice" but only if "HeadsetL
> Mixer AudioL1" is muted. But no pop from HeaderR controls.

HeadsetR does not triggers the ramp up/down mantra.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Tue, 12 May 2009 14:31:41 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> > Do difference IMO.
> 
> Is it better or worst?

A day between the tests and no noticeable difference to my ears/memory.

> Can you try also with:
> 
Tests below are tested with your hack patch on top of your yesterday's
patch.

> amixer sset 'HS ramp delay' '55/40/27 ms'

Probably a little bit lower 'tuck' than with '27/20/14 ms'.

> amixer sset 'HS ramp delay' '109/81/55 ms'

Again a bit lower than with '55/40/27 ms' but now I hear silent beep (!)
when starting the aplay and about 5 s after stopping it followed by
the 'tuck'. Length of beep was less than half of second.

Probably this this is the issue reported by Anuj?


-- 
Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Tue, 12 May 2009 14:52:29 +0300
Jarkko Nikula <jhnikula@gmail.com> wrote:

> > amixer sset 'HS ramp delay' '109/81/55 ms'
> 
> Again a bit lower than with '55/40/27 ms' but now I hear silent beep
> (!) when starting the aplay and about 5 s after stopping it followed
> by the 'tuck'. Length of beep was less than half of second.
> 
> Probably this this is the issue reported by Anuj?
> 
Looks like the beep is caused by the ramp operation since beep is gone
in power-down when reverting both of your patches but it's still present
in power-up.


-- 
Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Tuesday 12 May 2009 15:02:46 ext Jarkko Nikula wrote:
> On Tue, 12 May 2009 14:52:29 +0300
>
> Jarkko Nikula <jhnikula@gmail.com> wrote:
> > > amixer sset 'HS ramp delay' '109/81/55 ms'
> >
> > Again a bit lower than with '55/40/27 ms' but now I hear silent beep
> > (!) when starting the aplay and about 5 s after stopping it followed
> > by the 'tuck'. Length of beep was less than half of second.
> >
> > Probably this this is the issue reported by Anuj?
>
> Looks like the beep is caused by the ramp operation since beep is gone
> in power-down when reverting both of your patches but it's still present
> in power-up.

But the 'tuck' still there after you have reverted the patches?

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Tue, 12 May 2009 15:16:40 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> > Looks like the beep is caused by the ramp operation since beep is
> > gone in power-down when reverting both of your patches but it's
> > still present in power-up.
> 
> But the 'tuck' still there after you have reverted the patches?
> 
Yep it was there even it sounds bit more silent when going to longer
ramp times. Beep was present only with '109/81/55 ms'.


-- 
Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Tuesday 12 May 2009 15:21:55 ext Jarkko Nikula wrote:
> On Tue, 12 May 2009 15:16:40 +0300
>
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > > Looks like the beep is caused by the ramp operation since beep is
> > > gone in power-down when reverting both of your patches but it's
> > > still present in power-up.
> >
> > But the 'tuck' still there after you have reverted the patches?
>
> Yep it was there even it sounds bit more silent when going to longer
> ramp times. Beep was present only with '109/81/55 ms'.

Now I'm even more confused...
Without the patches that you have reverted, the ramp delay does not matter 
after the playback. It just cuts the VMID and that's it.
On startup the ramp is initiated. If you set the ramp delay longer, than it is 
possible that the beginning of the audio is played already when the VMID is 
ramping up.

So with the ramp (down) delay - the patches that you have reverted - the 
'tuck' was worst after playback, than without the proper ramp (down) delay 
handling?

I can see on the scope, that the ramp up also has kind of 'tuck' problem, but 
it comes when the VMID is quite low, so it is harder to hear it. By making the 
ramp delay longer, this 'tuck' happens when the VMID is close to 0, so it 
makes it less audible.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Tue, 12 May 2009 16:16:08 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> On Tuesday 12 May 2009 15:21:55 ext Jarkko Nikula wrote:
> > On Tue, 12 May 2009 15:16:40 +0300
> >
> > Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > > > Looks like the beep is caused by the ramp operation since beep
> > > > is gone in power-down when reverting both of your patches but
> > > > it's still present in power-up.
> > >
> > > But the 'tuck' still there after you have reverted the patches?
> >
> > Yep it was there even it sounds bit more silent when going to longer
> > ramp times. Beep was present only with '109/81/55 ms'.
> 
> Now I'm even more confused...
> Without the patches that you have reverted, the ramp delay does not
> matter after the playback. It just cuts the VMID and that's it.
> On startup the ramp is initiated. If you set the ramp delay longer,
> than it is possible that the beginning of the audio is played already
> when the VMID is ramping up.
> 
> So with the ramp (down) delay - the patches that you have reverted -
> the 'tuck' was worst after playback, than without the proper ramp
> (down) delay handling?
> 
Hopefully I didn't confuse you too much :-)

So with your patches reverted, the low-frequency pop was gone in
power-down since there were no ramp implemented but there still were
the sharp snap. I didn't notice were there any difference on that.

Then this low-frequency pop went more silent as the ramp time
increased. Allways in power-up phase and also in power-down when your
patches were in implementing the ramp down.

Third observation was that the beep was present with ramp time of
'109/81/55 ms'. Without your patches it was present only in power-up
and with your patches is was also in power-down phase. So it sounds to
me that beep was there when the ramp up or down was going.

> I can see on the scope, that the ramp up also has kind of 'tuck'
> problem, but it comes when the VMID is quite low, so it is harder to
> hear it. By making the ramp delay longer, this 'tuck' happens when
> the VMID is close to 0, so it makes it less audible.
> 
I think for me this low-frequency power-up and -down (when your
patches are in) pops are caused when the capasitors connected into
HSOL/HSOR are charging/discharging through the headphones.

What I'm wondering how it is possible to hear such a slow ramp like 100
ms?


-- 
Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Tuesday 12 May 2009 16:56:25 ext Jarkko Nikula wrote:
> On Tue, 12 May 2009 16:16:08 +0300
>
> Hopefully I didn't confuse you too much :-)

You did :D

But now I can hear what you are hearing... I have soldered to my custom board 
a connector so I can connect a headset to HeadsetL of TWL codec.
On the scope it looked OK, but it does makes a sound...


> So with your patches reverted, the low-frequency pop was gone in
> power-down since there were no ramp implemented but there still were
> the sharp snap. I didn't notice were there any difference on that.

For me the snap is really sharp and sound bad.

> I think for me this low-frequency power-up and -down (when your
> patches are in) pops are caused when the capasitors connected into
> HSOL/HSOR are charging/discharging through the headphones.

I have also observed - if the ramp delay is longish - the 'beep' type of noise 
on the headset (can not be seen on the scope).
It is generated by the ramp up/down, you can verify it:
aplay -f dat -q /dev/urandom
# Now adjust the volumes (analog, digital) to a level that you can still hear 
# the noise, but as low as you could.
mkdir /debug
mount -t debugfs none /debug
amixer sset 'HS ramp delay' '3495/2581/1748 ms'
cat /debug/soc-audio/codec_reg | grep 24:
# it is 5e for the maximum ramp time.
# now you can initiate the ramp-down with flipping bit 1:
echo 24 5c > /debug/soc-audio/codec_reg
# you will still hear the noise, but underneath you will hear the beep tone.
# Now set the ramp-up:
echo 24 5e > /debug/soc-audio/codec_reg
# you will hear a click, than the beep tone and the noise from the playback

>
> What I'm wondering how it is possible to hear such a slow ramp like 100
> ms?

So now that I can hear the thing, here is what I got at the moment:
- really small and low volume click on playback start
- going to standby mode is almost unnoticeable (I usually miss it, if I did 
not look at the console printouts)
- Enabling/disabling the HeadsetL (flipping the DACL2 route): disable is 
unnoticeable, enable has a click, but the audio can be muted with Analog mute 
with really small click.

I have to check what exactly has been changed, but what I can recall:
- I have changed the power_up and power_down sequences a bit in soc-dapm.c
- I have the ramp delay patch with the mdelay (I have to check if it is the 
same as I have sent)
- Restructured the DAPM routing of the twl4030 codec (this was in my plans 
anyway)
- I have changed the way how the bias events are handled in the 
twl4030_set_bias_level function.
- While I was there I have fixed the Handsfree sequences, since that also 
generated quite a bit of click.

So quite a few changes...

What I have figured out that the user has to select the appropriate ramp_delay 
for the given board, on my board:
MCLK = 38.4MHz and RAMP_DELAY = 2 (109/81/55 ms) works best (meaning 55ms ramp 
delay), if it is shorter, than it clicks, if it is longer, than it does not 
click, but the beep tone can be observed.

On the way I have noticed some other things, which I will ask tomorrow.

Mark: About the  power up and down sequences in the soc-dapm.c file: are they 
written in stone, or is it possible to make some - I think - minor 
modification to the sequences?

I'll try to clean up the code and structure it for review in a couple of days 
(I hope).

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Mark Brown]

On Wed, May 13, 2009 at 03:57:09PM +0300, Peter Ujfalusi wrote:

> Mark: About the  power up and down sequences in the soc-dapm.c file: are they 
> written in stone, or is it possible to make some - I think - minor 
> modification to the sequences?

The change you've made is to power the amplifiers down after the mixers.
That's going to cause problems on other systems - the result is going to
be that the amplifiers end up amplifying any noise the mixers make on
the way down which isn't good.  There's bits of the sequence that can be
fairly readily reordered but I'd worry about that particular change.

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Wednesday 13 May 2009 16:05:49 ext Mark Brown wrote:
> On Wed, May 13, 2009 at 03:57:09PM +0300, Peter Ujfalusi wrote:
> > Mark: About the  power up and down sequences in the soc-dapm.c file: are
> > they written in stone, or is it possible to make some - I think - minor
> > modification to the sequences?
>
> The change you've made is to power the amplifiers down after the mixers.
> That's going to cause problems on other systems - the result is going to
> be that the amplifiers end up amplifying any noise the mixers make on
> the way down which isn't good.  There's bits of the sequence that can be
> fairly readily reordered but I'd worry about that particular change.

That is why I'm a bit nervous when I need to touch the core... It has been 
working fine for all other codes so far. 
In the twl4030 codec case, the playback involves the following chain in DAPM 
domain:

Headset, PreDrive, Carkit, Earpiece:
DAC -> PGA -> Mixer -> output pin.

Handsfree:
DAC -> PGA -> Mux -> output pin.


While for example the WM9713 has it in this way:
DAC -> Mixer -> PGA -> output pin


While I was looking at the sequences, I wonder why the mux and mixer handled 
in a differently?

I mean, that in power up the order is (simplified):
mux -> dac -> mixer -> pga

in power down:
pga -> mixer -> dac -> mux


Is it possible to add several SND_SOC_DAPM_PRE to the DAPM routing? For 
example to the HeadsetL, HeadsetR, HandsFreeL and HandsFreeR? How does it than 
would work?
If this is possible, than I can handle the correct power up/down sequences 
required for the twl4030 codec.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Thu, 14 May 2009 08:27:11 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> That is why I'm a bit nervous when I need to touch the core... It has
> been working fine for all other codes so far. 
> In the twl4030 codec case, the playback involves the following chain
> in DAPM domain:
> 
> Headset, PreDrive, Carkit, Earpiece:
> DAC -> PGA -> Mixer -> output pin.
> 
> Handsfree:
> DAC -> PGA -> Mux -> output pin.
> 
...
> Is it possible to add several SND_SOC_DAPM_PRE to the DAPM routing?
> For example to the HeadsetL, HeadsetR, HandsFreeL and HandsFreeR? How
> does it than would work?
> If this is possible, than I can handle the correct power up/down
> sequences required for the twl4030 codec.
> 
My two cents: Would it be possible to implement 'fake' PGA or amplifier
into TWL outputs which only controls the power state of mixer and mux?


-- 
Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Thursday 14 May 2009 09:08:08 ext Jarkko Nikula wrote:
> On Thu, 14 May 2009 08:27:11 +0300

> My two cents: Would it be possible to implement 'fake' PGA or amplifier
> into TWL outputs which only controls the power state of mixer and mux?

That would be also possible. I'll try it out and see if the order is correct 
with 'fake PGA'.
BTW: we have quite a bit of 'fake' DAPM things in the twl4030 codec, that I 
started to wonder is the codec real at all??? ;)

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Thursday 14 May 2009 09:08:08 ext Jarkko Nikula wrote:
> My two cents: Would it be possible to implement 'fake' PGA or amplifier
> into TWL outputs which only controls the power state of mixer and mux?

With the 'fake PGA' on the headsetL:
Startup has a click, going standby is quite smooth.
The sequences with the twl4030 codec (soc-damp.c routings are reverted to be 
the original) are for
Startup:
dac -> HS ramp up (HS enable, fake PGA) -> PGA

Standby:
HS ramp down (HS disable, fake PGA) -> PGA -> dac

If the order of the startup can be somehow altered to be:
dac -> PGA -> HS ramp up (HS enable, fake PGA)

Than it would be good.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Jarkko Nikula]

On Thu, 14 May 2009 11:09:08 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> Startup:
> dac -> HS ramp up (HS enable, fake PGA) -> PGA
> 
> Standby:
> HS ramp down (HS disable, fake PGA) -> PGA -> dac
> 
> If the order of the startup can be somehow altered to be:
> dac -> PGA -> HS ramp up (HS enable, fake PGA)
> 
Did you implement this fake or actually real PGA between "HeadsetL
Mixer" and "HSOL"?

I seems that there is an error in audio path that output pin control is
tied to mixer and there is no widget in audio path defining the output
amplifier. See

SND_SOC_DAPM_MIXER_E("HeadsetL Mixer", SND_SOC_NOPM, 0, 0,
	&twl4030_dapm_hsol_controls[0],
	ARRAY_SIZE(twl4030_dapm_hsol_controls), headsetl_event,
	SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD),
...
	{"HSOL", NULL, "HeadsetL Mixer"},
	{"HSOR", NULL, "HeadsetR Mixer"},

So startup/shutdown order should be correct if you define a new PGA
between the HSOx and Headsetx mixer and let that to manage output pin
power.


-- 
Jarkko

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Thursday 14 May 2009 12:05:50 ext Jarkko Nikula wrote:
> Did you implement this fake or actually real PGA between "HeadsetL
> Mixer" and "HSOL"?
>
> I seems that there is an error in audio path that output pin control is
> tied to mixer and there is no widget in audio path defining the output
> amplifier. See
>
> SND_SOC_DAPM_MIXER_E("HeadsetL Mixer", SND_SOC_NOPM, 0, 0,
> 	&twl4030_dapm_hsol_controls[0],
> 	ARRAY_SIZE(twl4030_dapm_hsol_controls), headsetl_event,
> 	SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD),
> ...
> 	{"HSOL", NULL, "HeadsetL Mixer"},
> 	{"HSOR", NULL, "HeadsetR Mixer"},

I have this:
 SND_SOC_DAPM_MIXER_E("HeadsetL Mixer", SND_SOC_NOPM, 0, 0,
 	&twl4030_dapm_hsol_controls[0],
 	ARRAY_SIZE(twl4030_dapm_hsol_controls), headsetl_event,
 	SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD),
...
	SND_SOC_DAPM_PGA_E("HeadsetL fake PGA", SND_SOC_NOPM,
			0, 0, NULL, 0, headsetlfake_event,
 	SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD),
...
	{"HeadsetL Mixer", "Voice", "VDL_APGA"},
	{"HeadsetL Mixer", "AudioL1", "ARXL1_APGA"},
	{"HeadsetL Mixer", "AudioL2", "ARXL2_APGA"},
	{"HeadsetL fake PGA", NULL, "HeadsetL Mixer"},
...
	{"HSOL", NULL, "HeadsetL fake PGA"},

I have moved the code form the headsetl_event to headsetlfake_event, so the 
ramp is configured in the fake PGA event handler.

>
> So startup/shutdown order should be correct if you define a new PGA
> between the HSOx and Headsetx mixer and let that to manage output pin
> power.

The order between the PGAs are not correct for the startup. In playback case 
we have two PGA with this new 'fake PGA':
ARXL2_APGA and the "HeadsetL fake PGA"

DAPM is handling first the "HeadsetL fake PGA" than the ARXL2_APGA both for 
startup and for standby. For standby it is OK, but for startup the ARXL2_APGA 
should be handled prior to the "HeadsetL fake PGA" => in reverse order.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Mark Brown]

On Thu, May 14, 2009 at 08:27:11AM +0300, Peter Ujfalusi wrote:

> While I was looking at the sequences, I wonder why the mux and mixer handled 
> in a differently?

I'm not aware of any great reason for that; I suspect it's most likely
that muxes are more common on inputs than outputs.

> Is it possible to add several SND_SOC_DAPM_PRE to the DAPM routing? For 
> example to the HeadsetL, HeadsetR, HandsFreeL and HandsFreeR? How does it than 
> would work?

Yes, you can have as many of those as you like.

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Mark Brown]

On Thu, May 14, 2009 at 11:09:08AM +0300, Peter Ujfalusi wrote:

> If the order of the startup can be somehow altered to be:
> dac -> PGA -> HS ramp up (HS enable, fake PGA)

Looking at that (without any reference to the TWL4030 itself) I would do
this by having the headset ramp up be handled by an POST_PMU event on
the PGA.

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Thursday 14 May 2009 12:24:55 ext Mark Brown wrote:
> On Thu, May 14, 2009 at 11:09:08AM +0300, Peter Ujfalusi wrote:
> > If the order of the startup can be somehow altered to be:
> > dac -> PGA -> HS ramp up (HS enable, fake PGA)
>
> Looking at that (without any reference to the TWL4030 itself) I would do
> this by having the headset ramp up be handled by an POST_PMU event on
> the PGA.

This is not really possible with the twl4030 codec.
For simplicity, let's take the Audio left2 route:
DACL2 -> PGAL2 ---> PreDriveL
                |-> PreDriveR
                |-> HeadsetL
                |-> CarkitL
                |-> HandsFreeL
                |-> HandsFreeR
                \-> Earpiece

At least the Headset and HandsFree needs special mantra for the pop removal.
If the Headset is not connected/used than there is no need to do the HS pop 
attenuation.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Mark Brown]

On Thu, May 14, 2009 at 12:48:49PM +0300, Peter Ujfalusi wrote:

> This is not really possible with the twl4030 codec.
> For simplicity, let's take the Audio left2 route:
> DACL2 -> PGAL2 ---> PreDriveL
>                 |-> PreDriveR
>                 |-> HeadsetL
>                 |-> CarkitL
>                 |-> HandsFreeL
>                 |-> HandsFreeR
>                 \-> Earpiece

> At least the Headset and HandsFree needs special mantra for the pop removal.
> If the Headset is not connected/used than there is no need to do the HS pop 
> attenuation.

The event could check the current setup and do whatever's appropriate?

BTW, since you're saying the required ramp time is clock dependent
shouldn't it also be possible to configure the ramp time automatically
based on the clock that's been configured?

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Thursday 14 May 2009 12:56:21 ext Mark Brown wrote:
> On Thu, May 14, 2009 at 12:48:49PM +0300, Peter Ujfalusi wrote:
> > This is not really possible with the twl4030 codec.
> > For simplicity, let's take the Audio left2 route:
> > DACL2 -> PGAL2 ---> PreDriveL
> >
> >                 |-> PreDriveR
> >                 |-> HeadsetL
> >                 |-> CarkitL
> >                 |-> HandsFreeL
> >                 |-> HandsFreeR
> >
> >                 \-> Earpiece
> >
> > At least the Headset and HandsFree needs special mantra for the pop
> > removal. If the Headset is not connected/used than there is no need to do
> > the HS pop attenuation.
>
> The event could check the current setup and do whatever's appropriate?

I have to think about this, but at first sight it does look awfully 
complicated thing to do (4 PGA, 7 output with mixers, 2+1 output mux)...

> BTW, since you're saying the required ramp time is clock dependent
> shouldn't it also be possible to configure the ramp time automatically
> based on the clock that's been configured?

The patch that I have sent is calculating the needed delay based on the MCLK + 
RAMP_DELAY value.
I think I don't want to fixate the ramp delay to any value (meaning that the 
delay would last around 50ms). This seams to be also board specific, so I 
would rather leave this to the user to find the best value for the given 
board.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY?on headsetl_event: POST_PMD

[Mark Brown]

On Thu, May 14, 2009 at 03:44:47PM +0300, Peter Ujfalusi wrote:
> On Thursday 14 May 2009 12:56:21 ext Mark Brown wrote:

> > The event could check the current setup and do whatever's appropriate?

> I have to think about this, but at first sight it does look awfully 
> complicated thing to do (4 PGA, 7 output with mixers, 2+1 output mux)...

It might help to have the mixers and muxes maintain some state when they
change?  It's just a suggestion, I'm not familiar enough with the codec
to know if it's a sensible one for this device or if it's more trouble
than it's worth.

> I think I don't want to fixate the ramp delay to any value (meaning that the 
> delay would last around 50ms). This seams to be also board specific, so I 
> would rather leave this to the user to find the best value for the given 
> board.

Ah, right.  Possibly worth making it platform data, I suppose, but
that'd be a pain when experimenting to find the values.

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY?on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Thursday 14 May 2009 15:53:34 ext Mark Brown wrote:
> On Thu, May 14, 2009 at 03:44:47PM +0300, Peter Ujfalusi wrote:
> > On Thursday 14 May 2009 12:56:21 ext Mark Brown wrote:
> > > The event could check the current setup and do whatever's appropriate?
> >
> > I have to think about this, but at first sight it does look awfully
> > complicated thing to do (4 PGA, 7 output with mixers, 2+1 output mux)...
>
> It might help to have the mixers and muxes maintain some state when they
> change?  It's just a suggestion, I'm not familiar enough with the codec
> to know if it's a sensible one for this device or if it's more trouble
> than it's worth.

I have given this one a though and it might be not that problematic. What I'm 
afraid is that there are some corner cases, which might need some special 
attention.

>
> > I think I don't want to fixate the ramp delay to any value (meaning that
> > the delay would last around 50ms). This seams to be also board specific,
> > so I would rather leave this to the user to find the best value for the
> > given board.
>
> Ah, right.  Possibly worth making it platform data, I suppose, but
> that'd be a pain when experimenting to find the values.

In the future I can add platform data for the initial ramp delay value, which 
can be passed from the board file, but still the user will be able to change 
that if he/she wanted to.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Wait RAMP_DELAY?on headsetl_event: POST_PMD

[Peter Ujfalusi]

On Friday 15 May 2009 09:30:08 Ujfalusi Peter (Nokia-D/Tampere) wrote:
> On Thursday 14 May 2009 15:53:34 ext Mark Brown wrote:
> > It might help to have the mixers and muxes maintain some state when they
> > change?  It's just a suggestion, I'm not familiar enough with the codec
> > to know if it's a sensible one for this device or if it's more trouble
> > than it's worth.
>
> I have given this one a though and it might be not that problematic. What
> I'm afraid is that there are some corner cases, which might need some
> special attention.

I have tried this. Worked well with the playback stop/start, but than getting 
the loopbacks working again proven to be close to impossible...
Anyway I have tried another approach, which seams to be working in all 
scenarios for the twl4030 codec.
It does not involve changes outside of the twl4030.c.
I will clean up the code and send the patches for testing.
In short:
I have moved the APGA control from the SND_SOC_DAPM_PGA to SND_SOC_DAPM_MIXER, 
also I have added another level of mixer for the digital path, which actually 
controls the DAC state.
Than I have SND_SOC_DAPM_PGA_E for the headset outputs.

Now the power up and power down order is correct and a minimal (almost no) pop 
can be heard on startup and on standby.

I know that this is kind of a hack, but there seams to be no other way to 
handle the twl4030 codec power on/off sequences.

-- 
Péter