[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]

RnJvbTogTmljb2xpbiBDaGVuDQo+IEl0J3MgcXVpdGUgY3JpY2lhbCB0byBjbGVhciBlcnJvciBm
bGFncyBiZWNhdXNlIFNBSSBtaWdodCBoYW5nIGlmIGdldHRpbmcNCj4gRklGTyB1bmRlcnJ1biBk
dXJpbmcgcGxheWJhY2sgKEkgaGF2ZW4ndCBjb25maXJtZWQgdGhlIHNhbWUgaXNzdWUgb24gUngN
Cj4gb3ZlcmZsb3cgdGhvdWdoKS4NCj4gDQo+IFNvIHRoaXMgcGF0Y2ggZW5hYmxlcyB0aG9zZSBp
cnEgYW5kIGFkZHMgaXNyKCkgdG8gY2xlYXIgdGhlIGZsYWdzIHNvIGFzIHRvDQo+IGtlZXAgcGxh
eWJhY2sgZW50aXJlbHkgc2FmZS4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IE5pY29saW4gQ2hlbiA8
R3Vhbmd5dS5DaGVuQGZyZWVzY2FsZS5jb20+DQo+IC0tLQ0KPiAgc291bmQvc29jL2ZzbC9mc2xf
c2FpLmMgfCA2OSArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
LS0tDQo+ICBzb3VuZC9zb2MvZnNsL2ZzbF9zYWkuaCB8ICA5ICsrKysrKysNCj4gIDIgZmlsZXMg
Y2hhbmdlZCwgNzUgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1n
aXQgYS9zb3VuZC9zb2MvZnNsL2ZzbF9zYWkuYyBiL3NvdW5kL3NvYy9mc2wvZnNsX3NhaS5jDQo+
IGluZGV4IGM0YTQyMzEuLjVmOTFhZmYgMTAwNjQ0DQo+IC0tLSBhL3NvdW5kL3NvYy9mc2wvZnNs
X3NhaS5jDQo+ICsrKyBiL3NvdW5kL3NvYy9mc2wvZnNsX3NhaS5jDQo+IEBAIC0yMyw2ICsyMyw1
NSBAQA0KPiANCj4gICNpbmNsdWRlICJmc2xfc2FpLmgiDQo+IA0KPiArI2RlZmluZSBGU0xfU0FJ
X0ZMQUdTIChGU0xfU0FJX0NTUl9TRUlFIHxcDQo+ICsJCSAgICAgICBGU0xfU0FJX0NTUl9GRUlF
IHxcDQo+ICsJCSAgICAgICBGU0xfU0FJX0NTUl9GV0lFKQ0KPiArDQo+ICtzdGF0aWMgaXJxcmV0
dXJuX3QgZnNsX3NhaV9pc3IoaW50IGlycSwgdm9pZCAqZGV2aWQpDQo+ICt7DQo+ICsJc3RydWN0
IGZzbF9zYWkgKnNhaSA9IChzdHJ1Y3QgZnNsX3NhaSAqKWRldmlkOw0KPiArCXN0cnVjdCBkZXZp
Y2UgKmRldiA9ICZzYWktPnBkZXYtPmRldjsNCj4gKwl1MzIgeGNzcjsNCj4gKw0KPiArCXJlZ21h
cF9yZWFkKHNhaS0+cmVnbWFwLCBGU0xfU0FJX1RDU1IsICZ4Y3NyKTsNCj4gKwlyZWdtYXBfd3Jp
dGUoc2FpLT5yZWdtYXAsIEZTTF9TQUlfVENTUiwgeGNzcik7DQoNCkFzc3VtaW5nIHRoZXNlIGFy
ZSAnd3JpdGUgdG8gY2xlYXInIGJpdHMsIHlvdSBtaWdodCB3YW50DQp0byBtYWtlIHRoZSB3cml0
ZSAoYWJvdmUpIGFuZCBhbGwgdGhlIHRyYWNlcyAoYmVsb3cpDQpjb25kaXRpb25hbCBvbiB0aGUg
dmFsdWUgYmVpbmcgbm9uLXplcm8uDQoNCj4gKwlpZiAoeGNzciAmIEZTTF9TQUlfQ1NSX1dTRikN
Cj4gKwkJZGV2X2RiZyhkZXYsICJpc3I6IFN0YXJ0IG9mIFR4IHdvcmQgZGV0ZWN0ZWRcbiIpOw0K
PiArDQo+ICsJaWYgKHhjc3IgJiBGU0xfU0FJX0NTUl9TRUYpDQo+ICsJCWRldl9kYmcoZGV2LCAi
aXNyOiBUeCBGcmFtZSBzeW5jIGVycm9yIGRldGVjdGVkXG4iKTsNCj4gKw0KPiArCWlmICh4Y3Ny
ICYgRlNMX1NBSV9DU1JfRkVGKQ0KPiArCQlkZXZfZGJnKGRldiwgImlzcjogVHJhbnNtaXQgdW5k
ZXJydW4gZGV0ZWN0ZWRcbiIpOw0KPiArDQo+ICsJaWYgKHhjc3IgJiBGU0xfU0FJX0NTUl9GV0Yp
DQo+ICsJCWRldl9kYmcoZGV2LCAiaXNyOiBFbmFibGVkIHRyYW5zbWl0IEZJRk8gaXMgZW1wdHlc
biIpOw0KPiArDQo+ICsJaWYgKHhjc3IgJiBGU0xfU0FJX0NTUl9GUkYpDQo+ICsJCWRldl9kYmco
ZGV2LCAiaXNyOiBUcmFuc21pdCBGSUZPIHdhdGVybWFyayBoYXMgYmVlbiByZWFjaGVkXG4iKTsN
Cg0KU29tZSBvZiB0aG9zZSBsb29rIGxpa2UgJ25vcm1hbCcgaW50ZXJydXB0cywgb3RoZXJzIGFy
ZSBjbGVhcmx5DQphYm5vcm1hbCBjb25kaXRpb25zLg0KTWF5YmUgdGhlIHRyYWNpbmcgc2hvdWxk
IHJlZmxlY3QgdGhpcy4NCg0KPiArDQo+ICsJcmVnbWFwX3JlYWQoc2FpLT5yZWdtYXAsIEZTTF9T
QUlfUkNTUiwgJnhjc3IpOw0KPiArCXJlZ21hcF93cml0ZShzYWktPnJlZ21hcCwgRlNMX1NBSV9S
Q1NSLCB4Y3NyKTsNCg0KU2FtZSBjb21tZW50cyBhcHBseS4uLg0KDQoJRGF2aWQNCg0K

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: [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: [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

[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 ?
>=20
> 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 inter=
fere
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

[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]

> Subject: Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
>=20
> 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 fi=
eld
> > > > 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 sh=
ould
> do
> > > the self-clearance.
> >
> > Yes, I do think we should let it do the self-clearance, and shouldn't
> interfere
> > of them...
>=20
> SAI is supposed to ignore the interference, isn't it?
>=20

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 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]

> > > > > > > +	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 SA=
I
> 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.
>=20
> Double-confirmed? Because FWF and FRF should be read-only bits.
>=20

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

[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
>=20
> I'm talking about FWF and FRF bits, not TCSR as a register.
>=20
> > -----
> >
> > I have checked in the Vybrid and LS1 SoC datasheets, and they are all t=
he
> > Same as above, and nothing else.
> >
> > Have I missed ?
>=20
> What i.MX IC team told me is SAI ignores what we do to FWF and FRF, so yo=
u
> 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 I=
C
> team if they're also W1C.
>=20

Well, if so, that's fine.

Thanks,

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
>=20
> > > +	regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > +	regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
>=20
> > 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.
>=20
> 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: 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 --]

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 --]