| Message ID | 1421756480-7055-3-git-send-email-zidan.wang@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote: > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, > u32 word_width = snd_pcm_format_width(params_format(params)); > u32 val_cr4 = 0, val_cr5 = 0; > int ret; > + u32 bclk; > + > + if (channels == 1) > + channels = 2; > + > + if (!sai->slots || sai->slots % channels) > + sai->slots = channels; > + > + sai->slots = sai->slots / channels; > + > + if (sai->slot_width < word_width || sai->is_dsp_mode) > + sai->slot_width = word_width; Could you pls explain a bit what's this overriding for? Or it might be better to put into a comment. Nicolin
On Wed, Jan 21, 2015 at 10:08:03AM -0800, Nicolin Chen wrote: > On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote: > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, > > u32 word_width = snd_pcm_format_width(params_format(params)); > > u32 val_cr4 = 0, val_cr5 = 0; > > int ret; > > + u32 bclk; > > + > > + if (channels == 1) > > + channels = 2; > > + > > + if (!sai->slots || sai->slots % channels) > > + sai->slots = channels; > > + > > + sai->slots = sai->slots / channels; > > + > > + if (sai->slot_width < word_width || sai->is_dsp_mode) > > + sai->slot_width = word_width; > > Could you pls explain a bit what's this overriding for? > Or it might be better to put into a comment. > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots and slot width. In my opinion, slots in machine driver means the slot number of all channels. But in sai driver, the slots means the slot number of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots to caculate bclk. So i use "sai->slots = sai->slots / channels" to calculate slots per channel. If we missing set slots, use channel number to set slots and get one slot per channel. If we missing set slot width, set default slot width to word width. If slot width is 32 and wrod width is 16, for 2 channels and one slot per channel, it will be 64 bit clock for one frame. val_cr5 |= FSL_SAI_CR5_WNW(sai->slot_width). val_cr5 |= FSL_SAI_CR5_W0W(sai->slot_width); So sai word length is 32, it will trans 32 bit data per channel. But dma only trans 16 bit data to fifo, the continues 16 bit will be 0. So for 16 bit data formate, it will just one channel has data. When it is dsp mode, let slot width equal to word width can fix the issue. > Nicolin
On Thu, Jan 22, 2015 at 12:55:35PM +0800, Zidan Wang wrote: > On Wed, Jan 21, 2015 at 10:08:03AM -0800, Nicolin Chen wrote: > > On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote: > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, > > > u32 word_width = snd_pcm_format_width(params_format(params)); > > > u32 val_cr4 = 0, val_cr5 = 0; > > > int ret; > > > + u32 bclk; > > > + > > > + if (channels == 1) > > > + channels = 2; > > > + > > > + if (!sai->slots || sai->slots % channels) > > > + sai->slots = channels; > > > + > > > + sai->slots = sai->slots / channels; > > > + > > > + if (sai->slot_width < word_width || sai->is_dsp_mode) > > > + sai->slot_width = word_width; > > > > Could you pls explain a bit what's this overriding for? > > Or it might be better to put into a comment. > > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots > and slot width. In my opinion, slots in machine driver means the slot > number of all channels. But in sai driver, the slots means the slot number > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots > to caculate bclk. Could you pls use the standard way as you described for slot number in machine driver instead of defining some other meanings? That one is really confusing, not to mention a code without any comment. You can add some extra local variables with extinguished names and calculate the slot number and channels you want without changing the original sai->slots and channels in the hw_params(). As you said, you only need that new slot number and channels for snd_soc_calc_bclk call. So you may also put this code right before the call -- a normal routine doesn't need to recalculate the special slots and channels, right? Nicolin
On Wed, Jan 21, 2015 at 09:44:10PM -0800, Nicolin Chen wrote: > On Thu, Jan 22, 2015 at 12:55:35PM +0800, Zidan Wang wrote: > > On Wed, Jan 21, 2015 at 10:08:03AM -0800, Nicolin Chen wrote: > > > On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote: > > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, > > > > u32 word_width = snd_pcm_format_width(params_format(params)); > > > > u32 val_cr4 = 0, val_cr5 = 0; > > > > int ret; > > > > + u32 bclk; > > > > + > > > > + if (channels == 1) > > > > + channels = 2; > > > > + > > > > + if (!sai->slots || sai->slots % channels) > > > > + sai->slots = channels; > > > > + > > > > + sai->slots = sai->slots / channels; > > > > + > > > > + if (sai->slot_width < word_width || sai->is_dsp_mode) > > > > + sai->slot_width = word_width; > > > > > > Could you pls explain a bit what's this overriding for? > > > Or it might be better to put into a comment. > > > > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots > > and slot width. In my opinion, slots in machine driver means the slot > > number of all channels. But in sai driver, the slots means the slot number > > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots > > to caculate bclk. > > Could you pls use the standard way as you described for slot number > in machine driver instead of defining some other meanings? That one > is really confusing, not to mention a code without any comment. > > You can add some extra local variables with extinguished names and > calculate the slot number and channels you want without changing > the original sai->slots and channels in the hw_params(). As you said, > you only need that new slot number and channels for snd_soc_calc_bclk > call. So you may also put this code right before the call -- a normal > routine doesn't need to recalculate the special slots and channels, > right? > > Nicolin Actually i don't kown which meaning is the standard way. Because i found in wm8993 and wm8904 codec driver, they also use slots/channels to set tdm_slots in set_tdm_slot function. Best Regards, Zidan
On Thu, Jan 22, 2015 at 02:20:06PM +0800, Zidan Wang wrote: > > > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, > > > > > u32 word_width = snd_pcm_format_width(params_format(params)); > > > > > u32 val_cr4 = 0, val_cr5 = 0; > > > > > int ret; > > > > > + u32 bclk; > > > > > + > > > > > + if (channels == 1) > > > > > + channels = 2; > > > > > + > > > > > + if (!sai->slots || sai->slots % channels) > > > > > + sai->slots = channels; > > > > > + > > > > > + sai->slots = sai->slots / channels; > > > > > + > > > > > + if (sai->slot_width < word_width || sai->is_dsp_mode) > > > > > + sai->slot_width = word_width; > > > > > > > > Could you pls explain a bit what's this overriding for? > > > > Or it might be better to put into a comment. > > > > > > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots > > > and slot width. In my opinion, slots in machine driver means the slot > > > number of all channels. But in sai driver, the slots means the slot number > > > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots > > > to caculate bclk. > > > > Could you pls use the standard way as you described for slot number > > in machine driver instead of defining some other meanings? That one > > is really confusing, not to mention a code without any comment. > > > > You can add some extra local variables with extinguished names and > > calculate the slot number and channels you want without changing > > the original sai->slots and channels in the hw_params(). As you said, > > you only need that new slot number and channels for snd_soc_calc_bclk > > call. So you may also put this code right before the call -- a normal > > routine doesn't need to recalculate the special slots and channels, > > right? > Actually i don't kown which meaning is the standard way. Because i found > in wm8993 and wm8904 codec driver, they also use slots/channels to set > tdm_slots in set_tdm_slot function. The slots via machine driver should be total slot number per frame. For I2S, we're passing 2 for slots parameter in our machine drivers. So why not just following that pattern. You code for calculating slot per channel is used to calculate bclk. But keeping slot per frame will also allow you to do that, meanwhile you can drop some code to do the extra calculation so as to keep it neat. I suggest you to take a look at fsl_esai.c and to follow that way: Override slots and slot_width if specified from machine driver. And another problem of your patch is the configurations for FRSZ and xMR fields. For monaural case, the channels == 1, the current code passes it directly to set them while your code overrides it to 2. I'm not sure if you've tested the monaural case and confirmed it works. But, apparently, the configurations look pretty different. Nicolin
On Thu, Jan 22, 2015 at 03:50:20PM -0800, Nicolin Chen wrote: > On Thu, Jan 22, 2015 at 02:20:06PM +0800, Zidan Wang wrote: > > > > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, > > > > > > u32 word_width = snd_pcm_format_width(params_format(params)); > > > > > > u32 val_cr4 = 0, val_cr5 = 0; > > > > > > int ret; > > > > > > + u32 bclk; > > > > > > + > > > > > > + if (channels == 1) > > > > > > + channels = 2; > > > > > > + > > > > > > + if (!sai->slots || sai->slots % channels) > > > > > > + sai->slots = channels; > > > > > > + > > > > > > + sai->slots = sai->slots / channels; > > > > > > + > > > > > > + if (sai->slot_width < word_width || sai->is_dsp_mode) > > > > > > + sai->slot_width = word_width; > > > > > > > > > > Could you pls explain a bit what's this overriding for? > > > > > Or it might be better to put into a comment. > > > > > > > > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots > > > > and slot width. In my opinion, slots in machine driver means the slot > > > > number of all channels. But in sai driver, the slots means the slot number > > > > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots > > > > to caculate bclk. > > > > > > Could you pls use the standard way as you described for slot number > > > in machine driver instead of defining some other meanings? That one > > > is really confusing, not to mention a code without any comment. > > > > > > You can add some extra local variables with extinguished names and > > > calculate the slot number and channels you want without changing > > > the original sai->slots and channels in the hw_params(). As you said, > > > you only need that new slot number and channels for snd_soc_calc_bclk > > > call. So you may also put this code right before the call -- a normal > > > routine doesn't need to recalculate the special slots and channels, > > > right? > > > Actually i don't kown which meaning is the standard way. Because i found > > in wm8993 and wm8904 codec driver, they also use slots/channels to set > > tdm_slots in set_tdm_slot function. > > The slots via machine driver should be total slot number per frame. > For I2S, we're passing 2 for slots parameter in our machine drivers. > > So why not just following that pattern. You code for calculating slot > per channel is used to calculate bclk. But keeping slot per frame will > also allow you to do that, meanwhile you can drop some code to do the > extra calculation so as to keep it neat. > > I suggest you to take a look at fsl_esai.c and to follow that way: > Override slots and slot_width if specified from machine driver. > I will follow slot per frame pattern like fsl_esai.c, thanks very mush. I have another question. Can a channel have several slots? Or channel just means slot. > And another problem of your patch is the configurations for FRSZ and > xMR fields. For monaural case, the channels == 1, the current code > passes it directly to set them while your code overrides it to 2. > > I'm not sure if you've tested the monaural case and confirmed it works. > But, apparently, the configurations look pretty different. > I have tested the mono case, it works well. Best Regards, Zidan
On Fri, Jan 23, 2015 at 10:55:09AM +0800, Zidan Wang wrote: > I have another question. > > Can a channel have several slots? Or channel just means slot. Strictly speaking, a channel surely may have several slots IMO. The idea for TDM, even for I2S, left channel can bear multiple slots -- each may be masked and then received by a different peripheral. The reason I've said that much against your code is not fully objection against your idea of using Slot Per Channel but for the implementation -- you divided them and then multiplied them again while your true motivation was just to calculate the bclk. And I could not understand it without seeing your later explain. Code and things would be much easier if you make it straight forward. But actually I don't mind if you feel comfortable to make a detour like that, but as I commented at the first place. Code like that should be companied with comments to tell its meaning. And I still don't fully understand why you override the channels for monaural cases. > > And another problem of your patch is the configurations for FRSZ and > > xMR fields. For monaural case, the channels == 1, the current code > > passes it directly to set them while your code overrides it to 2. > > > > I'm not sure if you've tested the monaural case and confirmed it works. > > But, apparently, the configurations look pretty different. > I have tested the mono case, it works well. I just ran a simple test with speaker-test -Dhw:0 -c1, it works but the FRSZ value is changed to 0x1 for a monaural case (0x0 originally). Setting FRSZ to 0x0 makes more sense to me since it looks like the configuration for active slot number. But that depends on the internal logic design of SAI so it may be better for you to double check with IC owner and run more tests. If you confirm that, I for sure will be willing to ignore the change here. Nicolin
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 499cbd9..4c5040d 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -115,6 +115,17 @@ out: return IRQ_HANDLED; } +static int fsl_sai_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask, + u32 rx_mask, int slots, int slot_width) +{ + struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + + sai->slots = slots; + sai->slot_width = slot_width; + + return 0; +} + static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int fsl_dir) { @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, u32 word_width = snd_pcm_format_width(params_format(params)); u32 val_cr4 = 0, val_cr5 = 0; int ret; + u32 bclk; + + if (channels == 1) + channels = 2; + + if (!sai->slots || sai->slots % channels) + sai->slots = channels; + + sai->slots = sai->slots / channels; + + if (sai->slot_width < word_width || sai->is_dsp_mode) + sai->slot_width = word_width; if (!sai->is_slave_mode) { - ret = fsl_sai_set_bclk(cpu_dai, tx, - 2 * word_width * params_rate(params)); + bclk = snd_soc_calc_bclk(params_rate(params), sai->slot_width, + channels, sai->slots); + + ret = fsl_sai_set_bclk(cpu_dai, tx, bclk); + if (ret) return ret; @@ -385,20 +411,19 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, sai->mclk_streams |= BIT(substream->stream); } + if (!sai->is_dsp_mode) + val_cr4 |= FSL_SAI_CR4_SYWD(sai->slot_width); } - if (!sai->is_dsp_mode) - val_cr4 |= FSL_SAI_CR4_SYWD(word_width); - - val_cr5 |= FSL_SAI_CR5_WNW(word_width); - val_cr5 |= FSL_SAI_CR5_W0W(word_width); + val_cr5 |= FSL_SAI_CR5_WNW(sai->slot_width); + val_cr5 |= FSL_SAI_CR5_W0W(sai->slot_width); if (sai->is_lsb_first) val_cr5 |= FSL_SAI_CR5_FBT(0); else val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1); - val_cr4 |= FSL_SAI_CR4_FRSZ(channels); + val_cr4 |= FSL_SAI_CR4_FRSZ(sai->slots * channels); regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK, @@ -406,7 +431,9 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, regmap_update_bits(sai->regmap, FSL_SAI_xCR5(tx), FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK | FSL_SAI_CR5_FBT_MASK, val_cr5); - regmap_write(sai->regmap, FSL_SAI_xMR(tx), ~0UL - ((1 << channels) - 1)); + + regmap_write(sai->regmap, FSL_SAI_xMR(tx), + ~0UL - ((1 << params_channels(params) * sai->slots) - 1)); return 0; } @@ -531,6 +558,7 @@ static void fsl_sai_shutdown(struct snd_pcm_substream *substream, static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = { .set_sysclk = fsl_sai_set_dai_sysclk, .set_fmt = fsl_sai_set_dai_fmt, + .set_tdm_slot = fsl_sai_set_dai_tdm_slot, .hw_params = fsl_sai_hw_params, .hw_free = fsl_sai_hw_free, .trigger = fsl_sai_trigger, diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index 2d8c830..111dfce 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -140,6 +140,9 @@ struct fsl_sai { bool sai_on_imx; bool synchronous[2]; + unsigned int slots; + unsigned int slot_width; + unsigned int mclk_id; unsigned int mclk_streams; struct snd_dmaengine_dai_dma_data dma_params_rx;
slots and slot width is set from set_dai_tdm_slot in machine driver. We should calculate the actual slots per channel using slots/channels. When using tdm slots, we should generate bclk depends channels, slots and slot width. And there may be unused BCLK cycles before each LRCLK transition. Set TCR2 WNW bit to slot width and TCR4 FRSZ bit to slots * channels to configure frame Length. And it is no need to set TCR4 SYWD to set frame sync length for sai slave mode, so just do it when it is sai master mode. Signed-off-by: Zidan Wang <zidan.wang@freescale.com> --- sound/soc/fsl/fsl_sai.c | 46 +++++++++++++++++++++++++++++++++++++--------- sound/soc/fsl/fsl_sai.h | 3 +++ 2 files changed, 40 insertions(+), 9 deletions(-)