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."
In this commit add a tasklet to handle reset of ESAI
after xrun happens
Signed-off-by: Shengjiu Wang <[email protected]>
---
sound/soc/fsl/fsl_esai.c | 166 +++++++++++++++++++++++++++++++++++++++
1 file changed, 166 insertions(+)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 10d2210c91ef..149972894c95 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -52,17 +52,20 @@ struct fsl_esai {
struct clk *extalclk;
struct clk *fsysclk;
struct clk *spbaclk;
+ struct tasklet_struct task;
u32 fifo_depth;
u32 slot_width;
u32 slots;
u32 tx_mask;
u32 rx_mask;
+ u32 tx_channels;
u32 hck_rate[2];
u32 sck_rate[2];
bool hck_dir[2];
bool sck_div[2];
bool slave_mode;
bool synchronous;
+ bool reset_at_xrun;
char name[32];
};
@@ -71,8 +74,14 @@ 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, &esr);
+ regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
+
+ if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE))
+ && esai_priv->reset_at_xrun)
+ tasklet_schedule(&esai_priv->task);
if (esr & ESAI_ESR_TINIT_MASK)
dev_dbg(&pdev->dev, "isr: Transmission Initialized\n");
@@ -552,6 +561,9 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
u32 mask;
+ if (tx)
+ esai_priv->tx_channels = channels;
+
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
@@ -585,10 +597,16 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE);
+
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),
+ 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),
@@ -618,6 +636,145 @@ static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.set_tdm_slot = fsl_esai_set_dai_tdm_slot,
};
+static void fsl_esai_reset(unsigned long arg)
+{
+ struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
+ u32 saisr;
+ u32 tsma, tsmb, rsma, rsmb, tcr, rcr, tfcr, rfcr;
+ int i;
+
+ /*
+ * stop the tx & rx
+ */
+ regmap_read(esai_priv->regmap, REG_ESAI_TSMA, &tsma);
+ regmap_read(esai_priv->regmap, REG_ESAI_TSMB, &tsmb);
+ regmap_read(esai_priv->regmap, REG_ESAI_RSMA, &rsma);
+ regmap_read(esai_priv->regmap, REG_ESAI_RSMB, &rsmb);
+
+ regmap_read(esai_priv->regmap, REG_ESAI_TCR, &tcr);
+ regmap_read(esai_priv->regmap, REG_ESAI_RCR, &rcr);
+
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xEIE_MASK | ESAI_xCR_TE_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xEIE_MASK | ESAI_xCR_RE_MASK, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
+ ESAI_xSMA_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
+ ESAI_xSMB_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
+ ESAI_xSMA_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
+ ESAI_xSMB_xS_MASK, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR, 0);
+
+ /*
+ * reset the esai, and restore the registers
+ */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
+ ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
+ ESAI_ECR_ESAIEN | ESAI_ECR_ERST);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
+ ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
+ ESAI_ECR_ESAIEN);
+
+ 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);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
+ ESAI_PRRC_PDC_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
+ ESAI_PCRC_PC_MASK, 0);
+
+ /*
+ * Add fifo reset here, because the regcache_sync will
+ * write one more data to ETDR.
+ * Which will cause channel shift.
+ */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR);
+
+ regcache_mark_dirty(esai_priv->regmap);
+ regcache_sync(esai_priv->regmap);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR_MASK, 0);
+
+ 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, &saisr);
+
+ /*
+ * restart tx / rx, if they already enabled
+ */
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFEN_MASK, tfcr & ESAI_xFCR_xFEN);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFEN_MASK, rfcr & ESAI_xFCR_xFEN);
+
+ /* Write initial words reqiured by ESAI as normal procedure */
+ for (i = 0; i < esai_priv->tx_channels; i++)
+ regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_TE_MASK,
+ ESAI_xCR_TE_MASK & tcr);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_RE_MASK,
+ ESAI_xCR_RE_MASK & rcr);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
+ ESAI_xSMB_xS_MASK,
+ ESAI_xSMB_xS_MASK & tsmb);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
+ ESAI_xSMA_xS_MASK,
+ ESAI_xSMA_xS_MASK & tsma);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
+ ESAI_xSMB_xS_MASK,
+ ESAI_xSMB_xS_MASK & rsmb);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
+ ESAI_xSMA_xS_MASK,
+ ESAI_xSMA_xS_MASK & rsma);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE & tcr);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE & rcr);
+}
+
static int fsl_esai_dai_probe(struct snd_soc_dai *dai)
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
@@ -787,6 +944,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
esai_priv->pdev = pdev;
snprintf(esai_priv->name, sizeof(esai_priv->name), "%pOFn", np);
+ if (of_device_is_compatible(np, "fsl,vf610-esai") ||
+ of_device_is_compatible(np, "fsl,imx35-esai"))
+ esai_priv->reset_at_xrun = true;
+
/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
regs = devm_ioremap_resource(&pdev->dev, res);
@@ -899,6 +1060,8 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ tasklet_init(&esai_priv->task, fsl_esai_reset, (unsigned long)esai_priv);
+
pm_runtime_enable(&pdev->dev);
regcache_cache_only(esai_priv->regmap, true);
@@ -912,7 +1075,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
static int fsl_esai_remove(struct platform_device *pdev)
{
+ struct fsl_esai *esai_priv = platform_get_drvdata(pdev);
+
pm_runtime_disable(&pdev->dev);
+ tasklet_kill(&esai_priv->task);
return 0;
}
--
2.21.0
On Fri, May 17, 2019 at 03:09:22AM +0000, S.j. Wang wrote:
> 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."
>
> In this commit add a tasklet to handle reset of ESAI
> after xrun happens
>
> Signed-off-by: Shengjiu Wang <[email protected]>
> ---
> sound/soc/fsl/fsl_esai.c | 166 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 166 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 10d2210c91ef..149972894c95 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -52,17 +52,20 @@ struct fsl_esai {
> struct clk *extalclk;
> struct clk *fsysclk;
> struct clk *spbaclk;
> + struct tasklet_struct task;
[...]
> + u32 tx_channels;
[...]
> + bool reset_at_xrun;
Please add descriptions for them in the comments of the struct.
> @@ -71,8 +74,14 @@ 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, &esr);
> + regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
> +
> + if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE))
> + && esai_priv->reset_at_xrun)
Please follow the coding style:
+ if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE)) &&
+ esai_priv->reset_at_xrun)
> + tasklet_schedule(&esai_priv->task);
And maybe a dev_dbg also to inform people it's recovering.
> @@ -552,6 +561,9 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
> u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
> u32 mask;
>
> + if (tx)
> + esai_priv->tx_channels = channels;
> +
> switch (cmd) {
> case SNDRV_PCM_TRIGGER_START:
> case SNDRV_PCM_TRIGGER_RESUME:
> @@ -585,10 +597,16 @@ static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
> ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
>
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> + ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE);
A line of comments please.
> +static void fsl_esai_reset(unsigned long arg)
> +{
> + struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
> + u32 saisr;
> + u32 tsma, tsmb, rsma, rsmb, tcr, rcr, tfcr, rfcr;
Could we merge these two lines?
> + /*
> + * stop the tx & rx
> + */
Single-line style please.
> + regmap_read(esai_priv->regmap, REG_ESAI_TSMA, &tsma);
> + regmap_read(esai_priv->regmap, REG_ESAI_TSMB, &tsmb);
> + regmap_read(esai_priv->regmap, REG_ESAI_RSMA, &rsma);
> + regmap_read(esai_priv->regmap, REG_ESAI_RSMB, &rsmb);
> +
> + regmap_read(esai_priv->regmap, REG_ESAI_TCR, &tcr);
> + regmap_read(esai_priv->regmap, REG_ESAI_RCR, &rcr);
> +
> + regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
> + regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
I think this chunk is to save register values other than "stop".
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xEIE_MASK | ESAI_xCR_TE_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xEIE_MASK | ESAI_xCR_RE_MASK, 0);
Indentation:
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xEIE_MASK | ESAI_xCR_RE_MASK, 0);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
> + ESAI_xSMA_xS_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
> + ESAI_xSMB_xS_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
> + ESAI_xSMA_xS_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
> + ESAI_xSMB_xS_MASK, 0);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
> + ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
> + ESAI_xFCR_xFR, 0);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
> + ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
> + ESAI_xFCR_xFR, 0);
Just a thought that I'd like to discuss: since these operations
are completely same as TRIGGER_STOP(tx) + TRIGGER_STOP(rx), can
we abstract a function of fsl_esai_trigger_stop(.., bool tx)?
Benefits would be A) easier to read B) Won't miss an operation,
as we might add something new to one of the stop routines while
forgetting the other side.
> + /*
> + * reset the esai, and restore the registers
> + */
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
> + ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
> + ESAI_ECR_ESAIEN | ESAI_ECR_ERST);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
> + ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
> + ESAI_ECR_ESAIEN);
> +
> + 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);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> + ESAI_PRRC_PDC_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
> + ESAI_PCRC_PC_MASK, 0);
And this could be abstracted too by sharing with probe().
> + /*
> + * Add fifo reset here, because the regcache_sync will
> + * write one more data to ETDR.
> + * Which will cause channel shift.
Sounds like a bug to me...should fix it first by marking the
data registers as volatile.
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
> + ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
> + ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR);
> +
> + regcache_mark_dirty(esai_priv->regmap);
> + regcache_sync(esai_priv->regmap);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
> + ESAI_xFCR_xFR_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
> + ESAI_xFCR_xFR_MASK, 0);
> +
> + 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);
Also same as suspend()-resume().
> + 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, &saisr);
> +
> + /*
> + * restart tx / rx, if they already enabled
> + */
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
> + ESAI_xFCR_xFEN_MASK, tfcr & ESAI_xFCR_xFEN);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
> + ESAI_xFCR_xFEN_MASK, rfcr & ESAI_xFCR_xFEN);
Btw, this xFEN should be xFE...a typo in the driver itself...
> +
> + /* Write initial words reqiured by ESAI as normal procedure */
> + for (i = 0; i < esai_priv->tx_channels; i++)
> + regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_TE_MASK,
> + ESAI_xCR_TE_MASK & tcr);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_RE_MASK,
> + ESAI_xCR_RE_MASK & rcr);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
> + ESAI_xSMB_xS_MASK,
> + ESAI_xSMB_xS_MASK & tsmb);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
> + ESAI_xSMA_xS_MASK,
> + ESAI_xSMA_xS_MASK & tsma);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
> + ESAI_xSMB_xS_MASK,
> + ESAI_xSMB_xS_MASK & rsmb);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
> + ESAI_xSMA_xS_MASK,
> + ESAI_xSMA_xS_MASK & rsma);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE & tcr);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE & rcr);
Similarly having an fsl_esai_trigger_start() could do:
if (tfcr & ESAI_xFCR_xFE)
fsl_esai_trigger_start(tx);
if (rfcr & ESAI_xFCR_xFE)
fsl_esai_trigger_start(rx);
Thank you
Hi
> > + /*
> > + * Add fifo reset here, because the regcache_sync will
> > + * write one more data to ETDR.
> > + * Which will cause channel shift.
>
> Sounds like a bug to me...should fix it first by marking the data registers as
> volatile.
>
The ETDR is a writable register, it is not volatile. Even we change it to
Volatile, I don't think we can't avoid this issue. for the regcache_sync
Just to write this register, it is correct behavior.
Best regards
Wang shengjiu
Hello Shengjiu,
On Thu, May 23, 2019 at 09:53:42AM +0000, S.j. Wang wrote:
> > > + /*
> > > + * Add fifo reset here, because the regcache_sync will
> > > + * write one more data to ETDR.
> > > + * Which will cause channel shift.
> >
> > Sounds like a bug to me...should fix it first by marking the data registers as
> > volatile.
> >
> The ETDR is a writable register, it is not volatile. Even we change it to
> Volatile, I don't think we can't avoid this issue. for the regcache_sync
> Just to write this register, it is correct behavior.
Is that so? Quoting the comments of regcache_sync():
"* regcache_sync - Sync the register cache with the hardware.
*
* @map: map to configure.
*
* Any registers that should not be synced should be marked as
* volatile."
If regcache_sync() does sync volatile registers too as you said,
I don't mind having this FIFO reset WAR for now, though I think
this mismatch between the comments and the actual behavior then
should get people's attention.
Thank you
Hi
> On Thu, May 23, 2019 at 09:53:42AM +0000, S.j. Wang wrote:
> > > > + /*
> > > > + * Add fifo reset here, because the regcache_sync will
> > > > + * write one more data to ETDR.
> > > > + * Which will cause channel shift.
> > >
> > > Sounds like a bug to me...should fix it first by marking the data
> > > registers as volatile.
> > >
> > The ETDR is a writable register, it is not volatile. Even we change it
> > to Volatile, I don't think we can't avoid this issue. for the
> > regcache_sync Just to write this register, it is correct behavior.
>
> Is that so? Quoting the comments of regcache_sync():
> "* regcache_sync - Sync the register cache with the hardware.
> *
> * @map: map to configure.
> *
> * Any registers that should not be synced should be marked as
> * volatile."
>
> If regcache_sync() does sync volatile registers too as you said, I don't mind
> having this FIFO reset WAR for now, though I think this mismatch between
> the comments and the actual behavior then should get people's attention.
>
> Thank you
ETDR is not volatile, if we mark it is volatile, is it correct?
Bets regards
Wang shengjiu
On Thu, May 23, 2019 at 11:04:03AM +0000, S.j. Wang wrote:
> > On Thu, May 23, 2019 at 09:53:42AM +0000, S.j. Wang wrote:
> > > > > + /*
> > > > > + * Add fifo reset here, because the regcache_sync will
> > > > > + * write one more data to ETDR.
> > > > > + * Which will cause channel shift.
> > > >
> > > > Sounds like a bug to me...should fix it first by marking the data
> > > > registers as volatile.
> > > >
> > > The ETDR is a writable register, it is not volatile. Even we change it
> > > to Volatile, I don't think we can't avoid this issue. for the
> > > regcache_sync Just to write this register, it is correct behavior.
> >
> > Is that so? Quoting the comments of regcache_sync():
> > "* regcache_sync - Sync the register cache with the hardware.
> > *
> > * @map: map to configure.
> > *
> > * Any registers that should not be synced should be marked as
> > * volatile."
> >
> > If regcache_sync() does sync volatile registers too as you said, I don't mind
> > having this FIFO reset WAR for now, though I think this mismatch between
> > the comments and the actual behavior then should get people's attention.
> >
> > Thank you
>
> ETDR is not volatile, if we mark it is volatile, is it correct?
Well, you have a point -- it might not be ideally true, but it sounds
like a correct fix to me according to this comments.
We can wait for Mark's comments or just send a patch to the mail list
for review.
Thanks you
Hi
> > > > > Sounds like a bug to me...should fix it first by marking the
> > > > > data registers as volatile.
> > > > >
> > > > The ETDR is a writable register, it is not volatile. Even we
> > > > change it to Volatile, I don't think we can't avoid this issue.
> > > > for the regcache_sync Just to write this register, it is correct behavior.
> > >
> > > Is that so? Quoting the comments of regcache_sync():
> > > "* regcache_sync - Sync the register cache with the hardware.
> > > *
> > > * @map: map to configure.
> > > *
> > > * Any registers that should not be synced should be marked as
> > > * volatile."
> > >
> > > If regcache_sync() does sync volatile registers too as you said, I
> > > don't mind having this FIFO reset WAR for now, though I think this
> > > mismatch between the comments and the actual behavior then should
> get people's attention.
> > >
> > > Thank you
> >
> > ETDR is not volatile, if we mark it is volatile, is it correct?
>
> Well, you have a point -- it might not be ideally true, but it sounds like a
> correct fix to me according to this comments.
>
> We can wait for Mark's comments or just send a patch to the mail list for
> review.
>
> Thanks you
I test this patch, we don't need to reset the FIFO, and regcache_sync didn't
Write the ETDR even the EDTR is not volatile. This fault maybe caused by
Legacy, in the beginning we add this patch in internal branch, there maybe
Something cause this issue, but now can't reproduced.
So I will remove the reset of FIFO.
Best regards
Wang Shengjiu
Hello Shengjiu,
On Wed, Jun 05, 2019 at 10:29:37AM +0000, S.j. Wang wrote:
> > > ETDR is not volatile, if we mark it is volatile, is it correct?
> >
> > Well, you have a point -- it might not be ideally true, but it sounds like a
> > correct fix to me according to this comments.
> >
> > We can wait for Mark's comments or just send a patch to the mail list for
> > review.
>
> I test this patch, we don't need to reset the FIFO, and regcache_sync didn't
> Write the ETDR even the EDTR is not volatile. This fault maybe caused by
The fsl_esai driver uses FLAT type cache so regcache_sync() would
go through regcache_default_sync() that would bypass cache sync at
the regcache_reg_needs_sync() check when the cached register value
matches its default value: in case of ETDR who has a default value
0x0, it'd just "continue" without doing that _regmap_write() when
the cached value equals to 0x0.
> Legacy, in the beginning we add this patch in internal branch, there maybe
> Something cause this issue, but now can't reproduced.
The "legacy" case might happen to have two mismatched ETDR values
between the cached value and default 0x0. And I am worried it may
appear once again someday.
So I feel we still need to change ETDR to volatile type. And for
your question "ETDR is not volatile, if we mark it is volatile,
is it correct?", I double checked the definition of volatile_reg,
and it says:
* @volatile_reg: Optional callback returning true if the register
* value can't be cached. If this field is NULL but
So it seems correct to me then, as the "volatile" should be also
transcribed as "non-cacheable".
Thanks
Nicolin