[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
---
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, 0x, 0x0);
- regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0x, 0x0);
+ regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0x,
FSL_SAI_FLAGS);
+ regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0x,
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_WSFBIT(20)
+#define FSL_SAI_CSR_SEFBIT(19)
+#define FSL_SAI_CSR_FEFBIT(18)
#define FSL_SAI_CSR_FWFBIT(17)
+#define FSL_SAI_CSR_FRFBIT(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
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
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
> ---
> 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
On Wed, Mar 26, 2014 at 11:59:53AM +, 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. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> + 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> 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... ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
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? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error 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. > So let's just ignore the clearance of these bits in isr(). + SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : _h - I have checked in the Vybrid and LS1 SoC datasheets, and they are all the Same as above, and nothing else. Have I missed ? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
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 : _h 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> > So let's just ignore the clearance of these bits in isr(). > > > > + > > SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : _h > > 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, ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Mark Brown > On Wed, Mar 26, 2014 at 11:59:53AM +, 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
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. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
On Thu, Mar 27, 2014 at 09:41:20AM +, 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. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
On Thu, Mar 27, 2014 at 01:14:24AM +, Mark Brown wrote: > On Wed, Mar 26, 2014 at 11:59:53AM +, 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-de...@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
