All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fsl_ssi: set fifo watermark to more reliable value
@ 2017-01-03 18:22 Caleb Crome
  2017-01-03 21:08 ` [alsa-devel] " Fabio Estevam
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Caleb Crome @ 2017-01-03 18:22 UTC (permalink / raw)
  To: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Liam Girdwood,
	Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel,
	linuxppc-dev, linux-kernel, Arnaud Mouiche
  Cc: Caleb Crome

From: Caleb Crome <caleb@crome.org>

The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
activate DMA on FIFO when only 2 spaces are left.)  This means the
DMA must service the fifo within 2 audio samples, which is just not
enough time  for many use cases with high data rate.  In many
configurations the audio channel slips (causing l/r swap in stereo
configurations, or channel slipping in multi-channel configurations).

This patch gives more breathing room and allows the SSI to operate
reliably by changing the fifio refill watermark to 8.

There is no change in behavior for older chips (with an 8-deep fifo).
Only the newer chips with a 15-deep fifo get the new behavior. I
suspect a new fifo depth setting could be optimized on the older
chips too, but I have not tested.

Signed-off-by: Caleb Crome <caleb@crome.org>
---
 sound/soc/fsl/fsl_ssi.c | 74 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index bedec4a..56245e6 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -224,6 +224,12 @@ struct fsl_ssi_soc_data {
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ *
+ * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
+ *             there are @fifo_watermark or fewer words in TX fifo or
+ *             @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: max number of words to transfer in one go.  So far,
+ *             this is always the same as fifo_watermark.
  */
 struct fsl_ssi_private {
 	struct regmap *regs;
@@ -263,6 +269,9 @@ struct fsl_ssi_private {
 
 	const struct fsl_ssi_soc_data *soc;
 	struct device *dev;
+
+	u32 fifo_watermark;
+	u32 dma_maxburst;
 };
 
 /*
@@ -1051,21 +1060,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
 	regmap_write(regs, CCSR_SSI_SRCR, srcr);
 	regmap_write(regs, CCSR_SSI_SCR, scr);
 
-	/*
-	 * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
-	 * use FIFO 1. We program the transmit water to signal a DMA transfer
-	 * if there are only two (or fewer) elements left in the FIFO. Two
-	 * elements equals one frame (left channel, right channel). This value,
-	 * however, depends on the depth of the transmit buffer.
-	 *
-	 * We set the watermark on the same level as the DMA burstsize.  For
-	 * fiq it is probably better to use the biggest possible watermark
-	 * size.
-	 */
-	if (ssi_private->use_dma)
-		wm = ssi_private->fifo_depth - 2;
-	else
-		wm = ssi_private->fifo_depth;
+	wm = ssi_private->fifo_watermark;
 
 	regmap_write(regs, CCSR_SSI_SFCSR,
 			CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
@@ -1373,12 +1368,8 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 		dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
 			 PTR_ERR(ssi_private->baudclk));
 
-	/*
-	 * We have burstsize be "fifo_depth - 2" to match the SSI
-	 * watermark setting in fsl_ssi_startup().
-	 */
-	ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
-	ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
+	ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
+	ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
 	ssi_private->dma_params_tx.addr = ssi_private->ssi_phys + CCSR_SSI_STX0;
 	ssi_private->dma_params_rx.addr = ssi_private->ssi_phys + CCSR_SSI_SRX0;
 
@@ -1543,6 +1534,47 @@ static int fsl_ssi_probe(struct platform_device *pdev)
                 /* Older 8610 DTs didn't have the fifo-depth property */
 		ssi_private->fifo_depth = 8;
 
+	/*
+	 * Set the watermark for transmit FIFO 0 and receive FIFO 0. We don't
+	 * use FIFO 1 but set the watermark appropriately nontheless.
+	 * We program the transmit water to signal a DMA transfer
+	 * if there are N elements left in the FIFO. For chips with 15-deep
+	 * FIFOs, set watermark to 8.  This allows the SSI to operate at a
+	 * high data rate without channel slipping. Behavior is unchanged
+	 * for the older chips with a fifo depth of only 8.  A value of 4
+	 * might be appropriate for the older chips, but is left at
+	 * fifo_depth-2 until sombody has a chance to test.
+	 *
+	 * We set the watermark on the same level as the DMA burstsize.  For
+	 * fiq it is probably better to use the biggest possible watermark
+	 * size.
+	 */
+	switch (ssi_private->fifo_depth) {
+	case 15:
+		/*
+		 * 2 samples is not enough when running at high data
+		 * rates (like 48kHz @ 16 bits/channel, 16 channels)
+		 * 8 seems to split things evenly and leave enough time
+		 * for the DMA to fill the FIFO before it's over/under
+		 * run.
+		 */
+		ssi_private->fifo_watermark = 8;
+		ssi_private->dma_maxburst = 8;
+		break;
+	case 8:
+	default:
+		/*
+		 * maintain old behavior for older chips.
+		 * Keeping it the same because I don't have an older
+		 * board to test with.
+		 * I suspect this could be changed to be something to
+		 * leave some more space in the fifo.
+		 */
+		ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
+		ssi_private->dma_maxburst = ssi_private->fifo_depth - 2;
+		break;
+	}
+
 	dev_set_drvdata(&pdev->dev, ssi_private);
 
 	if (ssi_private->soc->imx) {
-- 
2.7.4

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

* Re: [alsa-devel] [PATCH] fsl_ssi: set fifo watermark to more reliable value
  2017-01-03 18:22 [PATCH] fsl_ssi: set fifo watermark to more reliable value Caleb Crome
@ 2017-01-03 21:08 ` Fabio Estevam
  2017-01-04 18:27 ` Mark Brown
  2017-01-04 18:37   ` Mark Brown
  2 siblings, 0 replies; 9+ messages in thread
From: Fabio Estevam @ 2017-01-03 21:08 UTC (permalink / raw)
  To: Caleb Crome
  Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Liam Girdwood,
	Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel,
	linuxppc-dev, linux-kernel, Arnaud Mouiche, Caleb Crome

On Tue, Jan 3, 2017 at 4:22 PM, Caleb Crome <ccrome@gmail.com> wrote:
> From: Caleb Crome <caleb@crome.org>
>
> The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
> activate DMA on FIFO when only 2 spaces are left.)  This means the
> DMA must service the fifo within 2 audio samples, which is just not
> enough time  for many use cases with high data rate.  In many
> configurations the audio channel slips (causing l/r swap in stereo
> configurations, or channel slipping in multi-channel configurations).
>
> This patch gives more breathing room and allows the SSI to operate
> reliably by changing the fifio refill watermark to 8.
>
> There is no change in behavior for older chips (with an 8-deep fifo).
> Only the newer chips with a 15-deep fifo get the new behavior. I
> suspect a new fifo depth setting could be optimized on the older
> chips too, but I have not tested.
>
> Signed-off-by: Caleb Crome <caleb@crome.org>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

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

* Re: [PATCH] fsl_ssi: set fifo watermark to more reliable value
  2017-01-03 18:22 [PATCH] fsl_ssi: set fifo watermark to more reliable value Caleb Crome
  2017-01-03 21:08 ` [alsa-devel] " Fabio Estevam
@ 2017-01-04 18:27 ` Mark Brown
  2017-01-04 18:31     ` ccrome
  2017-01-04 18:37   ` Mark Brown
  2 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2017-01-04 18:27 UTC (permalink / raw)
  To: Caleb Crome
  Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai, alsa-devel, linuxppc-dev,
	linux-kernel, Arnaud Mouiche, Caleb Crome

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On Tue, Jan 03, 2017 at 10:22:57AM -0800, Caleb Crome wrote:
> From: Caleb Crome <caleb@crome.org>
> 
> The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
> activate DMA on FIFO when only 2 spaces are left.)  This means the

Please submit patches using subject lines reflecting the style for the
subsystem.  This makes it easier for people to identify relevant
patches.  Look at what existing commits in the area you're changing are
doing and make sure your subject lines visually resemble what they're
doing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH] fsl_ssi: set fifo watermark to more reliable value
  2017-01-04 18:27 ` Mark Brown
@ 2017-01-04 18:31     ` ccrome
  0 siblings, 0 replies; 9+ messages in thread
From: ccrome @ 2017-01-04 18:31 UTC (permalink / raw)
  To: Mark Brown
  Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai, alsa-devel, linuxppc-dev,
	linux-kernel, Arnaud Mouiche

On Wed, Jan 4, 2017 at 10:27 AM, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jan 03, 2017 at 10:22:57AM -0800, Caleb Crome wrote:
> > From: Caleb Crome <caleb@crome.org>
> >
> > The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
> > activate DMA on FIFO when only 2 spaces are left.)  This means the
>
> Please submit patches using subject lines reflecting the style for the
> subsystem.  This makes it easier for people to identify relevant
> patches.  Look at what existing commits in the area you're changing are
> doing and make sure your subject lines visually resemble what they're
> doing.

Shoot, yes of course.  I forgot to check the subject format against
extant patches.

The subject should be

[PATCH] ASoC: fsl_ssi: set fifo watermark to more reliable value

Correct?

Should I re-submit?

Thanks,
 -Caleb

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

* Re: [PATCH] fsl_ssi: set fifo watermark to more reliable value
@ 2017-01-04 18:31     ` ccrome
  0 siblings, 0 replies; 9+ messages in thread
From: ccrome @ 2017-01-04 18:31 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Timur Tabi, Xiubo Li, linux-kernel, Takashi Iwai,
	Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Arnaud Mouiche,
	Fabio Estevam, linuxppc-dev

On Wed, Jan 4, 2017 at 10:27 AM, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jan 03, 2017 at 10:22:57AM -0800, Caleb Crome wrote:
> > From: Caleb Crome <caleb@crome.org>
> >
> > The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
> > activate DMA on FIFO when only 2 spaces are left.)  This means the
>
> Please submit patches using subject lines reflecting the style for the
> subsystem.  This makes it easier for people to identify relevant
> patches.  Look at what existing commits in the area you're changing are
> doing and make sure your subject lines visually resemble what they're
> doing.

Shoot, yes of course.  I forgot to check the subject format against
extant patches.

The subject should be

[PATCH] ASoC: fsl_ssi: set fifo watermark to more reliable value

Correct?

Should I re-submit?

Thanks,
 -Caleb

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

* Re: [PATCH] fsl_ssi: set fifo watermark to more reliable value
  2017-01-04 18:31     ` ccrome
@ 2017-01-04 18:35       ` Mark Brown
  -1 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2017-01-04 18:35 UTC (permalink / raw)
  To: ccrome
  Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai, alsa-devel, linuxppc-dev,
	linux-kernel, Arnaud Mouiche

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Wed, Jan 04, 2017 at 10:31:42AM -0800, ccrome wrote:

> The subject should be
> 
> [PATCH] ASoC: fsl_ssi: set fifo watermark to more reliable value
> 
> Correct?

Yes.

> Should I re-submit?

No, it's fine - if I'm sending one of those mails it'll be fine as I've
actually seen the mail to reply to it, the problem is if I don't see the
mail.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH] fsl_ssi: set fifo watermark to more reliable value
@ 2017-01-04 18:35       ` Mark Brown
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2017-01-04 18:35 UTC (permalink / raw)
  To: ccrome
  Cc: alsa-devel, Timur Tabi, Xiubo Li, linux-kernel, Takashi Iwai,
	Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Arnaud Mouiche,
	Fabio Estevam, linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Wed, Jan 04, 2017 at 10:31:42AM -0800, ccrome wrote:

> The subject should be
> 
> [PATCH] ASoC: fsl_ssi: set fifo watermark to more reliable value
> 
> Correct?

Yes.

> Should I re-submit?

No, it's fine - if I'm sending one of those mails it'll be fine as I've
actually seen the mail to reply to it, the problem is if I don't see the
mail.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Applied "ASoC: fsl_ssi: set fifo watermark to more reliable value" to the asoc tree
  2017-01-03 18:22 [PATCH] fsl_ssi: set fifo watermark to more reliable value Caleb Crome
@ 2017-01-04 18:37   ` Mark Brown
  2017-01-04 18:27 ` Mark Brown
  2017-01-04 18:37   ` Mark Brown
  2 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2017-01-04 18:37 UTC (permalink / raw)
  To: Caleb Crome
  Cc: Mark Brown, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam,
	Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	alsa-devel, linuxppc-dev, linux-kernel, Arnaud Mouiche

The patch

   ASoC: fsl_ssi: set fifo watermark to more reliable value

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 4ee437fbf626b5ad756889d8bc0fcead3d66dde7 Mon Sep 17 00:00:00 2001
From: Caleb Crome <caleb@crome.org>
Date: Tue, 3 Jan 2017 10:22:57 -0800
Subject: [PATCH] ASoC: fsl_ssi: set fifo watermark to more reliable value

The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
activate DMA on FIFO when only 2 spaces are left.)  This means the
DMA must service the fifo within 2 audio samples, which is just not
enough time  for many use cases with high data rate.  In many
configurations the audio channel slips (causing l/r swap in stereo
configurations, or channel slipping in multi-channel configurations).

This patch gives more breathing room and allows the SSI to operate
reliably by changing the fifio refill watermark to 8.

There is no change in behavior for older chips (with an 8-deep fifo).
Only the newer chips with a 15-deep fifo get the new behavior. I
suspect a new fifo depth setting could be optimized on the older
chips too, but I have not tested.

Signed-off-by: Caleb Crome <caleb@crome.org>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_ssi.c | 74 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 50349437d961..fde08660b63b 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -224,6 +224,12 @@ struct fsl_ssi_soc_data {
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ *
+ * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
+ *             there are @fifo_watermark or fewer words in TX fifo or
+ *             @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: max number of words to transfer in one go.  So far,
+ *             this is always the same as fifo_watermark.
  */
 struct fsl_ssi_private {
 	struct regmap *regs;
@@ -263,6 +269,9 @@ struct fsl_ssi_private {
 
 	const struct fsl_ssi_soc_data *soc;
 	struct device *dev;
+
+	u32 fifo_watermark;
+	u32 dma_maxburst;
 };
 
 /*
@@ -1051,21 +1060,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
 	regmap_write(regs, CCSR_SSI_SRCR, srcr);
 	regmap_write(regs, CCSR_SSI_SCR, scr);
 
-	/*
-	 * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
-	 * use FIFO 1. We program the transmit water to signal a DMA transfer
-	 * if there are only two (or fewer) elements left in the FIFO. Two
-	 * elements equals one frame (left channel, right channel). This value,
-	 * however, depends on the depth of the transmit buffer.
-	 *
-	 * We set the watermark on the same level as the DMA burstsize.  For
-	 * fiq it is probably better to use the biggest possible watermark
-	 * size.
-	 */
-	if (ssi_private->use_dma)
-		wm = ssi_private->fifo_depth - 2;
-	else
-		wm = ssi_private->fifo_depth;
+	wm = ssi_private->fifo_watermark;
 
 	regmap_write(regs, CCSR_SSI_SFCSR,
 			CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
@@ -1373,12 +1368,8 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 		dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
 			 PTR_ERR(ssi_private->baudclk));
 
-	/*
-	 * We have burstsize be "fifo_depth - 2" to match the SSI
-	 * watermark setting in fsl_ssi_startup().
-	 */
-	ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
-	ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
+	ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
+	ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
 	ssi_private->dma_params_tx.addr = ssi_private->ssi_phys + CCSR_SSI_STX0;
 	ssi_private->dma_params_rx.addr = ssi_private->ssi_phys + CCSR_SSI_SRX0;
 
@@ -1543,6 +1534,47 @@ static int fsl_ssi_probe(struct platform_device *pdev)
                 /* Older 8610 DTs didn't have the fifo-depth property */
 		ssi_private->fifo_depth = 8;
 
+	/*
+	 * Set the watermark for transmit FIFO 0 and receive FIFO 0. We don't
+	 * use FIFO 1 but set the watermark appropriately nontheless.
+	 * We program the transmit water to signal a DMA transfer
+	 * if there are N elements left in the FIFO. For chips with 15-deep
+	 * FIFOs, set watermark to 8.  This allows the SSI to operate at a
+	 * high data rate without channel slipping. Behavior is unchanged
+	 * for the older chips with a fifo depth of only 8.  A value of 4
+	 * might be appropriate for the older chips, but is left at
+	 * fifo_depth-2 until sombody has a chance to test.
+	 *
+	 * We set the watermark on the same level as the DMA burstsize.  For
+	 * fiq it is probably better to use the biggest possible watermark
+	 * size.
+	 */
+	switch (ssi_private->fifo_depth) {
+	case 15:
+		/*
+		 * 2 samples is not enough when running at high data
+		 * rates (like 48kHz @ 16 bits/channel, 16 channels)
+		 * 8 seems to split things evenly and leave enough time
+		 * for the DMA to fill the FIFO before it's over/under
+		 * run.
+		 */
+		ssi_private->fifo_watermark = 8;
+		ssi_private->dma_maxburst = 8;
+		break;
+	case 8:
+	default:
+		/*
+		 * maintain old behavior for older chips.
+		 * Keeping it the same because I don't have an older
+		 * board to test with.
+		 * I suspect this could be changed to be something to
+		 * leave some more space in the fifo.
+		 */
+		ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
+		ssi_private->dma_maxburst = ssi_private->fifo_depth - 2;
+		break;
+	}
+
 	dev_set_drvdata(&pdev->dev, ssi_private);
 
 	if (ssi_private->soc->imx) {
-- 
2.11.0

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

* Applied "ASoC: fsl_ssi: set fifo watermark to more reliable value" to the asoc tree
@ 2017-01-04 18:37   ` Mark Brown
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2017-01-04 18:37 UTC (permalink / raw)
  To: Caleb Crome
  Cc: alsa-devel, Xiubo Li, linux-kernel, Takashi Iwai, Liam Girdwood,
	Timur Tabi, Nicolin Chen, Mark Brown, Arnaud Mouiche,
	Fabio Estevam, Jaroslav Kysela, linuxppc-dev

The patch

   ASoC: fsl_ssi: set fifo watermark to more reliable value

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 4ee437fbf626b5ad756889d8bc0fcead3d66dde7 Mon Sep 17 00:00:00 2001
From: Caleb Crome <caleb@crome.org>
Date: Tue, 3 Jan 2017 10:22:57 -0800
Subject: [PATCH] ASoC: fsl_ssi: set fifo watermark to more reliable value

The fsl_ssi fifo watermark is by default set to 2 free spaces (i.e.
activate DMA on FIFO when only 2 spaces are left.)  This means the
DMA must service the fifo within 2 audio samples, which is just not
enough time  for many use cases with high data rate.  In many
configurations the audio channel slips (causing l/r swap in stereo
configurations, or channel slipping in multi-channel configurations).

This patch gives more breathing room and allows the SSI to operate
reliably by changing the fifio refill watermark to 8.

There is no change in behavior for older chips (with an 8-deep fifo).
Only the newer chips with a 15-deep fifo get the new behavior. I
suspect a new fifo depth setting could be optimized on the older
chips too, but I have not tested.

Signed-off-by: Caleb Crome <caleb@crome.org>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_ssi.c | 74 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 50349437d961..fde08660b63b 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -224,6 +224,12 @@ struct fsl_ssi_soc_data {
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ *
+ * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
+ *             there are @fifo_watermark or fewer words in TX fifo or
+ *             @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: max number of words to transfer in one go.  So far,
+ *             this is always the same as fifo_watermark.
  */
 struct fsl_ssi_private {
 	struct regmap *regs;
@@ -263,6 +269,9 @@ struct fsl_ssi_private {
 
 	const struct fsl_ssi_soc_data *soc;
 	struct device *dev;
+
+	u32 fifo_watermark;
+	u32 dma_maxburst;
 };
 
 /*
@@ -1051,21 +1060,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
 	regmap_write(regs, CCSR_SSI_SRCR, srcr);
 	regmap_write(regs, CCSR_SSI_SCR, scr);
 
-	/*
-	 * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
-	 * use FIFO 1. We program the transmit water to signal a DMA transfer
-	 * if there are only two (or fewer) elements left in the FIFO. Two
-	 * elements equals one frame (left channel, right channel). This value,
-	 * however, depends on the depth of the transmit buffer.
-	 *
-	 * We set the watermark on the same level as the DMA burstsize.  For
-	 * fiq it is probably better to use the biggest possible watermark
-	 * size.
-	 */
-	if (ssi_private->use_dma)
-		wm = ssi_private->fifo_depth - 2;
-	else
-		wm = ssi_private->fifo_depth;
+	wm = ssi_private->fifo_watermark;
 
 	regmap_write(regs, CCSR_SSI_SFCSR,
 			CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
@@ -1373,12 +1368,8 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 		dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
 			 PTR_ERR(ssi_private->baudclk));
 
-	/*
-	 * We have burstsize be "fifo_depth - 2" to match the SSI
-	 * watermark setting in fsl_ssi_startup().
-	 */
-	ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
-	ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
+	ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
+	ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
 	ssi_private->dma_params_tx.addr = ssi_private->ssi_phys + CCSR_SSI_STX0;
 	ssi_private->dma_params_rx.addr = ssi_private->ssi_phys + CCSR_SSI_SRX0;
 
@@ -1543,6 +1534,47 @@ static int fsl_ssi_probe(struct platform_device *pdev)
                 /* Older 8610 DTs didn't have the fifo-depth property */
 		ssi_private->fifo_depth = 8;
 
+	/*
+	 * Set the watermark for transmit FIFO 0 and receive FIFO 0. We don't
+	 * use FIFO 1 but set the watermark appropriately nontheless.
+	 * We program the transmit water to signal a DMA transfer
+	 * if there are N elements left in the FIFO. For chips with 15-deep
+	 * FIFOs, set watermark to 8.  This allows the SSI to operate at a
+	 * high data rate without channel slipping. Behavior is unchanged
+	 * for the older chips with a fifo depth of only 8.  A value of 4
+	 * might be appropriate for the older chips, but is left at
+	 * fifo_depth-2 until sombody has a chance to test.
+	 *
+	 * We set the watermark on the same level as the DMA burstsize.  For
+	 * fiq it is probably better to use the biggest possible watermark
+	 * size.
+	 */
+	switch (ssi_private->fifo_depth) {
+	case 15:
+		/*
+		 * 2 samples is not enough when running at high data
+		 * rates (like 48kHz @ 16 bits/channel, 16 channels)
+		 * 8 seems to split things evenly and leave enough time
+		 * for the DMA to fill the FIFO before it's over/under
+		 * run.
+		 */
+		ssi_private->fifo_watermark = 8;
+		ssi_private->dma_maxburst = 8;
+		break;
+	case 8:
+	default:
+		/*
+		 * maintain old behavior for older chips.
+		 * Keeping it the same because I don't have an older
+		 * board to test with.
+		 * I suspect this could be changed to be something to
+		 * leave some more space in the fifo.
+		 */
+		ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
+		ssi_private->dma_maxburst = ssi_private->fifo_depth - 2;
+		break;
+	}
+
 	dev_set_drvdata(&pdev->dev, ssi_private);
 
 	if (ssi_private->soc->imx) {
-- 
2.11.0

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

end of thread, other threads:[~2017-01-04 18:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-03 18:22 [PATCH] fsl_ssi: set fifo watermark to more reliable value Caleb Crome
2017-01-03 21:08 ` [alsa-devel] " Fabio Estevam
2017-01-04 18:27 ` Mark Brown
2017-01-04 18:31   ` ccrome
2017-01-04 18:31     ` ccrome
2017-01-04 18:35     ` Mark Brown
2017-01-04 18:35       ` Mark Brown
2017-01-04 18:37 ` Applied "ASoC: fsl_ssi: set fifo watermark to more reliable value" to the asoc tree Mark Brown
2017-01-04 18:37   ` Mark Brown

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.