Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

From: "arnaud.mouiche@invoxia.com" <arnaud.mouiche@invoxia.com>
To: Caleb Crome <caleb@crome.org>
Cc: Fabio Estevam <fabio.estevam@freescale.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Markus Pargmann <mpa@pengutronix.de>,
	Mark Brown <broonie@kernel.org>,
	Roberto Fichera <kernel@tekno-soft.it>,
	"shawn.guo@linaro.org" <shawn.guo@linaro.org>
Subject: Re: [PATCH 0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions
Date: Thu, 14 Jan 2016 09:40:44 +0100	[thread overview]
Message-ID: <56975F0C.3080906@invoxia.com> (raw)
In-Reply-To: <CAG5mAdzgAHPcqp5o12m+Ba5d69qx3ocWaa5s=ns5A60BWfcm7g@mail.gmail.com>

[..]
>
> 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,
> + *             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) {