ASoC: fsl_sai: Set SAI Channel Mode to Output Mode
diff mbox series

Message ID 20190830225514.5283-1-daniel.baluta@nxp.com
State New
Headers show
Series
  • ASoC: fsl_sai: Set SAI Channel Mode to Output Mode
Related show

Commit Message

Daniel Baluta Aug. 30, 2019, 10:55 p.m. UTC
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(-)

Comments

Cosmin-Gabriel Samoila Sept. 1, 2019, 7:49 p.m. UTC | #1
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)
Mark Brown Sept. 2, 2019, 12:39 p.m. UTC | #2
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.
Daniel Baluta Sept. 2, 2019, 1:35 p.m. UTC | #3
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.
Mark Brown Sept. 2, 2019, 3:52 p.m. UTC | #4
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.

Patch
diff mbox series

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)