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?
[PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
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 @@ -32,6 +32,7 @@ * @extalclk: esai clock source to derive HCK, SCK and FS * @fsysclk: system clock source to derive HCK, SCK and FS * @spbaclk: SPBA clock (optional, depending on SoC design) + * @task: tasklet to handle the reset operation * @fifo_depth: depth of tx/rx FIFO * @slot_width: width of each DAI slot * @slots: number of slots @@ -42,6 +43,7 @@ * @sck_div: if using PSR/PM dividers for SCKx clock * @slave_mode: if fully using DAI slave mode * @synchronous: if using tx/rx synchronous mode + * @reset_at_xrun: flags for enable reset operaton * @name: driver name */ struct fsl_esai { @@ -53,6 +55,7 @@ struct fsl_esai { struct clk *extalclk; struct clk *fsysclk; struct clk *spbaclk; + struct tasklet_struct task; u32 fifo_depth; u32 slot_width; u32 slots; @@ -65,6 +68,7 @@ struct fsl_esai { bool sck_div[2]; bool slave_mode; bool synchronous; + bool reset_at_xrun; char name[32]; }; @@ -73,8 +77,16 @@ static irqreturn_t esai_isr(int irq, void *devid) struct fsl_esai *esai_priv = (struct fsl_esai *)devid; struct platform_device *pdev = esai_priv->pdev; u32 esr; + u32 saisr; regmap_read(esai_priv->regmap, REG_ESAI_ESR, ); + regmap_read(esai_priv->regmap, REG_ESAI_SAISR, ); + + if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE)) && + esai_priv->reset_at_xrun) { + dev_dbg(>dev, "reset module for xrun\n"); + tasklet_schedule(_priv->task); + } if (esr & ESAI_ESR_TINIT_MASK) dev_dbg(>dev, "isr: Transmission Initialized\n"); @@ -634,10 +646,17 @@ static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool 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)); + + /* Enable Exception interrupt */ + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), + ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE); } static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx) { + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), + ESAI_xCR_xEIE_MASK, 0); + 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), @@ -652,6 +671,53 @@ static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx) ESAI_xFCR_xFR, 0); } +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); + + 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); + + 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)); + + regmap_read(esai_priv->regmap, REG_ESAI_SAISR, ); + + /* restart tx / rx, if they already enabled