| Message ID | 1513207108-30430-4-git-send-email-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/13/17 5:18 PM, Nicolin Chen wrote: > - * 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 This is an example of a bad change, IMHO. The original was written in elegant prose. The new version is just two short sentences.
Hi, I am outside so can't use mutt. Sorry for that. This comment is going to be replaced in the 2nd set anyway because the whole function will be replaced. And please point out all comments that you think I need to rework. I am totally fine to do that. I don't think every single one is bad. And this patch has to go in as it also adds a lot of new comments. Thank you for your effort Nicolin On Dec 15, 2017 20:43, "Timur Tabi" <timur@tabi.org> wrote: On 12/13/17 5:18 PM, Nicolin Chen wrote: > - * 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 > This is an example of a bad change, IMHO. The original was written in elegant prose. The new version is just two short sentences.
On 12/16/17 12:10 AM, Nicolin Chen wrote: > Hi, > > I am outside so can't use mutt. Sorry for that. > > This comment is going to be replaced in the 2nd set anyway because the > whole function will be replaced. So you're asking me to review comment changes that will soon be deleted? Can you send out a new patch without changes to comments, so that I can focus on the ones that matter? > And please point out all comments that you think I need to rework. I am > totally fine to do that. I don't think every single one is bad. And this > patch has to go in as it also adds a lot of new comments. Ok.
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
On Sat, Dec 16, 2017 at 9:15 AM, Timur Tabi <timur@tabi.org> wrote: > > 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 Having come to work on this driver with very little knowledge about kernel programming, and i.MX, I have to agree with Timur. It's an amazingly complex driver (with support of so many variants). By eliminating verbose commentary, it's also wiping away a lot of knowledge. The more sparse commentary makes things harder to understand for newcomers, or really anybody who isn't already steeped in knowledge about the SSI port and linux, and it's interaction with DMA. (IMO, the hardware design is fundamentally flawed, which makes this thing such a bear to program -- interrupts & DMA requests are based on numbers of samples rather than frames -- but that's a discussion for a different mailing list ;-) ) -Caleb
Hi, On Sat, Dec 16, 2017 at 10:27:06AM -0600, Timur Tabi wrote: > On 12/16/17 12:10 AM, Nicolin Chen wrote: > >Hi, > > > >I am outside so can't use mutt. Sorry for that. > > > >This comment is going to be replaced in the 2nd set anyway because the > >whole function will be replaced. > > So you're asking me to review comment changes that will soon be deleted? I never said "deleted". > Can you send out a new patch without changes to comments, so that I can > focus on the ones that matter? Please don't make any assumption. I am trying my best to do that. And that's the reason why I have this patch here. I have already done as much as I can to integrate all comments here. Thanks Nicolin
On 12/16/17 11:30 AM, Caleb Crome wrote: > Having come to work on this driver with very little knowledge about > kernel programming, and i.MX, I have to agree with Timur. It's an > amazingly complex driver (with support of so many variants). By > eliminating verbose commentary, it's also wiping away a lot of > knowledge. The more sparse commentary makes things harder to > understand for newcomers, or really anybody who isn't already steeped > in knowledge about the SSI port and linux, and it's interaction with > DMA. This is exactly why I wrote the comments the way I did. As I was learning about the hardware and ASoC drivers, whenever I was confused about something and then figured it out, I would add a comment about that. I figured if it wasn't obvious to me, it wouldn't be obvious to anyone else. And since this was one of the first ASoC drivers, and the first to use device tree, it would become a reference driver for years to come. That made it even more important that I document everything I learned. I am pleased that other developers kept up that commenting style. This patch destroys all of that.
On Sat, Dec 16, 2017 at 09:30:14AM -0800, Caleb Crome wrote: > Having come to work on this driver with very little knowledge about > kernel programming, and i.MX, I have to agree with Timur. It's an > amazingly complex driver (with support of so many variants). By > eliminating verbose commentary, it's also wiping away a lot of > knowledge. The more sparse commentary makes things harder to > understand for newcomers, or really anybody who isn't already steeped > in knowledge about the SSI port and linux, and it's interaction with > DMA. I never said that I don't agree with Timur. Every change here is to simplify things. As long as Timur or any reviewer feels one of new comments is harder to understand, I am totally fine to rework. I respect everyone's opinion, but I hope everyone can respect my effort too by telling me which one needs to rework and why? Thanks Nicolin
On 12/16/17 11:49 AM, Nicolin Chen wrote: > I never said that I don't agree with Timur. Every change here is > to simplify things. As long as Timur or any reviewer feels one of > new comments is harder to understand, I am totally fine to rework. > I respect everyone's opinion, but I hope everyone can respect my > effort too by telling me which one needs to rework and why? I do respect it. I apologize if I came across as unduly harsh.
First of all, thanks a lot for the review. And I will send a v4 after I refine these comments. But please please don't think in the way like "you can touch it unless it's untrue." I never said anything or anyone is wrong here. As the other patches that shortens variable names, this patch just does something similar. The point here is just to make the driver necessarily explained while being brief. I am totally fine if you feel some of my new comments are worse. I will refine it until you get satisfied. And I hope you (or anyone else) can tell me more about what is wrong with my new comments instead of asking me to drop them all. I locally have more than 20 patches based on this one. Any change I make to this one will give me a lot of trouble to rebase them. So I am more willing to refine those that people really feel hard to understand. On Sat, Dec 16, 2017 at 11:15:43AM -0600, Timur Tabi wrote: > 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? It's moved to the comments of structure fsl_ssi. Since we defined a pdev at the first place, we should have explained it along with the definition. > >-/** > >- * 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? Nothing wrong. An interrupt handler is way too common sense in a driver code. I am okay to retain it if you strongly feel it's that necessary. But I would feel more plausible to clean it away. > >- * 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. It's literally stating the same thing. And SOR register comments are moved to the header file. > >- * 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 The code also does disabling as well however it doesn't mention at all. Just like you might have hard time to understand my new comments, I also had a hard time to understand this one. So I'll have to change it. My new comments are shorter but covers both enable and disable. I could make it more descriptive if necessary. > >- /* > >- * Configure single direction units while the SSI unit is running > >- * (online configuration) > >- */ > >+ /* Online configure single direction while SSI is running */ > > Ditto It's literally the same but shorter. I don't think anyone would have trouble to understand mine... > >- /* > >- * 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? I have new comments covering necessary steps. But I could bring some parts back if that makes you happy. > >- /* > >- * 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. All register related comments are moved to the header file. And I have new comments covering the 2nd part of original one. > >- * 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. The 1st part is saying the same thing as mine, the 2nd part is not that necessary to mention some work in the machine driver since we have already applied the safest way here. > >-/* > >- * 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? "Setup register values" sounds more like touching the registers. But the function just caches them into a structure which will be used later. I think mine is more accurate. > >-/** > >- * 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? * If this is the first stream open, then grab the IRQ and program most of * the SSI registers. There is no corresponding code in this function any more. > >- * 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. Mine also has "require SSI to be temporarily disabled". > > */ > > 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. The comments about WL is moved to the previous one that you just reviewed. Besides, this is fixed since hw_params() returns the 2nd stream when running in the synchronous mode (you may take a quick look at the driver code.) > >- * 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? Firstly, FIFO 1 is now being used on i.MX. Secondly, I have some comments covering important part in my opinion. You may tell me which part that I am missing 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. Simplification is an improvement in my opinion. And this is the purpose of the whole set. Touching comments is a sensitive step and I realized it before sending this patch. But I am willing to take it as there probably won't be any second chance to do this kinda cleanup again. If some of the changes made things worse or even *destroyed* something, I'll definitely change it. Otherwise, I would like to take this aggressive step. Thanks Nicolin
On Sat, Dec 16, 2017 at 12:31:34PM -0600, Timur Tabi wrote: > On 12/16/17 11:49 AM, Nicolin Chen wrote: > >I never said that I don't agree with Timur. Every change here is > >to simplify things. As long as Timur or any reviewer feels one of > >new comments is harder to understand, I am totally fine to rework. > >I respect everyone's opinion, but I hope everyone can respect my > >effort too by telling me which one needs to rework and why? > > I do respect it. I apologize if I came across as unduly harsh. I believe everyone here has a good heart to make things better. So I am not and won't take it personally. Let's just act more cooperative. Thanks.
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index e903c92..fe11a23 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -187,42 +187,48 @@ struct fsl_ssi_soc_data { /** * fsl_ssi: per-SSI private data * - * @reg: Pointer to the regmap registers + * @regs: Pointer to the regmap registers * @irq: IRQ of this SSI * @cpu_dai_drv: CPU DAI driver for this device * * @dai_fmt: DAI configuration this device is currently used with - * @i2s_mode: i2s and network mode configuration of the device. Is used to - * switch between normal and i2s/network mode - * mode depending on the number of channels + * @i2s_mode: I2S and Network mode configuration of SCR register * @use_dma: DMA is used or FIQ with stream filter - * @use_dual_fifo: DMA with support for both FIFOs used - * @fifo_deph: Depth of the SSI FIFOs - * @slot_width: width of each DAI slot - * @slots: number of slots - * @rxtx_reg_val: Specific register settings for receive/transmit configuration + * @use_dual_fifo: DMA with support for dual FIFO mode + * @has_ipg_clk_name: If "ipg" is in the clock name list of device tree + * @fifo_depth: Depth of the SSI FIFOs + * @slot_width: Width of each DAI slot + * @slots: Number of slots + * @rxtx_reg_val: Specific RX/TX register settings * - * @clk: SSI clock - * @baudclk: SSI baud clock for master mode + * @clk: Clock source to access register + * @baudclk: Clock source to generate bit and frame-sync clocks * @baudclk_streams: Active streams that are using baudclk * + * @regcache_sfcsr: Cache sfcsr register value during suspend and resume + * @regcache_sacnt: Cache sacnt register value during suspend and resume + * * @dma_params_tx: DMA transmit parameters * @dma_params_rx: DMA receive parameters * @ssi_phys: physical address of the SSI registers * * @fiq_params: FIQ stream filtering parameters * - * @pdev: Pointer to pdev used for deprecated fsl-ssi sound card + * @pdev: Pointer to pdev when using fsl-ssi as sound card (ppc only) + * TODO: Should be replaced with simple-sound-card * * @dbg_stats: Debugging statistics * * @soc: SoC specific data + * @dev: Pointer to &pdev->dev + * + * @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. * - * @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. + * @ac97_reg_lock: Mutex lock to serialize AC97 register access operations */ struct fsl_ssi { struct regmap *regs; @@ -243,20 +249,15 @@ struct fsl_ssi { struct clk *baudclk; unsigned int baudclk_streams; - /* regcache for volatile regs */ u32 regcache_sfcsr; u32 regcache_sacnt; - /* DMA params */ struct snd_dmaengine_dai_dma_data dma_params_tx; struct snd_dmaengine_dai_dma_data dma_params_rx; dma_addr_t ssi_phys; - /* params for non-dma FIQ stream filtered mode */ struct imx_pcm_fiq_params fiq_params; - /* 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; struct fsl_ssi_dbg dbg_stats; @@ -271,19 +272,19 @@ struct fsl_ssi { }; /* - * imx51 and later SoCs have a slightly different IP that allows the - * SSI configuration while the SSI unit is running. - * - * More important, it is necessary on those SoCs to configure the - * sperate TX/RX DMA bits just before starting the stream - * (fsl_ssi_trigger). The SDMA unit has to be configured before fsl_ssi - * sends any DMA requests to the SDMA unit, otherwise it is not defined - * how the SDMA unit handles the DMA request. + * SoC specific data * - * SDMA units are present on devices starting at imx35 but the imx35 - * reference manual states that the DMA bits should not be changed - * while the SSI unit is running (SSIEN). So we support the necessary - * online configuration of fsl-ssi starting at imx51. + * Notes: + * 1) SSI in earlier SoCS has crtical bits in control registers that + * cannot be changed after SSI starts running -- a software reset + * (set SSIEN to 0) is required to change their values. So adding + * an offline_config flag for these SoCs. + * 2) SDMA is available since imx35. However, imx35 does not support + * DMA bits changing when SSI is running, so set offline_config. + * 3) imx51 and later versions support register configurations when + * SSI is running (SSIEN); For these versions, DMA needs to be + * configured before SSI sends DMA request to avoid an undefined + * DMA request on the SDMA side. */ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = { @@ -342,18 +343,7 @@ static bool fsl_ssi_is_i2s_cbm_cfs(struct fsl_ssi *ssi) return (ssi->dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFS; } -/** - * 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 - */ + static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) { struct fsl_ssi *ssi = dev_id; @@ -361,10 +351,6 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) __be32 sisr; __be32 sisr2; - /* We got an interrupt, so read the status register to see what we - were interrupted for. We mask it with the Interrupt Enable register - so that we only check for events that we're interested in. - */ regmap_read(regs, CCSR_SSI_SISR, &sisr); sisr2 = sisr & ssi->soc->sisr_write_mask; @@ -377,8 +363,8 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) return IRQ_HANDLED; } -/* - * Enable/Disable all rx/tx config flags at once. +/** + * Enable or disable all rx/tx config flags at once */ static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable) { @@ -405,13 +391,8 @@ static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool 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. +/** + * Clear remaining data in the FIFO to avoid dirty data or channel slipping */ static void fsl_ssi_fifo_clear(struct fsl_ssi *ssi, bool is_rx) { @@ -424,7 +405,7 @@ static void fsl_ssi_fifo_clear(struct fsl_ssi *ssi, bool is_rx) } } -/* +/** * Calculate the bits that have to be disabled for the current stream that is * getting disabled. This keeps the bits enabled that are necessary for the * second stream to work if 'stream_active' is true. @@ -444,9 +425,8 @@ static void fsl_ssi_fifo_clear(struct fsl_ssi *ssi, bool is_rx) ((vals_disable) & \ ((vals_disable) ^ ((vals_stream) * (u32)!!(stream_active)))) -/* - * Enable/Disable a ssi configuration. You have to pass either - * ssi->rxtx_reg_val.rx or tx as vals parameter. +/** + * Enable or disable SSI configuration. */ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, struct fsl_ssi_reg_val *vals) @@ -467,24 +447,23 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, else keep_active = 0; - /* Find the other direction values rx or tx which we do not want to - * modify */ + /* Get the opposite direction to keep its values untouched */ if (&ssi->rxtx_reg_val.rx == vals) avals = &ssi->rxtx_reg_val.tx; else avals = &ssi->rxtx_reg_val.rx; - /* If vals should be disabled, start with disabling the unit */ if (!enable) { + /* Exclude necessary bits for the opposite stream */ u32 scr = fsl_ssi_disable_val(vals->scr, avals->scr, keep_active); + /* Safely disable SCR register for the stream */ regmap_update_bits(regs, CCSR_SSI_SCR, scr, 0); } /* - * 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 */ if (ssi->soc->offline_config) { if ((enable && !nr_active_streams) || @@ -494,10 +473,7 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, goto config_done; } - /* - * Configure single direction units while the SSI unit is running - * (online configuration) - */ + /* Online configure single direction while SSI is running */ if (enable) { fsl_ssi_fifo_clear(ssi, vals->scr & CCSR_SSI_SCR_RE); @@ -509,16 +485,7 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, u32 srcr; u32 stcr; - /* - * 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 */ sier = fsl_ssi_disable_val(vals->sier, avals->sier, keep_active); srcr = fsl_ssi_disable_val(vals->srcr, avals->srcr, @@ -526,6 +493,7 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, stcr = fsl_ssi_disable_val(vals->stcr, avals->stcr, keep_active); + /* Safely disable other control registers for the stream */ regmap_update_bits(regs, CCSR_SSI_SRCR, srcr, 0); regmap_update_bits(regs, CCSR_SSI_STCR, stcr, 0); regmap_update_bits(regs, CCSR_SSI_SIER, sier, 0); @@ -534,26 +502,17 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, config_done: /* Enabling of subunits is done after configuration */ if (enable) { + /* Start DMA before setting TE to avoid underrun */ + /* TODO: FIQ cases might also need this upon testing */ if (ssi->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; + + /* Enable SSI first to send TX DMA request */ regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN, CCSR_SSI_SCR_SSIEN); + + /* Wait until TX FIFO not empty -- DMA working */ for (i = 0; i < max_loop; i++) { u32 sfcsr; regmap_read(regs, CCSR_SSI_SFCSR, &sfcsr); @@ -565,6 +524,7 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable, "Timeout waiting TX FIFO filling\n"); } } + /* Enable all remaining bits */ regmap_update_bits(regs, CCSR_SSI_SCR, vals->scr, vals->scr); } } @@ -581,20 +541,9 @@ static void fsl_ssi_tx_ac97_saccst_setup(struct fsl_ssi *ssi) /* no SACC{ST,EN,DIS} regs on imx21-class SSI */ if (!ssi->soc->imx21regs) { - /* - * 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 */ regmap_write(regs, CCSR_SSI_SACCDIS, 0xff); + /* Enable slots 3 & 4 -- PCM Playback Left & Right channels */ regmap_write(regs, CCSR_SSI_SACCEN, 0x300); } } @@ -602,23 +551,11 @@ static void fsl_ssi_tx_ac97_saccst_setup(struct fsl_ssi *ssi) static void fsl_ssi_tx_config(struct fsl_ssi *ssi, bool enable) { /* - * 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. */ 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 */ static void fsl_ssi_setup_reg_vals(struct fsl_ssi *ssi) { @@ -642,6 +577,7 @@ static void fsl_ssi_setup_reg_vals(struct fsl_ssi *ssi) reg->tx.stcr = CCSR_SSI_STCR_TFEN0; reg->tx.scr = 0; + /* AC97 has already enabled SSIEN, RE and TE, so ignore them */ if (!fsl_ssi_is_ac97(ssi)) { reg->rx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE; reg->tx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE; @@ -663,24 +599,17 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi *ssi) { struct regmap *regs = ssi->regs; - /* - * Setup the clock control register - */ + /* Setup the clock control register */ regmap_write(regs, CCSR_SSI_STCCR, CCSR_SSI_SxCCR_WL(17) | CCSR_SSI_SxCCR_DC(13)); regmap_write(regs, CCSR_SSI_SRCCR, CCSR_SSI_SxCCR_WL(17) | CCSR_SSI_SxCCR_DC(13)); - /* - * Enable AC97 mode and startup the SSI - */ + /* Enable AC97 mode and startup the SSI */ regmap_write(regs, CCSR_SSI_SACNT, CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV); - /* - * Enable SSI, Transmit and Receive. AC97 has to communicate with the - * codec before a stream is started. - */ + /* AC97 has to communicate with codec before starting a stream */ regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE, CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE); @@ -688,14 +617,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi *ssi) regmap_write(regs, CCSR_SSI_SOR, CCSR_SSI_SOR_WAIT(3)); } -/** - * 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. - */ static int fsl_ssi_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -707,7 +628,8 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, if (ret) return ret; - /* When using dual fifo mode, it is safer to ensure an even period + /* + * When using dual fifo mode, it is safer to ensure an even period * size. If appearing to an odd number while DMA always starts its * task from fifo0, fifo1 would be neglected at the end of each * period. But SSI would still access fifo1 with an invalid data. @@ -719,10 +641,6 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, return 0; } -/** - * fsl_ssi_shutdown: shutdown the SSI - * - */ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -734,7 +652,7 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, } /** - * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock + * Configure Digital Audio Interface bit clock * * Note: This function can be only called when using SSI as DAI master * @@ -851,17 +769,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream, } /** - * 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. */ 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. - */ - /* In synchronous mode, the SSI uses STCCR for capture */ if ((substream->stream == SNDRV_PCM_STREAM_PLAYBACK) || ssi->cpu_dai_drv.symmetric_rates) @@ -972,6 +877,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev, regmap_read(regs, CCSR_SSI_SCR, &scr); scr &= ~(CCSR_SSI_SCR_SYN | CCSR_SSI_SCR_I2S_MODE_MASK); + /* Synchronize frame sync clock for TE to avoid data slipping */ scr |= CCSR_SSI_SCR_SYNC_TX_FS; mask = CCSR_SSI_STCR_TXBIT0 | CCSR_SSI_STCR_TFDIR | CCSR_SSI_STCR_TXDIR | @@ -982,6 +888,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev, stcr &= ~mask; srcr &= ~mask; + /* Use Network mode as default */ ssi->i2s_mode = CCSR_SSI_SCR_NET; switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_I2S: @@ -1022,6 +929,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev, CCSR_SSI_STCR_TXBIT0; break; case SND_SOC_DAIFMT_AC97: + /* Data on falling edge of bclk, frame high, 1clk before data */ ssi->i2s_mode |= CCSR_SSI_SCR_I2S_MODE_NORMAL; break; default: @@ -1054,13 +962,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev, /* DAI clock master masks */ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBS_CFS: + /* Output bit and frame sync clocks */ strcr |= CCSR_SSI_STCR_TFDIR | CCSR_SSI_STCR_TXDIR; scr |= CCSR_SSI_SCR_SYS_CLK_EN; break; case SND_SOC_DAIFMT_CBM_CFM: + /* Input bit or frame sync clocks */ scr &= ~CCSR_SSI_SCR_SYS_CLK_EN; break; case SND_SOC_DAIFMT_CBM_CFS: + /* Input bit clock but output frame sync clock */ strcr &= ~CCSR_SSI_STCR_TXDIR; strcr |= CCSR_SSI_STCR_TFDIR; scr &= ~CCSR_SSI_SCR_SYS_CLK_EN; @@ -1073,8 +984,8 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev, stcr |= strcr; srcr |= strcr; + /* Set SYN mode and clear RXDIR bit when using SYN or AC97 mode */ if (ssi->cpu_dai_drv.symmetric_rates || fsl_ssi_is_ac97(ssi)) { - /* Need to clear RXDIR when using SYNC or AC97 mode */ srcr &= ~CCSR_SSI_SRCR_RXDIR; scr |= CCSR_SSI_SCR_SYN; } @@ -1106,12 +1017,13 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev, } /** - * fsl_ssi_set_dai_fmt - configure Digital Audio Interface Format. + * Configure Digital Audio Interface (DAI) Format */ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) { struct fsl_ssi *ssi = snd_soc_dai_get_drvdata(cpu_dai); + /* AC97 configured DAIFMT earlier in the probe() */ if (fsl_ssi_is_ac97(ssi)) return 0; @@ -1119,9 +1031,7 @@ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) } /** - * fsl_ssi_set_dai_tdm_slot - set TDM slot number - * - * Note: This function can be only called when using SSI as DAI master + * Set TDM slot number and slot width */ static int fsl_ssi_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask, u32 rx_mask, int slots, int slot_width) @@ -1149,17 +1059,17 @@ static int fsl_ssi_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask, regmap_update_bits(regs, CCSR_SSI_SRCCR, CCSR_SSI_SxCCR_DC_MASK, CCSR_SSI_SxCCR_DC(slots)); - /* The register SxMSKs needs SSI to provide essential clock due to - * hardware design. So we here temporarily enable SSI to set them. - */ + /* Save SSIEN bit of the SCR register */ regmap_read(regs, CCSR_SSI_SCR, &val); val &= CCSR_SSI_SCR_SSIEN; + /* Temporarily enable SSI to allow SxMSKs to be configurable */ regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN, CCSR_SSI_SCR_SSIEN); regmap_write(regs, CCSR_SSI_STMSK, ~tx_mask); regmap_write(regs, CCSR_SSI_SRMSK, ~rx_mask); + /* Restore the value of SSIEN bit */ regmap_update_bits(regs, CCSR_SSI_SCR, CCSR_SSI_SCR_SSIEN, val); ssi->slot_width = slot_width; @@ -1169,13 +1079,7 @@ static int fsl_ssi_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask, } /** - * 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. + * Start or stop SSI and corresponding DMA transaction. */ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) @@ -1207,6 +1111,7 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, return -EINVAL; } + /* Clear corresponding FIFO */ if (fsl_ssi_is_ac97(ssi)) { if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) regmap_write(regs, CCSR_SSI_SOR, CCSR_SSI_SOR_TX_CLR); @@ -1239,7 +1144,6 @@ static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { .trigger = fsl_ssi_trigger, }; -/* Template for the CPU dai driver structure */ static struct snd_soc_dai_driver fsl_ssi_dai_template = { .probe = fsl_ssi_dai_probe, .playback = { @@ -1383,6 +1287,7 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, u32 dmas[4]; int ret; + /* Backward compatible for a DT without ipg clock name assigned */ if (ssi->has_ipg_clk_name) ssi->clk = devm_clk_get(dev, "ipg"); else @@ -1393,6 +1298,7 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, return ret; } + /* Enable the clock since regmap will not handle it in this case */ if (!ssi->has_ipg_clk_name) { ret = clk_prepare_enable(ssi->clk); if (ret) { @@ -1401,9 +1307,7 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, } } - /* For those SLAVE implementations, we ignore non-baudclk cases - * and, instead, abandon MASTER mode that needs baud clock. - */ + /* Do not error out for slave cases that live without a baud clock */ ssi->baudclk = devm_clk_get(dev, "baud"); if (IS_ERR(ssi->baudclk)) dev_dbg(dev, "could not get baud clock: %ld\n", @@ -1414,25 +1318,20 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, ssi->dma_params_tx.addr = ssi->ssi_phys + CCSR_SSI_STX0; ssi->dma_params_rx.addr = ssi->ssi_phys + CCSR_SSI_SRX0; + /* Set to dual FIFO mode according to the SDMA sciprt */ ret = of_property_read_u32_array(np, "dmas", dmas, 4); if (ssi->use_dma && !ret && dmas[2] == IMX_DMATYPE_SSI_DUAL) { ssi->use_dual_fifo = true; - /* When using dual fifo mode, we need to keep watermark - * as even numbers due to dma script limitation. + /* + * Use even numbers to avoid channel swap due to SDMA + * script design */ ssi->dma_params_tx.maxburst &= ~0x1; ssi->dma_params_rx.maxburst &= ~0x1; } if (!ssi->use_dma) { - - /* - * 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 */ ssi->fiq_params.irq = ssi->irq; ssi->fiq_params.base = iomem; ssi->fiq_params.dma_params_rx = &ssi->dma_params_rx; @@ -1490,12 +1389,14 @@ static int fsl_ssi_probe(struct platform_device *pdev) ssi->soc = of_id->data; ssi->dev = dev; + /* Check if being used in AC97 mode */ sprop = of_get_property(np, "fsl,mode", NULL); if (sprop) { if (!strcmp(sprop, "ac97-slave")) ssi->dai_fmt = SND_SOC_DAIFMT_AC97; } + /* Select DMA or FIQ */ ssi->use_dma = !of_property_read_bool(np, "fsl,fiq-stream-filter"); if (fsl_ssi_is_ac97(ssi)) { @@ -1504,7 +1405,6 @@ static int fsl_ssi_probe(struct platform_device *pdev) fsl_ac97_data = ssi; } else { - /* Initialize this copy of the CPU DAI driver structure */ memcpy(&ssi->cpu_dai_drv, &fsl_ssi_dai_template, sizeof(fsl_ssi_dai_template)); } @@ -1517,10 +1417,7 @@ static int fsl_ssi_probe(struct platform_device *pdev) ssi->ssi_phys = res->start; if (ssi->soc->imx21regs) { - /* - * According to datasheet imx21-class SSI - * don't have SACC{ST,EN,DIS} regs. - */ + /* No SACC{ST,EN,DIS} regs in imx21-class SSI */ regconfig.max_register = CCSR_SSI_SRMSK; regconfig.num_reg_defaults_raw = CCSR_SSI_SRMSK / sizeof(uint32_t) + 1; @@ -1546,7 +1443,7 @@ static int fsl_ssi_probe(struct platform_device *pdev) return ssi->irq; } - /* Are the RX and the TX clocks locked? */ + /* Set software limitations for synchronous mode */ if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) { if (!fsl_ssi_is_ac97(ssi)) { ssi->cpu_dai_drv.symmetric_rates = 1; @@ -1556,50 +1453,28 @@ static int fsl_ssi_probe(struct platform_device *pdev) ssi->cpu_dai_drv.symmetric_channels = 1; } - /* Determine the FIFO depth. */ + /* Fetch FIFO depth; Set to 8 for older DT without this property */ iprop = of_get_property(np, "fsl,fifo-depth", NULL); if (iprop) ssi->fifo_depth = be32_to_cpup(iprop); else - /* Older 8610 DTs didn't have the fifo-depth property */ ssi->fifo_depth = 8; /* - * 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. */ 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 */ ssi->fifo_watermark = 8; ssi->dma_maxburst = 8; break; 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. - */ + /* Use old watermark configurations for older chips */ ssi->fifo_watermark = ssi->fifo_depth - 2; ssi->dma_maxburst = ssi->fifo_depth - 2; break; @@ -1642,18 +1517,14 @@ static int fsl_ssi_probe(struct platform_device *pdev) if (ret) goto error_asoc_register; - /* - * 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 */ if (!of_get_property(np, "codec-handle", NULL)) goto done; - /* Trigger the machine driver's probe function. The platform driver - * name of the machine driver is taken from /compatible property of the - * device tree. We also pass the address of the CPU DAI driver - * structure. + /* + * Backward compatible for older bindings by manually triggering the + * machine driver's probe(). Use /compatible property, including the + * address of CPU DAI driver structure, as the name of machine driver. */ sprop = of_get_property(of_find_node_by_path("/"), "compatible", NULL); /* Sometimes the compatible name has a "fsl," prefix, so we strip it. */ diff --git a/sound/soc/fsl/fsl_ssi.h b/sound/soc/fsl/fsl_ssi.h index 5065105..1ad3bde 100644 --- a/sound/soc/fsl/fsl_ssi.h +++ b/sound/soc/fsl/fsl_ssi.h @@ -1,5 +1,5 @@ /* - * fsl_ssi.h - ALSA SSI interface for the Freescale MPC8610 SoC + * fsl_ssi.h - ALSA SSI interface for the Freescale MPC8610 and i.MX SoC * * Author: Timur Tabi <timur@freescale.com> * @@ -12,31 +12,75 @@ #ifndef _MPC8610_I2S_H #define _MPC8610_I2S_H -/* SSI registers */ +/* -- SSI Register Map -- */ + +/* SSI Transmit Data Register 0 */ #define CCSR_SSI_STX0 0x00 +/* SSI Transmit Data Register 1 */ #define CCSR_SSI_STX1 0x04 +/* SSI Receive Data Register 0 */ #define CCSR_SSI_SRX0 0x08 +/* SSI Receive Data Register 1 */ #define CCSR_SSI_SRX1 0x0c +/* SSI Control Register */ #define CCSR_SSI_SCR 0x10 +/* SSI Interrupt Status Register */ #define CCSR_SSI_SISR 0x14 +/* SSI Interrupt Enable Register */ #define CCSR_SSI_SIER 0x18 +/* SSI Transmit Configuration Register */ #define CCSR_SSI_STCR 0x1c +/* SSI Receive Configuration Register */ #define CCSR_SSI_SRCR 0x20 +/* SSI Transmit Clock Control Register */ #define CCSR_SSI_STCCR 0x24 +/* SSI Receive Clock Control Register */ #define CCSR_SSI_SRCCR 0x28 +/* SSI FIFO Control/Status Register */ #define CCSR_SSI_SFCSR 0x2c +/* + * SSI Test Register (Intended for debugging purposes only) + * + * Note: STR is not documented in recent IMX datasheet, but + * is described in IMX51 reference manual at section 56.3.3.14 + */ #define CCSR_SSI_STR 0x30 +/* + * SSI Option Register (Intended for internal use only) + * + * Note: SOR is not documented in recent IMX datasheet, but + * is described in IMX51 reference manual at section 56.3.3.15 + */ #define CCSR_SSI_SOR 0x34 +/* SSI AC97 Control Register */ #define CCSR_SSI_SACNT 0x38 +/* SSI AC97 Command Address Register */ #define CCSR_SSI_SACADD 0x3c +/* SSI AC97 Command Data Register */ #define CCSR_SSI_SACDAT 0x40 +/* SSI AC97 Tag Register */ #define CCSR_SSI_SATAG 0x44 +/* SSI Transmit Time Slot Mask Register */ #define CCSR_SSI_STMSK 0x48 +/* SSI Receive Time Slot Mask Register */ #define CCSR_SSI_SRMSK 0x4c +/* + * SSI AC97 Channel Status Register + * + * The status could be changed by: + * 1) Writing a '1' bit at some position in SACCEN sets relevant bit in SACCST + * 2) Writing a '1' bit at some position in SACCDIS unsets the relevant bit + * 3) Receivng a '1' in SLOTREQ bit from external CODEC via AC Link + */ #define CCSR_SSI_SACCST 0x50 +/* SSI AC97 Channel Enable Register -- Set bits in SACCST */ #define CCSR_SSI_SACCEN 0x54 +/* SSI AC97 Channel Disable Register -- Clear bits in SACCST */ #define CCSR_SSI_SACCDIS 0x58 +/* -- SSI Register Field Maps -- */ + +/* SSI Control Register -- CCSR_SSI_SCR 0x10 */ #define CCSR_SSI_SCR_SYNC_TX_FS 0x00001000 #define CCSR_SSI_SCR_RFR_CLK_DIS 0x00000800 #define CCSR_SSI_SCR_TFR_CLK_DIS 0x00000400 @@ -52,6 +96,7 @@ #define CCSR_SSI_SCR_TE 0x00000002 #define CCSR_SSI_SCR_SSIEN 0x00000001 +/* SSI Interrupt Status Register -- CCSR_SSI_SISR 0x14 */ #define CCSR_SSI_SISR_RFRC 0x01000000 #define CCSR_SSI_SISR_TFRC 0x00800000 #define CCSR_SSI_SISR_CMDAU 0x00040000 @@ -74,6 +119,7 @@ #define CCSR_SSI_SISR_TFE1 0x00000002 #define CCSR_SSI_SISR_TFE0 0x00000001 +/* SSI Interrupt Enable Register -- CCSR_SSI_SIER 0x18 */ #define CCSR_SSI_SIER_RFRC_EN 0x01000000 #define CCSR_SSI_SIER_TFRC_EN 0x00800000 #define CCSR_SSI_SIER_RDMAE 0x00400000 @@ -100,6 +146,7 @@ #define CCSR_SSI_SIER_TFE1_EN 0x00000002 #define CCSR_SSI_SIER_TFE0_EN 0x00000001 +/* SSI Transmit Configuration Register -- CCSR_SSI_STCR 0x1C */ #define CCSR_SSI_STCR_TXBIT0 0x00000200 #define CCSR_SSI_STCR_TFEN1 0x00000100 #define CCSR_SSI_STCR_TFEN0 0x00000080 @@ -111,6 +158,7 @@ #define CCSR_SSI_STCR_TFSL 0x00000002 #define CCSR_SSI_STCR_TEFS 0x00000001 +/* SSI Receive Configuration Register -- CCSR_SSI_SRCR 0x20 */ #define CCSR_SSI_SRCR_RXEXT 0x00000400 #define CCSR_SSI_SRCR_RXBIT0 0x00000200 #define CCSR_SSI_SRCR_RFEN1 0x00000100 @@ -123,7 +171,10 @@ #define CCSR_SSI_SRCR_RFSL 0x00000002 #define CCSR_SSI_SRCR_REFS 0x00000001 -/* STCCR and SRCCR */ +/* + * SSI Transmit Clock Control Register -- CCSR_SSI_STCCR 0x24 + * SSI Receive Clock Control Register -- CCSR_SSI_SRCCR 0x28 + */ #define CCSR_SSI_SxCCR_DIV2_SHIFT 18 #define CCSR_SSI_SxCCR_DIV2 0x00040000 #define CCSR_SSI_SxCCR_PSR_SHIFT 17 @@ -142,9 +193,10 @@ ((((x) - 1) << CCSR_SSI_SxCCR_PM_SHIFT) & CCSR_SSI_SxCCR_PM_MASK) /* - * The xFCNT bits are read-only, and the xFWM bits are read/write. Use the - * CCSR_SSI_SFCSR_xFCNTy() macros to read the FIFO counters, and use the - * CCSR_SSI_SFCSR_xFWMy() macros to set the watermarks. + * SSI FIFO Control/Status Register -- CCSR_SSI_SFCSR 0x2c + * + * Tx or Rx FIFO Counter -- CCSR_SSI_SFCSR_xFCNTy Read-Only + * Tx or Rx FIFO Watermarks -- CCSR_SSI_SFCSR_xFWMy Read/Write */ #define CCSR_SSI_SFCSR_RFCNT1_SHIFT 28 #define CCSR_SSI_SFCSR_RFCNT1_MASK 0xF0000000 @@ -179,6 +231,7 @@ #define CCSR_SSI_SFCSR_TFWM0(x) \ (((x) << CCSR_SSI_SFCSR_TFWM0_SHIFT) & CCSR_SSI_SFCSR_TFWM0_MASK) +/* SSI Test Register -- CCSR_SSI_STR 0x30 */ #define CCSR_SSI_STR_TEST 0x00008000 #define CCSR_SSI_STR_RCK2TCK 0x00004000 #define CCSR_SSI_STR_RFS2TFS 0x00002000 @@ -188,6 +241,7 @@ #define CCSR_SSI_STR_TFS2RFS 0x00000020 #define CCSR_SSI_STR_TXSTATE(x) ((x) & 0x1F) +/* SSI Option Register -- CCSR_SSI_SOR 0x34 */ #define CCSR_SSI_SOR_CLKOFF 0x00000040 #define CCSR_SSI_SOR_RX_CLR 0x00000020 #define CCSR_SSI_SOR_TX_CLR 0x00000010 @@ -197,6 +251,7 @@ #define CCSR_SSI_SOR_WAIT(x) (((x) & 3) << CCSR_SSI_SOR_WAIT_SHIFT) #define CCSR_SSI_SOR_SYNRST 0x00000001 +/* SSI AC97 Control Register -- CCSR_SSI_SACNT 0x38 */ #define CCSR_SSI_SACNT_FRDIV(x) (((x) & 0x3f) << 5) #define CCSR_SSI_SACNT_WR 0x00000010 #define CCSR_SSI_SACNT_RD 0x00000008 diff --git a/sound/soc/fsl/fsl_ssi_dbg.c b/sound/soc/fsl/fsl_ssi_dbg.c index 5469ffb..9c7fa6c 100644 --- a/sound/soc/fsl/fsl_ssi_dbg.c +++ b/sound/soc/fsl/fsl_ssi_dbg.c @@ -82,9 +82,10 @@ void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *dbg, u32 sisr) dbg->stats.tfe0++; } -/* 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 */ #define SIER_SHOW(flag, name) \ do { \ @@ -94,10 +95,9 @@ void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *dbg, u32 sisr) /** - * fsl_sysfs_ssi_show: display SSI statistics + * Display the statistics for the current SSI device * - * Display the statistics for the current SSI device. To avoid confusion, - * we only show those counts that are enabled. + * To avoid confusion, only show those counts that are enabled */ static int fsl_ssi_stats_show(struct seq_file *s, void *unused) {