| Message ID | 20190830225514.5283-1-daniel.baluta@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: fsl_sai: Set SAI Channel Mode to Output Mode | expand |
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)
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.
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.
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.
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)