Subject: Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
To: Nicolin Chen <nicoleotsuka@gmail.com>, broonie@kernel.org
Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        alsa-devel@alsa-project.org, fabio.estevam@nxp.com,
        mail@maciej.szmigiero.name, caleb@crome.org, lgirdwood@gmail.com,
        arnaud.mouiche@invoxia.com, lukma@denx.de, kernel@pengutronix.de
References: <1513207108-30430-1-git-send-email-nicoleotsuka@gmail.com>
 <1513207108-30430-4-git-send-email-nicoleotsuka@gmail.com>
From: Timur Tabi <timur@tabi.org>
Message-ID: <01a7d97b-7e80-83b2-e850-d513d7dc35aa@tabi.org>
Date: Sat, 16 Dec 2017 11:15:43 -0600
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:52.0)
 Gecko/20100101 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <1513207108-30430-4-git-send-email-nicoleotsuka@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 11535
Lines: 298

On 12/13/17 5:18 PM, Nicolin Chen wrote:

> -	/* Used when using fsl-ssi as sound-card. This is only used by ppc and
> -	 * should be replaced with simple-sound-card. */
>   	struct platform_device *pdev;

Is this comment no longer true?

> + * 1) SSI in earlier SoCS has crtical bits in control registers that

critical

> -/**
> - * fsl_ssi_isr: SSI interrupt handler
> - *
> - * Although it's possible to use the interrupt handler to send and receive
> - * data to/from the SSI, we use the DMA instead.  Programming is more
> - * complicated, but the performance is much better.
> - *
> - * This interrupt handler is used only to gather statistics.
> - *
> - * @irq: IRQ of the SSI device
> - * @dev_id: pointer to the fsl_ssi structure for this SSI device
> - */

What's wrong with this comment?

> -/*
> - * 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.
> +/**
> + * Clear remaining data in the FIFO to avoid dirty data or channel slipping

I think the original is better, unless there's something untrue about it.

> -	 * We are running on a SoC which does not support online SSI
> -	 * reconfiguration, so we have to enable all necessary flags at once
> -	 * even if we do not use them later (capture and playback configuration)
> +	 * Online configuration is not supported
> +	 * Enable or Disable all necessary bits at once

Ditto

> -	/*
> -	 * Configure single direction units while the SSI unit is running
> -	 * (online configuration)
> -	 */
> +	/* Online configure single direction while SSI is running */

Ditto

> -		/*
> -		 * Disabling the necessary flags for one of rx/tx while the
> -		 * other stream is active is a little bit more difficult. We
> -		 * have to disable only those flags that differ between both
> -		 * streams (rx XOR tx) and that are set in the stream that is
> -		 * disabled now. Otherwise we could alter flags of the other
> -		 * stream
> -		 */
> -
> -		/* These assignments are simply vals without bits set in avals*/
> +		/* Exclude necessary bits for the opposite stream */

Ditto

> -			/*
> -			 * 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.
> -			 */

What's wrong with this comment?

> -		/*
> -		 * Note that these below aren't just normal registers.
> -		 * They are a way to disable or enable bits in SACCST
> -		 * register:
> -		 * - writing a '1' bit at some position in SACCEN sets the
> -		 * relevant bit in SACCST,
> -		 * - writing a '1' bit at some position in SACCDIS unsets
> -		 * the relevant bit in SACCST register.
> -		 *
> -		 * The two writes below first disable all channels slots,
> -		 * then enable just slots 3 & 4 ("PCM Playback Left Channel"
> -		 * and "PCM Playback Right Channel").
> -		 */
> +		/* Disable all channel slots */

Ditto.


> -	 * Why are we setting up SACCST everytime we are starting a
> -	 * playback?
> -	 * Some CODECs (like VT1613 CODEC on UDOO board) like to
> -	 * (sometimes) set extra bits in their SLOTREQ requests.
> -	 * When a bit is set in a SLOTREQ request then SSI sets the
> -	 * relevant bit in SACCST automatically (it is enough if a bit was
> -	 * set in a SLOTREQ just once, bits in SACCST are 'sticky').
> -	 * If an extra slot gets enabled that's a disaster for playback
> -	 * because some of normal left or right channel samples are
> -	 * redirected instead to this extra slot.
> +	 * SACCST might be modified via AC Link by a CODEC if it sends
> +	 * extra bits in their SLOTREQ requests, which'll accidentally
> +	 * send valid data to slots other than normal playback slots.
>   	 *
> -	 * A workaround implemented in fsl-asoc-card of setting an
> -	 * appropriate CODEC register so that slots 3 & 4 (the normal
> -	 * stereo playback slots) are used for S/PDIF seems to mostly fix
> -	 * this issue on the UDOO board but since this CODEC is so
> -	 * untrustworthy let's play safe here and make sure that no extra
> -	 * slots are enabled every time a playback is started.
> +	 * To be safe, configure SACCST right before TX starts.

I think the original is better, unless there's something untrue about it.

>   	 */
>   	if (enable && fsl_ssi_is_ac97(ssi))
>   		fsl_ssi_tx_ac97_saccst_setup(ssi);
> @@ -626,10 +563,8 @@ static void fsl_ssi_tx_config(struct fsl_ssi *ssi, bool enable)
>   	fsl_ssi_config(ssi, enable, &ssi->rxtx_reg_val.tx);
>   }
>   
> -/*
> - * Setup rx/tx register values used to enable/disable the streams. These will
> - * be used later in fsl_ssi_config to setup the streams without the need to
> - * check for all different SSI modes.
> +/**
> + * Cache critical bits of SIER, SRCR, STCR and SCR to later set them safely

This is different comment altogether.  Is the original wrong?

> -/**
> - * fsl_ssi_startup: create a new substream
> - *
> - * This is the first function called when a stream is opened.
> - *
> - * If this is the first stream open, then grab the IRQ and program most of
> - * the SSI registers.
> - */

What's wrong with this?

> - * fsl_ssi_hw_params - program the sample size
> + * Configure SSI based on PCM hardware parameters
>    *
> - * Most of the SSI registers have been programmed in the startup function,
> - * but the word length must be programmed here.  Unfortunately, programming
> - * the SxCCR.WL bits requires the SSI to be temporarily disabled.  This can
> - * cause a problem with supporting simultaneous playback and capture.  If
> - * the SSI is already playing a stream, then that stream may be temporarily
> - * stopped when you start capture.
> - *
> - * Note: The SxCCR.DC and SxCCR.PM bits are only used if the SSI is the
> - * clock master.
> + * Notes:
> + * 1) SxCCR.WL bits are critical bits that require SSI to be temporarily
> + *    disabled on offline_config SoCs. Even for online configurable SoCs
> + *    running in synchronous mode (both TX and RX use STCCR), it is not
> + *    safe to re-configure them when both two streams start running.
> + * 2) SxCCR.PM, SxCCR.DIV2 and SxCCR.PSR bits will be configured in the
> + *    fsl_ssi_set_bclk() if SSI is the DAI clock master.

I think the comment about the stream being temporarily stopped should be 
kept, since it was a real issue I spent a lot of time trying to debug. 
Unless it's been fixed, of course.

>    */
>   static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
>   	struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *cpu_dai)
> @@ -879,8 +795,10 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
>   	enabled = scr_val & CCSR_SSI_SCR_SSIEN;
>   
>   	/*
> -	 * If we're in synchronous mode, and the SSI is already enabled,
> -	 * then STCCR is already set properly.
> +	 * SSI is properly configured if it is enabled and running in
> +	 * the synchronous mode; Note that AC97 mode is an exception
> +	 * that should set separate configurations for STCCR and SRCCR
> +	 * despite running in the synchronous mode.
>   	 */
>   	if (enabled && ssi->cpu_dai_drv.symmetric_rates)
>   		return 0;
> @@ -902,10 +820,7 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
>   
>   	if (!fsl_ssi_is_ac97(ssi)) {
>   		u8 i2smode;
> -		/*
> -		 * Switch to normal net mode in order to have a frame sync
> -		 * signal every 32 bits instead of 16 bits
> -		 */
> +		/* Normal + Network mode to send 16-bit data in 32-bit frames */
>   		if (fsl_ssi_is_i2s_cbm_cfs(ssi) && sample_size == 16)
>   			i2smode = CCSR_SSI_SCR_I2S_MODE_NORMAL |
>   				CCSR_SSI_SCR_NET;
> @@ -917,16 +832,6 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
>   				channels == 1 ? 0 : i2smode);
>   	}
>   
> -	/*
> -	 * FIXME: The documentation says that SxCCR[WL] should not be
> -	 * modified while the SSI is enabled.  The only time this can
> -	 * happen is if we're trying to do simultaneous playback and
> -	 * capture in asynchronous mode.  Unfortunately, I have been enable
> -	 * to get that to work at all on the P1022DS.  Therefore, we don't
> -	 * bother to disable/enable the SSI when setting SxCCR[WL], because
> -	 * the SSI will stop anyway.  Maybe one day, this will get fixed.
> -	 */

Has this been fixed?  If not, then don't delete the comment.

>   /**
> - * fsl_ssi_trigger: start and stop the DMA transfer.
> - *
> - * This function is called by ALSA to start, stop, pause, and resume the DMA
> - * transfer of data.
> - *
> - * The DMA channel is in external master start and pause mode, which
> - * means the SSI completely controls the flow of data.

This last paragraph is important.

> -
> -		/*
> -		 * Some boards use an incompatible codec. To get it
> -		 * working, we are using imx-fiq-pcm-audio, that
> -		 * can handle those codecs. DMA is not possible in this
> -		 * situation.
> -		 */
> -
> +		/* Use imx-fiq-pcm-audio for codec incompatible with DMA */

Original is clearer.

> -	 * 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.
> +	 * Configure TX and RX DMA watermarks.
>   	 *
> -	 * 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.
> +	 * Values should be tested to avoid FIFO under/over run. Set maxburst
> +	 * to fifo_watermark to maxiumize DMA transaction to reduce overhead.

Why in the world would you delete all this good info?

>   	 */
>   	switch (ssi->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.
> -		 */
> +		/* Tested with cases running at 48kHz @ 16 bits x 16 channels */

Same here.

> -	/*
> -	 * If codec-handle property is missing from SSI node, we assume
> -	 * that the machine driver uses new binding which does not require
> -	 * SSI driver to trigger machine driver's probe.
> -	 */
> +	/* Bypass it if using newer DT bindings of ASoC machine drivers */

Not an improvement.

> -/* Show the statistics of a flag only if its interrupt is enabled.  The
> - * compiler will optimze this code to a no-op if the interrupt is not
> - * enabled.
> +/**
> + * Show the statistics of a flag only if its interrupt is enabled
> + *
> + * Compilers will optimze it to a no-op if the interrupt is disabled

optimize