Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
> > > > + > > > > + /* restore registers by regcache_sync */ > > > > + fsl_esai_register_restore(esai_priv); > > > > + > > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, > > > > +ESAI_xCR_xPR_MASK, 0); > > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR, > > > > +ESAI_xCR_xPR_MASK, 0); > > > > > > And just for curious, can (or shall) we stuff this personal reset to > > > the reset() function? I found this one is a part of the reset > > > routine being mentioned in the RM -- it was done after ESAI reset is > done via ECR register. > > > > > > > There is a problem to do this, TPR/RPR need to be clear after > > configure the control register. (TCCR, TCR). So it seems not only one > > place (reset function) need to be changed. > > Do you know (or remember) why we suddenly involve this TPR/PRP? > The driver has no problem so far, even if we don't have them. > > The "personal reset" sounds like a feature that we would use to reset TX or > RX individually, while this hw_reset() does a full reset for both TX and RX. > So I wonder whether they're necessary. The hw_reset flow is suggested by design team, so involve TRP/RPP is from them, I don't know the detail. Best regards Wang shengjiu
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
On Fri, Jul 05, 2019 at 07:03:47AM +, S.j. Wang wrote: > > > > > + > > > + /* restore registers by regcache_sync */ > > > + fsl_esai_register_restore(esai_priv); > > > + > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, > > > +ESAI_xCR_xPR_MASK, 0); > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR, > > > +ESAI_xCR_xPR_MASK, 0); > > > > And just for curious, can (or shall) we stuff this personal reset to the > > reset() > > function? I found this one is a part of the reset routine being mentioned in > > the RM -- it was done after ESAI reset is done via ECR register. > > > > There is a problem to do this, TPR/RPR need to be clear after configure the > control > register. (TCCR, TCR). So it seems not only one place (reset function) need > to be > changed. Do you know (or remember) why we suddenly involve this TPR/PRP? The driver has no problem so far, even if we don't have them. The "personal reset" sounds like a feature that we would use to reset TX or RX individually, while this hw_reset() does a full reset for both TX and RX. So I wonder whether they're necessary.
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
> > > + > > + /* restore registers by regcache_sync */ > > + fsl_esai_register_restore(esai_priv); > > + > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, > > +ESAI_xCR_xPR_MASK, 0); > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR, > > +ESAI_xCR_xPR_MASK, 0); > > And just for curious, can (or shall) we stuff this personal reset to the > reset() > function? I found this one is a part of the reset routine being mentioned in > the RM -- it was done after ESAI reset is done via ECR register. > There is a problem to do this, TPR/RPR need to be clear after configure the control register. (TCCR, TCR). So it seems not only one place (reset function) need to be changed. Best regards Wang shengjiu
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
On 2019-07-03 08:42, shengjiu.w...@nxp.com wrote:
+static void fsl_esai_reset(unsigned long arg)
+{
+ struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
+ u32 saisr, tfcr, rfcr;
+
+ /* save the registers */
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, );
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, );
+
+ /* stop the tx & rx */
+ fsl_esai_trigger_stop(esai_priv, 1);
+ fsl_esai_trigger_stop(esai_priv, 0);
+
+ /* reset the esai, and restore the registers */
+ fsl_esai_init(esai_priv);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xPR_MASK,
+ ESAI_xCR_xPR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xPR_MASK,
+ ESAI_xCR_xPR);
+
+ /* restore registers by regcache_sync */
+ fsl_esai_register_restore(esai_priv);
+
Both _init and _restore may fail given their declaration in 1/2 "ASoC:
fsl_esai: Wrap some operations to be functions" yet here you simply
ignore the return values.
If failure of said functions is permissive, it might be a good place for
a comment.
Czarek
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
On Wed, Jul 03, 2019 at 02:42:05PM +0800, shengjiu.w...@nxp.com wrote:
> From: Shengjiu Wang
>
> There is chip errata ERR008000, the reference doc is
> (https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf),
>
> The issue is "While using ESAI transmit or receive and
> an underrun/overrun happens, channel swap may occur.
> The only recovery mechanism is to reset the ESAI."
>
> This issue exist in imx3/imx5/imx6(partial) series.
>
> In this commit add a tasklet to handle reset of ESAI
> after xrun happens to recover the channel swap.
>
> Signed-off-by: Shengjiu Wang
> ---
> sound/soc/fsl/fsl_esai.c | 76
> 1 file changed, 76 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 20039ae9893b..8c92e49ad6d8 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> +static void fsl_esai_reset(unsigned long arg)
Similarly fsl_esai_hw_reset? This one isn't really that bad though,
yet it feels better to have function naming in a similar style.
> +{
> + struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
> + u32 saisr, tfcr, rfcr;
> +
> + /* save the registers */
> + regmap_read(esai_priv->regmap, REG_ESAI_TFCR, );
> + regmap_read(esai_priv->regmap, REG_ESAI_RFCR, );
Instead of having this implicit comments, we could have:
+ bool tx = true, rx = false, enabled[2];
+
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, );
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, );
+ enabled[tx] = tfcr & ESAI_xFCR_xFEN;
+ enabled[rx] = rfcr & ESAI_xFCR_xFEN;
> +
> + /* stop the tx & rx */
> + fsl_esai_trigger_stop(esai_priv, 1);
> + fsl_esai_trigger_stop(esai_priv, 0);
And we could reuse the boolean 'tx' and 'rx' here.
> +
> + /* reset the esai, and restore the registers */
> + fsl_esai_init(esai_priv);
> +
[...]
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> +ESAI_xCR_xPR_MASK,
> +ESAI_xCR_xPR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> +ESAI_xCR_xPR_MASK,
> +ESAI_xCR_xPR);
Mask and value might fit into one line?
> +
> + /* restore registers by regcache_sync */
> + fsl_esai_register_restore(esai_priv);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> +ESAI_xCR_xPR_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> +ESAI_xCR_xPR_MASK, 0);
And just for curious, can (or shall) we stuff this personal reset
to the reset() function? I found this one is a part of the reset
routine being mentioned in the RM -- it was done after ESAI reset
is done via ECR register.
[...]
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> +ESAI_PRRC_PDC_MASK,
> +ESAI_PRRC_PDC(ESAI_GPIO));
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
> +ESAI_PCRC_PC_MASK,
> +ESAI_PCRC_PC(ESAI_GPIO));
Mask and value might fit into one line?
