[PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

When testing AC'97 capture on UDOO board (currently the only user of
fsl_ssi driver in the AC'97 mode) it become obvious that there is a massive
distortion above certain, small input signal.

This problem has been traced to silicon errata ERR003778:
"In AC97, 16-bit mode, received data is shifted by 4-bit locations" that
has "No fix scheduled".
This errata suggests a workaround of doing a 4-bit shift back in SDMA
script for this specific operation mode, however our SDMA scripts are
shared between various SoC peripherals so we can't really modify them.

There is a simple way to avoid this problem, however, that is to disallow
recording in 16-bit mode and only support it in AC'97-native 20-bit mode.
We have to use a 4-byte format for this since SSI FIFOs do not allow 3-byte
accesses (and these aren't supported by imx-sdma driver anyway).
With this change the capture distortion is gone.

We can also add this format as an additional one supported for playback,
using this opportunity to make sure that we use CPU-endian-native formats
in AC'97 mode as we already do in I2S mode.

There is no problem in using different bit widths in playback and capture
in AC'97 mode so allow this, too.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
Changes from v1: Adapt format name to changes in the first patch from
this series.

Changes from v2, v3: None.

 sound/soc/fsl/fsl_ssi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 20ef09e1a395..c350117c8e31 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1278,14 +1278,15 @@ static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S20,
 	},
 	.capture = {
 		.stream_name = "AC97 Capture",
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		/* 16-bit capture is broken (errata ERR003778) */
+		.formats = SNDRV_PCM_FMTBIT_S20,
 	},
 	.ops = &fsl_ssi_dai_ops,
 };
@@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
 	/* Are the RX and the TX clocks locked? */
 	if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
-		if (!fsl_ssi_is_ac97(ssi_private))
+		if (!fsl_ssi_is_ac97(ssi_private)) {
 			ssi_private->cpu_dai_drv.symmetric_rates = 1;
+			ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
+		}
 
 		ssi_private->cpu_dai_drv.symmetric_channels = 1;
-		ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
 	}
 
 	/* Determine the FIFO depth. */

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Nicolin Chen]

Hi Maciej,

On Mon, Nov 27, 2017 at 11:34:44PM +0100, Maciej S. Szmigiero wrote:
> There is no problem in using different bit widths in playback and capture
> in AC'97 mode so allow this, too.

> @@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>  
>  	/* Are the RX and the TX clocks locked? */
>  	if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
> -		if (!fsl_ssi_is_ac97(ssi_private))
> +		if (!fsl_ssi_is_ac97(ssi_private)) {
>  			ssi_private->cpu_dai_drv.symmetric_rates = 1;
> +			ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
> +		}
>  
>  		ssi_private->cpu_dai_drv.symmetric_channels = 1;
> -		ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>  	}

I was actually wondering how the AC97 works in the synchronous mode
while being able to handle different bit widths. Then I found that
the drivers does corresponding configurations for synchronous mode
only if symmetric_rates is set -- which is unset for AC97 cases. So
in fact AC97 case (symmetric_rates unset) is probably being treated
as asynchronous mode by the driver -- it'd be better if you confirm
this for me.

And I am not so sure about the physical pin connections in an AC97
situation, but I started to think that, instead of having a change
above, AC97 cases might be supposed to have "fsl,ssi-asynchronous"
property in DT since it's working when the driver sets both TX and
RX control registers (i.e. asynchronous mode), not like synchronous
mode that only sets TX's registers.

Thanks
Nicolin

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Nicolin Chen]

Hi Maciej,

On Mon, Nov 27, 2017 at 11:34:44PM +0100, Maciej S. Szmigiero wrote:
> There is no problem in using different bit widths in playback and capture
> in AC'97 mode so allow this, too.

> @@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>  
>  	/* Are the RX and the TX clocks locked? */
>  	if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
> -		if (!fsl_ssi_is_ac97(ssi_private))
> +		if (!fsl_ssi_is_ac97(ssi_private)) {
>  			ssi_private->cpu_dai_drv.symmetric_rates = 1;
> +			ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
> +		}
>  
>  		ssi_private->cpu_dai_drv.symmetric_channels = 1;
> -		ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>  	}

I was actually wondering how the AC97 works in the synchronous mode
while being able to handle different bit widths. Then I found that
the drivers does corresponding configurations for synchronous mode
only if symmetric_rates is set -- which is unset for AC97 cases. So
in fact AC97 case (symmetric_rates unset) is probably being treated
as asynchronous mode by the driver -- it'd be better if you confirm
this for me.

And I am not so sure about the physical pin connections in an AC97
situation, but I started to think that, instead of having a change
above, AC97 cases might be supposed to have "fsl,ssi-asynchronous"
property in DT since it's working when the driver sets both TX and
RX control registers (i.e. asynchronous mode), not like synchronous
mode that only sets TX's registers.

Thanks
Nicolin

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

Hi Nicolin,

On 30.11.2017 08:23, Nicolin Chen wrote:
> Hi Maciej,
> 
> On Mon, Nov 27, 2017 at 11:34:44PM +0100, Maciej S. Szmigiero wrote:
>> There is no problem in using different bit widths in playback and capture
>> in AC'97 mode so allow this, too.
> 
>> @@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>>  
>>  	/* Are the RX and the TX clocks locked? */
>>  	if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
>> -		if (!fsl_ssi_is_ac97(ssi_private))
>> +		if (!fsl_ssi_is_ac97(ssi_private)) {
>>  			ssi_private->cpu_dai_drv.symmetric_rates = 1;
>> +			ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>> +		}
>>  
>>  		ssi_private->cpu_dai_drv.symmetric_channels = 1;
>> -		ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>>  	}
> 
> I was actually wondering how the AC97 works in the synchronous mode
> while being able to handle different bit widths. Then I found that
> the drivers does corresponding configurations for synchronous mode
> only if symmetric_rates is set -- which is unset for AC97 cases. So
> in fact AC97 case (symmetric_rates unset) is probably being treated
> as asynchronous mode by the driver -- it'd be better if you confirm
> this for me.
> 
> And I am not so sure about the physical pin connections in an AC97
> situation, but I started to think that, instead of having a change
> above, AC97 cases might be supposed to have "fsl,ssi-asynchronous"
> property in DT since it's working when the driver sets both TX and
> RX control registers (i.e. asynchronous mode), not like synchronous
> mode that only sets TX's registers
In the AC'97 mode we have to differentiate two things:
1) Bit width of the physical AC'97 interface ("AC-link"),
2) Bit width of samples that are accepted during a playback and output
during a recording by the SSI (and, so by the sound card that it driven
by this CPU).

Bit width of the physical AC'97 interface is fixed at 20 bits per sample
(both in playback and capture direction).

Bit width of samples that are accepted (or produced) by the SSI could
be set in its STCCR and SRCCR registers (although in the AC'97 mode
only settings of either 16 or 20 bits are possible).
Each direction could be set independently from the other one.

That's what the driver configures depending on what an ALSA application
had requested.

Regarding a sample rate in AC'97 mode its effective value isn't really
controlled by the CPU (that is, SSI), but by a CODEC since it is
the CODEC which tells the CPU when it should send a next sample for
playback and when a next capture sample is ready.
There are no problems if they are different (as long as the CODEC
supports this, naturally, but it's up to its driver to restrict the
sample rate space accordingly).

A comment above "fsl,ssi-asynchronous" property says that it is about
whether "the RX and the TX clocks [are] locked".
They are on the physical AC'97 interface, but they aren't on the logical
playback / capture interface.
IMHO, this configurable property simply makes no sense in the AC'97
mode.

> Thanks
> Nicolin
> 

Best regards,
Maciej

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

Hi Nicolin,

On 30.11.2017 08:23, Nicolin Chen wrote:
> Hi Maciej,
> 
> On Mon, Nov 27, 2017 at 11:34:44PM +0100, Maciej S. Szmigiero wrote:
>> There is no problem in using different bit widths in playback and capture
>> in AC'97 mode so allow this, too.
> 
>> @@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>>  
>>  	/* Are the RX and the TX clocks locked? */
>>  	if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
>> -		if (!fsl_ssi_is_ac97(ssi_private))
>> +		if (!fsl_ssi_is_ac97(ssi_private)) {
>>  			ssi_private->cpu_dai_drv.symmetric_rates = 1;
>> +			ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>> +		}
>>  
>>  		ssi_private->cpu_dai_drv.symmetric_channels = 1;
>> -		ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>>  	}
> 
> I was actually wondering how the AC97 works in the synchronous mode
> while being able to handle different bit widths. Then I found that
> the drivers does corresponding configurations for synchronous mode
> only if symmetric_rates is set -- which is unset for AC97 cases. So
> in fact AC97 case (symmetric_rates unset) is probably being treated
> as asynchronous mode by the driver -- it'd be better if you confirm
> this for me.
> 
> And I am not so sure about the physical pin connections in an AC97
> situation, but I started to think that, instead of having a change
> above, AC97 cases might be supposed to have "fsl,ssi-asynchronous"
> property in DT since it's working when the driver sets both TX and
> RX control registers (i.e. asynchronous mode), not like synchronous
> mode that only sets TX's registers
In the AC'97 mode we have to differentiate two things:
1) Bit width of the physical AC'97 interface ("AC-link"),
2) Bit width of samples that are accepted during a playback and output
during a recording by the SSI (and, so by the sound card that it driven
by this CPU).

Bit width of the physical AC'97 interface is fixed at 20 bits per sample
(both in playback and capture direction).

Bit width of samples that are accepted (or produced) by the SSI could
be set in its STCCR and SRCCR registers (although in the AC'97 mode
only settings of either 16 or 20 bits are possible).
Each direction could be set independently from the other one.

That's what the driver configures depending on what an ALSA application
had requested.

Regarding a sample rate in AC'97 mode its effective value isn't really
controlled by the CPU (that is, SSI), but by a CODEC since it is
the CODEC which tells the CPU when it should send a next sample for
playback and when a next capture sample is ready.
There are no problems if they are different (as long as the CODEC
supports this, naturally, but it's up to its driver to restrict the
sample rate space accordingly).

A comment above "fsl,ssi-asynchronous" property says that it is about
whether "the RX and the TX clocks [are] locked".
They are on the physical AC'97 interface, but they aren't on the logical
playback / capture interface.
IMHO, this configurable property simply makes no sense in the AC'97
mode.

> Thanks
> Nicolin
> 

Best regards,
Maciej

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Nicolin Chen]

On Thu, Nov 30, 2017 at 08:20:08PM +0100, Maciej S. Szmigiero wrote:

> In the AC'97 mode we have to differentiate two things:
> 1) Bit width of the physical AC'97 interface ("AC-link"),
> 2) Bit width of samples that are accepted during a playback and output
> during a recording by the SSI (and, so by the sound card that it driven
> by this CPU).
> 
> Bit width of the physical AC'97 interface is fixed at 20 bits per sample
> (both in playback and capture direction).
> 
> Bit width of samples that are accepted (or produced) by the SSI could
> be set in its STCCR and SRCCR registers (although in the AC'97 mode
> only settings of either 16 or 20 bits are possible).
> Each direction could be set independently from the other one.

I checked the reference manual regarding the AC97 configurations.
It seems that AC97 sets SYNC bit in SCR register. However, unlike
I2S which only uses STCCR for both TX and RX, AC97 does use STCCR
and SRCCR separately. So bypassing SRCCR if SYNC bit is set isn't
correct for AC97 cases.

I will clean up the driver a bit and I think the change would be
highly related to AC97 code. So I'll later need you review/test.

> Regarding a sample rate in AC'97 mode its effective value isn't really
> controlled by the CPU (that is, SSI), but by a CODEC since it is
> the CODEC which tells the CPU when it should send a next sample for
> playback and when a next capture sample is ready.
> There are no problems if they are different (as long as the CODEC
> supports this, naturally, but it's up to its driver to restrict the
> sample rate space accordingly).

It's because CODEC drives the bit clock and framesync clock, isn't
it? Could SSI act as the clock master side? It doesn't seem to be
configurable to do so according to the reference manual though.

> A comment above "fsl,ssi-asynchronous" property says that it is about
> whether "the RX and the TX clocks [are] locked".
> They are on the physical AC'97 interface, but they aren't on the logical
> playback / capture interface.
> IMHO, this configurable property simply makes no sense in the AC'97
> mode.

The property is more on the hardware level. And we should refer to
the DT binding doc:
    fsl,ssi-asynchronous:
         If specified, the SSI is to be programmed in asynchronous
         mode.  In this mode, pins SRCK, STCK, SRFS, and STFS must
         all be connected to valid signals.  In synchronous mode,
         SRCK and SRFS are ignored.  Asynchronous mode allows
         playback and capture to use different sample sizes and
         sample rates.  Some drivers may require that SRCK and STCK
         be connected together, and SRFS and STFS be connected
         together.  This would still allow different sample sizes,
         but not different sample rates.

If AC97 doesn't need SRFS and SRCK at all, it's fine to ignore it.

Thanks
Nic

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

On 01.12.2017 00:53, Nicolin Chen wrote:
> On Thu, Nov 30, 2017 at 08:20:08PM +0100, Maciej S. Szmigiero wrote:
> 
>> In the AC'97 mode we have to differentiate two things:
>> 1) Bit width of the physical AC'97 interface ("AC-link"),
>> 2) Bit width of samples that are accepted during a playback and output
>> during a recording by the SSI (and, so by the sound card that it driven
>> by this CPU).
>>
>> Bit width of the physical AC'97 interface is fixed at 20 bits per sample
>> (both in playback and capture direction).
>>
>> Bit width of samples that are accepted (or produced) by the SSI could
>> be set in its STCCR and SRCCR registers (although in the AC'97 mode
>> only settings of either 16 or 20 bits are possible).
>> Each direction could be set independently from the other one.
> 
> I checked the reference manual regarding the AC97 configurations.
> It seems that AC97 sets SYNC bit in SCR register. However, unlike
> I2S which only uses STCCR for both TX and RX, AC97 does use STCCR
> and SRCCR separately. So bypassing SRCCR if SYNC bit is set isn't
> correct for AC97 cases.

I assume by 'SYNC' bit you mean the 'SYN' bit, not the 'SYNC_TX_FS' one.

You are right, we can't ignore SRCCR only because 'SYN' is set (at least
not in the AC'97 mode).
That's why the current driver doesn't do it this way, the
'symmetric_rates' flag is used instead.

> 
> I will clean up the driver a bit and I think the change would be
> highly related to AC97 code. So I'll later need you review/test.

OK.

>From my perspective it would be great if the whole cleanup was in one
series, so the whole testing doesn't need to be repeated per patch
(it involves a lot of manual work).

>> Regarding a sample rate in AC'97 mode its effective value isn't really
>> controlled by the CPU (that is, SSI), but by a CODEC since it is
>> the CODEC which tells the CPU when it should send a next sample for
>> playback and when a next capture sample is ready.
>> There are no problems if they are different (as long as the CODEC
>> supports this, naturally, but it's up to its driver to restrict the
>> sample rate space accordingly).
> 
> It's because CODEC drives the bit clock and framesync clock, isn't
> it? 

Strictly speaking, the frame sync is driven by the controller (SSI),
but it is simply the CODEC-provided bit clock divided by 256.
And the CODEC-provided bit clock is fixed at 12.288MHz by the AC'97
specs.

But every frame from CODEC also has 'TAG' bits which tell the
controller whether this frame contains valid capture samples or not.
If the capture sample rate currently programmed in CODEC is less
than 48kHz (the frame rate) it simply means that some of incoming
frames will contain 'TAG' bits indicating that these frames do not
contain valid capture samples (for example, if the capture rate is
24kHz then only half of the frames, on average, will be marked by CODEC
as containing valid capture samples).

The situation with playback is similar: the frame from CODEC also has
'SLOTREQ' bits which tell the controller if it should send playback
samples (and which) in the next frame - for example, if the playback
rate is 24kHz then in half of the frames, on average, the CODEC will
request playback samples.

Hope it is clear now.

> Could SSI act as the clock master side? It doesn't seem to be
> configurable to do so according to the reference manual though.

In the AC'97 mode?
I couldn't find in the AC'97 specs any reference to a mode of
operation in which the controller generates the bit clock.

Also, the SSI manual (from imx6q) states that in the AC'97 mode:
"TXDIR bit (SSI.STCR[5]) is forced to 0 internally to select external
generated bit clock".
This suggests it isn't possible to make the SSI generate the bit clock
in this mode of operation.

> 
>> A comment above "fsl,ssi-asynchronous" property says that it is about
>> whether "the RX and the TX clocks [are] locked".
>> They are on the physical AC'97 interface, but they aren't on the logical
>> playback / capture interface.
>> IMHO, this configurable property simply makes no sense in the AC'97
>> mode.
> 
> The property is more on the hardware level. And we should refer to
> the DT binding doc:
>     fsl,ssi-asynchronous:
>          If specified, the SSI is to be programmed in asynchronous
>          mode.  In this mode, pins SRCK, STCK, SRFS, and STFS must
>          all be connected to valid signals.  In synchronous mode,
>          SRCK and SRFS are ignored.  Asynchronous mode allows
>          playback and capture to use different sample sizes and
>          sample rates.  Some drivers may require that SRCK and STCK
>          be connected together, and SRFS and STFS be connected
>          together.  This would still allow different sample sizes,
>          but not different sample rates.
> 
> If AC97 doesn't need SRFS and SRCK at all, it's fine to ignore it.

Yes, the AC'97 mode doesn't use SRFS and SRCK at all.

> Thanks
> Nic

Best regards,
Maciej

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

On 01.12.2017 00:53, Nicolin Chen wrote:
> On Thu, Nov 30, 2017 at 08:20:08PM +0100, Maciej S. Szmigiero wrote:
> 
>> In the AC'97 mode we have to differentiate two things:
>> 1) Bit width of the physical AC'97 interface ("AC-link"),
>> 2) Bit width of samples that are accepted during a playback and output
>> during a recording by the SSI (and, so by the sound card that it driven
>> by this CPU).
>>
>> Bit width of the physical AC'97 interface is fixed at 20 bits per sample
>> (both in playback and capture direction).
>>
>> Bit width of samples that are accepted (or produced) by the SSI could
>> be set in its STCCR and SRCCR registers (although in the AC'97 mode
>> only settings of either 16 or 20 bits are possible).
>> Each direction could be set independently from the other one.
> 
> I checked the reference manual regarding the AC97 configurations.
> It seems that AC97 sets SYNC bit in SCR register. However, unlike
> I2S which only uses STCCR for both TX and RX, AC97 does use STCCR
> and SRCCR separately. So bypassing SRCCR if SYNC bit is set isn't
> correct for AC97 cases.

I assume by 'SYNC' bit you mean the 'SYN' bit, not the 'SYNC_TX_FS' one.

You are right, we can't ignore SRCCR only because 'SYN' is set (at least
not in the AC'97 mode).
That's why the current driver doesn't do it this way, the
'symmetric_rates' flag is used instead.

> 
> I will clean up the driver a bit and I think the change would be
> highly related to AC97 code. So I'll later need you review/test.

OK.

>From my perspective it would be great if the whole cleanup was in one
series, so the whole testing doesn't need to be repeated per patch
(it involves a lot of manual work).

>> Regarding a sample rate in AC'97 mode its effective value isn't really
>> controlled by the CPU (that is, SSI), but by a CODEC since it is
>> the CODEC which tells the CPU when it should send a next sample for
>> playback and when a next capture sample is ready.
>> There are no problems if they are different (as long as the CODEC
>> supports this, naturally, but it's up to its driver to restrict the
>> sample rate space accordingly).
> 
> It's because CODEC drives the bit clock and framesync clock, isn't
> it? 

Strictly speaking, the frame sync is driven by the controller (SSI),
but it is simply the CODEC-provided bit clock divided by 256.
And the CODEC-provided bit clock is fixed at 12.288MHz by the AC'97
specs.

But every frame from CODEC also has 'TAG' bits which tell the
controller whether this frame contains valid capture samples or not.
If the capture sample rate currently programmed in CODEC is less
than 48kHz (the frame rate) it simply means that some of incoming
frames will contain 'TAG' bits indicating that these frames do not
contain valid capture samples (for example, if the capture rate is
24kHz then only half of the frames, on average, will be marked by CODEC
as containing valid capture samples).

The situation with playback is similar: the frame from CODEC also has
'SLOTREQ' bits which tell the controller if it should send playback
samples (and which) in the next frame - for example, if the playback
rate is 24kHz then in half of the frames, on average, the CODEC will
request playback samples.

Hope it is clear now.

> Could SSI act as the clock master side? It doesn't seem to be
> configurable to do so according to the reference manual though.

In the AC'97 mode?
I couldn't find in the AC'97 specs any reference to a mode of
operation in which the controller generates the bit clock.

Also, the SSI manual (from imx6q) states that in the AC'97 mode:
"TXDIR bit (SSI.STCR[5]) is forced to 0 internally to select external
generated bit clock".
This suggests it isn't possible to make the SSI generate the bit clock
in this mode of operation.

> 
>> A comment above "fsl,ssi-asynchronous" property says that it is about
>> whether "the RX and the TX clocks [are] locked".
>> They are on the physical AC'97 interface, but they aren't on the logical
>> playback / capture interface.
>> IMHO, this configurable property simply makes no sense in the AC'97
>> mode.
> 
> The property is more on the hardware level. And we should refer to
> the DT binding doc:
>     fsl,ssi-asynchronous:
>          If specified, the SSI is to be programmed in asynchronous
>          mode.  In this mode, pins SRCK, STCK, SRFS, and STFS must
>          all be connected to valid signals.  In synchronous mode,
>          SRCK and SRFS are ignored.  Asynchronous mode allows
>          playback and capture to use different sample sizes and
>          sample rates.  Some drivers may require that SRCK and STCK
>          be connected together, and SRFS and STFS be connected
>          together.  This would still allow different sample sizes,
>          but not different sample rates.
> 
> If AC97 doesn't need SRFS and SRCK at all, it's fine to ignore it.

Yes, the AC'97 mode doesn't use SRFS and SRCK at all.

> Thanks
> Nic

Best regards,
Maciej

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Nicolin Chen]

On Fri, Dec 01, 2017 at 02:02:29AM +0100, Maciej S. Szmigiero wrote:

> > I will clean up the driver a bit and I think the change would be
> > highly related to AC97 code. So I'll later need you review/test.

> From my perspective it would be great if the whole cleanup was in one
> series, so the whole testing doesn't need to be repeated per patch
> (it involves a lot of manual work).

Understood.

> >> Regarding a sample rate in AC'97 mode its effective value isn't really
> >> controlled by the CPU (that is, SSI), but by a CODEC since it is
> >> the CODEC which tells the CPU when it should send a next sample for
> >> playback and when a next capture sample is ready.
> >> There are no problems if they are different (as long as the CODEC
> >> supports this, naturally, but it's up to its driver to restrict the
> >> sample rate space accordingly).
> > 
> > It's because CODEC drives the bit clock and framesync clock, isn't
> > it? 
> 
> Strictly speaking, the frame sync is driven by the controller (SSI),
> but it is simply the CODEC-provided bit clock divided by 256.
> And the CODEC-provided bit clock is fixed at 12.288MHz by the AC'97
> specs.
> 
> But every frame from CODEC also has 'TAG' bits which tell the
> controller whether this frame contains valid capture samples or not.
> If the capture sample rate currently programmed in CODEC is less
> than 48kHz (the frame rate) it simply means that some of incoming
> frames will contain 'TAG' bits indicating that these frames do not
> contain valid capture samples (for example, if the capture rate is
> 24kHz then only half of the frames, on average, will be marked by CODEC
> as containing valid capture samples).
> 
> The situation with playback is similar: the frame from CODEC also has
> 'SLOTREQ' bits which tell the controller if it should send playback
> samples (and which) in the next frame - for example, if the playback
> rate is 24kHz then in half of the frames, on average, the CODEC will
> request playback samples.
> 
> Hope it is clear now.

Thanks for the explain. It's clear now.

Nicolin

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Nicolin Chen]

On Fri, Dec 01, 2017 at 02:02:29AM +0100, Maciej S. Szmigiero wrote:

> > I will clean up the driver a bit and I think the change would be
> > highly related to AC97 code. So I'll later need you review/test.

> From my perspective it would be great if the whole cleanup was in one
> series, so the whole testing doesn't need to be repeated per patch
> (it involves a lot of manual work).

Understood.

> >> Regarding a sample rate in AC'97 mode its effective value isn't really
> >> controlled by the CPU (that is, SSI), but by a CODEC since it is
> >> the CODEC which tells the CPU when it should send a next sample for
> >> playback and when a next capture sample is ready.
> >> There are no problems if they are different (as long as the CODEC
> >> supports this, naturally, but it's up to its driver to restrict the
> >> sample rate space accordingly).
> > 
> > It's because CODEC drives the bit clock and framesync clock, isn't
> > it? 
> 
> Strictly speaking, the frame sync is driven by the controller (SSI),
> but it is simply the CODEC-provided bit clock divided by 256.
> And the CODEC-provided bit clock is fixed at 12.288MHz by the AC'97
> specs.
> 
> But every frame from CODEC also has 'TAG' bits which tell the
> controller whether this frame contains valid capture samples or not.
> If the capture sample rate currently programmed in CODEC is less
> than 48kHz (the frame rate) it simply means that some of incoming
> frames will contain 'TAG' bits indicating that these frames do not
> contain valid capture samples (for example, if the capture rate is
> 24kHz then only half of the frames, on average, will be marked by CODEC
> as containing valid capture samples).
> 
> The situation with playback is similar: the frame from CODEC also has
> 'SLOTREQ' bits which tell the controller if it should send playback
> samples (and which) in the next frame - for example, if the playback
> rate is 24kHz then in half of the frames, on average, the CODEC will
> request playback samples.
> 
> Hope it is clear now.

Thanks for the explain. It's clear now.

Nicolin