16 Dec
2017
16 Dec
'17
6:15 p.m.
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. */
Is this comment no longer true?
- 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:
- 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.
- 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.@@ -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 */@@ -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