[PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Daniel Baluta]

From SAI datasheet:

CHMOD, configures if transmit data pins are configured for TDM mode
or Output mode.
	* (0) TDM mode, transmit data pins are tri-stated when slots are
	masked or channels are disabled.
	* (1) Output mode, transmit data pins are never tri-stated and
	will output zero when slots are masked or channels are disabled.

When data pins are tri-stated, there is noise on some channels
when FS clock value is high and data is read while fsclk is
transitioning from high to low.

Fix this by setting CHMOD to Output Mode so that pins will output zero
when slots are masked or channels are disabled.

Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/fsl/fsl_sai.c | 15 ++++++++++++---
 sound/soc/fsl/fsl_sai.h |  2 ++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index e896b577b1f7..b9daab0eb6eb 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -467,6 +467,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 
 	val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
 
+	/*
+	 * set CHMOD to Output Mode so that transmit data pins will
+	 * output zero when slots are masked or channels are disabled
+	 */
+	val_cr4 |= FSL_SAI_CR4_CHMOD;
+
 	/*
 	 * For SAI master mode, when Tx(Rx) sync with Rx(Tx) clock, Rx(Tx) will
 	 * generate bclk and frame clock for Tx(Rx), we should set RCR4(TCR4),
@@ -477,7 +483,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	if (!sai->is_slave_mode) {
 		if (!sai->synchronous[TX] && sai->synchronous[RX] && !tx) {
 			regmap_update_bits(sai->regmap, FSL_SAI_TCR4(ofs),
-				FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+				FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+				FSL_SAI_CR4_CHMOD_MASK,
 				val_cr4);
 			regmap_update_bits(sai->regmap, FSL_SAI_TCR5(ofs),
 				FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
@@ -486,7 +493,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 				~0UL - ((1 << channels) - 1));
 		} else if (!sai->synchronous[RX] && sai->synchronous[TX] && tx) {
 			regmap_update_bits(sai->regmap, FSL_SAI_RCR4(ofs),
-				FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+				FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+				FSL_SAI_CR4_CHMOD_MASK,
 				val_cr4);
 			regmap_update_bits(sai->regmap, FSL_SAI_RCR5(ofs),
 				FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
@@ -497,7 +505,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx, ofs),
-			   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+			   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+			   FSL_SAI_CR4_CHMOD_MASK,
 			   val_cr4);
 	regmap_update_bits(sai->regmap, FSL_SAI_xCR5(tx, ofs),
 			   FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index f96f8d97489d..1e3b4a6889a8 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -119,6 +119,8 @@
 #define FSL_SAI_CR4_FRSZ_MASK	(0x1f << 16)
 #define FSL_SAI_CR4_SYWD(x)	(((x) - 1) << 8)
 #define FSL_SAI_CR4_SYWD_MASK	(0x1f << 8)
+#define FSL_SAI_CR4_CHMOD	BIT(5)
+#define FSL_SAI_CR4_CHMOD_MASK	GENMASK(5, 5)
 #define FSL_SAI_CR4_MF		BIT(4)
 #define FSL_SAI_CR4_FSE		BIT(3)
 #define FSL_SAI_CR4_FSP		BIT(1)
-- 
2.17.1

Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Cosmin-Gabriel Samoila]

Looks good to me!

Best regards,
Cosmin

On Sat, 2019-08-31 at 01:55 +0300, Daniel Baluta wrote:
> From SAI datasheet:
> 
> CHMOD, configures if transmit data pins are configured for TDM mode
> or Output mode.
> 	* (0) TDM mode, transmit data pins are tri-stated when slots
> are
> 	masked or channels are disabled.
> 	* (1) Output mode, transmit data pins are never tri-stated and
> 	will output zero when slots are masked or channels are
> disabled.
> 
> When data pins are tri-stated, there is noise on some channels
> when FS clock value is high and data is read while fsclk is
> transitioning from high to low.
> 
> Fix this by setting CHMOD to Output Mode so that pins will output
> zero
> when slots are masked or channels are disabled.
> 
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  sound/soc/fsl/fsl_sai.c | 15 ++++++++++++---
>  sound/soc/fsl/fsl_sai.h |  2 ++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index e896b577b1f7..b9daab0eb6eb 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -467,6 +467,12 @@ static int fsl_sai_hw_params(struct
> snd_pcm_substream *substream,
>  
>  	val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
>  
> +	/*
> +	 * set CHMOD to Output Mode so that transmit data pins will
> +	 * output zero when slots are masked or channels are disabled
> +	 */
> +	val_cr4 |= FSL_SAI_CR4_CHMOD;
> +
>  	/*
>  	 * For SAI master mode, when Tx(Rx) sync with Rx(Tx) clock,
> Rx(Tx) will
>  	 * generate bclk and frame clock for Tx(Rx), we should set
> RCR4(TCR4),
> @@ -477,7 +483,8 @@ static int fsl_sai_hw_params(struct
> snd_pcm_substream *substream,
>  	if (!sai->is_slave_mode) {
>  		if (!sai->synchronous[TX] && sai->synchronous[RX] &&
> !tx) {
>  			regmap_update_bits(sai->regmap,
> FSL_SAI_TCR4(ofs),
> -				FSL_SAI_CR4_SYWD_MASK |
> FSL_SAI_CR4_FRSZ_MASK,
> +				FSL_SAI_CR4_SYWD_MASK |
> FSL_SAI_CR4_FRSZ_MASK |
> +				FSL_SAI_CR4_CHMOD_MASK,
>  				val_cr4);
>  			regmap_update_bits(sai->regmap,
> FSL_SAI_TCR5(ofs),
>  				FSL_SAI_CR5_WNW_MASK |
> FSL_SAI_CR5_W0W_MASK |
> @@ -486,7 +493,8 @@ static int fsl_sai_hw_params(struct
> snd_pcm_substream *substream,
>  				~0UL - ((1 << channels) - 1));
>  		} else if (!sai->synchronous[RX] && sai-
> >synchronous[TX] && tx) {
>  			regmap_update_bits(sai->regmap,
> FSL_SAI_RCR4(ofs),
> -				FSL_SAI_CR4_SYWD_MASK |
> FSL_SAI_CR4_FRSZ_MASK,
> +				FSL_SAI_CR4_SYWD_MASK |
> FSL_SAI_CR4_FRSZ_MASK |
> +				FSL_SAI_CR4_CHMOD_MASK,
>  				val_cr4);
>  			regmap_update_bits(sai->regmap,
> FSL_SAI_RCR5(ofs),
>  				FSL_SAI_CR5_WNW_MASK |
> FSL_SAI_CR5_W0W_MASK |
> @@ -497,7 +505,8 @@ static int fsl_sai_hw_params(struct
> snd_pcm_substream *substream,
>  	}
>  
>  	regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx, ofs),
> -			   FSL_SAI_CR4_SYWD_MASK |
> FSL_SAI_CR4_FRSZ_MASK,
> +			   FSL_SAI_CR4_SYWD_MASK |
> FSL_SAI_CR4_FRSZ_MASK |
> +			   FSL_SAI_CR4_CHMOD_MASK,
>  			   val_cr4);
>  	regmap_update_bits(sai->regmap, FSL_SAI_xCR5(tx, ofs),
>  			   FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK
> |
> diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> index f96f8d97489d..1e3b4a6889a8 100644
> --- a/sound/soc/fsl/fsl_sai.h
> +++ b/sound/soc/fsl/fsl_sai.h
> @@ -119,6 +119,8 @@
>  #define FSL_SAI_CR4_FRSZ_MASK	(0x1f << 16)
>  #define FSL_SAI_CR4_SYWD(x)	(((x) - 1) << 8)
>  #define FSL_SAI_CR4_SYWD_MASK	(0x1f << 8)
> +#define FSL_SAI_CR4_CHMOD	BIT(5)
> +#define FSL_SAI_CR4_CHMOD_MASK	GENMASK(5, 5)
>  #define FSL_SAI_CR4_MF		BIT(4)
>  #define FSL_SAI_CR4_FSE		BIT(3)
>  #define FSL_SAI_CR4_FSP		BIT(1)

Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On Sat, Aug 31, 2019 at 01:55:14AM +0300, Daniel Baluta wrote:

> Fix this by setting CHMOD to Output Mode so that pins will output zero
> when slots are masked or channels are disabled.

This patch seems to do this unconditionally.  This is fine for
configurations where the SoC is the only thing driving the bus but will
mean that for TDM configurations where something else also drives some
of the slots we'll end up with both devices driving simultaneously.  The
safest thing would be to set this only if TDM isn't configured.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Daniel Baluta]

On Mon, Sep 2, 2019 at 3:42 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, Aug 31, 2019 at 01:55:14AM +0300, Daniel Baluta wrote:
>
> > Fix this by setting CHMOD to Output Mode so that pins will output zero
> > when slots are masked or channels are disabled.
>
> This patch seems to do this unconditionally.  This is fine for
> configurations where the SoC is the only thing driving the bus but will
> mean that for TDM configurations where something else also drives some
> of the slots we'll end up with both devices driving simultaneously.  The
> safest thing would be to set this only if TDM isn't configured.

I thought that the SAI IP is the single owner of the audio data lines,
so even in TDM
mode SAI IP (which is inside SoC) is the only one adding data on the bus.

Now, you say that there could be two devices driving some of he masked
slots right?
I'm not sure how to really figure out that SAI is running in TDM mode.

RM says:

When enabled, the SAI continuously transmits and/or receives frames of
data. Each
frame consists of a fixed number of words and each word consists of a
fixed number of
bits. Within each frame, any given word can be masked causing the
receiver to ignore
that word and the transmitter to tri-state for the duration of that word.

Will try to ask IP designer about this, thanks a lot for your comment!

Daniel.

Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Mon, Sep 02, 2019 at 04:35:56PM +0300, Daniel Baluta wrote:
> On Mon, Sep 2, 2019 at 3:42 PM Mark Brown <broonie@kernel.org> wrote:

> > This patch seems to do this unconditionally.  This is fine for
> > configurations where the SoC is the only thing driving the bus but will
> > mean that for TDM configurations where something else also drives some
> > of the slots we'll end up with both devices driving simultaneously.  The
> > safest thing would be to set this only if TDM isn't configured.

> I thought that the SAI IP is the single owner of the audio data lines,
> so even in TDM
> mode SAI IP (which is inside SoC) is the only one adding data on the bus.

> Now, you say that there could be two devices driving some of he masked
> slots right?

Doing that is the major point of TDM modes.  It could even be another
SAI on the same bus.

> I'm not sure how to really figure out that SAI is running in TDM mode.

As a first approximation you could just check if set_tdm_slots() has
been called, it might still be the only device but it's a good first
guess.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Shengjiu Wang]

Transmit data pins will output zero when slots are masked or channels
are disabled. In CHMOD TDM mode, transmit data pins are tri-stated when
slots are masked or channels are disabled. When data pins are tri-stated,
there is noise on some channels when FS clock value is high and data is
read while fsclk is transitioning from high to low.

Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 sound/soc/fsl/fsl_sai.c | 12 ++++++++++--
 sound/soc/fsl/fsl_sai.h |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 62c5fdb678fc..33b194a5c1dc 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -486,6 +486,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 
 	val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
 
+	/* Output Mode - data pins transmit 0 when slots are masked
+	 * or channels are disabled
+	 */
+	if (tx)
+		val_cr4 |= FSL_SAI_CR4_CHMOD;
+
 	/*
 	 * For SAI master mode, when Tx(Rx) sync with Rx(Tx) clock, Rx(Tx) will
 	 * generate bclk and frame clock for Tx(Rx), we should set RCR4(TCR4),
@@ -494,7 +500,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 
 	if (!sai->is_slave_mode && fsl_sai_dir_is_synced(sai, adir)) {
 		regmap_update_bits(sai->regmap, FSL_SAI_xCR4(!tx, ofs),
-				   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+				   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+				   FSL_SAI_CR4_CHMOD_MASK,
 				   val_cr4);
 		regmap_update_bits(sai->regmap, FSL_SAI_xCR5(!tx, ofs),
 				   FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
@@ -502,7 +509,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx, ofs),
-			   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+			   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+			   FSL_SAI_CR4_CHMOD_MASK,
 			   val_cr4);
 	regmap_update_bits(sai->regmap, FSL_SAI_xCR5(tx, ofs),
 			   FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 6aba7d28f5f3..19cd4e1bbff9 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -119,6 +119,8 @@
 #define FSL_SAI_CR4_FRSZ_MASK	(0x1f << 16)
 #define FSL_SAI_CR4_SYWD(x)	(((x) - 1) << 8)
 #define FSL_SAI_CR4_SYWD_MASK	(0x1f << 8)
+#define FSL_SAI_CR4_CHMOD       BIT(5)
+#define FSL_SAI_CR4_CHMOD_MASK  BIT(5)
 #define FSL_SAI_CR4_MF		BIT(4)
 #define FSL_SAI_CR4_FSE		BIT(3)
 #define FSL_SAI_CR4_FSP		BIT(1)
-- 
2.27.0

Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Nicolin Chen]

On Thu, Sep 03, 2020 at 11:09:15AM +0800, Shengjiu Wang wrote:
> Transmit data pins will output zero when slots are masked or channels
> are disabled. In CHMOD TDM mode, transmit data pins are tri-stated when
> slots are masked or channels are disabled. When data pins are tri-stated,
> there is noise on some channels when FS clock value is high and data is
> read while fsclk is transitioning from high to low.
> 
> Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>

Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>

Though one nit inline:

> ---
>  sound/soc/fsl/fsl_sai.c | 12 ++++++++++--
>  sound/soc/fsl/fsl_sai.h |  2 ++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 62c5fdb678fc..33b194a5c1dc 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -486,6 +486,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
>  
>  	val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
>  
> +	/* Output Mode - data pins transmit 0 when slots are masked
> +	 * or channels are disabled
> +	 */

Coding style for multi-line comments. Yet, probably can simplify?

	/* Set to output mode to avoid tri-stated data pins */

Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

[Shengjiu Wang]

On Thu, Sep 3, 2020 at 11:42 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Thu, Sep 03, 2020 at 11:09:15AM +0800, Shengjiu Wang wrote:
> > Transmit data pins will output zero when slots are masked or channels
> > are disabled. In CHMOD TDM mode, transmit data pins are tri-stated when
> > slots are masked or channels are disabled. When data pins are tri-stated,
> > there is noise on some channels when FS clock value is high and data is
> > read while fsclk is transitioning from high to low.
> >
> > Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>
> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
>
> Though one nit inline:
>
> > ---
> >  sound/soc/fsl/fsl_sai.c | 12 ++++++++++--
> >  sound/soc/fsl/fsl_sai.h |  2 ++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index 62c5fdb678fc..33b194a5c1dc 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
> > @@ -486,6 +486,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> >
> >       val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
> >
> > +     /* Output Mode - data pins transmit 0 when slots are masked
> > +      * or channels are disabled
> > +      */
>
> Coding style for multi-line comments. Yet, probably can simplify?
>
>         /* Set to output mode to avoid tri-stated data pins */
Ok, will update in v2.

best regards
wang shengjiu