| Message ID | 20230414140203.707729-6-pan@semihalf.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Chameleon v3 ASoC audio | expand |
On Fri, Apr 14, 2023 at 7:35 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł Anikiel wrote: > > > Apply a workaround for what seems to be a hardware quirk: when using > > an external MCLK signal, powering on Output and DAC for the first time > > produces output distortions unless they're powered together with whole > > chip power. > > This doesn't seem coherent, these are multiple register writes so > clearly can't be done at the same moment as initial power on. Clearly > there's some other constraint here. The "at the same time" part is done by writing multiple bits at once to SSM2602_PWR. But before that, SSM2602_ACTIVE has to be set, and then the chip is reset (SSM2602_RESET) to power everything down again. > > > The workaround powers them on in probe for the first time, as doing it > > later may be impossible (e.g. when starting playback while recording, > > whole chip power will already be on). > > It doesn't do that, it powers them on at component probe. Yes, I meant component probe. > > > Here are some sequences run at the very start before a sw reset (and > > later using one of the NOT OK sequences from above): > > > > ssmset 0x09 0x01 # core > > ssmset 0x06 0x07 # chip, out, dac > > OK > > I can't tell what any of this is trying to say, especially given all the > magic numbers, and obviously no actual use of the driver should be > writing directly to the register map. These are shell commands run from userspace (with no ssm2602 driver present in the kernel). ssmset is a wrapper for the i2cset command: ssmset() { i2cset -y 0 0x1a $(($1*2)) $2 } I definitely should have made that more clear. Do you think these logs are worth adding? If so, I'll improve the explanation what these mean. > > > + /* Workaround for what seems to be a hardware quirk: when using an > > + * external MCLK signal, powering on Output and DAC for the first > > + * time produces output distortions unless they're powered together > > + * with whole chip power. We power them here for the first time, > > + * as doing it later may be impossible (e.g. when starting playback > > + * while recording, whole chip power will already be on) > > + */ > > + regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01); > > + regmap_write(ssm2602->regmap, SSM2602_PWR, 0x07); > > + regmap_write(ssm2602->regmap, SSM2602_RESET, 0x00); > > + > > The rest of the driver uses symbolic names for register values, this > code should too. Ok, I'll correct that. > > This also seems buggy in that it writes non-default values to the > hardware then does a reset, meaning that the cache and hardware values > will be out of sync, and since it only happens on probe there will be an > issue after suspend if power is removed. It looks like this would be > most comfortably implemented as a register patch applied as soon as the > regmap is instantiated. See regmap_register_patch(). I haven't considered that. I will look at regmap_register_patch() and try to use it. Regards, Paweł
On Tue, Apr 25, 2023 at 06:02:20PM +0200, Paweł Anikiel wrote: > On Fri, Apr 14, 2023 at 7:35 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł Anikiel wrote: > > > Apply a workaround for what seems to be a hardware quirk: when using > > > an external MCLK signal, powering on Output and DAC for the first time > > > produces output distortions unless they're powered together with whole > > > chip power. > > This doesn't seem coherent, these are multiple register writes so > > clearly can't be done at the same moment as initial power on. Clearly > > there's some other constraint here. > The "at the same time" part is done by writing multiple bits at once > to SSM2602_PWR. But before that, SSM2602_ACTIVE has to be set, and > then the chip is reset (SSM2602_RESET) to power everything down again. So you need to power up the chip then do a register write sequence - that's noticably different to what the description says. > > > Here are some sequences run at the very start before a sw reset (and > > > later using one of the NOT OK sequences from above): > > > ssmset 0x09 0x01 # core > > > ssmset 0x06 0x07 # chip, out, dac > > > OK > > I can't tell what any of this is trying to say, especially given all the > > magic numbers, and obviously no actual use of the driver should be > > writing directly to the register map. > These are shell commands run from userspace (with no ssm2602 driver > present in the kernel). ssmset is a wrapper for the i2cset command: > ssmset() { > i2cset -y 0 0x1a $(($1*2)) $2 > } > I definitely should have made that more clear. > Do you think these logs are worth adding? If so, I'll improve the > explanation what these mean. Probably? Since I couldn't really follow what these were trying to tell me it's a bit hard to say. It looks like you worked this out yourself rather than using an erratum so documenting where the workaround comes from seems useful.
diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c index cbbe83b85ada..021e0c860fa1 100644 --- a/sound/soc/codecs/ssm2602.c +++ b/sound/soc/codecs/ssm2602.c @@ -589,6 +589,17 @@ static int ssm260x_component_probe(struct snd_soc_component *component) return ret; } + /* Workaround for what seems to be a hardware quirk: when using an + * external MCLK signal, powering on Output and DAC for the first + * time produces output distortions unless they're powered together + * with whole chip power. We power them here for the first time, + * as doing it later may be impossible (e.g. when starting playback + * while recording, whole chip power will already be on) + */ + regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01); + regmap_write(ssm2602->regmap, SSM2602_PWR, 0x07); + regmap_write(ssm2602->regmap, SSM2602_RESET, 0x00); + /* set the update bits */ regmap_update_bits(ssm2602->regmap, SSM2602_LINVOL, LINVOL_LRIN_BOTH, LINVOL_LRIN_BOTH);
Apply a workaround for what seems to be a hardware quirk: when using an external MCLK signal, powering on Output and DAC for the first time produces output distortions unless they're powered together with whole chip power. The workaround powers them on in probe for the first time, as doing it later may be impossible (e.g. when starting playback while recording, whole chip power will already be on). Here are some initialization sequences run after all other control registers were set (`ssmset reg val` sets the value of a register via i2c): ssmset 0x09 0x01 # core ssmset 0x06 0x07 # chip, out OK ssmset 0x09 0x01 # core ssmset 0x06 0x87 # out, dac ssmset 0x06 0x07 # chip OK (disable MCLK) ssmset 0x09 0x01 # core ssmset 0x06 0x1f # chip ssmset 0x06 0x07 # out, dac (enable MCLK) OK ssmset 0x09 0x01 # core ssmset 0x06 0x1f # chip ssmset 0x06 0x07 # out, dac NOT OK ssmset 0x06 0x1f # chip ssmset 0x09 0x01 # core ssmset 0x06 0x07 # out, dac NOT OK ssmset 0x09 0x01 # core ssmset 0x06 0x0f # chip, out ssmset 0x06 0x07 # dac NOT OK ssmset 0x09 0x01 # core ssmset 0x06 0x17 # chip, dac ssmset 0x06 0x07 # out NOT OK Here are some sequences run at the very start before a sw reset (and later using one of the NOT OK sequences from above): ssmset 0x09 0x01 # core ssmset 0x06 0x07 # chip, out, dac OK (disable MCLK) ssmset 0x09 0x01 # core ssmset 0x06 0x07 # chip, out, dac (enable MCLK after reset) NOT OK ssmset 0x09 0x01 # core ssmset 0x06 0x17 # chip, dac NOT OK ssmset 0x09 0x01 # core ssmset 0x06 0x0f # chip, out NOT OK ssmset 0x06 0x07 # chip, out, dac NOT OK This was tested on a Google Chameleon v3 board using an SSM2603 with an external MCLK. This doesn't seem to just be a PCB issue, as this was also observed on a ZYBO Z7-10: https://ez.analog.com/audio/f/q-a/543726/solved-ssm2603-right-output-offset-issue/480229 Signed-off-by: Paweł Anikiel <pan@semihalf.com> --- sound/soc/codecs/ssm2602.c | 11 +++++++++++ 1 file changed, 11 insertions(+)