[alsa-devel] [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions
This series of patch is an attempt to fix the fsl_ssi driver to use it in TDM mode (DSP A or B) with a large number of channels/slots.
Bugs are detected and fixed on a imx6sl platform with linux 4.3, where 2 SSI interfaces are used. SSI3 configured as a master of the bus, SSI2 as a slave.
Various loopback scenario are tested: * scenario 1: SSI3 (master) with TXD to RXD loopback
| |---> TXD --\ SSI3 |<--- RXD --/ (master) |---> SYNC |---> BCLK | * scenario 2: SSI3 connected to SSI2 | |---> TXD --------\ SSI3 |<--- RXD -----\ | (master) |---> SYNC --\ | | |---> BCLK | | | | | | | | | | | | | |<--- BCLK | | | SSI2 |<--- SYNC --/ | | (slave) |---> TXD ------/ | |<--- RXD --------/ |
A test software (called atest) was developed and available. [1] It basically generate/check specials frames with S16_LE sequence samples NNN0, NNN1, NNN2 ... NNNC with NNN = frame number, and C the number of channels-1
Patch 1: Limitation fix. Prerequisite to use the fsl_ssi driver with up to 32 channels / slots
Patch 2: Bug fix. Prerequisite to setup a relative high bitclk for our tests in 8ch / 16bits / 48kHz
Patch 3: Simply save a 'dev' reference inside ssi_private for dev_err() purpose. (ssi_private is only available in lot of places, and ssi_private->pdev is deprecated and NULL indeed)
Patch 4: Fix playback samples being dropped because the TX fifo was not ready at the time the DMA starts filling (only in case where playback is started AFTER the capture)
Detected in loopback scenario 1, with following script (> 80 % of reproducibility) $ atest -D SSI3 -c 8 -r 48000 capture play dbg: set period size: 960 dbg: SSI3: capture_start dbg: SSI3: playback_start err: invalid frame after 1 null frames err: 0000 0000 0013 0014 0015 0016 0017 0020 dbg: SIGINT total number of sequence errors: 21119 here we see that samples 0000 0001 ... 0012 are not present in the output. In addition, this the number of samples dropped is not a multiple of the number of channel, we have a a channel slip.
Patch 5: This is the Caleb Crome's case [2], where the SSI starts to generate samples while the TX FIFO is not filled by the DMA yet. Void samples can be inserted before DMA streaming.
Detected in loopback scenario 2, with following script (reproducibility < 1%) $ atest -D SSI3 -c 8 -r 48000 capture & $ sleep 0.2 $ atest -d 1 -D SSI2 -c 8 -r 48000 play dbg: SSI3: capture_start dbg: set period size: 960 dbg: SSI2: playback_start dbg: start a 1 seconds duration timer err: invalid frame after 11568 null frames err: 0000 0010 0011 0012 0013 0014 0015 0016 err: 0017 0020 0021 0022 0023 0024 0025 0026
Patch 6: Deals with Capture restart whereas Playback is still running (or the opposite, Playback restart whereas Capture is till running). Capture restart whereas Playback case is detected in loopback scenario 1 (reproducibility 100%). A playback session is running in background, and 2 consecutive Captures are performed. The first is fine, the second fails. We see at the 2nd capture startup that we receive 15 samples still pending in the RX FIFO from the previous capture session. => We are receiving invalid samples + slips the channels $ atest -D SSI3 -c 8 -r 48000 play & dbg: SSI3: playback_start $ atest -d 1 -D SSI3 -c 8 -r 48000 capture dbg: SSI3: capture_start dbg: start a 1 seconds duration timer warn: Valid frame after 1 null frames warn: 3a90 3a91 3a92 3a93 3a94 3a95 3a96 3a97 total number of sequence errors: 0 $ atest -I 5 -d 1 -D SSI3 -c 8 -r 48000 capture # restart the capture dbg: SSI3: capture_start dbg: start a 1 seconds duration timer err: invalid frame after 1 null frames err: 8dd6 8dd7 8de0 8de1 8de2 8de3 8de4 8de5 err: 8de6 8de7 8df0 8df1 8df2 8df3 8df4 c0c0 err: c0c1 c0c2 c0c3 c0c4 c0c5 c0c6 c0c7 c0d0 err: c0d1 c0d2 c0d3 c0d4 c0d5 c0d6 c0d7 c0e0 err: c0e1 c0e2 c0e3 c0e4 c0e5 c0e6 c0e7 c0f0
Playback restart whereas Capture case is also detected with loopback scenario 1 (reproducibility 100%), capturing continuously, and playing by periods of time. $ atest -a -D SSI3 -c 8 -r 48000 capture play -r 1000,200 dbg: SSI3: capture_start dbg: SSI3: playback_start dbg: SSI3: will stop every 1000 ms during 200 ms warn: Valid frame after 4 null frames warn: 0010 0011 0012 0013 0014 0015 0016 0017 warn: SSI3: PT_W4_STOP warn: SSI3: PT_W4_RESTART err: invalid frame after 1602 null frames err: eaa1 eaa2 eaa3 eaa4 eaa5 eaa6 eaa7 eab0 err: eab1 eab2 eab3 eab4 5810 5811 5812 5813 dbg: stop on first error total number of sequence errors: 143
Both cases are resolved by using the mostly undocumented SOR.RX_CLR and SOR TX_CLR (we can find the documentation in IMX51 reference manual at section 56.3.3.15). Arnaud
[1] https://github.com/amouiche/atest [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099221.htm...
Arnaud Mouiche (5): ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk. ASoC: fsl_ssi: Save a dev reference for dev_err() purpose. ASoC: fsl_ssi: Fix samples being dropped as Playback startup ASoC: fsl_ssi: Fix channel slipping in Playback at startup ASoC: fsl_ssi: Fix channel slipping on capture (or playback) restart in full duplex.
sound/soc/fsl/fsl_ssi.c | 69 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 9 deletions(-)
im6sl reference manual 47.7.4: " Bit clock - Used to serially clock the data bits in and out of the SSI port. This clock is either generated internally (from SSI's sys clock) or taken from external clock source (through the Tx/Rx clock ports). [...] Care should be taken to ensure that the bit clock frequency (either internally generated by dividing the SSI's sys clock or sourced from external device through Tx/Rx clock ports) is never greater than 1/5 of the ipg_clk (from CCM) frequency. "
Since, in master mode, the sysclk is a multiple of bitclk, we can easily reach a high sysclk value, whereas keeping a reasonable bitclk.
ex: 8ch x 16bit x 48kHz = 6144000, requires a 24576000 sysclk (PM=1) yet ipg_clk/5 = 66Mhz/5 = 13.2
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com --- sound/soc/fsl/fsl_ssi.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 4e5d7db..b34c09f 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -595,6 +595,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, if (IS_ERR(ssi_private->baudclk)) return -EINVAL;
+ /* + * Hardware limitation: The bclk rate must be + * never greater than 1/5 IPG clock rate + */ + if (freq * 5 > clk_get_rate(ssi_private->clk)) { + dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); + return -EINVAL; + } + baudclk_is_used = ssi_private->baudclk_streams & ~(BIT(substream->stream));
/* It should be already enough to divide clock by setting pm alone */ @@ -611,13 +620,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, else clkrate = clk_round_rate(ssi_private->baudclk, tmprate);
- /* - * Hardware limitation: The bclk rate must be - * never greater than 1/5 IPG clock rate - */ - if (clkrate * 5 > clk_get_rate(ssi_private->clk)) - continue; - clkrate /= factor; afreq = clkrate / (i + 1);
The patch
ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk.
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 e09745f2e6a1f692fc63db01850aacf025475aad Mon Sep 17 00:00:00 2001
From: Arnaud Mouiche arnaud.mouiche@invoxia.com Date: Tue, 3 May 2016 14:13:56 +0200 Subject: [PATCH] ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk.
im6sl reference manual 47.7.4: " Bit clock - Used to serially clock the data bits in and out of the SSI port. This clock is either generated internally (from SSI's sys clock) or taken from external clock source (through the Tx/Rx clock ports). [...] Care should be taken to ensure that the bit clock frequency (either internally generated by dividing the SSI's sys clock or sourced from external device through Tx/Rx clock ports) is never greater than 1/5 of the ipg_clk (from CCM) frequency. "
Since, in master mode, the sysclk is a multiple of bitclk, we can easily reach a high sysclk value, whereas keeping a reasonable bitclk.
ex: 8ch x 16bit x 48kHz = 6144000, requires a 24576000 sysclk (PM=1) yet ipg_clk/5 = 66Mhz/5 = 13.2
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com Reviewed-by: Fabio Estevam fabio.estevam@nxp.com Tested-by: Caleb Crome caleb@crome.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/fsl/fsl_ssi.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 8d5f3c192de2..86229c8902d2 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -670,6 +670,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, if (IS_ERR(ssi_private->baudclk)) return -EINVAL;
+ /* + * Hardware limitation: The bclk rate must be + * never greater than 1/5 IPG clock rate + */ + if (freq * 5 > clk_get_rate(ssi_private->clk)) { + dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); + return -EINVAL; + } + baudclk_is_used = ssi_private->baudclk_streams & ~(BIT(substream->stream));
/* It should be already enough to divide clock by setting pm alone */ @@ -686,13 +695,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, else clkrate = clk_round_rate(ssi_private->baudclk, tmprate);
- /* - * Hardware limitation: The bclk rate must be - * never greater than 1/5 IPG clock rate - */ - if (clkrate * 5 > clk_get_rate(ssi_private->clk)) - continue; - clkrate /= factor; afreq = clkrate / (i + 1);
Most of functions only receive the ssi_private reference and don't have a knowledge of 'dev' pointer, even for debug purpose.
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com --- sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index b34c09f..243c9af 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -191,6 +191,7 @@ struct fsl_ssi_private { struct fsl_ssi_dbg dbg_stats;
const struct fsl_ssi_soc_data *soc; + struct device *dev; };
/* @@ -1329,6 +1330,7 @@ static int fsl_ssi_probe(struct platform_device *pdev) }
ssi_private->soc = of_id->data; + ssi_private->dev = &pdev->dev;
sprop = of_get_property(np, "fsl,mode", NULL); if (sprop) {
The patch
ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.
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 0096b693962d3abde4f41b13cf03c765f33e9d8d Mon Sep 17 00:00:00 2001
From: Arnaud Mouiche arnaud.mouiche@invoxia.com Date: Tue, 3 May 2016 14:13:57 +0200 Subject: [PATCH] ASoC: fsl_ssi: Save a dev reference for dev_err() purpose.
Most of functions only receive the ssi_private reference and don't have a knowledge of 'dev' pointer, even for debug purpose.
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com Tested-by: Caleb Crome caleb@crome.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 86229c8902d2..149df3ca4f5e 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -261,6 +261,7 @@ struct fsl_ssi_private { struct fsl_ssi_dbg dbg_stats;
const struct fsl_ssi_soc_data *soc; + struct device *dev; };
/* @@ -1404,6 +1405,7 @@ static int fsl_ssi_probe(struct platform_device *pdev) }
ssi_private->soc = of_id->data; + ssi_private->dev = &pdev->dev;
sprop = of_get_property(np, "fsl,mode", NULL); if (sprop) {
If the capture is already running while playback is started, it is highly probable (>80% in a 8 channels scenario) that samples are lost between the DMA and TX fifo.
The reason is that SIER.TDMAE is set before STCR.TFEN0, leaving a time window where the FIFO doesn't receive the samples written by the DMA.
This particular case happened only if capture is already enabled as SCR.SSIEN is already set at the playback startup instant.
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com --- sound/soc/fsl/fsl_ssi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 243c9af..78ea6e1 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -404,9 +404,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable, * (online configuration) */ if (enable) { - regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier); regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr); regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr); + regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier); } else { u32 sier; u32 srcr;
The patch
ASoC: fsl_ssi: Fix samples being dropped at Playback startup
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 d9f2a202877c15818d98268f47d6b4bcfcb84437 Mon Sep 17 00:00:00 2001
From: Arnaud Mouiche arnaud.mouiche@invoxia.com Date: Tue, 3 May 2016 14:13:58 +0200 Subject: [PATCH] ASoC: fsl_ssi: Fix samples being dropped at Playback startup
If the capture is already running while playback is started, it is highly probable (>80% in a 8 channels scenario) that samples are lost between the DMA and TX fifo.
The reason is that SIER.TDMAE is set before STCR.TFEN0, leaving a time window where the FIFO doesn't receive the samples written by the DMA.
This particular case happened only if capture is already enabled as SCR.SSIEN is already set at the playback startup instant.
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com Reviewed-by: Fabio Estevam fabio.estevam@nxp.com Tested-by: Caleb Crome caleb@crome.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/fsl/fsl_ssi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 149df3ca4f5e..47ebb835f3f5 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -475,9 +475,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable, * (online configuration) */ if (enable) { - regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier); regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr); regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr); + regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier); } else { u32 sier; u32 srcr;
Previously, SCR.SSIEN and SCR.TE were enabled at once if no capture stream was also running. This may not give a chance for the DMA to write the first sample in TX FIFO before the streaming starts on the PCM bus, inserting void samples first. Those void samples are then responsible for slipping the channels.
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com --- sound/soc/fsl/fsl_ssi.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 78ea6e1..c71e194 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -436,8 +436,39 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable,
config_done: /* Enabling of subunits is done after configuration */ - if (enable) + if (enable) { + if (ssi_private->use_dma && (vals->scr & CCSR_SSI_SCR_TE)) { + /* + * Be sure the Tx FIFO is filled when TE is set. + * Otherwise, there are some chances to start the + * playback with some void samples inserted first, + * generating a channel slip. + * + * First, SSIEN must be set, to let the FIFO be filled. + * + * Notes: + * - Limit this fix to the DMA case until FIQ cases can + * be tested. + * - Limit the length of the busy loop to not lock the + * system too long, even if 1-2 loops are sufficient + * in general. + */ + int i; + int max_loop = 100; + regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN, + CCSR_SSI_SCR_SSIEN); + for (i=0; i < max_loop; i++) { + u32 sfcsr; + regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr); + if (CCSR_SSI_SFCSR_TFCNT0(sfcsr)) break; + } + if (i == max_loop) { + dev_err(ssi_private->dev, + "Timeout waiting TX FIFO filling\n"); + } + } regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr); + } }
Happened when the Playback (or Capture) is running continuously and Capture (or Playback) is restarted (xrun, manual stop/start...)
Since the RX (or TX) FIFO are only reset when the whole SSI is disabled, pending samples from previous capture (or playback) session may still be present. They must be erased to not introduce channel slipping.
FIFO Clear register fields are documented in IMX51, IMX35 reference manual. They are not documented in IMX50 or IMX6 RM, despite they are working as expected on IMX6SL.
Signed-off-by: Arnaud Mouiche arnaud.mouiche@invoxia.com --- sound/soc/fsl/fsl_ssi.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index c71e194..576a72b 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -404,6 +404,22 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable, * (online configuration) */ if (enable) { + /* + * Clear RX or TX FIFO to remove samples from the previous stream + * session which may be still present in the FIFO and may + * introduce bad samples and/or channel slipping. + * + * Note: The SOR is not documented in recent IMX datasheet, but + * is described in IMX51 reference manual at section 56.3.3.15. + */ + if (vals->scr & CCSR_SSI_SCR_RE) { + regmap_update_bits(regs, CCSR_SSI_SOR, + CCSR_SSI_SOR_RX_CLR, CCSR_SSI_SOR_RX_CLR ); + } else { + regmap_update_bits(regs, CCSR_SSI_SOR, + CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR ); + } + regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr); regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr); regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier);
On Thu, Nov 26, 2015 at 6:07 AM, Arnaud Mouiche arnaud.mouiche@invoxia.com wrote:
This series of patch is an attempt to fix the fsl_ssi driver to use it in TDM mode (DSP A or B) with a large number of channels/slots.
Bugs are detected and fixed on a imx6sl platform with linux 4.3, where 2 SSI interfaces are used. SSI3 configured as a master of the bus, SSI2 as a slave.
Various loopback scenario are tested:
scenario 1: SSI3 (master) with TXD to RXD loopback
| |---> TXD --\ SSI3 |<--- RXD --/ (master) |---> SYNC |---> BCLK |scenario 2: SSI3 connected to SSI2 | |---> TXD --------\ SSI3 |<--- RXD -----\ | (master) |---> SYNC --\ | | |---> BCLK | | | | | | | | | | | | | |<--- BCLK | | | SSI2 |<--- SYNC --/ | | (slave) |---> TXD ------/ | |<--- RXD --------/ |
A test software (called atest) was developed and available. [1] It basically generate/check specials frames with S16_LE sequence samples NNN0, NNN1, NNN2 ... NNNC with NNN = frame number, and C the number of channels-1
Patch 1: Limitation fix. Prerequisite to use the fsl_ssi driver with up to 32 channels / slots
Patch 2: Bug fix. Prerequisite to setup a relative high bitclk for our tests in 8ch / 16bits / 48kHz
Patch 3: Simply save a 'dev' reference inside ssi_private for dev_err() purpose. (ssi_private is only available in lot of places, and ssi_private->pdev is deprecated and NULL indeed)
Patch 4: Fix playback samples being dropped because the TX fifo was not ready at the time the DMA starts filling (only in case where playback is started AFTER the capture)
Detected in loopback scenario 1, with following script (> 80 % of reproducibility) $ atest -D SSI3 -c 8 -r 48000 capture play dbg: set period size: 960 dbg: SSI3: capture_start dbg: SSI3: playback_start err: invalid frame after 1 null frames err: 0000 0000 0013 0014 0015 0016 0017 0020 dbg: SIGINT total number of sequence errors: 21119 here we see that samples 0000 0001 ... 0012 are not present in the output. In addition, this the number of samples dropped is not a multiple of the number of channel, we have a a channel slip.Patch 5: This is the Caleb Crome's case [2], where the SSI starts to generate samples while the TX FIFO is not filled by the DMA yet. Void samples can be inserted before DMA streaming.
Detected in loopback scenario 2, with following script (reproducibility < 1%) $ atest -D SSI3 -c 8 -r 48000 capture & $ sleep 0.2 $ atest -d 1 -D SSI2 -c 8 -r 48000 play dbg: SSI3: capture_start dbg: set period size: 960 dbg: SSI2: playback_start dbg: start a 1 seconds duration timer err: invalid frame after 11568 null frames err: 0000 0010 0011 0012 0013 0014 0015 0016 err: 0017 0020 0021 0022 0023 0024 0025 0026Patch 6: Deals with Capture restart whereas Playback is still running (or the opposite, Playback restart whereas Capture is till running).
Capture restart whereas Playback case is detected in loopback scenario 1 (reproducibility 100%). A playback session is running in background, and 2 consecutive Captures are performed. The first is fine, the second fails. We see at the 2nd capture startup that we receive 15 samples still pending in the RX FIFO from the previous capture session. => We are receiving invalid samples + slips the channels $ atest -D SSI3 -c 8 -r 48000 play & dbg: SSI3: playback_start $ atest -d 1 -D SSI3 -c 8 -r 48000 capture dbg: SSI3: capture_start dbg: start a 1 seconds duration timer warn: Valid frame after 1 null frames warn: 3a90 3a91 3a92 3a93 3a94 3a95 3a96 3a97 total number of sequence errors: 0 $ atest -I 5 -d 1 -D SSI3 -c 8 -r 48000 capture # restart the capture dbg: SSI3: capture_start dbg: start a 1 seconds duration timer err: invalid frame after 1 null frames err: 8dd6 8dd7 8de0 8de1 8de2 8de3 8de4 8de5 err: 8de6 8de7 8df0 8df1 8df2 8df3 8df4 c0c0 err: c0c1 c0c2 c0c3 c0c4 c0c5 c0c6 c0c7 c0d0 err: c0d1 c0d2 c0d3 c0d4 c0d5 c0d6 c0d7 c0e0 err: c0e1 c0e2 c0e3 c0e4 c0e5 c0e6 c0e7 c0f0 Playback restart whereas Capture case is also detected with loopback scenario 1 (reproducibility 100%), capturing continuously, and playing by periods of time. $ atest -a -D SSI3 -c 8 -r 48000 capture play -r 1000,200 dbg: SSI3: capture_start dbg: SSI3: playback_start dbg: SSI3: will stop every 1000 ms during 200 ms warn: Valid frame after 4 null frames warn: 0010 0011 0012 0013 0014 0015 0016 0017 warn: SSI3: PT_W4_STOP warn: SSI3: PT_W4_RESTART err: invalid frame after 1602 null frames err: eaa1 eaa2 eaa3 eaa4 eaa5 eaa6 eaa7 eab0 err: eab1 eab2 eab3 eab4 5810 5811 5812 5813 dbg: stop on first error total number of sequence errors: 143 Both cases are resolved by using the mostly undocumented SOR.RX_CLR and SOR TX_CLR (we can find the documentation in IMX51 reference manual at section 56.3.3.15).Arnaud
[1] https://github.com/amouiche/atest [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/099221.htm...
Arnaud Mouiche (5): ASoC: fsl_ssi: The IPG/5 limitation concerns the bitclk, not the sysclk. ASoC: fsl_ssi: Save a dev reference for dev_err() purpose. ASoC: fsl_ssi: Fix samples being dropped as Playback startup ASoC: fsl_ssi: Fix channel slipping in Playback at startup ASoC: fsl_ssi: Fix channel slipping on capture (or playback) restart in full duplex.
sound/soc/fsl/fsl_ssi.c | 69 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 9 deletions(-)
-- 1.9.1
Hello Arnaud, I have finally gotten to test your patches, and I'm still having trouble with channel slips.
I applied your v2 patch set, along with your changes for using a dummy codec.
The full changes are here: https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
This ignores most of my previous patches, and uses your code to bring up the SSI (without a codec) on a wandboard.
I am using SSI3, and doing a hardware loopback between TX and RX.
Here's what I run: ./atest -r 16000 -c 8 -p 2048 -D default play
which plays continuously.
and in another shell: ./atest -r 16000 -c 8 -p 2048 -D default -d 10 capture
which captures for 10 seconds.
The first time I run the capture command, it succeeds, no problem.
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer warn: First valid frame warn: 3400 3401 3402 3403 3404 3405 3406 3407 dbg: end of tests total number of sequence errors: 0 global tests exit status: OK
But the second and all subsequent captures, it fails with channel slips:
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer err: invalid frame after 0 null frames err: d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0 err: d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0 dbg: end of tests total number of sequence errors: 430080 global tests exit status: OK
I verified with a scope that the data on the data bus is correct, the problem is with restarting RX, the RX doesn't synchronize to the frame sync. BTW, the RX slot is random. 1/8 times or so, the restarted capture actually works.
My setup is a single core wandboard (i.mx6 solo), with J1.16 jumpered over to J1.20 (tx->rx)
Thanks for the patches and for the atest program! It's really handy :-)
So, I have not messed with water marks, dual fifo, etc with your patches yet.
Any other patches that you think might need to be applied to make the RX restart work? -Caleb
Hello Caleb
Le 09/01/2016 01:47, Caleb Crome a écrit :
[...] Hello Arnaud, I have finally gotten to test your patches, and I'm still having trouble with channel slips.
I applied your v2 patch set, along with your changes for using a dummy codec.
The full changes are here: https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
This ignores most of my previous patches, and uses your code to bring up the SSI (without a codec) on a wandboard.
I am using SSI3, and doing a hardware loopback between TX and RX.
Here's what I run: ./atest -r 16000 -c 8 -p 2048 -D default play
which plays continuously.
and in another shell: ./atest -r 16000 -c 8 -p 2048 -D default -d 10 capture
which captures for 10 seconds.
The first time I run the capture command, it succeeds, no problem.
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer warn: First valid frame warn: 3400 3401 3402 3403 3404 3405 3406 3407 dbg: end of tests total number of sequence errors: 0 global tests exit status: OK
But the second and all subsequent captures, it fails with channel slips:
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer err: invalid frame after 0 null frames err: d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0 err: d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0 dbg: end of tests total number of sequence errors: 430080 global tests exit status: OK
Can you use -I option to get a little more log of the error $ ./atest -r 16000 -c 8 -p 2048 -D default -d 10 -I 10 capture
Just to know if the wrong frames comes from the previous record session, meaning the RX fifo was not cleared correctly, as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect. (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be the start of the new recording session)
May be you can read the RX fifo level just avec applying CCSR_SSI_SOR_RX_CLR to assert the fifo is really empty ?
In case it is still not empty, we will still have the possibility to empty the fifo by ready the samples one by one manually.
Regards, arnaud
I verified with a scope that the data on the data bus is correct, the problem is with restarting RX, the RX doesn't synchronize to the frame sync. BTW, the RX slot is random. 1/8 times or so, the restarted capture actually works.
My setup is a single core wandboard (i.mx6 solo), with J1.16 jumpered over to J1.20 (tx->rx)
Thanks for the patches and for the atest program! It's really handy :-)
So, I have not messed with water marks, dual fifo, etc with your patches yet.
Any other patches that you think might need to be applied to make the RX restart work? -Caleb
On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
Hello Caleb
Le 09/01/2016 01:47, Caleb Crome a écrit :
[...]
Hello Arnaud, I have finally gotten to test your patches, and I'm still having trouble with channel slips.
I applied your v2 patch set, along with your changes for using a dummy codec.
The full changes are here: https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
This ignores most of my previous patches, and uses your code to bring up the SSI (without a codec) on a wandboard.
I am using SSI3, and doing a hardware loopback between TX and RX.
Here's what I run: ./atest -r 16000 -c 8 -p 2048 -D default play
which plays continuously.
and in another shell: ./atest -r 16000 -c 8 -p 2048 -D default -d 10 capture
which captures for 10 seconds.
The first time I run the capture command, it succeeds, no problem.
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer warn: First valid frame warn: 3400 3401 3402 3403 3404 3405 3406 3407 dbg: end of tests total number of sequence errors: 0 global tests exit status: OK
But the second and all subsequent captures, it fails with channel slips:
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer err: invalid frame after 0 null frames err: d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0 err: d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0 dbg: end of tests total number of sequence errors: 430080 global tests exit status: OK
Can you use -I option to get a little more log of the error $ ./atest -r 16000 -c 8 -p 2048 -D default -d 10 -I 10 capture
Just to know if the wrong frames comes from the previous record session, meaning the RX fifo was not cleared correctly, as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect. (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be the start of the new recording session)
That does appear to be what's happening. I read the SFCSR before and after you update the SOR register (SOR_RX_CLR), and in both cases the rx buffer is full on the second enablement.
I can't seem to empty that buffer, either by using the SOR_RX_CLR or by reading the SRX0 register. This is another one of those cases where there you cannot modify the register (i.e. fifo or RX0) when RE is disabled.
So, instead of clearing on enable, I'm clearing on shutdown and on enable. I attempted using the SOR_RX_CLR, but it doesn't seem to work on the MX6. Rather, I clear the fifo by reading all the words in a loop, just before RE is disabled.
May be you can read the RX fifo level just avec applying CCSR_SSI_SOR_RX_CLR to assert the fifo is really empty ?
In case it is still not empty, we will still have the possibility to empty the fifo by ready the samples one by one manually.
so far, with cursory testing, this patch, on top of your V2 patches seems to work.
From 15a218858a75163e2d72ed8329a26b0d22a0bfc0 Mon Sep 17 00:00:00 2001
From: Caleb Crome caleb@crome.org Date: Mon, 11 Jan 2016 15:29:13 -0800 Subject: [PATCH] ASoC: fsl_ssi: Empty rx fifo on rx start and finish to eliminate channel slips
Arnaud's previous patches mostly, but not completely work on the MX6. The previous patch used the SOR_RX_CLR register, which doesn't seem to work on the MX6. I guess it's undocumented for a reason.
This patch clears the RX fifo by using successive reads of the SRX0 (and optionally SRX1) register until the SISR_RDR0/1 bit is clear. This ensure's an empty fifo to start capturing with.
Signed-off-by: Caleb Crome caleb@crome.org --- sound/soc/fsl/fsl_ssi.c | 44 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index e44c9c3..5329aaa 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -366,6 +366,40 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) return IRQ_HANDLED; }
+// empty fifo: must be called when the unit is ENABLED! +// otherwise it will not be able to empty the fifos. +static u32 fsl_empty_fifo(struct device *dev, struct regmap *regs, int is_rx, int dual_fifo) +{ + u32 val; + u32 sisr; + u32 err = 0; + int limit = 100; + if (is_rx) { + do { + regmap_read(regs, CCSR_SSI_SRX0, &val); + regmap_read(regs, CCSR_SSI_SISR, &sisr); + } while ((limit-- >=0) && (sisr & CCSR_SSI_SISR_RDR0)); + if (limit <= 0) { + dev_err(dev, "Couldn't empty out the RX " + "FIFO 0. SISR = 0x%08x\n", sisr); + err = 1; + } + if (dual_fifo) { + do { + regmap_read(regs, CCSR_SSI_SRX1, &val); + regmap_read(regs, CCSR_SSI_SISR, &sisr); + } while ((limit-- >=0) && + (sisr & CCSR_SSI_SISR_RDR1)); + if (limit <= 0) { + dev_err(dev, "Couldn't empty out the RX " + "FIFO 1. SISR = 0x%08x\n", sisr); + err = 1; + } + } + } + return err; +} + /* * Enable/Disable all rx/tx config flags at once. */ @@ -428,6 +462,7 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable, u32 scr_val; int keep_active;
+ int is_rx = vals->scr & CCSR_SSI_SCR_RE; regmap_read(regs, CCSR_SSI_SCR, &scr_val);
nr_active_streams = !!(scr_val & CCSR_SSI_SCR_TE) + @@ -478,13 +513,7 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable, * Note: The SOR is not documented in recent IMX datasheet, but * is described in IMX51 reference manual at section 56.3.3.15. */ - if (vals->scr & CCSR_SSI_SCR_RE) { - regmap_update_bits(regs, CCSR_SSI_SOR, - CCSR_SSI_SOR_RX_CLR, CCSR_SSI_SOR_RX_CLR); - } else { - regmap_update_bits(regs, CCSR_SSI_SOR, - CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR); - } + fsl_empty_fifo(ssi_private->dev, regs, is_rx, ssi_private->use_dual_fifo);
regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr); regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr); @@ -502,6 +531,7 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, bool enable, * disabled now. Otherwise we could alter flags of the other * stream */ + fsl_empty_fifo(ssi_private->dev, regs, is_rx, ssi_private->use_dual_fifo);
/* These assignments are simply vals without bits set in avals*/ sier = fsl_ssi_disable_val(vals->sier, avals->sier,
Le 12/01/2016 00:44, Caleb Crome a écrit :
On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
Hello Caleb
Le 09/01/2016 01:47, Caleb Crome a écrit :
[...]
Hello Arnaud, I have finally gotten to test your patches, and I'm still having trouble with channel slips.
I applied your v2 patch set, along with your changes for using a dummy codec.
The full changes are here: https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
This ignores most of my previous patches, and uses your code to bring up the SSI (without a codec) on a wandboard.
I am using SSI3, and doing a hardware loopback between TX and RX.
Here's what I run: ./atest -r 16000 -c 8 -p 2048 -D default play
which plays continuously.
and in another shell: ./atest -r 16000 -c 8 -p 2048 -D default -d 10 capture
which captures for 10 seconds.
The first time I run the capture command, it succeeds, no problem.
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer warn: First valid frame warn: 3400 3401 3402 3403 3404 3405 3406 3407 dbg: end of tests total number of sequence errors: 0 global tests exit status: OK
But the second and all subsequent captures, it fails with channel slips:
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer err: invalid frame after 0 null frames err: d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0 err: d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0 dbg: end of tests total number of sequence errors: 430080 global tests exit status: OK
Can you use -I option to get a little more log of the error $ ./atest -r 16000 -c 8 -p 2048 -D default -d 10 -I 10 capture
Just to know if the wrong frames comes from the previous record session, meaning the RX fifo was not cleared correctly, as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect. (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be the start of the new recording session)
That does appear to be what's happening. I read the SFCSR before and after you update the SOR register (SOR_RX_CLR), and in both cases the rx buffer is full on the second enablement.
I can't seem to empty that buffer, either by using the SOR_RX_CLR or by reading the SRX0 register. This is another one of those cases where there you cannot modify the register (i.e. fifo or RX0) when RE is disabled.
So, instead of clearing on enable, I'm clearing on shutdown and on enable. I attempted using the SOR_RX_CLR, but it doesn't seem to work on the MX6. Rather, I clear the fifo by reading all the words in a loop, just before RE is disabled.
so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or some things have changes between 4.3 and 4.4.
May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because de the SRCR.RFEN0 [or1] are not enabled yet.
In fact, clearing the fifos at the end may still introduce a race since the RX path is still enabled when your read from fifo, and Samples may still be received in the meantime...
Also, I see that the CCSR_SSI_SOR register is not in the regmap fsl_ssi_volatile_reg() list (line 140). May be there is an implication of some caching mechanism ?
Can you try to move the the fifo clearing part just before the write in CCSR_SSI_SIER with the following change in top of my patches ? Also may be you can additionally add the RX0 / RX1 reading from clearing mechanism at the same place.
On my side, I will check the 4.4 tree on imx6sl
Regards, Arnaud
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 0277097..bd163b2 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -420,6 +420,9 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, int enable, * (online configuration) */ if (enable) { + + regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr); + regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr); /* * Clear RX or TX FIFO to remove samples from the previous * stream session which may be still present in the FIFO and @@ -435,9 +438,6 @@ static void fsl_ssi_config(struct fsl_ssi_private *ssi_private, int enable, regmap_update_bits(regs, CCSR_SSI_SOR, CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR);
} - - regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, vals->srcr); - regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, vals->stcr); regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, vals->sier); } else { u32 sier;
On Wed, Jan 13, 2016 at 6:45 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
Le 12/01/2016 00:44, Caleb Crome a écrit :
On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
Hello Caleb
Le 09/01/2016 01:47, Caleb Crome a écrit :
[...]
Hello Arnaud, I have finally gotten to test your patches, and I'm still having trouble with channel slips.
I applied your v2 patch set, along with your changes for using a dummy codec.
The full changes are here: https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
This ignores most of my previous patches, and uses your code to bring up the SSI (without a codec) on a wandboard.
I am using SSI3, and doing a hardware loopback between TX and RX.
Here's what I run: ./atest -r 16000 -c 8 -p 2048 -D default play
which plays continuously.
and in another shell: ./atest -r 16000 -c 8 -p 2048 -D default -d 10 capture
which captures for 10 seconds.
The first time I run the capture command, it succeeds, no problem.
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer warn: First valid frame warn: 3400 3401 3402 3403 3404 3405 3406 3407 dbg: end of tests total number of sequence errors: 0 global tests exit status: OK
But the second and all subsequent captures, it fails with channel slips:
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer err: invalid frame after 0 null frames err: d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0 err: d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0 dbg: end of tests total number of sequence errors: 430080 global tests exit status: OK
Can you use -I option to get a little more log of the error $ ./atest -r 16000 -c 8 -p 2048 -D default -d 10 -I 10 capture
Just to know if the wrong frames comes from the previous record session, meaning the RX fifo was not cleared correctly, as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect. (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be the start of the new recording session)
That does appear to be what's happening. I read the SFCSR before and after you update the SOR register (SOR_RX_CLR), and in both cases the rx buffer is full on the second enablement.
I can't seem to empty that buffer, either by using the SOR_RX_CLR or by reading the SRX0 register. This is another one of those cases where there you cannot modify the register (i.e. fifo or RX0) when RE is disabled.
So, instead of clearing on enable, I'm clearing on shutdown and on enable. I attempted using the SOR_RX_CLR, but it doesn't seem to work on the MX6. Rather, I clear the fifo by reading all the words in a loop, just before RE is disabled.
so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or some things have changes between 4.3 and 4.4.
May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because de the SRCR.RFEN0 [or1] are not enabled yet.
In fact, clearing the fifos at the end may still introduce a race since the RX path is still enabled when your read from fifo, and Samples may still be received in the meantime...
You are correct about the race condition. It seems like a catch 22: you can't empty the fifo if SCR.RE is disabled, and if the SCR.RE is enabled, new samples might be received at any time. in fact, when disabling RE, it does keep receiving until the end of the frame.
Also, I see that the CCSR_SSI_SOR register is not in the regmap fsl_ssi_volatile_reg() list (line 140). May be there is an implication of some caching mechanism ?
Ah! Yes. When I add the SOR to the volatile list, then writing the SOR does clear the FIFO. That's one for the patch for sure :-)
Now at least RX get's cleared on RX start.
However, now I've got another issue:
FYI I'm running at 16 channels/16-bits/frame, so 256 bits/frame, with the codec as master. (codec 0 anyway).
I'm running atest play in one shell, and atest capture in another shell.
At low sample rates, it works quite reliably (a few hundred simulated xruns with no problem), but TX slips at higher sample rates.
I'm running shell a # ./atest -r SAMPLERATE -c 16 -p 1024 -D default play shell b # ./atest -r SAMPLERATE -c 16 -p 1024 -D default -d 100 capture -x200,100
8kHz: No problems after 100 seconds (~300 capture xruns) 16kHz: No problems 32kHz: tx slips after a few RX xruns (12) and never recovers. 48kHz: tx starts properly (usually), but slips immediately upon RX start.
FYI, I'm watching TX on a scope, and can verify that it is in fact the TX slipping. I caught it once, and found that it was a duplicated sample in the TX stream, which seems to say that the DMA is not keeping up maybe? Perhaps tweaking with watermarks will help that, or perhaps dual-fifo will help. Will try a couple things and get back to you.
In the meantime, any idea how to check to see what speed the SSI peripheral clock is running? I just want to make sure it's going fast enough.
here's a log of the failure at 32kHz: --------------------------------------------------- <first verified on the scope that play is running without slips> ... 20 or 30 valid xruns... ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred warn: First valid frame warn: 49a0 49a1 49a2 49a3 49a4 49a5 49a6 49a7 49a8 49a9 49aa 49ab 49ac 49ad 49ae 49af warn: default: force capture xrun warn: default: CT_W4_XRUN_END warn: default: capture read failed: Broken pipe ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred warn: First valid frame warn: 3a40 3a41 3a42 3a43 3a44 3a45 3a46 3a47 3a48 3a49 3a4a 3a4b 3a4c 3a4d 3a4e 3a4f warn: default: force capture xrun warn: default: CT_W4_XRUN_END warn: default: capture read failed: Broken pipe ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred warn: First valid frame warn: 3240 3241 3242 3243 3244 3245 3246 3247 3248 3249 324a 324b 324c 324d 324e 324f warn: first invalid frame while expecting frame 0x0617 warn: c2e0 c2e1 c2e2 c2e3 c2e4 c2e5 c2e6 c2e6 c2e7 c2e8 c2e9 c2ea c2eb c2ec c2ed c2ee warn: c2ef c300 c301 c302 c303 c304 c305 c306 c307 c308 c309 c30a c30b c30c c30d c30e warn: c30f c320 c321 c322 c323 c324 c325 c326 c327 c328 c329 c32a c32b c32c c32d c32e warn: c32f c340 c341 c342 c343 c344 c345 c346 c347 c348 c349 c34a c34b c34c c34d c34e err: c34f c360 c361 c362 c363 c364 c365 c366 c367 c368 c369 c36a c36b c36c c36d c36e err: c36f c380 c381 c382 c383 c384 c385 c386 c387 c388 c389 c38a c38b c38c c38d c38e err: c38f c3a0 c3a1 c3a2 c3a3 c3a4 c3a5 c3a6 c3a7 c3a8 c3a9 c3aa c3ab c3ac c3ad c3ae err: c3af c3c0 c3c1 c3c2 c3c3 c3c4 c3c5 c3c6 c3c7 c3c8 c3c9 c3ca c3cb c3cc c3cd c3ce err: c3cf c3e0 c3e1 c3e2 c3e3 c3e4 c3e5 c3e6 c3e7 c3e8 c3e9 c3ea c3eb c3ec c3ed c3ee err: c3ef c400 c401 c402 c403 c404 c405 c406 c407 c408 c409 c40a c40b c40c c40d c40e err: c40f c420 c421 c422 c423 c424 c425 c426 c427 c428 c429 c42a c42b c42c c42d c42e warn: default: force capture xrun warn: default: CT_W4_XRUN_END
As you can see, this is caused by duplicated c2e6, which I guess is a DMA underrun?
here's a log of the failure at 48kHz: --------------------------------------------------- <first verified on the scope that play is running without slips> caleb@arm:~/atest$ ./atest -r 48000 -c 16 -p 1024 -D default -d 100 capture -x200,100 dbg: dev: 'default' dbg: default: capture_start dbg: default: will simulate xrun every 200 ms dbg: start a 100 seconds duration timer err: invalid frame after 0 null frames err: 9b2f 9b40 9b41 9b42 9b43 9b44 9b45 9b46 9b47 9b48 9b49 9b4a 9b4b 9b4c 9b4d 9b4e err: 9b4f 9b60 9b61 9b62 9b63 9b64 9b65 9b66 9b67 9b68 9b69 9b6a 9b6b 9b6c 9b6d 9b6e warn: default: force capture xrun warn: default: CT_W4_XRUN_END warn: default: capture read failed: Broken pipe ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred err: invalid frame after 0 null frames
Thanks, -Caleb
On Wed, Jan 13, 2016 at 12:20 PM, Caleb Crome caleb@crome.org wrote:
On Wed, Jan 13, 2016 at 6:45 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
Le 12/01/2016 00:44, Caleb Crome a écrit :
On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
Hello Caleb
Le 09/01/2016 01:47, Caleb Crome a écrit :
[...]
Hello Arnaud, I have finally gotten to test your patches, and I'm still having trouble with channel slips.
I applied your v2 patch set, along with your changes for using a dummy codec.
The full changes are here: https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
This ignores most of my previous patches, and uses your code to bring up the SSI (without a codec) on a wandboard.
I am using SSI3, and doing a hardware loopback between TX and RX.
Here's what I run: ./atest -r 16000 -c 8 -p 2048 -D default play
which plays continuously.
and in another shell: ./atest -r 16000 -c 8 -p 2048 -D default -d 10 capture
which captures for 10 seconds.
The first time I run the capture command, it succeeds, no problem.
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer warn: First valid frame warn: 3400 3401 3402 3403 3404 3405 3406 3407 dbg: end of tests total number of sequence errors: 0 global tests exit status: OK
But the second and all subsequent captures, it fails with channel slips:
dbg: dev: 'default' dbg: default: capture_start dbg: start a 10 seconds duration timer err: invalid frame after 0 null frames err: d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0 err: d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0 dbg: end of tests total number of sequence errors: 430080 global tests exit status: OK
Can you use -I option to get a little more log of the error $ ./atest -r 16000 -c 8 -p 2048 -D default -d 10 -I 10 capture
Just to know if the wrong frames comes from the previous record session, meaning the RX fifo was not cleared correctly, as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect. (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be the start of the new recording session)
That does appear to be what's happening. I read the SFCSR before and after you update the SOR register (SOR_RX_CLR), and in both cases the rx buffer is full on the second enablement.
I can't seem to empty that buffer, either by using the SOR_RX_CLR or by reading the SRX0 register. This is another one of those cases where there you cannot modify the register (i.e. fifo or RX0) when RE is disabled.
So, instead of clearing on enable, I'm clearing on shutdown and on enable. I attempted using the SOR_RX_CLR, but it doesn't seem to work on the MX6. Rather, I clear the fifo by reading all the words in a loop, just before RE is disabled.
so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or some things have changes between 4.3 and 4.4.
May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because de the SRCR.RFEN0 [or1] are not enabled yet.
In fact, clearing the fifos at the end may still introduce a race since the RX path is still enabled when your read from fifo, and Samples may still be received in the meantime...
You are correct about the race condition. It seems like a catch 22: you can't empty the fifo if SCR.RE is disabled, and if the SCR.RE is enabled, new samples might be received at any time. in fact, when disabling RE, it does keep receiving until the end of the frame.
Also, I see that the CCSR_SSI_SOR register is not in the regmap fsl_ssi_volatile_reg() list (line 140). May be there is an implication of some caching mechanism ?
Ah! Yes. When I add the SOR to the volatile list, then writing the SOR does clear the FIFO. That's one for the patch for sure :-)
Now at least RX get's cleared on RX start.
However, now I've got another issue:
FYI I'm running at 16 channels/16-bits/frame, so 256 bits/frame, with the codec as master. (codec 0 anyway).
I'm running atest play in one shell, and atest capture in another shell.
At low sample rates, it works quite reliably (a few hundred simulated xruns with no problem), but TX slips at higher sample rates.
I'm running shell a # ./atest -r SAMPLERATE -c 16 -p 1024 -D default play shell b # ./atest -r SAMPLERATE -c 16 -p 1024 -D default -d 100 capture -x200,100
8kHz: No problems after 100 seconds (~300 capture xruns) 16kHz: No problems 32kHz: tx slips after a few RX xruns (12) and never recovers. 48kHz: tx starts properly (usually), but slips immediately upon RX start.
FYI, I'm watching TX on a scope, and can verify that it is in fact the TX slipping. I caught it once, and found that it was a duplicated sample in the TX stream, which seems to say that the DMA is not keeping up maybe? Perhaps tweaking with watermarks will help that, or perhaps dual-fifo will help. Will try a couple things and get back to you.
In the meantime, any idea how to check to see what speed the SSI peripheral clock is running? I just want to make sure it's going fast enough.
here's a log of the failure at 32kHz:
<first verified on the scope that play is running without slips> ... 20 or 30 valid xruns... ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred warn: First valid frame warn: 49a0 49a1 49a2 49a3 49a4 49a5 49a6 49a7 49a8 49a9 49aa 49ab 49ac 49ad 49ae 49af warn: default: force capture xrun warn: default: CT_W4_XRUN_END warn: default: capture read failed: Broken pipe ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred warn: First valid frame warn: 3a40 3a41 3a42 3a43 3a44 3a45 3a46 3a47 3a48 3a49 3a4a 3a4b 3a4c 3a4d 3a4e 3a4f warn: default: force capture xrun warn: default: CT_W4_XRUN_END warn: default: capture read failed: Broken pipe ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred warn: First valid frame warn: 3240 3241 3242 3243 3244 3245 3246 3247 3248 3249 324a 324b 324c 324d 324e 324f warn: first invalid frame while expecting frame 0x0617 warn: c2e0 c2e1 c2e2 c2e3 c2e4 c2e5 c2e6 c2e6 c2e7 c2e8 c2e9 c2ea c2eb c2ec c2ed c2ee warn: c2ef c300 c301 c302 c303 c304 c305 c306 c307 c308 c309 c30a c30b c30c c30d c30e warn: c30f c320 c321 c322 c323 c324 c325 c326 c327 c328 c329 c32a c32b c32c c32d c32e warn: c32f c340 c341 c342 c343 c344 c345 c346 c347 c348 c349 c34a c34b c34c c34d c34e err: c34f c360 c361 c362 c363 c364 c365 c366 c367 c368 c369 c36a c36b c36c c36d c36e err: c36f c380 c381 c382 c383 c384 c385 c386 c387 c388 c389 c38a c38b c38c c38d c38e err: c38f c3a0 c3a1 c3a2 c3a3 c3a4 c3a5 c3a6 c3a7 c3a8 c3a9 c3aa c3ab c3ac c3ad c3ae err: c3af c3c0 c3c1 c3c2 c3c3 c3c4 c3c5 c3c6 c3c7 c3c8 c3c9 c3ca c3cb c3cc c3cd c3ce err: c3cf c3e0 c3e1 c3e2 c3e3 c3e4 c3e5 c3e6 c3e7 c3e8 c3e9 c3ea c3eb c3ec c3ed c3ee err: c3ef c400 c401 c402 c403 c404 c405 c406 c407 c408 c409 c40a c40b c40c c40d c40e err: c40f c420 c421 c422 c423 c424 c425 c426 c427 c428 c429 c42a c42b c42c c42d c42e warn: default: force capture xrun warn: default: CT_W4_XRUN_END
As you can see, this is caused by duplicated c2e6, which I guess is a DMA underrun?
SUCCESS! So far...
With your patches (including the latest SOR register update), plus setting the watermark & DMA MAXBURST to 8, I don't get any more errors at 48kHz ... yet.
Even in single fifo mode, 48kHz, 16-bits+16channels seems to work now.
I'll keep you updated on if this really solves all the issues. Here's the last patch for updating the watermark.
commit b634014b831b9527df319b404ac50e54a3790742 Author: Caleb Crome caleb@crome.org Date: Wed Jan 13 13:12:37 2016 -0800
ASoC: fsl_ssi: Increase watermark and maxburst to allow SSI to work without slips at high data rates.
The DMA cannot keep up with the SSI consumpation with the watermark set to be fifo_depth-2 when running at 48kHz/16-bits/16-channels (12288 words/second). By increasing the watermark to 8, the DMA can keep up with the SSI.
Signed-off-by: Caleb Crome caleb@crome.org
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 5cfc540..026df79 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -221,6 +221,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; @@ -259,6 +265,9 @@ struct fsl_ssi_private {
const struct fsl_ssi_soc_data *soc; struct device *dev; + + u32 fifo_watermark; + u32 dma_maxburst; };
/* @@ -1037,21 +1046,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->watermark;
regmap_write(regs, CCSR_SSI_SFCSR, CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) | @@ -1359,12 +1354,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;
@@ -1518,6 +1509,42 @@ 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 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. + */ + 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; + 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) {
[..]
SUCCESS! So far...
Great :)
I'm preparing a v3 of my patches including the SOR register + rebased on top of v4.4. I will let you propose the water mark / maxburst patch. But it looks obvious to me that triggering the DMA when only 2 words are left in the FIFO can lead to DMA xruns at such data rate.
The downside is an increased number of DMA requests. So I don't know if you should propose a configuration through the device tree, or a static configuration as done in your patch.
Arnaud
With your patches (including the latest SOR register update), plus setting the watermark & DMA MAXBURST to 8, I don't get any more errors at 48kHz ... yet.
Even in single fifo mode, 48kHz, 16-bits+16channels seems to work now.
I'll keep you updated on if this really solves all the issues. Here's the last patch for updating the watermark.
commit b634014b831b9527df319b404ac50e54a3790742 Author: Caleb Crome caleb@crome.org Date: Wed Jan 13 13:12:37 2016 -0800
ASoC: fsl_ssi: Increase watermark and maxburst to allow SSI to work without slips at high data rates. The DMA cannot keep up with the SSI consumpation with the watermark set to be fifo_depth-2 when running at 48kHz/16-bits/16-channels (12288 words/second). By increasing the watermark to 8, the DMA can keep up with the SSI. Signed-off-by: Caleb Crome <caleb@crome.org>diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 5cfc540..026df79 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -221,6 +221,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,
*/ struct fsl_ssi_private { struct regmap *regs;
this is always the same as fifo_watermark.@@ -259,6 +265,9 @@ struct fsl_ssi_private {
const struct fsl_ssi_soc_data *soc; struct device *dev;
u32 fifo_watermark;
u32 dma_maxburst; };
/*
@@ -1037,21 +1046,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->watermark;
regmap_write(regs, CCSR_SSI_SFCSR, CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
@@ -1359,12 +1354,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;
@@ -1518,6 +1509,42 @@ 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 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.*/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;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) {
On Thu, Jan 14, 2016 at 12:40 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
[..]
SUCCESS! So far...
Great :)
I'm preparing a v3 of my patches including the SOR register + rebased on top of v4.4. I will let you propose the water mark / maxburst patch. But it looks obvious to me that triggering the DMA when only 2 words are left in the FIFO can lead to DMA xruns at such data rate.
The downside is an increased number of DMA requests. So I don't know if you should propose a configuration through the device tree, or a static configuration as done in your patch.
Sounds to me like the device tree is the right place for it. I'll submit an RFC.
For some reason... looks like it can't yet quite keep up at 48kHz. it was working reliably for a while, but now I can only get it reliable at 32kHz. It seems it's the TX that's broken at 48k.
I'll dig in to see where the failure is.
-C
Arnaud
With your patches (including the latest SOR register update), plus setting the watermark & DMA MAXBURST to 8, I don't get any more errors at 48kHz ... yet.
Even in single fifo mode, 48kHz, 16-bits+16channels seems to work now.
I'll keep you updated on if this really solves all the issues. Here's the last patch for updating the watermark.
commit b634014b831b9527df319b404ac50e54a3790742 Author: Caleb Crome caleb@crome.org Date: Wed Jan 13 13:12:37 2016 -0800
ASoC: fsl_ssi: Increase watermark and maxburst to allow SSI to work without slips at high data rates. The DMA cannot keep up with the SSI consumpation with the watermark set to be fifo_depth-2 when running at 48kHz/16-bits/16-channels (12288 words/second). By increasing the watermark to 8, the DMA can keep up with the SSI. Signed-off-by: Caleb Crome <caleb@crome.org>diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 5cfc540..026df79 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -221,6 +221,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,
*/ struct fsl_ssi_private { struct regmap *regs;
this is always the same as fifo_watermark.@@ -259,6 +265,9 @@ struct fsl_ssi_private {
const struct fsl_ssi_soc_data *soc; struct device *dev;
u32 fifo_watermark;
u32 dma_maxburst; };
/*
@@ -1037,21 +1046,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). Thisvalue,
* 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->watermark;
regmap_write(regs, CCSR_SSI_SFCSR, CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |
@@ -1359,12 +1354,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;
@@ -1518,6 +1509,42 @@ 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 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). Thisvalue,
* 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.*/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;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) {
On Thu, Jan 14, 2016 at 6:25 AM, Caleb Crome caleb@crome.org wrote:
On Thu, Jan 14, 2016 at 12:40 AM, arnaud.mouiche@invoxia.com arnaud.mouiche@invoxia.com wrote:
[..]
SUCCESS! So far...
Great :)
I'm preparing a v3 of my patches including the SOR register + rebased on top of v4.4. I will let you propose the water mark / maxburst patch. But it looks obvious to me that triggering the DMA when only 2 words are left in the FIFO can lead to DMA xruns at such data rate.
The downside is an increased number of DMA requests. So I don't know if you should propose a configuration through the device tree, or a static configuration as done in your patch.
Sounds to me like the device tree is the right place for it. I'll submit an RFC.
For some reason... looks like it can't yet quite keep up at 48kHz. it was working reliably for a while, but now I can only get it reliable at 32kHz. It seems it's the TX that's broken at 48k.
I'll dig in to see where the failure is.
-C
Okay, setting maxburst and watermark to 4 instead of 8 seems to solve the problem at 48kHz.
-Caleb
participants (4)
-
Arnaud Mouiche -
arnaud.mouiche＠invoxia.com
-
Caleb Crome -
Mark Brown