[PATCH] ASoC: sta32x: add workaround for ESD reset issue

[PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Johannes Stezenbach]

sta32x resets and loses all configuration during ESD test.
Work around by preserving coefficient RAM in a shadow,
poll once a second on the CONFA register and restore
all coeffcients and registers when CONFA changes unexpectedly.

Cc: Sven Neumann <s.neumann@raumfeld.com>
Cc: Daniel Mack <daniel@zonque.org>
Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
ESD test means they are shooting the poor thing with a taser
and then sneer at the weakling when it keels over in shock.
You wouldn't believe what atrocities some "engineers" are capable
of, all in the name of "quality assurance".

The patch applies cleanly against both for-3.2 and for-3.3
of git://opensource.wolfsonmicro.com/linux-2.6-asoc.git.

 sound/soc/codecs/sta32x.c |   75 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/sta32x.h |    1 +
 2 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/sta32x.c b/sound/soc/codecs/sta32x.c
index bb82408..3caa7c3 100644
--- a/sound/soc/codecs/sta32x.c
+++ b/sound/soc/codecs/sta32x.c
@@ -27,6 +27,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -76,6 +77,10 @@ struct sta32x_priv {
 
 	unsigned int mclk;
 	unsigned int format;
+
+	u32 coef_shadow[STA32X_COEF_COUNT];
+	struct delayed_work watchdog_work;
+	int shutdown;
 };
 
 static const DECLARE_TLV_DB_SCALE(mvol_tlv, -12700, 50, 1);
@@ -227,6 +232,7 @@ static int sta32x_coefficient_put(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	struct sta32x_priv *sta32x = snd_soc_codec_get_drvdata(codec);
 	int numcoef = kcontrol->private_value >> 16;
 	int index = kcontrol->private_value & 0xffff;
 	unsigned int cfud;
@@ -239,6 +245,11 @@ static int sta32x_coefficient_put(struct snd_kcontrol *kcontrol,
 	snd_soc_write(codec, STA32X_CFUD, cfud);
 
 	snd_soc_write(codec, STA32X_CFADDR2, index);
+	for (i = 0; i < numcoef && (index + (i + 1) * 3 < STA32X_COEF_COUNT); i++)
+		sta32x->coef_shadow[index + i] =
+			  (ucontrol->value.bytes.data[3 * i    ] << 16)
+			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
+			| (ucontrol->value.bytes.data[3 * i + 2]);
 	for (i = 0; i < 3 * numcoef; i++)
 		snd_soc_write(codec, STA32X_B1CF1 + i,
 			      ucontrol->value.bytes.data[i]);
@@ -252,6 +263,55 @@ static int sta32x_coefficient_put(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+/* work around ESD issue where sta32x resets and loses all configuration */
+static void sta32x_watchdog(struct work_struct *work)
+{
+	struct sta32x_priv *sta32x = container_of(work, struct sta32x_priv,
+						  watchdog_work.work);
+	struct snd_soc_codec *codec = sta32x->codec;
+	unsigned int value;
+	unsigned int cfud;
+	unsigned int mute;
+	unsigned int confa;
+	int i;
+
+	/* check if sta32x has reset itself */
+	snd_soc_cache_read(codec, STA32X_CONFA, &confa);
+	if (confa == codec->hw_read(codec, STA32X_CONFA))
+		goto ok;
+
+	/* mute during restore */
+	snd_soc_cache_read(codec, STA32X_MMUTE, &mute);
+	snd_soc_write(codec, STA32X_MMUTE, mute | STA32X_MMUTE_MMUTE);
+
+	for (i = 0; i < STA32X_REGISTER_COUNT; i++) {
+		if (snd_soc_codec_volatile_register(codec, i))
+			continue;
+		snd_soc_cache_read(codec, i, &value);
+		snd_soc_write(codec, i, value);
+	}
+
+	/* preserve reserved bits in STA32X_CFUD */
+	cfud = snd_soc_read(codec, STA32X_CFUD) & 0xf0;
+
+	for (i = 0; i < STA32X_COEF_COUNT; i++) {
+		snd_soc_write(codec, STA32X_CFADDR2, i);
+		snd_soc_write(codec, STA32X_B1CF1, (sta32x->coef_shadow[i] >> 16) & 0xff);
+		snd_soc_write(codec, STA32X_B1CF2, (sta32x->coef_shadow[i] >>  8) & 0xff);
+		snd_soc_write(codec, STA32X_B1CF3, (sta32x->coef_shadow[i]      ) & 0xff);
+		/* chip documentation does not say if the bits are self clearing,
+		 * so do it explicitly */
+		snd_soc_write(codec, STA32X_CFUD, cfud);
+		snd_soc_write(codec, STA32X_CFUD, cfud | 0x01);
+	}
+	snd_soc_write(codec, STA32X_MMUTE, mute);
+
+ok:
+	if (!sta32x->shutdown)
+		schedule_delayed_work(&sta32x->watchdog_work,
+				      round_jiffies_relative(HZ));
+}
+
 #define SINGLE_COEF(xname, index) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = sta32x_coefficient_info, \
@@ -790,10 +850,23 @@ static int sta32x_probe(struct snd_soc_codec *codec)
 			    STA32X_CxCFG_OM_MASK,
 			    2 << STA32X_CxCFG_OM_SHIFT);
 
+	/* initialize coefficient shadow RAM with reset values */
+	for (i = 4; i <= 49; i += 5)
+		sta32x->coef_shadow[i] = 0x400000;
+	for (i = 50; i <= 54; i ++)
+		sta32x->coef_shadow[i] = 0x7fffff;
+	sta32x->coef_shadow[55] = 0x5a9df7;
+	sta32x->coef_shadow[56] = 0x7fffff;
+	sta32x->coef_shadow[59] = 0x7fffff;
+	sta32x->coef_shadow[60] = 0x400000;
+	sta32x->coef_shadow[61] = 0x400000;
+
 	sta32x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 	/* Bias level configuration will have done an extra enable */
 	regulator_bulk_disable(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
 
+	INIT_DELAYED_WORK(&sta32x->watchdog_work, sta32x_watchdog);
+	schedule_delayed_work(&sta32x->watchdog_work, round_jiffies_relative(HZ));
 	return 0;
 
 err_get:
@@ -806,6 +879,8 @@ static int sta32x_remove(struct snd_soc_codec *codec)
 {
 	struct sta32x_priv *sta32x = snd_soc_codec_get_drvdata(codec);
 
+	sta32x->shutdown = 1;
+	cancel_delayed_work_sync(&sta32x->watchdog_work);
 	sta32x_set_bias_level(codec, SND_SOC_BIAS_OFF);
 	regulator_bulk_disable(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
 	regulator_bulk_free(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
diff --git a/sound/soc/codecs/sta32x.h b/sound/soc/codecs/sta32x.h
index b97ee5a..d8e32a6 100644
--- a/sound/soc/codecs/sta32x.h
+++ b/sound/soc/codecs/sta32x.h
@@ -19,6 +19,7 @@
 /* STA326 register addresses */
 
 #define STA32X_REGISTER_COUNT	0x2d
+#define STA32X_COEF_COUNT 62
 
 #define STA32X_CONFA	0x00
 #define STA32X_CONFB    0x01
-- 
1.7.7.2

Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Mark Brown]

On Wed, Nov 09, 2011 at 02:30:01PM +0100, Johannes Stezenbach wrote:
> sta32x resets and loses all configuration during ESD test.
> Work around by preserving coefficient RAM in a shadow,
> poll once a second on the CONFA register and restore
> all coeffcients and registers when CONFA changes unexpectedly.

So, clearly a constant poll isn't going to do power consumption or
anything any favours so we shouldn't be doing this by default.  The most
obvious idea is to only check while audio is active (since it doesn't
really matter if the device is reset while not playing audio anyway)
which would get rid of most of the problem.

Given that the driver already supports powering the device down it may
also be sufficient to simply enable idle_bias_off and assume that if the
device resets the application will get sufficiently upset to restart
things anyway.

If we do need to poll or anything else invasive I'd also expect this to
be optional as if the device has ESD weaknesses then it'd be likely that
boards would add external ESD protection which 

>  	snd_soc_write(codec, STA32X_CFADDR2, index);
> +	for (i = 0; i < numcoef && (index + (i + 1) * 3 < STA32X_COEF_COUNT); i++)
> +		sta32x->coef_shadow[index + i] =
> +			  (ucontrol->value.bytes.data[3 * i    ] << 16)
> +			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
> +			| (ucontrol->value.bytes.data[3 * i + 2]);

Does this need to be done when restoring to _STANDBY as well?

> +	/* check if sta32x has reset itself */
> +	snd_soc_cache_read(codec, STA32X_CONFA, &confa);
> +	if (confa == codec->hw_read(codec, STA32X_CONFA))
> +		goto ok;

Don't call hw_read() directly, use cache_bypass if you need to.

> +	/* mute during restore */
> +	snd_soc_cache_read(codec, STA32X_MMUTE, &mute);
> +	snd_soc_write(codec, STA32X_MMUTE, mute | STA32X_MMUTE_MMUTE);

Just use snd_soc_read() and let the cache do the right thing.

> +	for (i = 0; i < STA32X_REGISTER_COUNT; i++) {
> +		if (snd_soc_codec_volatile_register(codec, i))
> +			continue;
> +		snd_soc_cache_read(codec, i, &value);
> +		snd_soc_write(codec, i, value);
> +	}

Use cache_sync().

Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Johannes Stezenbach]

On Wed, Nov 09, 2011 at 02:36:47PM +0000, Mark Brown wrote:
> On Wed, Nov 09, 2011 at 02:30:01PM +0100, Johannes Stezenbach wrote:
> > sta32x resets and loses all configuration during ESD test.
> > Work around by preserving coefficient RAM in a shadow,
> > poll once a second on the CONFA register and restore
> > all coeffcients and registers when CONFA changes unexpectedly.
> 
> So, clearly a constant poll isn't going to do power consumption or
> anything any favours so we shouldn't be doing this by default.  The most
> obvious idea is to only check while audio is active (since it doesn't
> really matter if the device is reset while not playing audio anyway)
> which would get rid of most of the problem.

Um, well, the sta32x is a power amplifier, it draws 3mA
even when in power down.  I think a poll once per second
won't add measurable power consumption, especially since
there are other wakeup sources present in the system with
higher wakeup frequency.  I acknowledge that your are right
in principle, but since the sta32x won't be used in battery-powered
mobile devices I think it is not crucial to improve this.  Do you agree?

> Given that the driver already supports powering the device down it may
> also be sufficient to simply enable idle_bias_off and assume that if the
> device resets the application will get sufficiently upset to restart
> things anyway.

Unfortunately the app won't notice, the PXA I2S interface will
happily push the audio data to the codec which will be muted
without the app knowing it since the register cache will tell
something different.

> If we do need to poll or anything else invasive I'd also expect this to
> be optional as if the device has ESD weaknesses then it'd be likely that
> boards would add external ESD protection which 

OK, I'll add a module parameter.  Raumfeld engineers claim their
hw design is fine as is, of course...


> >  	snd_soc_write(codec, STA32X_CFADDR2, index);
> > +	for (i = 0; i < numcoef && (index + (i + 1) * 3 < STA32X_COEF_COUNT); i++)
> > +		sta32x->coef_shadow[index + i] =
> > +			  (ucontrol->value.bytes.data[3 * i    ] << 16)
> > +			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
> > +			| (ucontrol->value.bytes.data[3 * i + 2]);
> 
> Does this need to be done when restoring to _STANDBY as well?

No, all registers and coefs are preserved until hw reset or power loss.

> > +	/* check if sta32x has reset itself */
> > +	snd_soc_cache_read(codec, STA32X_CONFA, &confa);
> > +	if (confa == codec->hw_read(codec, STA32X_CONFA))
> > +		goto ok;
> 
> Don't call hw_read() directly, use cache_bypass if you need to.

OK

> > +	/* mute during restore */
> > +	snd_soc_cache_read(codec, STA32X_MMUTE, &mute);
> > +	snd_soc_write(codec, STA32X_MMUTE, mute | STA32X_MMUTE_MMUTE);
> 
> Just use snd_soc_read() and let the cache do the right thing.

hm, I think snd_soc_cache_read() is more explicit; but OK, will change

> > +	for (i = 0; i < STA32X_REGISTER_COUNT; i++) {
> > +		if (snd_soc_codec_volatile_register(codec, i))
> > +			continue;
> > +		snd_soc_cache_read(codec, i, &value);
> > +		snd_soc_write(codec, i, value);
> > +	}
> 
> Use cache_sync().

OK


Thanks
Johannes

Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Mark Brown]

On Wed, Nov 09, 2011 at 06:34:19PM +0100, Johannes Stezenbach wrote:
> On Wed, Nov 09, 2011 at 02:36:47PM +0000, Mark Brown wrote:

> > So, clearly a constant poll isn't going to do power consumption or
> > anything any favours so we shouldn't be doing this by default.  The most
> > obvious idea is to only check while audio is active (since it doesn't
> > really matter if the device is reset while not playing audio anyway)
> > which would get rid of most of the problem.

> Um, well, the sta32x is a power amplifier, it draws 3mA
> even when in power down.  I think a poll once per second
> won't add measurable power consumption, especially since
> there are other wakeup sources present in the system with
> higher wakeup frequency.  I acknowledge that your are right
> in principle, but since the sta32x won't be used in battery-powered
> mobile devices I think it is not crucial to improve this.  Do you agree?

I'd still rather avoid it.

> > Given that the driver already supports powering the device down it may
> > also be sufficient to simply enable idle_bias_off and assume that if the
> > device resets the application will get sufficiently upset to restart
> > things anyway.

> Unfortunately the app won't notice, the PXA I2S interface will
> happily push the audio data to the codec which will be muted
> without the app knowing it since the register cache will tell
> something different.

Yeah, but if it suddenly stops in the middle of playback then that'll
tend to register with users and if you keep it powered off at all other
times.

> > If we do need to poll or anything else invasive I'd also expect this to
> > be optional as if the device has ESD weaknesses then it'd be likely that
> > boards would add external ESD protection which 

> OK, I'll add a module parameter.  Raumfeld engineers claim their
> hw design is fine as is, of course...

Platform data seems more sensible, it's board level breakage from the
sounds of it.

> > > +	for (i = 0; i < numcoef && (index + (i + 1) * 3 < STA32X_COEF_COUNT); i++)
> > > +		sta32x->coef_shadow[index + i] =
> > > +			  (ucontrol->value.bytes.data[3 * i    ] << 16)
> > > +			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
> > > +			| (ucontrol->value.bytes.data[3 * i + 2]);

> > Does this need to be done when restoring to _STANDBY as well?

> No, all registers and coefs are preserved until hw reset or power loss.

Right, but the bias level management is disabling the regulators for the
device so power loss may happen then and _STANDBY is also used to resume
the device after suspend when power loss may also occur.

> > > +	/* mute during restore */
> > > +	snd_soc_cache_read(codec, STA32X_MMUTE, &mute);
> > > +	snd_soc_write(codec, STA32X_MMUTE, mute | STA32X_MMUTE_MMUTE);

> > Just use snd_soc_read() and let the cache do the right thing.

> hm, I think snd_soc_cache_read() is more explicit; but OK, will change

It's not particularly important that you read the cache here, you just
want to update the value in the register.  If this happens to not
involve hardware I/O that's nice but not essential.

Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Johannes Stezenbach]

On Wed, Nov 09, 2011 at 11:32:57PM +0000, Mark Brown wrote:
> On Wed, Nov 09, 2011 at 06:34:19PM +0100, Johannes Stezenbach wrote:
> > On Wed, Nov 09, 2011 at 02:36:47PM +0000, Mark Brown wrote:
> 
> > > Given that the driver already supports powering the device down it may
> > > also be sufficient to simply enable idle_bias_off and assume that if the
> > > device resets the application will get sufficiently upset to restart
> > > things anyway.
> 
> > Unfortunately the app won't notice, the PXA I2S interface will
> > happily push the audio data to the codec which will be muted
> > without the app knowing it since the register cache will tell
> > something different.
> 
> Yeah, but if it suddenly stops in the middle of playback then that'll
> tend to register with users and if you keep it powered off at all other
> times.

Hm, the goal is to fix the issue without user interaction.
The app will keep the device open and will play back
without noticing the problem.

> > > > +	for (i = 0; i < numcoef && (index + (i + 1) * 3 < STA32X_COEF_COUNT); i++)
> > > > +		sta32x->coef_shadow[index + i] =
> > > > +			  (ucontrol->value.bytes.data[3 * i    ] << 16)
> > > > +			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
> > > > +			| (ucontrol->value.bytes.data[3 * i + 2]);
> 
> > > Does this need to be done when restoring to _STANDBY as well?
> 
> > No, all registers and coefs are preserved until hw reset or power loss.
> 
> Right, but the bias level management is disabling the regulators for the
> device so power loss may happen then and _STANDBY is also used to resume
> the device after suspend when power loss may also occur.

Ah, you're right, if the system cuts power to the sta32x during
standby we need to restore the coef RAM after resume.  How I
see where my code is buggy.  I also see what you mean about
using idle_bias_off to restore the settings, namely

	level = codec->dapm.bias_level;
	BUG_ON(level == SND_SOC_BIAS_OFF);
	sta32x_set_bias_level(codec, SND_SOC_BIAS_OFF);
	sta32x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
	if (level > SND_SOC_BIAS_STANDBY);
		sta32x_set_bias_level(codec, SND_SOC_BIAS_PREPARE);
	if (level > SND_SOC_BIAS_PREPARE);
		sta32x_set_bias_level(codec, SND_SOC_BIAS_ON);

Right?

I'll work on a new version of the patch and send it after
it's been put through testing by Sven.


Thanks for the review!

Johannes

Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Mark Brown]

On Thu, Nov 10, 2011 at 03:27:08PM +0100, Johannes Stezenbach wrote:
> On Wed, Nov 09, 2011 at 11:32:57PM +0000, Mark Brown wrote:

> > Yeah, but if it suddenly stops in the middle of playback then that'll
> > tend to register with users and if you keep it powered off at all other
> > times.

> Hm, the goal is to fix the issue without user interaction.
> The app will keep the device open and will play back
> without noticing the problem.

Well, the user is going to hear a horrific glitch anyway...

> > Right, but the bias level management is disabling the regulators for the
> > device so power loss may happen then and _STANDBY is also used to resume
> > the device after suspend when power loss may also occur.

> Ah, you're right, if the system cuts power to the sta32x during
> standby we need to restore the coef RAM after resume.  How I
> see where my code is buggy.  I also see what you mean about
> using idle_bias_off to restore the settings, namely

> 	level = codec->dapm.bias_level;
> 	BUG_ON(level == SND_SOC_BIAS_OFF);
> 	sta32x_set_bias_level(codec, SND_SOC_BIAS_OFF);
> 	sta32x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> 	if (level > SND_SOC_BIAS_STANDBY);
> 		sta32x_set_bias_level(codec, SND_SOC_BIAS_PREPARE);
> 	if (level > SND_SOC_BIAS_PREPARE);
> 		sta32x_set_bias_level(codec, SND_SOC_BIAS_ON);

> Right?

Not really what I meant - I meant just let the framework power the
device down when it goes idle so you don't have to worry about it
resetting - though you will need to go through the
OFF->STANDBY->PREPARE->ON transition to bring the device up nicely if
you do restart while active.

Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Sven Neumann]

On Thu, 2011-11-10 at 15:11 +0000, Mark Brown wrote:
> On Thu, Nov 10, 2011 at 03:27:08PM +0100, Johannes Stezenbach wrote:
> > On Wed, Nov 09, 2011 at 11:32:57PM +0000, Mark Brown wrote:
> 
> > > Yeah, but if it suddenly stops in the middle of playback then that'll
> > > tend to register with users and if you keep it powered off at all other
> > > times.
> 
> > Hm, the goal is to fix the issue without user interaction.
> > The app will keep the device open and will play back
> > without noticing the problem.
> 
> Well, the user is going to hear a horrific glitch anyway...

Well, the problem we are facing here is not really showing up during
normal operation. The amplifier is resetting only if there's a strong
electrostatic discharge on the front panel of the device. This is most
likely only going to occur during an EMC test. To pass the EMC test a
short dropout is acceptable as long as the device continues normal
operation afterwards.

Of course there's a small likelihood that such an electrostatic
discharge will happen in real life, but we have never seen this
happening so far and if it will happen the user will most likely prefer
a short dropout over complete failure.


Regards,
Sven


-- 
Sven Neumann
Head of RAUMFELD Software Development

Lautsprecher Teufel GmbH | Bülowstr. 66 | 10783 Berlin | Germany
Tel: +49 (0)30-300 930 153 | s.neumann@raumfeld.com

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel