[PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

In AC'97 mode we configure and start SSI RX / TX on probe path via
a call to _fsl_ssi_set_dai_fmt() function.
We don't need to call this function again later and in fact don't want to
do it since this function temporarily sets STCR, SRCR and SCR to some
intermediate values.

We need to make sure, however, that only proper channel slots are enabled
at playback start time since some AC'97 CODECs (like VT1613) were observed
requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
an AC'97 link is started but before the CODEC is configured by its driver.
Theoretically, this should be necessary only for the very first playback
but let's play safe here and make sure that no extra slots are enabled
every time a playback is started.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 48bb850a34d9..dad80b4b0cfc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -630,12 +630,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private *ssi_private)
 	regmap_write(regs, CCSR_SSI_SACNT,
 			CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
 
-	/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
-	if (!ssi_private->soc->imx21regs) {
-		regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-		regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
-	}
-
 	/*
 	 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
 	 * codec before a stream is started.
@@ -1076,6 +1070,9 @@ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
 {
 	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
 
+	if (fsl_ssi_is_ac97(ssi_private))
+		return 0;
+
 	return _fsl_ssi_set_dai_fmt(cpu_dai->dev, ssi_private, fmt);
 }
 
@@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+			if (fsl_ssi_is_ac97(ssi_private) &&
+				!ssi_private->soc->imx21regs) {
+				regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+				regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+			}
+
 			fsl_ssi_tx_config(ssi_private, true);
-		else
+		} else
 			fsl_ssi_rx_config(ssi_private, true);
 		break;
 

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Nicolin Chen]

On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
> In AC'97 mode we configure and start SSI RX / TX on probe path via
> a call to _fsl_ssi_set_dai_fmt() function.
> We don't need to call this function again later and in fact don't want to
> do it since this function temporarily sets STCR, SRCR and SCR to some
> intermediate values.
> 
> We need to make sure, however, that only proper channel slots are enabled
> at playback start time since some AC'97 CODECs (like VT1613) were observed
> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
> an AC'97 link is started but before the CODEC is configured by its driver.

I don't really understand this part. Why do we need to *make sure*
and set SACCDIS and SACCEN again since they're initialized already?

Could you please elaborate a bit more?

> Theoretically, this should be necessary only for the very first playback
> but let's play safe here and make sure that no extra slots are enabled
> every time a playback is started.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  sound/soc/fsl/fsl_ssi.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 48bb850a34d9..dad80b4b0cfc 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -630,12 +630,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private *ssi_private)
>  	regmap_write(regs, CCSR_SSI_SACNT,
>  			CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
>  
> -	/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
> -	if (!ssi_private->soc->imx21regs) {
> -		regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
> -		regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
> -	}

> @@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
> +			if (fsl_ssi_is_ac97(ssi_private) &&
> +				!ssi_private->soc->imx21regs) {
> +				regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
> +				regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
> +			}

And second, why could we ignore them for STREAM_CAPTURE here?

Well, at least this part could be moved into fsl_ssi_tx_config()
since we have an abstraction layer of register configurations.

Thanks
Nic

> +
>  			fsl_ssi_tx_config(ssi_private, true);
> -		else
> +		} else
>  			fsl_ssi_rx_config(ssi_private, true);
>  		break;
>  
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

On 21.11.2017 01:00, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
(..)
>> We need to make sure, however, that only proper channel slots are enabled
>> at playback start time since some AC'97 CODECs (like VT1613) were observed
>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
>> an AC'97 link is started but before the CODEC is configured by its driver.
> 
> I don't really understand this part. Why do we need to *make sure*
> and set SACCDIS and SACCEN again since they're initialized already> 
> Could you please elaborate a bit more?

SACCDIS and SACCEN aren't just normal registers.
They are a way to disable or enable bits in SACCST register - writing
a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
writing a '1' bit at some position in SACCEN sets the relevant bit in
SACCST.

But a bit in SACCST can also be set (and so an AC'97 channel slot
enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
enough if this bit was set in SLOTREQ just once, bits in SACCST are
'sticky').
If an extra slot gets enabled that's a disaster for playback because some
of normal left or right channel samples are instead redirected to this
extra slot.

Unfortunately, the VT1613 codec on UDOO board (which is the only user of
fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
set extra bits in its SLOTREQ request - I've confirmed this on two boards
bought a few months apart directly from UDOO shop.

A workaround implemented in fsl-asoc-card of setting an appropriate
CODEC register so that slots 3/4 (the normal stereo playback slots) are
used for S/PDIF seems to mostly fix this issue but since this codec is so
untrustworthy then:
>> let's play safe here and make sure that no extra slots are enabled
>> every time a playback is started.

(..)
>> @@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
>>  	case SNDRV_PCM_TRIGGER_START:
>>  	case SNDRV_PCM_TRIGGER_RESUME:
>>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +			/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
>> +			if (fsl_ssi_is_ac97(ssi_private) &&
>> +				!ssi_private->soc->imx21regs) {
>> +				regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
>> +				regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
>> +			}
> 
> And second, why could we ignore them for STREAM_CAPTURE here?

Capture is not affected by SACCST register content.

> Well, at least this part could be moved into fsl_ssi_tx_config()
> since we have an abstraction layer of register configurations.

Will move this into fsl_ssi_tx_config() in a respin.

> 
> Thanks
> Nic

Thanks,
Maciej

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Nicolin Chen]

On Tue, Nov 21, 2017 at 01:32:09AM +0100, Maciej S. Szmigiero wrote:
> On 21.11.2017 01:00, Nicolin Chen wrote:
> > On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
> (..)
> >> We need to make sure, however, that only proper channel slots are enabled
> >> at playback start time since some AC'97 CODECs (like VT1613) were observed
> >> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
> >> an AC'97 link is started but before the CODEC is configured by its driver.
> > 
> > I don't really understand this part. Why do we need to *make sure*
> > and set SACCDIS and SACCEN again since they're initialized already> 
> > Could you please elaborate a bit more?
> 
> SACCDIS and SACCEN aren't just normal registers.
> They are a way to disable or enable bits in SACCST register - writing
> a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
> writing a '1' bit at some position in SACCEN sets the relevant bit in
> SACCST.
> 
> But a bit in SACCST can also be set (and so an AC'97 channel slot
> enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
> enough if this bit was set in SLOTREQ just once, bits in SACCST are
> 'sticky').

This makes sense now. It could be mentioned a bit in the commit log
as well.

> If an extra slot gets enabled that's a disaster for playback because some
> of normal left or right channel samples are instead redirected to this
> extra slot.
> 
> Unfortunately, the VT1613 codec on UDOO board (which is the only user of
> fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
> set extra bits in its SLOTREQ request - I've confirmed this on two boards
> bought a few months apart directly from UDOO shop.
> 
> A workaround implemented in fsl-asoc-card of setting an appropriate
> CODEC register so that slots 3/4 (the normal stereo playback slots) are
> used for S/PDIF seems to mostly fix this issue but since this codec is so
> untrustworthy then:

If this move is also a workaround, probably it'd be better to have
an fsl_ssi_ac97_xxx_war() function with some comments/descriptions,
and include it in the config(). Since it doesn't hurt to set these
registers again, I am personally fine with that. But it needs to be
clear -- otherwise, people may try to remove it later with a change
like: Removing redundant SACCEN/SACCDIS settings since they're done
during probe().

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

On 21.11.2017 02:14, Nicolin Chen wrote:
> On Tue, Nov 21, 2017 at 01:32:09AM +0100, Maciej S. Szmigiero wrote:
>> On 21.11.2017 01:00, Nicolin Chen wrote:
>>> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
>> (..)
>>>> We need to make sure, however, that only proper channel slots are enabled
>>>> at playback start time since some AC'97 CODECs (like VT1613) were observed
>>>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
>>>> an AC'97 link is started but before the CODEC is configured by its driver.
>>>
>>> I don't really understand this part. Why do we need to *make sure*
>>> and set SACCDIS and SACCEN again since they're initialized already> 
>>> Could you please elaborate a bit more?
>>
>> SACCDIS and SACCEN aren't just normal registers.
>> They are a way to disable or enable bits in SACCST register - writing
>> a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
>> writing a '1' bit at some position in SACCEN sets the relevant bit in
>> SACCST.
>>
>> But a bit in SACCST can also be set (and so an AC'97 channel slot
>> enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
>> enough if this bit was set in SLOTREQ just once, bits in SACCST are
>> 'sticky').
> 
> This makes sense now. It could be mentioned a bit in the commit log
> as well.

Will do.

>> If an extra slot gets enabled that's a disaster for playback because some
>> of normal left or right channel samples are instead redirected to this
>> extra slot.
>>
>> Unfortunately, the VT1613 codec on UDOO board (which is the only user of
>> fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
>> set extra bits in its SLOTREQ request - I've confirmed this on two boards
>> bought a few months apart directly from UDOO shop.
>>
>> A workaround implemented in fsl-asoc-card of setting an appropriate
>> CODEC register so that slots 3/4 (the normal stereo playback slots) are
>> used for S/PDIF seems to mostly fix this issue but since this codec is so
>> untrustworthy then:
> 
> If this move is also a workaround, probably it'd be better to have
> an fsl_ssi_ac97_xxx_war() function with some comments/descriptions,
> and include it in the config(). Since it doesn't hurt to set these
> registers again, I am personally fine with that. But it needs to be
> clear -- otherwise, people may try to remove it later with a change
> like: Removing redundant SACCEN/SACCDIS settings since they're done
> during probe().
> 

That's a good point, will do.

Maciej