From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757054AbdLPRPt (ORCPT ); Sat, 16 Dec 2017 12:15:49 -0500 Received: from muin.pair.com ([209.68.1.55]:61327 "EHLO muin.pair.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756728AbdLPRPr (ORCPT ); Sat, 16 Dec 2017 12:15:47 -0500 Subject: Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments To: Nicolin Chen , 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 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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