[PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Nicolin Chen]

It's quite cricial to clear error flags because SAI might hang if getting
FIFO underrun during playback (I haven't confirmed the same issue on Rx
overflow though).

So this patch enables those irq and adds isr() to clear the flags so as to
keep playback entirely safe.

Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
---
 sound/soc/fsl/fsl_sai.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++---
 sound/soc/fsl/fsl_sai.h |  9 +++++++
 2 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index c4a4231..5f91aff 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -23,6 +23,55 @@
 
 #include "fsl_sai.h"
 
+#define FSL_SAI_FLAGS (FSL_SAI_CSR_SEIE |\
+		       FSL_SAI_CSR_FEIE |\
+		       FSL_SAI_CSR_FWIE)
+
+static irqreturn_t fsl_sai_isr(int irq, void *devid)
+{
+	struct fsl_sai *sai = (struct fsl_sai *)devid;
+	struct device *dev = &sai->pdev->dev;
+	u32 xcsr;
+
+	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
+	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
+
+	if (xcsr & FSL_SAI_CSR_WSF)
+		dev_dbg(dev, "isr: Start of Tx word detected\n");
+
+	if (xcsr & FSL_SAI_CSR_SEF)
+		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
+
+	if (xcsr & FSL_SAI_CSR_FEF)
+		dev_dbg(dev, "isr: Transmit underrun detected\n");
+
+	if (xcsr & FSL_SAI_CSR_FWF)
+		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
+
+	if (xcsr & FSL_SAI_CSR_FRF)
+		dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
+
+	regmap_read(sai->regmap, FSL_SAI_RCSR, &xcsr);
+	regmap_write(sai->regmap, FSL_SAI_RCSR, xcsr);
+
+	if (xcsr & FSL_SAI_CSR_WSF)
+		dev_dbg(dev, "isr: Start of Rx word detected\n");
+
+	if (xcsr & FSL_SAI_CSR_SEF)
+		dev_dbg(dev, "isr: Rx Frame sync error detected\n");
+
+	if (xcsr & FSL_SAI_CSR_FEF)
+		dev_dbg(dev, "isr: Receive overflow detected\n");
+
+	if (xcsr & FSL_SAI_CSR_FWF)
+		dev_dbg(dev, "isr: Enabled receive FIFO is full\n");
+
+	if (xcsr & FSL_SAI_CSR_FRF)
+		dev_dbg(dev, "isr: Receive FIFO watermark has been reached\n");
+
+	return IRQ_HANDLED;
+}
+
 static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
 		int clk_id, unsigned int freq, int fsl_dir)
 {
@@ -373,8 +422,8 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *cpu_dai)
 {
 	struct fsl_sai *sai = dev_get_drvdata(cpu_dai->dev);
 
-	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, 0x0);
-	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, 0x0);
+	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, FSL_SAI_FLAGS);
+	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, FSL_SAI_FLAGS);
 	regmap_update_bits(sai->regmap, FSL_SAI_TCR1, FSL_SAI_CR1_RFW_MASK,
 			   FSL_SAI_MAXBURST_TX * 2);
 	regmap_update_bits(sai->regmap, FSL_SAI_RCR1, FSL_SAI_CR1_RFW_MASK,
@@ -490,12 +539,14 @@ static int fsl_sai_probe(struct platform_device *pdev)
 	struct fsl_sai *sai;
 	struct resource *res;
 	void __iomem *base;
-	int ret;
+	int irq, ret;
 
 	sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL);
 	if (!sai)
 		return -ENOMEM;
 
+	sai->pdev = pdev;
+
 	sai->big_endian_regs = of_property_read_bool(np, "big-endian-regs");
 	if (sai->big_endian_regs)
 		fsl_sai_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG;
@@ -514,6 +565,18 @@ static int fsl_sai_probe(struct platform_device *pdev)
 		return PTR_ERR(sai->regmap);
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "no irq for node %s\n", np->full_name);
+		return irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, fsl_sai_isr, 0, np->name, sai);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to claim irq %u\n", irq);
+		return ret;
+	}
+
 	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
 	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
 	sai->dma_params_rx.maxburst = FSL_SAI_MAXBURST_RX;
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index e432260..05d1a1f 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -37,7 +37,15 @@
 
 /* SAI Transmit/Recieve Control Register */
 #define FSL_SAI_CSR_TERE	BIT(31)
+#define FSL_SAI_CSR_WSF		BIT(20)
+#define FSL_SAI_CSR_SEF		BIT(19)
+#define FSL_SAI_CSR_FEF		BIT(18)
 #define FSL_SAI_CSR_FWF		BIT(17)
+#define FSL_SAI_CSR_FRF		BIT(16)
+#define FSL_SAI_CSR_WSIE	BIT(12)
+#define FSL_SAI_CSR_SEIE	BIT(11)
+#define FSL_SAI_CSR_FEIE	BIT(10)
+#define FSL_SAI_CSR_FWIE	BIT(9)
 #define FSL_SAI_CSR_FRIE	BIT(8)
 #define FSL_SAI_CSR_FRDE	BIT(0)
 
@@ -99,6 +107,7 @@
 #define FSL_SAI_MAXBURST_RX 6
 
 struct fsl_sai {
+	struct platform_device *pdev;
 	struct regmap *regmap;
 
 	bool big_endian_regs;
-- 
1.8.4

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2197 bytes --]

From: Nicolin Chen
> It's quite cricial to clear error flags because SAI might hang if getting
> FIFO underrun during playback (I haven't confirmed the same issue on Rx
> overflow though).
> 
> So this patch enables those irq and adds isr() to clear the flags so as to
> keep playback entirely safe.
> 
> Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
> ---
>  sound/soc/fsl/fsl_sai.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++---
>  sound/soc/fsl/fsl_sai.h |  9 +++++++
>  2 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index c4a4231..5f91aff 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -23,6 +23,55 @@
> 
>  #include "fsl_sai.h"
> 
> +#define FSL_SAI_FLAGS (FSL_SAI_CSR_SEIE |\
> +		       FSL_SAI_CSR_FEIE |\
> +		       FSL_SAI_CSR_FWIE)
> +
> +static irqreturn_t fsl_sai_isr(int irq, void *devid)
> +{
> +	struct fsl_sai *sai = (struct fsl_sai *)devid;
> +	struct device *dev = &sai->pdev->dev;
> +	u32 xcsr;
> +
> +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);

Assuming these are 'write to clear' bits, you might want
to make the write (above) and all the traces (below)
conditional on the value being non-zero.

> +	if (xcsr & FSL_SAI_CSR_WSF)
> +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> +
> +	if (xcsr & FSL_SAI_CSR_SEF)
> +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> +
> +	if (xcsr & FSL_SAI_CSR_FEF)
> +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> +
> +	if (xcsr & FSL_SAI_CSR_FWF)
> +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> +
> +	if (xcsr & FSL_SAI_CSR_FRF)
> +		dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");

Some of those look like 'normal' interrupts, others are clearly
abnormal conditions.
Maybe the tracing should reflect this.

> +
> +	regmap_read(sai->regmap, FSL_SAI_RCSR, &xcsr);
> +	regmap_write(sai->regmap, FSL_SAI_RCSR, xcsr);

Same comments apply...

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Wed, Mar 26, 2014 at 11:59:53AM +0000, David Laight wrote:
> From: Nicolin Chen

> > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);

> Assuming these are 'write to clear' bits, you might want
> to make the write (above) and all the traces (below)
> conditional on the value being non-zero.

The trace is already conditional?  I'd also expect to see the driver
only acknowledging sources it knows about and only reporting that the
interrupt was handled if it saw one of them - right now all interrupts
are unconditionally acknowleged.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Nicolin Chen]

On Thu, Mar 27, 2014 at 01:14:24AM +0000, Mark Brown wrote:
> On Wed, Mar 26, 2014 at 11:59:53AM +0000, David Laight wrote:
> > From: Nicolin Chen
> 
> > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> 
> > Assuming these are 'write to clear' bits, you might want
> > to make the write (above) and all the traces (below)
> > conditional on the value being non-zero.
> 
> The trace is already conditional?  I'd also expect to see the driver
> only acknowledging sources it knows about and only reporting that the
> interrupt was handled if it saw one of them - right now all interrupts
> are unconditionally acknowleged.

Will revise it based on the comments from both of you.

Thank you.


> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[David Laight]

From: Mark Brown
> On Wed, Mar 26, 2014 at 11:59:53AM +0000, David Laight wrote:
> > From: Nicolin Chen
> 
> > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> 
> > Assuming these are 'write to clear' bits, you might want
> > to make the write (above) and all the traces (below)
> > conditional on the value being non-zero.
> 
> The trace is already conditional?  I'd also expect to see the driver
> only acknowledging sources it knows about and only reporting that the
> interrupt was handled if it saw one of them - right now all interrupts
> are unconditionally acknowleged.

The traces are separately conditional on their own bits.
That is a lot of checks that will normally be false.

Also the driver may need to clear all the active interrupt
bits in order to make the IRQ go away.
It should trace that bits it doesn't expect to be set though.

	David

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

On Thu, Mar 27, 2014 at 09:41:20AM +0000, David Laight wrote:
> From: Mark Brown

> > The trace is already conditional?  I'd also expect to see the driver
> > only acknowledging sources it knows about and only reporting that the
> > interrupt was handled if it saw one of them - right now all interrupts
> > are unconditionally acknowleged.

> The traces are separately conditional on their own bits.
> That is a lot of checks that will normally be false.

Oh, I see what you mean.  I'm not sure that level of optimisation is
worth it, the overhead of taking an interrupt in the first place is
going to dwarf the optimisation and the indentation probably won't help
writing clear messages.

> Also the driver may need to clear all the active interrupt
> bits in order to make the IRQ go away.
> It should trace that bits it doesn't expect to be set though.

The interrupt core will eventually take care of it if the interrupt is
left screaming with nothing handling it (and provide diagnostics).  I
would *hope* that this is only happening for unmasked interrupt sources
we explicitly unmask anyway, we really don't want interrupts on FIFO
level changes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Li.Xiubo]

> +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> +
> +	if (xcsr & FSL_SAI_CSR_WSF)
> +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> +
> +	if (xcsr & FSL_SAI_CSR_SEF)
> +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> +
> +	if (xcsr & FSL_SAI_CSR_FEF)
> +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> +

Actually, the above three isrs should to write a logic 1 to this field
to clear this flag.


> +	if (xcsr & FSL_SAI_CSR_FWF)
> +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> +
> +	if (xcsr & FSL_SAI_CSR_FRF)
> +		dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
> +

While are these ones really needed to clear manually ?


Thanks,
--

Best Regards,
Xiubo

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Nicolin Chen]

On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > +
> > +	if (xcsr & FSL_SAI_CSR_WSF)
> > +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> > +
> > +	if (xcsr & FSL_SAI_CSR_SEF)
> > +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > +
> > +	if (xcsr & FSL_SAI_CSR_FEF)
> > +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> > +
> 
> Actually, the above three isrs should to write a logic 1 to this field
> to clear this flag.
> 
> 
> > +	if (xcsr & FSL_SAI_CSR_FWF)
> > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > +
> > +	if (xcsr & FSL_SAI_CSR_FRF)
> > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
> > +
> 
> While are these ones really needed to clear manually ?

The reference manual doesn't mention about the requirement. So SAI should do
the self-clearance.

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Li.Xiubo]

> On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > +
> > > +	if (xcsr & FSL_SAI_CSR_WSF)
> > > +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > +
> > > +	if (xcsr & FSL_SAI_CSR_SEF)
> > > +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > +
> > > +	if (xcsr & FSL_SAI_CSR_FEF)
> > > +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > +
> >
> > Actually, the above three isrs should to write a logic 1 to this field
> > to clear this flag.
> >
> >
> > > +	if (xcsr & FSL_SAI_CSR_FWF)
> > > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > +
> > > +	if (xcsr & FSL_SAI_CSR_FRF)
> > > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
> > > +
> >
> > While are these ones really needed to clear manually ?
> 
> The reference manual doesn't mention about the requirement. So SAI should do
> the self-clearance.

Yes, I do think we should let it do the self-clearance, and shouldn't interfere
of them...

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Nicolin Chen]

On Thu, Mar 27, 2014 at 10:53:50AM +0800, Xiubo Li-B47053 wrote:
> > On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > > +
> > > > +	if (xcsr & FSL_SAI_CSR_WSF)
> > > > +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > > +
> > > > +	if (xcsr & FSL_SAI_CSR_SEF)
> > > > +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > > +
> > > > +	if (xcsr & FSL_SAI_CSR_FEF)
> > > > +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > > +
> > >
> > > Actually, the above three isrs should to write a logic 1 to this field
> > > to clear this flag.
> > >
> > >
> > > > +	if (xcsr & FSL_SAI_CSR_FWF)
> > > > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > +
> > > > +	if (xcsr & FSL_SAI_CSR_FRF)
> > > > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
> > > > +
> > >
> > > While are these ones really needed to clear manually ?
> > 
> > The reference manual doesn't mention about the requirement. So SAI should do
> > the self-clearance.
> 
> Yes, I do think we should let it do the self-clearance, and shouldn't interfere
> of them...

SAI is supposed to ignore the interference, isn't it?

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Li.Xiubo]

> Subject: Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> 
> On Thu, Mar 27, 2014 at 10:53:50AM +0800, Xiubo Li-B47053 wrote:
> > > On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > > > +
> > > > > +	if (xcsr & FSL_SAI_CSR_WSF)
> > > > > +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > > > +
> > > > > +	if (xcsr & FSL_SAI_CSR_SEF)
> > > > > +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > > > +
> > > > > +	if (xcsr & FSL_SAI_CSR_FEF)
> > > > > +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > > > +
> > > >
> > > > Actually, the above three isrs should to write a logic 1 to this field
> > > > to clear this flag.
> > > >
> > > >
> > > > > +	if (xcsr & FSL_SAI_CSR_FWF)
> > > > > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > +
> > > > > +	if (xcsr & FSL_SAI_CSR_FRF)
> > > > > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been
> reached\n");
> > > > > +
> > > >
> > > > While are these ones really needed to clear manually ?
> > >
> > > The reference manual doesn't mention about the requirement. So SAI should
> do
> > > the self-clearance.
> >
> > Yes, I do think we should let it do the self-clearance, and shouldn't
> interfere
> > of them...
> 
> SAI is supposed to ignore the interference, isn't it?
> 

Maybe, but I'm not very sure.
And these bits are all writable and readable.

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Nicolin Chen]

On Thu, Mar 27, 2014 at 11:41:02AM +0800, Xiubo Li-B47053 wrote:
> 
> > Subject: Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> > 
> > On Thu, Mar 27, 2014 at 10:53:50AM +0800, Xiubo Li-B47053 wrote:
> > > > On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > > > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > > > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > > > > +
> > > > > > +	if (xcsr & FSL_SAI_CSR_WSF)
> > > > > > +		dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > > > > +
> > > > > > +	if (xcsr & FSL_SAI_CSR_SEF)
> > > > > > +		dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > > > > +
> > > > > > +	if (xcsr & FSL_SAI_CSR_FEF)
> > > > > > +		dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > > > > +
> > > > >
> > > > > Actually, the above three isrs should to write a logic 1 to this field
> > > > > to clear this flag.
> > > > >
> > > > >
> > > > > > +	if (xcsr & FSL_SAI_CSR_FWF)
> > > > > > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > > +
> > > > > > +	if (xcsr & FSL_SAI_CSR_FRF)
> > > > > > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been
> > reached\n");
> > > > > > +
> > > > >
> > > > > While are these ones really needed to clear manually ?
> > > >
> > > > The reference manual doesn't mention about the requirement. So SAI should
> > do
> > > > the self-clearance.
> > >
> > > Yes, I do think we should let it do the self-clearance, and shouldn't
> > interfere
> > > of them...
> > 
> > SAI is supposed to ignore the interference, isn't it?
> > 
> 
> Maybe, but I'm not very sure.
> And these bits are all writable and readable.

Double-confirmed? Because FWF and FRF should be read-only bits.

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Li.Xiubo]

> > > > > > > +	if (xcsr & FSL_SAI_CSR_FWF)
> > > > > > > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > > > +
> > > > > > > +	if (xcsr & FSL_SAI_CSR_FRF)
> > > > > > > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been
> > > reached\n");
> > > > > > > +
> > > > > >
> > > > > > While are these ones really needed to clear manually ?
> > > > >
> > > > > The reference manual doesn't mention about the requirement. So SAI
> should
> > > do
> > > > > the self-clearance.
> > > >
> > > > Yes, I do think we should let it do the self-clearance, and shouldn't
> > > interfere
> > > > of them...
> > >
> > > SAI is supposed to ignore the interference, isn't it?
> > >
> >
> > Maybe, but I'm not very sure.
> > And these bits are all writable and readable.
> 
> Double-confirmed? Because FWF and FRF should be read-only bits.
> 

So let's just ignore the clearance of these bits in isr().

+++++
SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : 0000_0000h
-----

I have checked in the Vybrid and LS1 SoC datasheets, and they are all the
Same as above, and nothing else.

Have I missed ?

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Nicolin Chen]

On Thu, Mar 27, 2014 at 12:06:53PM +0800, Xiubo Li-B47053 wrote:
> > > > > > > > +	if (xcsr & FSL_SAI_CSR_FWF)
> > > > > > > > +		dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > > > > +
> > > > > > > > +	if (xcsr & FSL_SAI_CSR_FRF)
> > > > > > > > +		dev_dbg(dev, "isr: Transmit FIFO watermark has been
> > > > reached\n");
> > > > > > > > +
> > > > > > >
> > > > > > > While are these ones really needed to clear manually ?
> > > > > >
> > > > > > The reference manual doesn't mention about the requirement. So SAI
> > should
> > > > do
> > > > > > the self-clearance.
> > > > >
> > > > > Yes, I do think we should let it do the self-clearance, and shouldn't
> > > > interfere
> > > > > of them...
> > > >
> > > > SAI is supposed to ignore the interference, isn't it?
> > > >
> > >
> > > Maybe, but I'm not very sure.
> > > And these bits are all writable and readable.
> > 
> > Double-confirmed? Because FWF and FRF should be read-only bits.
> > 
> 
> So let's just ignore the clearance of these bits in isr().
> 
> +++++
> SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : 0000_0000h

I'm talking about FWF and FRF bits, not TCSR as a register.

> -----
> 
> I have checked in the Vybrid and LS1 SoC datasheets, and they are all the
> Same as above, and nothing else.
> 
> Have I missed ?

What i.MX IC team told me is SAI ignores what we do to FWF and FRF, so you
don't need to worry about it at all unless Vybrid makes them writable, in
which case we may also need to clear these bits and confirm with Vybrid IC
team if they're also W1C.

RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Li.Xiubo]

> > So let's just ignore the clearance of these bits in isr().
> >
> > +++++
> > SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : 0000_0000h
> 
> I'm talking about FWF and FRF bits, not TCSR as a register.
> 
> > -----
> >
> > I have checked in the Vybrid and LS1 SoC datasheets, and they are all the
> > Same as above, and nothing else.
> >
> > Have I missed ?
> 
> What i.MX IC team told me is SAI ignores what we do to FWF and FRF, so you
> don't need to worry about it at all unless Vybrid makes them writable, in
> which case we may also need to clear these bits and confirm with Vybrid IC
> team if they're also W1C.
> 

Well, if so, that's fine.

Thanks,

Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Thu, Mar 27, 2014 at 11:57:27AM +0800, Nicolin Chen wrote:
> On Thu, Mar 27, 2014 at 12:06:53PM +0800, Xiubo Li-B47053 wrote:

> > I have checked in the Vybrid and LS1 SoC datasheets, and they are all the
> > Same as above, and nothing else.

> > Have I missed ?

> What i.MX IC team told me is SAI ignores what we do to FWF and FRF, so you
> don't need to worry about it at all unless Vybrid makes them writable, in
> which case we may also need to clear these bits and confirm with Vybrid IC
> team if they're also W1C.

And even if it payed attention I'd expect that a lot of the time they'd
just be reasserted immediately as the condition still holds.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]