[alsa-devel] [PATCH 0/6] ASoC: davinci-mcasp: fixes and over/underrun handling
Hi,
Some fixes for the mcasp code: - rx formating fix - constraint for number of channels and sample bits
handling of over/underrun: When such event happens (in McASP vs DMA level) we force restart the stream in order to get McASP and the DMA in sync again.
Regards, Peter --- Misael Lopez Cruz (2): ASoC: davinci-mcasp: Active slots depend on active serializers ASoC: davinci-mcasp: Add overrun/underrun event handling
Peter Ujfalusi (4): ASoC: davinci-mcasp: Validate tdm_slots parameter at probe time ASoC: davinci-mcasp: Place constraint on number of channels ASoC: davinci-mcasp: Symmetric sample bits for IIS mode ASoC: davinci-mcasp: Fix rx format when more bclk is used on the bus
.../bindings/sound/davinci-mcasp-audio.txt | 2 +- sound/soc/davinci/davinci-mcasp.c | 212 +++++++++++++++++++-- sound/soc/davinci/davinci-mcasp.h | 10 + 3 files changed, 210 insertions(+), 14 deletions(-)
Instead of validating the tdm_slots parameter every time at hw_params we can do it once during probe. If the parameter is not valid (<2 or >32) print an error and fix it up.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 6b1bfd9de57d..10c264738a0b 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -632,13 +632,7 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream) u32 mask = 0; u32 busel = 0;
- if ((mcasp->tdm_slots < 2) || (mcasp->tdm_slots > 32)) { - dev_err(mcasp->dev, "tdm slot %d not supported\n", - mcasp->tdm_slots); - return -EINVAL; - } - - active_slots = (mcasp->tdm_slots > 31) ? 32 : mcasp->tdm_slots; + active_slots = mcasp->tdm_slots; for (i = 0; i < active_slots; i++) mask |= (1 << i);
@@ -650,12 +644,12 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream) mcasp_set_reg(mcasp, DAVINCI_MCASP_TXTDM_REG, mask); mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, busel | TXORD); mcasp_mod_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, - FSXMOD(mcasp->tdm_slots), FSXMOD(0x1FF)); + FSXMOD(active_slots), FSXMOD(0x1FF));
mcasp_set_reg(mcasp, DAVINCI_MCASP_RXTDM_REG, mask); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMT_REG, busel | RXORD); mcasp_mod_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, - FSRMOD(mcasp->tdm_slots), FSRMOD(0x1FF)); + FSRMOD(active_slots), FSRMOD(0x1FF));
return 0; } @@ -1237,7 +1231,21 @@ static int davinci_mcasp_probe(struct platform_device *pdev) }
mcasp->op_mode = pdata->op_mode; - mcasp->tdm_slots = pdata->tdm_slots; + /* sanity check for tdm slots parameter */ + if (mcasp->op_mode == DAVINCI_MCASP_IIS_MODE) { + if (pdata->tdm_slots < 2) { + dev_err(&pdev->dev, "invalid tdm slots: %d\n", + pdata->tdm_slots); + mcasp->tdm_slots = 2; + } else if (pdata->tdm_slots > 32) { + dev_err(&pdev->dev, "invalid tdm slots: %d\n", + pdata->tdm_slots); + mcasp->tdm_slots = 32; + } else { + mcasp->tdm_slots = pdata->tdm_slots; + } + } + mcasp->num_serializer = pdata->num_serializer; #ifdef CONFIG_PM_SLEEP mcasp->context.xrsr_regs = devm_kzalloc(&pdev->dev,
On Mon, Nov 10, 2014 at 12:32:15PM +0200, Peter Ujfalusi wrote:
Instead of validating the tdm_slots parameter every time at hw_params we can do it once during probe. If the parameter is not valid (<2 or >32) print an error and fix it up.
Applied, thanks.
In IIS (I2S, TDM, etc) mode the maximum number of allowed channels for either direction can be: number of serializers for the direction * tdm_slots. This constraint applicable for the first stream, while consequent stream should not have more channels then the first stream.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 60 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 10c264738a0b..10ae75e19d99 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -90,6 +90,9 @@ struct davinci_mcasp {
bool dat_port;
+ /* Used for comstraint setting on the second stream */ + u32 channels; + #ifdef CONFIG_PM_SLEEP struct davinci_mcasp_context context; #endif @@ -811,6 +814,9 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
davinci_config_channel_size(mcasp, word_length);
+ if (mcasp->op_mode == DAVINCI_MCASP_IIS_MODE) + mcasp->channels = channels; + return 0; }
@@ -839,7 +845,61 @@ static int davinci_mcasp_trigger(struct snd_pcm_substream *substream, return ret; }
+static int davinci_mcasp_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *cpu_dai) +{ + struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(cpu_dai); + u32 max_channels = 0; + int i, dir; + + if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE) + return 0; + + /* + * Limit the maximum allowed channels for the first stream: + * number of serializers for the direction * tdm slots per serializer + */ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + dir = TX_MODE; + else + dir = RX_MODE; + + for (i = 0; i < mcasp->num_serializer; i++) { + if (mcasp->serial_dir[i] == dir) + max_channels++; + } + max_channels *= mcasp->tdm_slots; + /* + * If the already active stream has less channels than the calculated + * limnit based on the seirializers * tdm_slots, we need to use that as + * a constraint for the second stream. + * Otherwise (first stream or less allowed channels) we use the + * calculated constraint. + */ + if (mcasp->channels && mcasp->channels < max_channels) + max_channels = mcasp->channels; + + snd_pcm_hw_constraint_minmax(substream->runtime, + SNDRV_PCM_HW_PARAM_CHANNELS, + 2, max_channels); + return 0; +} + +static void davinci_mcasp_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *cpu_dai) +{ + struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(cpu_dai); + + if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE) + return; + + if (!cpu_dai->active) + mcasp->channels = 0; +} + static const struct snd_soc_dai_ops davinci_mcasp_dai_ops = { + .startup = davinci_mcasp_startup, + .shutdown = davinci_mcasp_shutdown, .trigger = davinci_mcasp_trigger, .hw_params = davinci_mcasp_hw_params, .set_fmt = davinci_mcasp_set_dai_fmt,
On Mon, Nov 10, 2014 at 12:32:16PM +0200, Peter Ujfalusi wrote:
In IIS (I2S, TDM, etc) mode the maximum number of allowed channels for either direction can be: number of serializers for the direction * tdm_slots. This constraint applicable for the first stream, while consequent stream should not have more channels then the first stream.
Applied, thanks.
From: Misael Lopez Cruz misael.lopez@ti.com
Active slots count depends on the number of channels in the stream and the number of active serializers. Each serializer will handle at most the number of channels specified via 'tdm-slots' parameter in DT.
There are two possible scenarios:
- Single serializer: channel count fits in the max slots supported by McASP serializers, active slots is same as channel count
- Multiple serializers: channel count is bigger than max slots supported by a serializer. Channel count determines how many serializers are needed at their max slot count configuration
Signed-off-by: Misael Lopez Cruz misael.lopez@ti.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 10ae75e19d99..a9822c7bbb0b 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -629,13 +629,29 @@ static int mcasp_common_hw_param(struct davinci_mcasp *mcasp, int stream, return 0; }
-static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream) +static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream, + int channels) { int i, active_slots; + int total_slots; + int active_serializers; u32 mask = 0; u32 busel = 0;
- active_slots = mcasp->tdm_slots; + total_slots = mcasp->tdm_slots; + + /* + * If more than one serializer is needed, then use them with + * their specified tdm_slots count. Otherwise, one serializer + * can cope with the transaction using as many slots as channels + * in the stream, requires channels symmetry + */ + active_serializers = (channels + total_slots - 1) / total_slots; + if (active_serializers == 1) + active_slots = channels; + else + active_slots = total_slots; + for (i = 0; i < active_slots; i++) mask |= (1 << i);
@@ -647,12 +663,12 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream) mcasp_set_reg(mcasp, DAVINCI_MCASP_TXTDM_REG, mask); mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, busel | TXORD); mcasp_mod_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, - FSXMOD(active_slots), FSXMOD(0x1FF)); + FSXMOD(total_slots), FSXMOD(0x1FF));
mcasp_set_reg(mcasp, DAVINCI_MCASP_RXTDM_REG, mask); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMT_REG, busel | RXORD); mcasp_mod_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, - FSRMOD(active_slots), FSRMOD(0x1FF)); + FSRMOD(total_slots), FSRMOD(0x1FF));
return 0; } @@ -766,7 +782,8 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE) ret = mcasp_dit_hw_param(mcasp, params_rate(params)); else - ret = mcasp_i2s_hw_param(mcasp, substream->stream); + ret = mcasp_i2s_hw_param(mcasp, substream->stream, + channels);
if (ret) return ret;
On Mon, Nov 10, 2014 at 12:32:17PM +0200, Peter Ujfalusi wrote:
From: Misael Lopez Cruz misael.lopez@ti.com
Active slots count depends on the number of channels in the stream and the number of active serializers. Each serializer will handle at most the number of channels specified via 'tdm-slots' parameter in DT.
Applied, thanks.
In IIS mode the tx and rx configuration is symmetric, the BCLK and FSYNC is shared.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index a9822c7bbb0b..40cf4bc447e6 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1033,6 +1033,7 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { }, .ops = &davinci_mcasp_dai_ops,
+ .symmetric_samplebits = 1, }, { .name = "davinci-mcasp.1",
When the bus is configured to have more BCLK then the data type demands we need to use the rotation to move the data to correct place.
Reported-by: Misael Lopez Cruz misael.lopez@ti.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 40cf4bc447e6..6b15a974f585 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -494,8 +494,17 @@ static int davinci_config_channel_size(struct davinci_mcasp *mcasp, * both left and right channels), so it has to be divided by number of * tdm-slots (for I2S - divided by 2). */ - if (mcasp->bclk_lrclk_ratio) - word_length = mcasp->bclk_lrclk_ratio / mcasp->tdm_slots; + if (mcasp->bclk_lrclk_ratio) { + u32 slot_length = mcasp->bclk_lrclk_ratio / mcasp->tdm_slots; + + /* + * When we have more bclk then it is needed for the data, we + * need to use the rotation to move the received samples to have + * correct alignment. + */ + rx_rotate = (slot_length - word_length) / 4; + word_length = slot_length; + }
/* mapping of the XSSZ bit-field as described in the datasheet */ fmt = (word_length >> 1) - 1;
On Mon, Nov 10, 2014 at 12:32:19PM +0200, Peter Ujfalusi wrote:
When the bus is configured to have more BCLK then the data type demands we need to use the rotation to move the data to correct place.
Applied, thanks. These bug fixes should've been at the start of the series.
From: Misael Lopez Cruz misael.lopez@ti.com
An underrun (playback) event occurs when the serializer transfer data from the XRBUF buffer to the XRSR shift register, but the XRBUF hasn't been filled. Similarly, the overrun (capture) event occurs when data from the XRSR shift register is transferred to the XRBUF but it hasn't been read yet.
These events are handled as XRUN events that cause the pcm to stop. The stream has to be explicitly restarted by the userspace which ensures that after stopping/starting McASP the data transfer is aligned with DMA. The other possibility was to internally stop and start McASP without DMA even knowing about it.
Signed-off-by: Misael Lopez Cruz misael.lopez@ti.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- .../bindings/sound/davinci-mcasp-audio.txt | 2 +- sound/soc/davinci/davinci-mcasp.c | 91 ++++++++++++++++++++++ sound/soc/davinci/davinci-mcasp.h | 10 +++ 3 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt index 60ca07996458..46bc9829c71a 100644 --- a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt +++ b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt @@ -32,7 +32,7 @@ Optional properties: - rx-num-evt : FIFO levels. - sram-size-playback : size of sram to be allocated during playback - sram-size-capture : size of sram to be allocated during capture -- interrupts : Interrupt numbers for McASP, currently not used by the driver +- interrupts : Interrupt numbers for McASP - interrupt-names : Known interrupt names are "tx" and "rx" - pinctrl-0: Should specify pin control group used for this controller. - pinctrl-names: Should contain only one value - "default", for more details diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 6b15a974f585..a0bb17f5ebf3 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -70,6 +70,7 @@ struct davinci_mcasp { void __iomem *base; u32 fifo_base; struct device *dev; + struct snd_pcm_substream *substreams[2];
/* McASP specific data */ int tdm_slots; @@ -185,6 +186,9 @@ static void mcasp_start_rx(struct davinci_mcasp *mcasp) mcasp_set_ctl_reg(mcasp, DAVINCI_MCASP_GBLCTLR_REG, RXFSRST); if (mcasp_is_synchronous(mcasp)) mcasp_set_ctl_reg(mcasp, DAVINCI_MCASP_GBLCTLX_REG, TXFSRST); + + /* enable rececive overrun IRQ */ + mcasp_set_bits(mcasp, DAVINCI_MCASP_EVTCTLR_REG, ROVRN); }
static void mcasp_start_tx(struct davinci_mcasp *mcasp) @@ -214,6 +218,9 @@ static void mcasp_start_tx(struct davinci_mcasp *mcasp) mcasp_set_ctl_reg(mcasp, DAVINCI_MCASP_GBLCTLX_REG, TXSMRST); /* Release Frame Sync generator */ mcasp_set_ctl_reg(mcasp, DAVINCI_MCASP_GBLCTLX_REG, TXFSRST); + + /* enable transmit underrun IRQ */ + mcasp_set_bits(mcasp, DAVINCI_MCASP_EVTCTLX_REG, XUNDRN); }
static void davinci_mcasp_start(struct davinci_mcasp *mcasp, int stream) @@ -228,6 +235,9 @@ static void davinci_mcasp_start(struct davinci_mcasp *mcasp, int stream)
static void mcasp_stop_rx(struct davinci_mcasp *mcasp) { + /* disable IRQ sources */ + mcasp_clr_bits(mcasp, DAVINCI_MCASP_EVTCTLR_REG, ROVRN); + /* * In synchronous mode stop the TX clocks if no other stream is * running @@ -249,6 +259,9 @@ static void mcasp_stop_tx(struct davinci_mcasp *mcasp) { u32 val = 0;
+ /* disable IRQ sources */ + mcasp_clr_bits(mcasp, DAVINCI_MCASP_EVTCTLX_REG, XUNDRN); + /* * In synchronous mode keep TX clocks running if the capture stream is * still running. @@ -276,6 +289,52 @@ static void davinci_mcasp_stop(struct davinci_mcasp *mcasp, int stream) mcasp_stop_rx(mcasp); }
+static irqreturn_t davinci_mcasp_tx_irq_handler(int irq, void *data) +{ + struct davinci_mcasp *mcasp = (struct davinci_mcasp *)data; + struct snd_pcm_substream *substream; + u32 stat; + + stat = mcasp_get_reg(mcasp, DAVINCI_MCASP_TXSTAT_REG); + if (stat & XUNDRN) { + dev_warn(mcasp->dev, "Transmit buffer underflow\n"); + substream = mcasp->substreams[SNDRV_PCM_STREAM_PLAYBACK]; + if (substream) { + snd_pcm_stream_lock_irq(substream); + if (snd_pcm_running(substream)) + snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); + snd_pcm_stream_unlock_irq(substream); + } + } + + mcasp_set_reg(mcasp, DAVINCI_MCASP_TXSTAT_REG, stat); + + return IRQ_HANDLED; +} + +static irqreturn_t davinci_mcasp_rx_irq_handler(int irq, void *data) +{ + struct davinci_mcasp *mcasp = (struct davinci_mcasp *)data; + struct snd_pcm_substream *substream; + u32 stat; + + stat = mcasp_get_reg(mcasp, DAVINCI_MCASP_RXSTAT_REG); + if (stat & ROVRN) { + dev_warn(mcasp->dev, "Receive buffer overflow\n"); + substream = mcasp->substreams[SNDRV_PCM_STREAM_CAPTURE]; + if (substream) { + snd_pcm_stream_lock_irq(substream); + if (snd_pcm_running(substream)) + snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); + snd_pcm_stream_unlock_irq(substream); + } + } + + mcasp_set_reg(mcasp, DAVINCI_MCASP_RXSTAT_REG, stat); + + return IRQ_HANDLED; +} + static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) { @@ -878,6 +937,8 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream, u32 max_channels = 0; int i, dir;
+ mcasp->substreams[substream->stream] = substream; + if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE) return 0;
@@ -916,6 +977,8 @@ static void davinci_mcasp_shutdown(struct snd_pcm_substream *substream, { struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(cpu_dai);
+ mcasp->substreams[substream->stream] = NULL; + if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE) return;
@@ -1266,6 +1329,8 @@ static int davinci_mcasp_probe(struct platform_device *pdev) struct resource *mem, *ioarea, *res, *dat; struct davinci_mcasp_pdata *pdata; struct davinci_mcasp *mcasp; + char *irq_name; + int irq; int ret;
if (!pdev->dev.platform_data && !pdev->dev.of_node) { @@ -1346,6 +1411,32 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
mcasp->dev = &pdev->dev;
+ irq = platform_get_irq_byname(pdev, "rx"); + if (irq >= 0) { + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_rx\n", + dev_name(&pdev->dev)); + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + davinci_mcasp_rx_irq_handler, + IRQF_ONESHOT, irq_name, mcasp); + if (ret) { + dev_err(&pdev->dev, "RX IRQ request failed\n"); + goto err; + } + } + + irq = platform_get_irq_byname(pdev, "tx"); + if (irq >= 0) { + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_tx\n", + dev_name(&pdev->dev)); + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + davinci_mcasp_tx_irq_handler, + IRQF_ONESHOT, irq_name, mcasp); + if (ret) { + dev_err(&pdev->dev, "TX IRQ request failed\n"); + goto err; + } + } + dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat"); if (dat) mcasp->dat_port = true; diff --git a/sound/soc/davinci/davinci-mcasp.h b/sound/soc/davinci/davinci-mcasp.h index 9737108f0305..cbba1daa25ec 100644 --- a/sound/soc/davinci/davinci-mcasp.h +++ b/sound/soc/davinci/davinci-mcasp.h @@ -285,6 +285,16 @@ #define TXDATADMADIS BIT(0)
/* + * DAVINCI_MCASP_EVTCTLR_REG - Receiver Interrupt Control Register Bits + */ +#define ROVRN BIT(0) + +/* + * DAVINCI_MCASP_EVTCTLX_REG - Transmitter Interrupt Control Register Bits + */ +#define XUNDRN BIT(0) + +/* * DAVINCI_MCASP_W[R]FIFOCTL - Write/Read FIFO Control Register bits */ #define FIFO_ENABLE BIT(16)
On Mon, Nov 10, 2014 at 12:32:20PM +0200, Peter Ujfalusi wrote:
- stat = mcasp_get_reg(mcasp, DAVINCI_MCASP_TXSTAT_REG);
- if (stat & XUNDRN) {
dev_warn(mcasp->dev, "Transmit buffer underflow\n");
substream = mcasp->substreams[SNDRV_PCM_STREAM_PLAYBACK];
if (substream) {
snd_pcm_stream_lock_irq(substream);
if (snd_pcm_running(substream))
snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
snd_pcm_stream_unlock_irq(substream);
}
- }
- mcasp_set_reg(mcasp, DAVINCI_MCASP_TXSTAT_REG, stat);
- return IRQ_HANDLED;
This will unconditionally report that any interrupt that gets delivered is handled and acknowledged regardles of what was reported. This isn't ideal, I'd at least expect a warning to be printed if we're handling interrupts we don't understand.
On 11/10/2014 08:58 PM, Mark Brown wrote:
On Mon, Nov 10, 2014 at 12:32:20PM +0200, Peter Ujfalusi wrote:
- stat = mcasp_get_reg(mcasp, DAVINCI_MCASP_TXSTAT_REG);
- if (stat & XUNDRN) {
dev_warn(mcasp->dev, "Transmit buffer underflow\n");
substream = mcasp->substreams[SNDRV_PCM_STREAM_PLAYBACK];
if (substream) {
snd_pcm_stream_lock_irq(substream);
if (snd_pcm_running(substream))
snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
snd_pcm_stream_unlock_irq(substream);
}
- }
- mcasp_set_reg(mcasp, DAVINCI_MCASP_TXSTAT_REG, stat);
- return IRQ_HANDLED;
This will unconditionally report that any interrupt that gets delivered is handled and acknowledged regardles of what was reported. This isn't ideal, I'd at least expect a warning to be printed if we're handling interrupts we don't understand.
The setup code only enable interrupt generation for XUNDRN for tx or ROVRN for rx. If we receive the interrupt it is because either of these happened. We are not yet interested on the other bits in the status register and not all bits are signaling error. If for example an XSYNCERR happens, we will not know about it till to point we have XUNDRN, which is going to trigger the interrupt. If we would have prints for other bits, we should have at least strategy to deal with them. At the end of the function we ack the interrupt to the HW. Most probably we are going to handle more events, but right now we only care about under or overruns.
On Tue, Nov 11, 2014 at 09:47:29AM +0200, Peter Ujfalusi wrote:
On 11/10/2014 08:58 PM, Mark Brown wrote:
This will unconditionally report that any interrupt that gets delivered is handled and acknowledged regardles of what was reported. This isn't ideal, I'd at least expect a warning to be printed if we're handling interrupts we don't understand.
The setup code only enable interrupt generation for XUNDRN for tx or ROVRN for rx. If we receive the interrupt it is because either of these happened. We are not yet interested on the other bits in the status register and not all bits are signaling error.
Sure, I'm sure it's fine now - the goal here is to be robust against future changes.
If we would have prints for other bits, we should have at least strategy to deal with them.
The strategy I'm suggesting is to either log them so people know what's happening if they do go off and can go look at the code or not handle them so that the generic interrupt handing code can provide a default implementation of that.
At the end of the function we ack the interrupt to the HW.
This is the problem - it means that if they do go off they're silently ignored, and if they start screaming for some reason we don't have anything in place that will handle that.
participants (2)
-
Mark Brown
-
Peter Ujfalusi