diff mbox

[v2] ASoC: fsl_ssi: Fix channel swap on playback start

Message ID CAOMZO5CaqpELX0pPS0g7_bi0eEokhtxJtavphssnUi8iJ15XLQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fabio Estevam April 4, 2017, 8:28 p.m. UTC 
On Tue, Apr 4, 2017 at 5:09 PM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:

> SCR bit 3 (NET) is also set, so you should be in network mode with a long
> frame sync.
> In fact, you can entirely simulate a I2S behavior using Network mode. you
> should just be careful about the way everything is configured (eg. place of
> samples in the stream)

While debugging this issue I noticed that when I put the oscilloscope
probe in the LRCLK SGTL5000 pin the swap did not occur anymore.

After removing the probe the swap occurred frequently.

So decided to change the SGTL5000 LRCLK pin strength value:


and the swap does not happen.

So it seems that no change is needed on the imx-ssi side :-)

Comments

Arnaud Mouiche April 5, 2017, 7:54 a.m. UTC | #1 
On 04/04/2017 22:28, Fabio Estevam wrote:
> On Tue, Apr 4, 2017 at 5:09 PM, Arnaud Mouiche
> <arnaud.mouiche@invoxia.com> wrote:
>
>> SCR bit 3 (NET) is also set, so you should be in network mode with a long
>> frame sync.
>> In fact, you can entirely simulate a I2S behavior using Network mode. you
>> should just be careful about the way everything is configured (eg. place of
>> samples in the stream)
> While debugging this issue I noticed that when I put the oscilloscope
> probe in the LRCLK SGTL5000 pin the swap did not occur anymore.
>
> After removing the probe the swap occurred frequently.
>
> So decided to change the SGTL5000 LRCLK pin strength value:
>
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -1118,7 +1118,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
>                          SGTL5000_DAC_MUTE_RIGHT |
>                          SGTL5000_DAC_MUTE_LEFT);
>
> -       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
> +       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x035f);
>
>          snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,
>                          SGTL5000_HP_ZCD_EN |
>
> and the swap does not happen.
>
> So it seems that no change is needed on the imx-ssi side :-)
Good catch. All of this makes sense.
The SSI surely detect a glitch at the start of the stream and takes it 
for a sync frame, but not followed by the expected 32x2 bits.
It also explain why Caleb and I are not able to reproduce, since we 
connect SSI internally using the audmux, leaving no place for such glitch.

If only Max can validate this fix...
But what is strange is that writing TE and EN at once also avoid the 
issue... or it means the issue was really timing dependent.

Do you know which one is started first ?
- fsl_ssi_trigger(SNDRV_PCM_TRIGGER_START)
or
- stgl5000 PCM bus being turned on

We can expect that stgl5000 turns the PCM clocks first, and then SSI is 
turned on. Otherwise anything can happened when the codec starts its 
clocking.

Maybe we should look at the Fsync state when idle, and see how it behave 
during the startup. Depending of pull-up /down-down configuration of the 
pads, it may be leaved in a undefined state with undefined transitions 
when stgl5000 turns its output on...

Another way to definitively fix this kind of issue is to use 
SND_SOC_DAIFMT_CBS_CFS
- the codec generates the N*8*64 kHz or 44.1*64 kHz precise bitclock 
(something which is not flexible for the SSI who is connected to a fix 
PLL output clock)
- but the SSI generate the Sync, leaving no place for wrong detection.
Unfortunately, stgl5000 doesn't seem to support this mode.

Arnaud
Fabio Estevam April 5, 2017, 1:43 p.m. UTC | #2 
On Wed, Apr 5, 2017 at 4:54 AM, Arnaud Mouiche
<arnaud.mouiche@invoxia.com> wrote:

> Good catch. All of this makes sense.
> The SSI surely detect a glitch at the start of the stream and takes it for a
> sync frame, but not followed by the expected 32x2 bits.
> It also explain why Caleb and I are not able to reproduce, since we connect
> SSI internally using the audmux, leaving no place for such glitch.
>
> If only Max can validate this fix...

Yes, that would be nice.

> But what is strange is that writing TE and EN at once also avoid the
> issue... or it means the issue was really timing dependent.
>
> Do you know which one is started first ?
> - fsl_ssi_trigger(SNDRV_PCM_TRIGGER_START)
> or
> - stgl5000 PCM bus being turned on
>
> We can expect that stgl5000 turns the PCM clocks first, and then SSI is
> turned on. Otherwise anything can happened when the codec starts its
> clocking.

Correct: stgl5000 turns the PCM clocks first.

> Maybe we should look at the Fsync state when idle, and see how it behave
> during the startup. Depending of pull-up /down-down configuration of the
> pads, it may be leaved in a undefined state with undefined transitions when
> stgl5000 turns its output on...
>
> Another way to definitively fix this kind of issue is to use
> SND_SOC_DAIFMT_CBS_CFS
> - the codec generates the N*8*64 kHz or 44.1*64 kHz precise bitclock
> (something which is not flexible for the SSI who is connected to a fix PLL
> output clock)
> - but the SSI generate the Sync, leaving no place for wrong detection.
> Unfortunately, stgl5000 doesn't seem to support this mode.

Correct. I plan to submitting a sgtl5000 patch that allows setting the
lrclk pad strength via device tree.

Will wait for Max to confirm if this solves the swap channel issue on
his board as well.

I would like to thank you and Caleb for the tests.

Glad to see your atest application working as expected with the SSI driver :-)
Max Krummenacher April 5, 2017, 2:04 p.m. UTC | #3 
(resend from my alsa-devel registered email address)

On Wed, 2017-04-05 at 09:54 +0200, Arnaud Mouiche wrote:
> 
> 
> On 04/04/2017 22:28, Fabio Estevam wrote:
> > 
> > 
> > On Tue, Apr 4, 2017 at 5:09 PM, Arnaud Mouiche
> > <arnaud.mouiche@invoxia.com> wrote:
> > 
> > > 
> > > 
> > > SCR bit 3 (NET) is also set, so you should be in network mode with a long
> > > frame sync.
> > > In fact, you can entirely simulate a I2S behavior using Network mode. you
> > > should just be careful about the way everything is configured (eg. place of
> > > samples in the stream)
> > While debugging this issue I noticed that when I put the oscilloscope
> > probe in the LRCLK SGTL5000 pin the swap did not occur anymore.
> > 
> > After removing the probe the swap occurred frequently.
> > 
> > So decided to change the SGTL5000 LRCLK pin strength value:
> > 
> > --- a/sound/soc/codecs/sgtl5000.c
> > +++ b/sound/soc/codecs/sgtl5000.c
> > @@ -1118,7 +1118,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
> >                          SGTL5000_DAC_MUTE_RIGHT |
> >                          SGTL5000_DAC_MUTE_LEFT);
> > 
> > -       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
> > +       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x035f);
> > 
> >          snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,
> >                          SGTL5000_HP_ZCD_EN |
> > 
> > and the swap does not happen.
> > 
> > So it seems that no change is needed on the imx-ssi side :-)
> Good catch. All of this makes sense.
> The SSI surely detect a glitch at the start of the stream and takes it 
> for a sync frame, but not followed by the expected 32x2 bits.
> It also explain why Caleb and I are not able to reproduce, since we 
> connect SSI internally using the audmux, leaving no place for such glitch.
> 
> If only Max can validate this fix...

Reverting the previous patch (setting TE and RE) and changing the drive
strenght to 0x035f fixes the channel swap on the Colibri iMX6DL.
e.g. 1000 playback starts do all have the correct channel assignment.

Looks like having the the LRCLK a bit earlier through the increased drive
strength changes the timing in a way that when the SSI samples that signal
with its correct state.
Probably the previous patch with TE and RE changes the sampling time in
the SSI which made it work in our use case.

Max

> 
> But what is strange is that writing TE and EN at once also avoid the 
> issue... or it means the issue was really timing dependent.
> 
> Do you know which one is started first ?
> - fsl_ssi_trigger(SNDRV_PCM_TRIGGER_START)
> or
> - stgl5000 PCM bus being turned on
> 
> We can expect that stgl5000 turns the PCM clocks first, and then SSI is 
> turned on. Otherwise anything can happened when the codec starts its 
> clocking.
> 
> Maybe we should look at the Fsync state when idle, and see how it behave 
> during the startup. Depending of pull-up /down-down configuration of the 
> pads, it may be leaved in a undefined state with undefined transitions 
> when stgl5000 turns its output on...
> 
> Another way to definitively fix this kind of issue is to use 
> SND_SOC_DAIFMT_CBS_CFS
> - the codec generates the N*8*64 kHz or 44.1*64 kHz precise bitclock 
> (something which is not flexible for the SSI who is connected to a fix 
> PLL output clock)
> - but the SSI generate the Sync, leaving no place for wrong detection.
> Unfortunately, stgl5000 doesn't seem to support this mode.
> 
> Arnaud
>
diff mbox

Patch

--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1118,7 +1118,7 @@  static int sgtl5000_probe(struct snd_soc_codec *codec)
                        SGTL5000_DAC_MUTE_RIGHT |
                        SGTL5000_DAC_MUTE_LEFT);

-       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
+       snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x035f);

        snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,
                        SGTL5000_HP_ZCD_EN |