[alsa-devel] [PATCH] ASoC: fsl-ssi: Fix interrupt enable/disable
The fsl_ssi_trigger function enables all interrupts (for RX and TX) when a stream is started. The enabled interrupts are never cleared. We don't need all interrupts enabled for one active stream, e.g. RX interrupts are not necessary for playback. Next to the interrupts, the DMA enable bits are set at the same time. This can lead to lost DMA requests and SDMA starvation.
This patch seperates the SIER flags into DMA_TX and DMA_RX flags to seperatly enable/disable the TX/RX channels.
Based on Jürgen Beisert's patch 'ASoC: fsl-ssi: fix SDMA starvation'.
Cc: Jürgen Beisert jbe@pengutronix.de Signed-off-by: Markus Pargmann mpa@pengutronix.de --- sound/soc/fsl/fsl_ssi.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 6b81d0c..905b8db 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -107,11 +107,18 @@ static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set) #endif
/* SIER bitflag of interrupts to enable */ -#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \ - CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TUE0_EN | \ - CCSR_SSI_SIER_TUE1_EN | CCSR_SSI_SIER_RFRC_EN | \ - CCSR_SSI_SIER_RDMAE | CCSR_SSI_SIER_RIE | \ - CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN) +#define FSLSSI_TX_SIER_FLAGS (CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN) +#define FSLSSI_RX_SIER_FLAGS (CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN) + +#define FSLSSI_DMA_TX_SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | \ + CCSR_SSI_SIER_TDMAE | CCSR_SSI_SIER_TIE | \ + CCSR_SSI_SIER_TUE0_EN | CCSR_SSI_SIER_TUE1_EN) +#define FSLSSI_DMA_RX_SIER_FLAGS (CCSR_SSI_SIER_RFRC_EN | \ + CCSR_SSI_SIER_RDMAE | CCSR_SSI_SIER_RIE | \ + CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN) + +#define FSLSSI_SISR_MASK (FSLSSI_TX_SIER_FLAGS | FSLSSI_RX_SIER_FLAGS | \ + FSLSSI_DMA_TX_SIER_FLAGS | FSLSSI_DMA_RX_SIER_FLAGS)
/** * fsl_ssi_private: per-SSI private data @@ -201,7 +208,7 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) were interrupted for. We mask it with the Interrupt Enable register so that we only check for events that we're interested in. */ - sisr = read_ssi(&ssi->sisr) & SIER_FLAGS; + sisr = read_ssi(&ssi->sisr) & FSLSSI_SISR_MASK;
if (sisr & CCSR_SSI_SISR_RFRC) { ssi_private->stats.rfrc++; @@ -567,19 +574,20 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { if (ssi_private->use_dma) - sier_bits = SIER_FLAGS; + sier_bits = FSLSSI_DMA_TX_SIER_FLAGS; else - sier_bits = CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN; + sier_bits = FSLSSI_TX_SIER_FLAGS; } else { if (ssi_private->use_dma) - sier_bits = SIER_FLAGS; + sier_bits = FSLSSI_DMA_RX_SIER_FLAGS; else - sier_bits = CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN; + sier_bits = FSLSSI_RX_SIER_FLAGS; }
switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + write_ssi_mask(&ssi->sier, 0, sier_bits); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) write_ssi_mask(&ssi->scr, 0, CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE); @@ -590,6 +598,7 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + write_ssi_mask(&ssi->sier, sier_bits, 0); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_TE, 0); else @@ -604,8 +613,6 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, return -EINVAL; }
- write_ssi(sier_bits, &ssi->sier); - return 0; }
@@ -801,7 +808,7 @@ static struct snd_ac97_bus_ops fsl_ssi_ac97_ops = { */ #define SIER_SHOW(flag, name) \ do { \ - if (SIER_FLAGS & CCSR_SSI_SIER_##flag) \ + if (FSLSSI_SISR_MASK & CCSR_SSI_SIER_##flag) \ length += sprintf(buf + length, #name "=%u\n", \ ssi_private->stats.name); \ } while (0)
On Wed, Nov 13, 2013 at 02:12:10PM +0100, Markus Pargmann wrote:
The fsl_ssi_trigger function enables all interrupts (for RX and TX) when a stream is started. The enabled interrupts are never cleared. We don't need all interrupts enabled for one active stream, e.g. RX interrupts are not necessary for playback. Next to the interrupts, the DMA enable bits are set at the same time. This can lead to lost DMA requests and SDMA starvation.
This patch seperates the SIER flags into DMA_TX and DMA_RX flags to seperatly enable/disable the TX/RX channels.
This patch is not correct either, sorry.
imx53 reference manual does not list registers which may not be modified while the SSI unit is enabled, but e.g. imx27 reference manual contains such a list. SIER TDMAE and RDMAE is listed there, so it is not correct to change these bits for some SoCs when the ssi unit is already running.
I am not sure what's the best thing to do here as we can run into issues with the SDMA unit on imx53 when we have RDMAE and TDMAE enabled the whole time. We could set the SIER flags in different functions for different chips, but this doesn't seem the best way. Any ideas?
Regards,
Markus
Based on Jürgen Beisert's patch 'ASoC: fsl-ssi: fix SDMA starvation'.
Cc: Jürgen Beisert jbe@pengutronix.de Signed-off-by: Markus Pargmann mpa@pengutronix.de
sound/soc/fsl/fsl_ssi.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 6b81d0c..905b8db 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -107,11 +107,18 @@ static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set) #endif
/* SIER bitflag of interrupts to enable */ -#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TUE0_EN | \CCSR_SSI_SIER_TUE1_EN | CCSR_SSI_SIER_RFRC_EN | \CCSR_SSI_SIER_RDMAE | CCSR_SSI_SIER_RIE | \CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)+#define FSLSSI_TX_SIER_FLAGS (CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN) +#define FSLSSI_RX_SIER_FLAGS (CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN)
+#define FSLSSI_DMA_TX_SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | \
CCSR_SSI_SIER_TDMAE | CCSR_SSI_SIER_TIE | \CCSR_SSI_SIER_TUE0_EN | CCSR_SSI_SIER_TUE1_EN)+#define FSLSSI_DMA_RX_SIER_FLAGS (CCSR_SSI_SIER_RFRC_EN | \
CCSR_SSI_SIER_RDMAE | CCSR_SSI_SIER_RIE | \CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)+#define FSLSSI_SISR_MASK (FSLSSI_TX_SIER_FLAGS | FSLSSI_RX_SIER_FLAGS | \
FSLSSI_DMA_TX_SIER_FLAGS | FSLSSI_DMA_RX_SIER_FLAGS)/**
- fsl_ssi_private: per-SSI private data
@@ -201,7 +208,7 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) were interrupted for. We mask it with the Interrupt Enable register so that we only check for events that we're interested in. */
- sisr = read_ssi(&ssi->sisr) & SIER_FLAGS;
sisr = read_ssi(&ssi->sisr) & FSLSSI_SISR_MASK;
if (sisr & CCSR_SSI_SISR_RFRC) { ssi_private->stats.rfrc++;
@@ -567,19 +574,20 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { if (ssi_private->use_dma)
sier_bits = SIER_FLAGS;
sier_bits = FSLSSI_DMA_TX_SIER_FLAGS;
sier_bits = CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN;
sier_bits = FSLSSI_TX_SIER_FLAGS;
sier_bits = SIER_FLAGS;
sier_bits = FSLSSI_DMA_RX_SIER_FLAGS;
sier_bits = CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN;
sier_bits = FSLSSI_RX_SIER_FLAGS;}
switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
write_ssi_mask(&ssi->sier, 0, sier_bits);if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) write_ssi_mask(&ssi->scr, 0, CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE);
@@ -590,6 +598,7 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
write_ssi_mask(&ssi->sier, sier_bits, 0);@@ -604,8 +613,6 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, return -EINVAL; }
- write_ssi(sier_bits, &ssi->sier);
- return 0;
}
@@ -801,7 +808,7 @@ static struct snd_ac97_bus_ops fsl_ssi_ac97_ops = { */ #define SIER_SHOW(flag, name) \ do { \
if (SIER_FLAGS & CCSR_SSI_SIER_##flag) \
if (FSLSSI_SISR_MASK & CCSR_SSI_SIER_##flag) \ length += sprintf(buf + length, #name "=%u\n", \ ssi_private->stats.name); \-- 1.8.4.2
participants (1)
-
Markus Pargmann