All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-12 11:17 ` Markus Pargmann
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-12 11:17 UTC (permalink / raw)
  To: Timur Tabi, Mark Brown, Liam Girdwood
  Cc: alsa-devel, linux-arm-kernel, kernel, Juergen Beisert, stable,
	Markus Pargmann

From: Juergen Beisert <jbe@pengutronix.de>

In cases when capturing is already running and someone enables also
playback, the SDMA unit of the i.MX SoC does not see an edge on its TX DMA
request line. This is due to the TX FIFOs are empty, and this request line is
active all the time, when the CCSR_SSI_SIER_TDMAE bit is set.
In this case also the TX FIFO underrun interrupt will flood the system forever.

To ensure the first edge happens after enabling the TX side of the SSI unit and
to trigger the SDMA unit successfully enable the CCSR_SSI_SIER_TDMAE on
demand only. To be sure, this patch does the same for the CCSR_SSI_SIER_RDMAE
(RX side) bit.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
Cc: stable@vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
Hi,

The patch is based on Mark's "fix/fsl" branch.

Regards,

Markus Pargmann


 sound/soc/fsl/fsl_ssi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 6b81d0c..8c94632 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -107,10 +107,9 @@ 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 | \
+#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TIE | \
+		    CCSR_SSI_SIER_TUE0_EN | CCSR_SSI_SIER_TUE1_EN | \
+		    CCSR_SSI_SIER_RFRC_EN | CCSR_SSI_SIER_RIE | \
 		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
 
 /**
@@ -548,6 +547,9 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
  *
  * The DMA channel is in external master start and pause mode, which
  * means the SSI completely controls the flow of data.
+ *
+ * To use the i.MX's SDMA unit it is important to create an edge on the
+ * corresponding request line. Otherwise the request will be ignored!
  */
 static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 			   struct snd_soc_dai *dai)
@@ -580,20 +582,26 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 			write_ssi_mask(&ssi->scr, 0,
 				CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE);
-		else
+			write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_TDMAE);
+		} else {
 			write_ssi_mask(&ssi->scr, 0,
 				CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE);
+			write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_RDMAE);
+		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 			write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_TE, 0);
-		else
+			write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_TDMAE, 0);
+		} else {
 			write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_RE, 0);
+			write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_RDMAE, 0);
+		}
 
 		if (!ssi_private->imx_ac97 && (read_ssi(&ssi->scr) &
 					(CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE)) == 0)
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-12 11:17 ` Markus Pargmann
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-12 11:17 UTC (permalink / raw)
  To: linux-arm-kernel

From: Juergen Beisert <jbe@pengutronix.de>

In cases when capturing is already running and someone enables also
playback, the SDMA unit of the i.MX SoC does not see an edge on its TX DMA
request line. This is due to the TX FIFOs are empty, and this request line is
active all the time, when the CCSR_SSI_SIER_TDMAE bit is set.
In this case also the TX FIFO underrun interrupt will flood the system forever.

To ensure the first edge happens after enabling the TX side of the SSI unit and
to trigger the SDMA unit successfully enable the CCSR_SSI_SIER_TDMAE on
demand only. To be sure, this patch does the same for the CCSR_SSI_SIER_RDMAE
(RX side) bit.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
Cc: stable at vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
Hi,

The patch is based on Mark's "fix/fsl" branch.

Regards,

Markus Pargmann


 sound/soc/fsl/fsl_ssi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 6b81d0c..8c94632 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -107,10 +107,9 @@ 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 | \
+#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TIE | \
+		    CCSR_SSI_SIER_TUE0_EN | CCSR_SSI_SIER_TUE1_EN | \
+		    CCSR_SSI_SIER_RFRC_EN | CCSR_SSI_SIER_RIE | \
 		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
 
 /**
@@ -548,6 +547,9 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
  *
  * The DMA channel is in external master start and pause mode, which
  * means the SSI completely controls the flow of data.
+ *
+ * To use the i.MX's SDMA unit it is important to create an edge on the
+ * corresponding request line. Otherwise the request will be ignored!
  */
 static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 			   struct snd_soc_dai *dai)
@@ -580,20 +582,26 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 			write_ssi_mask(&ssi->scr, 0,
 				CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE);
-		else
+			write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_TDMAE);
+		} else {
 			write_ssi_mask(&ssi->scr, 0,
 				CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE);
+			write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_RDMAE);
+		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 			write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_TE, 0);
-		else
+			write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_TDMAE, 0);
+		} else {
 			write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_RE, 0);
+			write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_RDMAE, 0);
+		}
 
 		if (!ssi_private->imx_ac97 && (read_ssi(&ssi->scr) &
 					(CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE)) == 0)
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-12 11:17 ` Markus Pargmann
@ 2013-11-12 12:55   ` Timur Tabi
  -1 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-12 12:55 UTC (permalink / raw)
  To: Markus Pargmann, Mark Brown, Liam Girdwood
  Cc: alsa-devel, linux-arm-kernel, kernel, Juergen Beisert, stable

Markus Pargmann wrote:
>
> To ensure the first edge happens after enabling the TX side of the SSI unit and
> to trigger the SDMA unit successfully enable the CCSR_SSI_SIER_TDMAE on
> demand only. To be sure, this patch does the same for the CCSR_SSI_SIER_RDMAE
> (RX side) bit.

This patch impacts PowerPC as well.  Have you tested it on that platform?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-12 12:55   ` Timur Tabi
  0 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-12 12:55 UTC (permalink / raw)
  To: linux-arm-kernel

Markus Pargmann wrote:
>
> To ensure the first edge happens after enabling the TX side of the SSI unit and
> to trigger the SDMA unit successfully enable the CCSR_SSI_SIER_TDMAE on
> demand only. To be sure, this patch does the same for the CCSR_SSI_SIER_RDMAE
> (RX side) bit.

This patch impacts PowerPC as well.  Have you tested it on that platform?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-12 12:55   ` Timur Tabi
@ 2013-11-12 13:24     ` Markus Pargmann
  -1 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-12 13:24 UTC (permalink / raw)
  To: Timur Tabi
  Cc: alsa-devel, Juergen Beisert, Liam Girdwood, stable, Mark Brown,
	kernel, linux-arm-kernel

On Tue, Nov 12, 2013 at 06:55:05AM -0600, Timur Tabi wrote:
> Markus Pargmann wrote:
> >
> >To ensure the first edge happens after enabling the TX side of the SSI unit and
> >to trigger the SDMA unit successfully enable the CCSR_SSI_SIER_TDMAE on
> >demand only. To be sure, this patch does the same for the CCSR_SSI_SIER_RDMAE
> >(RX side) bit.
> 
> This patch impacts PowerPC as well.  Have you tested it on that platform?

No, I don't have access on PowerPC hardware, so I didn't test it on
PowerPC.

Regards,

Markus Pargmann

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-12 13:24     ` Markus Pargmann
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-12 13:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 12, 2013 at 06:55:05AM -0600, Timur Tabi wrote:
> Markus Pargmann wrote:
> >
> >To ensure the first edge happens after enabling the TX side of the SSI unit and
> >to trigger the SDMA unit successfully enable the CCSR_SSI_SIER_TDMAE on
> >demand only. To be sure, this patch does the same for the CCSR_SSI_SIER_RDMAE
> >(RX side) bit.
> 
> This patch impacts PowerPC as well.  Have you tested it on that platform?

No, I don't have access on PowerPC hardware, so I didn't test it on
PowerPC.

Regards,

Markus Pargmann

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-12 13:24     ` Markus Pargmann
@ 2013-11-12 20:17       ` Timur Tabi
  -1 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-12 20:17 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Timur Tabi, alsa-devel mailing list, Juergen Beisert,
	Liam Girdwood, stable, Mark Brown, Sascha Hauer,
	linux-arm-kernel

On Tue, Nov 12, 2013 at 7:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>
> No, I don't have access on PowerPC hardware, so I didn't test it on
> PowerPC.

Can you change the code so that the interrupts are enabled only on
i.MX?  On PowerPC, the interrupts are used only for debugging.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-12 20:17       ` Timur Tabi
  0 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-12 20:17 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 12, 2013 at 7:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>
> No, I don't have access on PowerPC hardware, so I didn't test it on
> PowerPC.

Can you change the code so that the interrupts are enabled only on
i.MX?  On PowerPC, the interrupts are used only for debugging.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-12 20:17       ` Timur Tabi
@ 2013-11-13  8:31         ` Jürgen Beisert
  -1 siblings, 0 replies; 20+ messages in thread
From: Jürgen Beisert @ 2013-11-13  8:31 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Markus Pargmann, alsa-devel mailing list, Liam Girdwood, stable,
	Mark Brown, Sascha Hauer, linux-arm-kernel

Hi Timur,

On Tuesday 12 November 2013 21:17:34 Timur Tabi wrote:
> On Tue, Nov 12, 2013 at 7:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > No, I don't have access on PowerPC hardware, so I didn't test it on
> > PowerPC.
>
> Can you change the code so that the interrupts are enabled only on
> i.MX?  On PowerPC, the interrupts are used only for debugging.

Hmm, you are right. My patch is partially bogus. It enables the
CCSR_SSI_SIER_TDMAE and CCSR_SSI_SIER_RDMAE bits on demand, but this setting
will be overwritten by the "write_ssi(sier_bits, &ssi->sier);" just a few
lines below. :(

But this patch works, because it removes the CCSR_SSI_SIER_TDMAE and
CCSR_SSI_SIER_RDMAE from the SIER_FLAGS macro. Its content is set
_unconditionally_ in lines 570 and 575 and is written to the register in line
607. This should hit PowerPC as well.

So, the real (and small) fix is to remove these bits from the SIER_FLAGS
macro:

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index c6b7439..f176e34 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -107,10 +107,10 @@ 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 | \
+#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | \
 		    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_RIE | \
 		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
 
 /**

@Markus: can you please test it on your hardware?

Thanks
jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-13  8:31         ` Jürgen Beisert
  0 siblings, 0 replies; 20+ messages in thread
From: Jürgen Beisert @ 2013-11-13  8:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Timur,

On Tuesday 12 November 2013 21:17:34 Timur Tabi wrote:
> On Tue, Nov 12, 2013 at 7:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > No, I don't have access on PowerPC hardware, so I didn't test it on
> > PowerPC.
>
> Can you change the code so that the interrupts are enabled only on
> i.MX?  On PowerPC, the interrupts are used only for debugging.

Hmm, you are right. My patch is partially bogus. It enables the
CCSR_SSI_SIER_TDMAE and CCSR_SSI_SIER_RDMAE bits on demand, but this setting
will be overwritten by the "write_ssi(sier_bits, &ssi->sier);" just a few
lines below. :(

But this patch works, because it removes the CCSR_SSI_SIER_TDMAE and
CCSR_SSI_SIER_RDMAE from the SIER_FLAGS macro. Its content is set
_unconditionally_ in lines 570 and 575 and is written to the register in line
607. This should hit PowerPC as well.

So, the real (and small) fix is to remove these bits from the SIER_FLAGS
macro:

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index c6b7439..f176e34 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -107,10 +107,10 @@ 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 | \
+#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | \
 		    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_RIE | \
 		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
 
 /**

@Markus: can you please test it on your hardware?

Thanks
jbe

-- 
Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| Juergen Beisert ? ? ? ? ? ? |
Linux Solutions for Science and Industry ? ? ?| http://www.pengutronix.de/  |

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-12 20:17       ` Timur Tabi
@ 2013-11-13 12:16         ` Markus Pargmann
  -1 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-13 12:16 UTC (permalink / raw)
  To: Timur Tabi
  Cc: alsa-devel mailing list, Juergen Beisert, Liam Girdwood, stable,
	Mark Brown, Sascha Hauer, linux-arm-kernel

Hi,

On Tue, Nov 12, 2013 at 02:17:34PM -0600, Timur Tabi wrote:
> On Tue, Nov 12, 2013 at 7:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> >
> > No, I don't have access on PowerPC hardware, so I didn't test it on
> > PowerPC.
> 
> Can you change the code so that the interrupts are enabled only on
> i.MX?  On PowerPC, the interrupts are used only for debugging.

Why is this necessary? The interrupts were enabled for all
architectures before.

However, this patch is wrong as Jürgen mentioned, the SIER_FLAGS are
written to the SIER register after we enable the DMA enable bits which
leads to missing DMA enable bits at the end.
I created a new patch to cleanup the whole interrupt flag setup. I will
post it later.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-13 12:16         ` Markus Pargmann
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-13 12:16 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Tue, Nov 12, 2013 at 02:17:34PM -0600, Timur Tabi wrote:
> On Tue, Nov 12, 2013 at 7:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> >
> > No, I don't have access on PowerPC hardware, so I didn't test it on
> > PowerPC.
> 
> Can you change the code so that the interrupts are enabled only on
> i.MX?  On PowerPC, the interrupts are used only for debugging.

Why is this necessary? The interrupts were enabled for all
architectures before.

However, this patch is wrong as J?rgen mentioned, the SIER_FLAGS are
written to the SIER register after we enable the DMA enable bits which
leads to missing DMA enable bits at the end.
I created a new patch to cleanup the whole interrupt flag setup. I will
post it later.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-13 12:16         ` Markus Pargmann
@ 2013-11-13 13:10           ` Timur Tabi
  -1 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-13 13:10 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: alsa-devel mailing list, Juergen Beisert, Liam Girdwood, stable,
	Mark Brown, Sascha Hauer, linux-arm-kernel

Markus Pargmann wrote:
>> >Can you change the code so that the interrupts are enabled only on
>> >i.MX?  On PowerPC, the interrupts are used only for debugging.

> Why is this necessary? The interrupts were enabled for all
> architectures before.

My concern is with these lines:

	write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_TDMAE);
...
	write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_TDMAE, 0);

This toggles the TDMAE interrupt on PowerPC, which I believe is VITAL to 
making it work.  The SSI and the DMA controller talk to each other 
automatically, and if I'm reading the documentation correctly, that 
communication depends on TDMAE and RDMAE.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-13 13:10           ` Timur Tabi
  0 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-13 13:10 UTC (permalink / raw)
  To: linux-arm-kernel

Markus Pargmann wrote:
>> >Can you change the code so that the interrupts are enabled only on
>> >i.MX?  On PowerPC, the interrupts are used only for debugging.

> Why is this necessary? The interrupts were enabled for all
> architectures before.

My concern is with these lines:

	write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_TDMAE);
...
	write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_TDMAE, 0);

This toggles the TDMAE interrupt on PowerPC, which I believe is VITAL to 
making it work.  The SSI and the DMA controller talk to each other 
automatically, and if I'm reading the documentation correctly, that 
communication depends on TDMAE and RDMAE.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-13  8:31         ` Jürgen Beisert
@ 2013-11-13 13:11           ` Timur Tabi
  -1 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-13 13:11 UTC (permalink / raw)
  To: Jürgen Beisert
  Cc: Markus Pargmann, alsa-devel mailing list, Liam Girdwood, stable,
	Mark Brown, Sascha Hauer, linux-arm-kernel

Jürgen Beisert wrote:
> -#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
> +#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | \
>   		    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_RIE | \
>   		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
>
>   /**

NACK, this will break PowerPC.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-13 13:11           ` Timur Tabi
  0 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-13 13:11 UTC (permalink / raw)
  To: linux-arm-kernel

J?rgen Beisert wrote:
> -#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
> +#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | \
>   		    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_RIE | \
>   		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
>
>   /**

NACK, this will break PowerPC.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-13 13:10           ` Timur Tabi
@ 2013-11-13 13:38             ` Markus Pargmann
  -1 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-13 13:38 UTC (permalink / raw)
  To: Timur Tabi
  Cc: alsa-devel mailing list, Juergen Beisert, Liam Girdwood, stable,
	Mark Brown, Sascha Hauer, linux-arm-kernel

On Wed, Nov 13, 2013 at 07:10:49AM -0600, Timur Tabi wrote:
> Markus Pargmann wrote:
> >>>Can you change the code so that the interrupts are enabled only on
> >>>i.MX?  On PowerPC, the interrupts are used only for debugging.
> 
> >Why is this necessary? The interrupts were enabled for all
> >architectures before.
> 
> My concern is with these lines:
> 
> 	write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_TDMAE);
> ...
> 	write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_TDMAE, 0);
> 
> This toggles the TDMAE interrupt on PowerPC, which I believe is
> VITAL to making it work.  The SSI and the DMA controller talk to
> each other automatically, and if I'm reading the documentation
> correctly, that communication depends on TDMAE and RDMAE.

Is it necessary to keep TDMAE enabled the whole time? In the patch we
enable it when playback starts and disable when it stops, so the
communication between DMA and SSI shouldn't be disturbed.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-13 13:38             ` Markus Pargmann
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Pargmann @ 2013-11-13 13:38 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 13, 2013 at 07:10:49AM -0600, Timur Tabi wrote:
> Markus Pargmann wrote:
> >>>Can you change the code so that the interrupts are enabled only on
> >>>i.MX?  On PowerPC, the interrupts are used only for debugging.
> 
> >Why is this necessary? The interrupts were enabled for all
> >architectures before.
> 
> My concern is with these lines:
> 
> 	write_ssi_mask(&ssi->sier, 0, CCSR_SSI_SIER_TDMAE);
> ...
> 	write_ssi_mask(&ssi->sier, CCSR_SSI_SIER_TDMAE, 0);
> 
> This toggles the TDMAE interrupt on PowerPC, which I believe is
> VITAL to making it work.  The SSI and the DMA controller talk to
> each other automatically, and if I'm reading the documentation
> correctly, that communication depends on TDMAE and RDMAE.

Is it necessary to keep TDMAE enabled the whole time? In the patch we
enable it when playback starts and disable when it stops, so the
communication between DMA and SSI shouldn't be disturbed.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
  2013-11-13 13:38             ` Markus Pargmann
@ 2013-11-13 14:15               ` Timur Tabi
  -1 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-13 14:15 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: alsa-devel mailing list, Juergen Beisert, Liam Girdwood, stable,
	Mark Brown, Sascha Hauer, linux-arm-kernel

Markus Pargmann wrote:
> Is it necessary to keep TDMAE enabled the whole time? In the patch we
> enable it when playback starts and disable when it stops, so the
> communication between DMA and SSI shouldn't be disturbed.

I will have to test it to be sure, but there's a risk that the bit won't 
be set early enough.  I've seen weird problems like that on my board.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [alsa-devel] [PATCH] ASoC: fsl-ssi: fix SDMA starvation
@ 2013-11-13 14:15               ` Timur Tabi
  0 siblings, 0 replies; 20+ messages in thread
From: Timur Tabi @ 2013-11-13 14:15 UTC (permalink / raw)
  To: linux-arm-kernel

Markus Pargmann wrote:
> Is it necessary to keep TDMAE enabled the whole time? In the patch we
> enable it when playback starts and disable when it stops, so the
> communication between DMA and SSI shouldn't be disturbed.

I will have to test it to be sure, but there's a risk that the bit won't 
be set early enough.  I've seen weird problems like that on my board.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2013-11-13 14:15 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-12 11:17 [PATCH] ASoC: fsl-ssi: fix SDMA starvation Markus Pargmann
2013-11-12 11:17 ` Markus Pargmann
2013-11-12 12:55 ` Timur Tabi
2013-11-12 12:55   ` Timur Tabi
2013-11-12 13:24   ` Markus Pargmann
2013-11-12 13:24     ` Markus Pargmann
2013-11-12 20:17     ` [alsa-devel] " Timur Tabi
2013-11-12 20:17       ` Timur Tabi
2013-11-13  8:31       ` Jürgen Beisert
2013-11-13  8:31         ` Jürgen Beisert
2013-11-13 13:11         ` Timur Tabi
2013-11-13 13:11           ` Timur Tabi
2013-11-13 12:16       ` Markus Pargmann
2013-11-13 12:16         ` Markus Pargmann
2013-11-13 13:10         ` Timur Tabi
2013-11-13 13:10           ` Timur Tabi
2013-11-13 13:38           ` Markus Pargmann
2013-11-13 13:38             ` Markus Pargmann
2013-11-13 14:15             ` Timur Tabi
2013-11-13 14:15               ` Timur Tabi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.