[alsa-devel] [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
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;
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
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.
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
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
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.
- 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
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.
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...
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?
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 beenreached\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.
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 beenreached\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.
> + 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 ?
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.
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,
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.
participants (4)
-
David Laight -
Li.Xiubo＠freescale.com -
Mark Brown -
Nicolin Chen