[V2] ASoC: fsl_esai: fix channel swap issue when stream starts
diff mbox series

Message ID e31c31acb3c62d16e6b32682e8226c7dbc05f147.1551062751.git.shengjiu.wang@nxp.com
State New
Headers show
Series
  • [V2] ASoC: fsl_esai: fix channel swap issue when stream starts
Related show

Commit Message

Shengjiu Wang Feb. 25, 2019, 2:48 a.m. UTC
There is very low possibility ( < 0.1% ) that channel swap happened
in beginning when multi output/input pin is enabled. The issue is
that hardware can't send data to correct pin in the beginning with
the normal enable flow.

This is hardware issue, but there is no errata, the workaround flow
is that: Each time playback/recording, firstly clear the xSMA/xSMB,
then enable TE/RE, then enable xSMB and xSMA (xSMB must be enabled
before xSMA). Which is to use the xSMA as the trigger start register,
previously the xCR_TE or xCR_RE is the bit for starting.

Fixes 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
Cc: <stable@vger.kernel.org>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
Changes in v2
- update commit comments.

 sound/soc/fsl/fsl_esai.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

Comments

Nicolin Chen Feb. 25, 2019, 9:14 p.m. UTC | #1
On Mon, Feb 25, 2019 at 02:48:18AM +0000, S.j. Wang wrote:
> There is very low possibility ( < 0.1% ) that channel swap happened
> in beginning when multi output/input pin is enabled. The issue is
> that hardware can't send data to correct pin in the beginning with
> the normal enable flow.
> 
> This is hardware issue, but there is no errata, the workaround flow
> is that: Each time playback/recording, firstly clear the xSMA/xSMB,
> then enable TE/RE, then enable xSMB and xSMA (xSMB must be enabled
> before xSMA). Which is to use the xSMA as the trigger start register,
> previously the xCR_TE or xCR_RE is the bit for starting.
> 
> Fixes 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> Changes in v2
> - update commit comments.
> 
>  sound/soc/fsl/fsl_esai.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index afe67c865330..23bd0ad4ac31 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -54,6 +54,8 @@ struct fsl_esai {
>  	u32 fifo_depth;
>  	u32 slot_width;
>  	u32 slots;
> +	u32 tx_mask;
> +	u32 rx_mask;
>  	u32 hck_rate[2];
>  	u32 sck_rate[2];
>  	bool hck_dir[2];
> @@ -361,21 +363,13 @@ static int fsl_esai_set_dai_tdm_slot(struct snd_soc_dai *dai, u32 tx_mask,
>  	regmap_update_bits(esai_priv->regmap, REG_ESAI_TCCR,
>  			   ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
>  
> -	regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
> -			   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(tx_mask));
> -	regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
> -			   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(tx_mask));
> -
>  	regmap_update_bits(esai_priv->regmap, REG_ESAI_RCCR,
>  			   ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
>  
> -	regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
> -			   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(rx_mask));
> -	regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
> -			   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(rx_mask));
> -
>  	esai_priv->slot_width = slot_width;
>  	esai_priv->slots = slots;
> +	esai_priv->tx_mask    = tx_mask;
> +	esai_priv->rx_mask    = rx_mask;

The two masks only got values here. If a machine driver doesn't
have a set_dai_tdm_slot() call, they will be remained as 0 and
then will seemly clean those four registers.

Thanks
Nicolin

>  
>  	return 0;
>  }
> @@ -596,6 +590,7 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
>  	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>  	u8 i, channels = substream->runtime->channels;
>  	u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
> +	u32 mask;
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> @@ -611,12 +606,23 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
>  		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
>  				   tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK,
>  				   tx ? ESAI_xCR_TE(pins) : ESAI_xCR_RE(pins));
> +		mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
> +
> +		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
> +				   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
> +		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
> +				   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
> +
>  		break;
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
>  				   tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
> +		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
> +				   ESAI_xSMA_xS_MASK, 0);
> +		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
> +				   ESAI_xSMB_xS_MASK, 0);
>  
>  		/* Disable and reset FIFO */
>  		regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
> @@ -906,6 +912,12 @@ static int fsl_esai_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/* Clear the TSMA, TSMB, RSMA, RSMB */
> +	regmap_write(esai_priv->regmap, REG_ESAI_TSMA, 0);
> +	regmap_write(esai_priv->regmap, REG_ESAI_TSMB, 0);
> +	regmap_write(esai_priv->regmap, REG_ESAI_RSMA, 0);
> +	regmap_write(esai_priv->regmap, REG_ESAI_RSMB, 0);
> +
>  	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_esai_component,
>  					      &fsl_esai_dai, 1);
>  	if (ret) {
> -- 
> 1.9.1
>
Shengjiu Wang Feb. 26, 2019, 2:01 a.m. UTC | #2
> 
> On Mon, Feb 25, 2019 at 02:48:18AM +0000, S.j. Wang wrote:
> > There is very low possibility ( < 0.1% ) that channel swap happened in
> > beginning when multi output/input pin is enabled. The issue is that
> > hardware can't send data to correct pin in the beginning with the
> > normal enable flow.
> >
> > This is hardware issue, but there is no errata, the workaround flow is
> > that: Each time playback/recording, firstly clear the xSMA/xSMB, then
> > enable TE/RE, then enable xSMB and xSMA (xSMB must be enabled
> before
> > xSMA). Which is to use the xSMA as the trigger start register,
> > previously the xCR_TE or xCR_RE is the bit for starting.
> >
> > Fixes 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> > Cc: <stable@vger.kernel.org>
> > Reviewed-by: Fabio Estevam <festevam@gmail.com>
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > Changes in v2
> > - update commit comments.
> >
> >  sound/soc/fsl/fsl_esai.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > afe67c865330..23bd0ad4ac31 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -54,6 +54,8 @@ struct fsl_esai {
> >  	u32 fifo_depth;
> >  	u32 slot_width;
> >  	u32 slots;
> > +	u32 tx_mask;
> > +	u32 rx_mask;
> >  	u32 hck_rate[2];
> >  	u32 sck_rate[2];
> >  	bool hck_dir[2];
> > @@ -361,21 +363,13 @@ static int fsl_esai_set_dai_tdm_slot(struct
> snd_soc_dai *dai, u32 tx_mask,
> >  	regmap_update_bits(esai_priv->regmap, REG_ESAI_TCCR,
> >  			   ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
> >
> > -	regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
> > -			   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(tx_mask));
> > -	regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
> > -			   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(tx_mask));
> > -
> >  	regmap_update_bits(esai_priv->regmap, REG_ESAI_RCCR,
> >  			   ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
> >
> > -	regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
> > -			   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(rx_mask));
> > -	regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
> > -			   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(rx_mask));
> > -
> >  	esai_priv->slot_width = slot_width;
> >  	esai_priv->slots = slots;
> > +	esai_priv->tx_mask    = tx_mask;
> > +	esai_priv->rx_mask    = rx_mask;
> 
> The two masks only got values here. If a machine driver doesn't have a
> set_dai_tdm_slot() call, they will be remained as 0 and then will seemly
> clean those four registers.
> 
> Thanks
> Nicolin

Then I think we need to add default value for tx_mask and rx_mask, that is
In the probe function to add:
esai_priv->tx_mask    =  0xFFFFFFFF;
esai_priv->rx_mask    =  0xFFFFFFFF;


best regards
wang shengjiu
> 
> >
> >  	return 0;
> >  }
> > @@ -596,6 +590,7 @@ static int fsl_esai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> >  	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> >  	u8 i, channels = substream->runtime->channels;
> >  	u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
> > +	u32 mask;
> >
> >  	switch (cmd) {
> >  	case SNDRV_PCM_TRIGGER_START:
> > @@ -611,12 +606,23 @@ static int fsl_esai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> >  		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> >  				   tx ? ESAI_xCR_TE_MASK :
> ESAI_xCR_RE_MASK,
> >  				   tx ? ESAI_xCR_TE(pins) :
> ESAI_xCR_RE(pins));
> > +		mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
> > +
> > +		regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMB(tx),
> > +				   ESAI_xSMB_xS_MASK,
> ESAI_xSMB_xS(mask));
> > +		regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMA(tx),
> > +				   ESAI_xSMA_xS_MASK,
> ESAI_xSMA_xS(mask));
> > +
> >  		break;
> >  	case SNDRV_PCM_TRIGGER_SUSPEND:
> >  	case SNDRV_PCM_TRIGGER_STOP:
> >  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> >  		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> >  				   tx ? ESAI_xCR_TE_MASK :
> ESAI_xCR_RE_MASK, 0);
> > +		regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMA(tx),
> > +				   ESAI_xSMA_xS_MASK, 0);
> > +		regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMB(tx),
> > +				   ESAI_xSMB_xS_MASK, 0);
> >
> >  		/* Disable and reset FIFO */
> >  		regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
> @@ -906,6
> > +912,12 @@ static int fsl_esai_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >
> > +	/* Clear the TSMA, TSMB, RSMA, RSMB */
> > +	regmap_write(esai_priv->regmap, REG_ESAI_TSMA, 0);
> > +	regmap_write(esai_priv->regmap, REG_ESAI_TSMB, 0);
> > +	regmap_write(esai_priv->regmap, REG_ESAI_RSMA, 0);
> > +	regmap_write(esai_priv->regmap, REG_ESAI_RSMB, 0);
> > +
> >  	ret = devm_snd_soc_register_component(&pdev->dev,
> &fsl_esai_component,
> >  					      &fsl_esai_dai, 1);
> >  	if (ret) {
> > --
> > 1.9.1
> >
Nicolin Chen Feb. 26, 2019, 2:33 a.m. UTC | #3
On Tue, Feb 26, 2019 at 02:01:14AM +0000, S.j. Wang wrote:
> > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > > afe67c865330..23bd0ad4ac31 100644
> > > --- a/sound/soc/fsl/fsl_esai.c
> > > +++ b/sound/soc/fsl/fsl_esai.c
> > > @@ -54,6 +54,8 @@ struct fsl_esai {
> > >  	u32 fifo_depth;
> > >  	u32 slot_width;
> > >  	u32 slots;
> > > +	u32 tx_mask;
> > > +	u32 rx_mask;

> > >  	esai_priv->slot_width = slot_width;
> > >  	esai_priv->slots = slots;
> > > +	esai_priv->tx_mask    = tx_mask;
> > > +	esai_priv->rx_mask    = rx_mask;
> > 
> > The two masks only got values here. If a machine driver doesn't have a
> > set_dai_tdm_slot() call, they will be remained as 0 and then will seemly
> > clean those four registers.
> > 
> Then I think we need to add default value for tx_mask and rx_mask, that is
> In the probe function to add:
> esai_priv->tx_mask    =  0xFFFFFFFF;
> esai_priv->rx_mask    =  0xFFFFFFFF;

Yea:) Please include them in v3.
Shengjiu Wang Feb. 26, 2019, 2:54 a.m. UTC | #4
> 
> On Tue, Feb 26, 2019 at 02:01:14AM +0000, S.j. Wang wrote:
> > > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> > > > index
> > > > afe67c865330..23bd0ad4ac31 100644
> > > > --- a/sound/soc/fsl/fsl_esai.c
> > > > +++ b/sound/soc/fsl/fsl_esai.c
> > > > @@ -54,6 +54,8 @@ struct fsl_esai {
> > > >  	u32 fifo_depth;
> > > >  	u32 slot_width;
> > > >  	u32 slots;
> > > > +	u32 tx_mask;
> > > > +	u32 rx_mask;
> 
> > > >  	esai_priv->slot_width = slot_width;
> > > >  	esai_priv->slots = slots;
> > > > +	esai_priv->tx_mask    = tx_mask;
> > > > +	esai_priv->rx_mask    = rx_mask;
> > >
> > > The two masks only got values here. If a machine driver doesn't have
> > > a
> > > set_dai_tdm_slot() call, they will be remained as 0 and then will
> > > seemly clean those four registers.
> > >
> > Then I think we need to add default value for tx_mask and rx_mask,
> > that is In the probe function to add:
> > esai_priv->tx_mask    =  0xFFFFFFFF;
> > esai_priv->rx_mask    =  0xFFFFFFFF;
> 
> Yea:) Please include them in v3.
Ok.

Patch
diff mbox series

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index afe67c865330..23bd0ad4ac31 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -54,6 +54,8 @@  struct fsl_esai {
 	u32 fifo_depth;
 	u32 slot_width;
 	u32 slots;
+	u32 tx_mask;
+	u32 rx_mask;
 	u32 hck_rate[2];
 	u32 sck_rate[2];
 	bool hck_dir[2];
@@ -361,21 +363,13 @@  static int fsl_esai_set_dai_tdm_slot(struct snd_soc_dai *dai, u32 tx_mask,
 	regmap_update_bits(esai_priv->regmap, REG_ESAI_TCCR,
 			   ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
 
-	regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
-			   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(tx_mask));
-	regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
-			   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(tx_mask));
-
 	regmap_update_bits(esai_priv->regmap, REG_ESAI_RCCR,
 			   ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
 
-	regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
-			   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(rx_mask));
-	regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
-			   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(rx_mask));
-
 	esai_priv->slot_width = slot_width;
 	esai_priv->slots = slots;
+	esai_priv->tx_mask    = tx_mask;
+	esai_priv->rx_mask    = rx_mask;
 
 	return 0;
 }
@@ -596,6 +590,7 @@  static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
 	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
 	u8 i, channels = substream->runtime->channels;
 	u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
+	u32 mask;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -611,12 +606,23 @@  static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
 		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
 				   tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK,
 				   tx ? ESAI_xCR_TE(pins) : ESAI_xCR_RE(pins));
+		mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
+
+		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
+				   ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
+		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
+				   ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+
 		break;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
 				   tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
+		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
+				   ESAI_xSMA_xS_MASK, 0);
+		regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
+				   ESAI_xSMB_xS_MASK, 0);
 
 		/* Disable and reset FIFO */
 		regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
@@ -906,6 +912,12 @@  static int fsl_esai_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Clear the TSMA, TSMB, RSMA, RSMB */
+	regmap_write(esai_priv->regmap, REG_ESAI_TSMA, 0);
+	regmap_write(esai_priv->regmap, REG_ESAI_TSMB, 0);
+	regmap_write(esai_priv->regmap, REG_ESAI_RSMA, 0);
+	regmap_write(esai_priv->regmap, REG_ESAI_RSMB, 0);
+
 	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_esai_component,
 					      &fsl_esai_dai, 1);
 	if (ret) {