[alsa-devel] [PATCH] ASoC: sta32x: add workaround for ESD reset issue
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
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().
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
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.
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
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.
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
participants (3)
-
Johannes Stezenbach -
Mark Brown -
Sven Neumann