| Message ID | f05c95e5371336a8e3b0c395dc4cef70922f0caa.1551248985.git.shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ff4e8c61b794a4bf6c854ab071a1abaaa80f358 |
| Headers | show |
| Series | [V4] ASoC: fsl_esai: fix channel swap issue when stream starts | expand |
Hi Mark
Can this patch be accepted? Or need I do any update?
Best regards
Wang shengjiu
>
> 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 commit 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/fsl_esai.c | 47 +++++++++++++++++++++++++++++++++++++---
> -------
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> afe67c865330..3623aa9a6f2e 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:
> @@ -608,15 +603,38 @@ static int fsl_esai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> for (i = 0; tx && i < channels; i++)
> regmap_write(esai_priv->regmap, REG_ESAI_ETDR,
> 0x0);
>
> + /*
> + * When set the TE/RE in the end of enablement flow, there
> + * will be channel swap issue for multi data line case.
> + * In order to workaround this issue, we switch the bit
> + * enablement sequence to below sequence
> + * 1) clear the xSMB & xSMA: which is done in probe and
> + * stop state.
> + * 2) set TE/RE
> + * 3) set xSMB
> + * 4) set xSMA: xSMA is the last one in this flow, which
> + * will trigger esai to start.
> + */
> 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 +924,15 @@ static int fsl_esai_probe(struct platform_device
> *pdev)
> return ret;
> }
>
> + esai_priv->tx_mask = 0xFFFFFFFF;
> + esai_priv->rx_mask = 0xFFFFFFFF;
> +
> + /* 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
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> ilman.alsa-project.org%2Fmailman%2Flistinfo%2Falsa-
> devel&data=02%7C01%7Cshengjiu.wang%40nxp.com%7Ce2940159f310
> 4473866a08d69c7d6c1e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636868459834438771&sdata=WdeHv2JxOBqb0mQ%2B%2FrRISAt64
> JZ6ATY2u02IzzFDaEY%3D&reserved=0
On Tue, Mar 19, 2019 at 02:08:20AM +0000, S.j. Wang wrote: > Hi Mark > > Can this patch be accepted? Or need I do any update? Please don't send content free pings and please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. If there have been review comments then people may be waiting for those to be addressed. Sending content free pings adds to the mail volume (if they are seen at all) which is often the problem and since they can't be reviewed directly if something has gone wrong you'll have to resend the patches anyway, so sending again is generally a better approach though there are some other maintainers who like them - if in doubt look at how patches for the subsystem are normally handled.
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index afe67c865330..3623aa9a6f2e 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: @@ -608,15 +603,38 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd, for (i = 0; tx && i < channels; i++) regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0); + /* + * When set the TE/RE in the end of enablement flow, there + * will be channel swap issue for multi data line case. + * In order to workaround this issue, we switch the bit + * enablement sequence to below sequence + * 1) clear the xSMB & xSMA: which is done in probe and + * stop state. + * 2) set TE/RE + * 3) set xSMB + * 4) set xSMA: xSMA is the last one in this flow, which + * will trigger esai to start. + */ 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 +924,15 @@ static int fsl_esai_probe(struct platform_device *pdev) return ret; } + esai_priv->tx_mask = 0xFFFFFFFF; + esai_priv->rx_mask = 0xFFFFFFFF; + + /* 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) {
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 commit 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver") Cc: <stable@vger.kernel.org> Reviewed-by: Fabio Estevam <festevam@gmail.com> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/fsl_esai.c | 47 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 10 deletions(-)