Hi Liam,<br><br><div class="gmail_quote">2010/6/23 Liam Girdwood <span dir="ltr"><<a href="mailto:lrg@slimlogic.co.uk">lrg@slimlogic.co.uk</a>></span><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im">On Wed, 2010-06-23 at 16:33 +0200, Raffaele Recalcati wrote:<br>
> From: Raffaele Recalcati <<a href="mailto:raffaele.recalcati@bticino.it">raffaele.recalcati@bticino.it</a>><br>
><br>
> Added audio playback support with [frame sync master - clock master] mode<br>
> and with [frame sync master - clock slave].<br>
> Clock slave can be important when the external codec need system clock<br>
> and bit clock synchronized.<br>
> In the clock master case there is a FIXME message in the source code, because we<br>
> (Davide and myself) have guessed a frequency of 122000000 that seems the base<br>
> to be divided.<br>
> This patch has been developed against the<br>
> <a href="http://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git" target="_blank">http://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git</a><br>
> git tree and has been tested on bmx board (similar to dm365 evm, but using<br>
> uda1345 as external audio codec).<br>
><br>
> Signed-off-by: Raffaele Recalcati <<a href="mailto:raffaele.recalcati@bticino.it">raffaele.recalcati@bticino.it</a>><br>
> Signed-off-by: Davide Bonfanti <<a href="mailto:davide.bonfanti@bticino.it">davide.bonfanti@bticino.it</a>><br>
<br>
</div>Had a quick check, it looks like you have made some unintended<br>
formatting changes that make the patch look more complex than necessary.<br></blockquote><div><br>I was making correction for keeping aligned my lines like these...<br>+ srgr = DAVINCI_MCBSP_SRGR_FSGM |<br>
+ DAVINCI_MCBSP_SRGR_CLKSM;<br><br>and so I changed some existing ones...<br>but looking at CodingStyle and Greg notes, seems that, when breaking lines, we need to keep alignment.<br>So maybe, if I'm right, in the future, we'll need to re-align all the file with another patch..<br>
I'll change anyway the code back to be simpler to be understood.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> ---<br>
> arch/arm/mach-davinci/include/mach/asp.h | 7 ++<br>
> sound/soc/davinci/davinci-i2s.c | 141 ++++++++++++++++++++++++++----<br>
> 2 files changed, 129 insertions(+), 19 deletions(-)<br>
><br>
> diff --git a/arch/arm/mach-davinci/include/mach/asp.h b/arch/arm/mach-davinci/include/mach/asp.h<br>
> index 834725f..b1faeb9 100644<br>
> --- a/arch/arm/mach-davinci/include/mach/asp.h<br>
> +++ b/arch/arm/mach-davinci/include/mach/asp.h<br>
> @@ -63,6 +63,13 @@ struct snd_platform_data {<br>
> unsigned sram_size_playback;<br>
> unsigned sram_size_capture;<br>
><br>
> + /*<br>
> + * This define works when both clock and FS are output for the cpu<br>
> + * and makes clock very fast (FS is not simmetrical, but sampling<br>
> + * frequency is better approximated<br>
> + */<br>
> + int i2s_fast_clock;<br>
> +<br>
> /* McASP specific fields */<br>
> int tdm_slots;<br>
> u8 op_mode;<br>
> diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c<br>
> index adadcd3..8811d25 100644<br>
> --- a/sound/soc/davinci/davinci-i2s.c<br>
> +++ b/sound/soc/davinci/davinci-i2s.c<br>
> @@ -68,16 +68,23 @@<br>
> #define DAVINCI_MCBSP_RCR_RDATDLY(v) ((v) << 16)<br>
> #define DAVINCI_MCBSP_RCR_RFIG (1 << 18)<br>
> #define DAVINCI_MCBSP_RCR_RWDLEN2(v) ((v) << 21)<br>
> +#define DAVINCI_MCBSP_RCR_RFRLEN2(v) ((v) << 24)<br>
> +#define DAVINCI_MCBSP_RCR_RPHASE (1 << 31)<br>
><br>
> #define DAVINCI_MCBSP_XCR_XWDLEN1(v) ((v) << 5)<br>
> #define DAVINCI_MCBSP_XCR_XFRLEN1(v) ((v) << 8)<br>
> #define DAVINCI_MCBSP_XCR_XDATDLY(v) ((v) << 16)<br>
> #define DAVINCI_MCBSP_XCR_XFIG (1 << 18)<br>
> #define DAVINCI_MCBSP_XCR_XWDLEN2(v) ((v) << 21)<br>
> +#define DAVINCI_MCBSP_XCR_XFRLEN2(v) ((v) << 24)<br>
> +#define DAVINCI_MCBSP_XCR_XPHASE (1 << 31)<br>
><br>
> +<br>
> +#define CLKGDV(v) (v) /* Bits 0:7 */<br>
<br>
</div></div>Should you not have a DAVINCI prefix here too ?<br></blockquote><div><br>Sorry, CLKGDV was not more used.<br>So I'm deleting it in the new patch coming today.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> #define DAVINCI_MCBSP_SRGR_FWID(v) ((v) << 8)<br>
> #define DAVINCI_MCBSP_SRGR_FPER(v) ((v) << 16)<br>
> #define DAVINCI_MCBSP_SRGR_FSGM (1 << 28)<br>
> +#define DAVINCI_MCBSP_SRGR_CLKSM (1 << 29)<br>
><br>
> #define DAVINCI_MCBSP_PCR_CLKRP (1 << 0)<br>
> #define DAVINCI_MCBSP_PCR_CLKXP (1 << 1)<br>
> @@ -144,8 +151,17 @@ struct davinci_mcbsp_dev {<br>
> * won't end up being swapped because of the underrun.<br>
> */<br>
> unsigned enable_channel_combine:1;<br>
> +<br>
> + int i2s_fast_clock;<br>
> +};<br>
> +<br>
> +struct davinci_mcbsp_data {<br>
> + unsigned int fmt;<br>
> + int clk_div;<br>
> };<br>
><br>
> +static struct davinci_mcbsp_data mcbsp_data;<br>
> +<br>
<br>
</div>This struct should be part of the dai private data.<br></blockquote><div><br>done.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,<br>
> int reg, u32 val)<br>
> {<br>
> @@ -255,24 +271,27 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,<br>
> unsigned int pcr;<br>
> unsigned int srgr;<br>
> srgr = DAVINCI_MCBSP_SRGR_FSGM |<br>
> - DAVINCI_MCBSP_SRGR_FPER(DEFAULT_BITPERSAMPLE * 2 - 1) |<br>
> - DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1);<br>
> + DAVINCI_MCBSP_SRGR_FPER(DEFAULT_BITPERSAMPLE * 2 - 1) |<br>
> + DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1);<br>
> + /* Attention srgr is updated by hw_params! */<br>
><br>
> + mcbsp_data.fmt = fmt;<br>
> /* set master/slave audio interface */<br>
> switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {<br>
> + case SND_SOC_DAIFMT_CBS_CFM:<br>
> case SND_SOC_DAIFMT_CBS_CFS:<br>
> /* cpu is master */<br>
> pcr = DAVINCI_MCBSP_PCR_FSXM |<br>
> - DAVINCI_MCBSP_PCR_FSRM |<br>
> - DAVINCI_MCBSP_PCR_CLKXM |<br>
> - DAVINCI_MCBSP_PCR_CLKRM;<br>
> + DAVINCI_MCBSP_PCR_FSRM |<br>
> + DAVINCI_MCBSP_PCR_CLKXM |<br>
> + DAVINCI_MCBSP_PCR_CLKRM;<br>
> break;<br>
> case SND_SOC_DAIFMT_CBM_CFS:<br>
> /* McBSP CLKR pin is the input for the Sample Rate Generator.<br>
> * McBSP FSR and FSX are driven by the Sample Rate Generator. */<br>
> pcr = DAVINCI_MCBSP_PCR_SCLKME |<br>
> - DAVINCI_MCBSP_PCR_FSXM |<br>
> - DAVINCI_MCBSP_PCR_FSRM;<br>
> + DAVINCI_MCBSP_PCR_FSXM |<br>
> + DAVINCI_MCBSP_PCR_FSRM;<br>
<br>
</div></div>These two just look like unintended formatting changes.<br></blockquote><div><br>in next patch I'll remove these changes.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> break;<br>
> case SND_SOC_DAIFMT_CBM_CFM:<br>
> /* codec is master */<br>
> @@ -372,6 +391,17 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,<br>
> return 0;<br>
> }<br>
><br>
> +static int davinci_i2s_dai_set_clkdiv(struct snd_soc_dai *cpu_dai,<br>
> + int div_id, int div)<br>
> +{<br>
> + struct davinci_mcbsp_dev *dev = cpu_dai->private_data;<br>
> + int srgr;<br>
> +<br>
> + mcbsp_data.clk_div = div;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +<br>
> static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,<br>
> struct snd_pcm_hw_params *params,<br>
> struct snd_soc_dai *dai)<br>
> @@ -380,11 +410,12 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,<br>
> struct davinci_pcm_dma_params *dma_params =<br>
> &dev->dma_params[substream->stream];<br>
> struct snd_interval *i = NULL;<br>
> - int mcbsp_word_length;<br>
> - unsigned int rcr, xcr, srgr;<br>
> + int mcbsp_word_length, master;<br>
> + unsigned int rcr, xcr, srgr, clk_div, freq, framesize;<br>
> u32 spcr;<br>
> snd_pcm_format_t fmt;<br>
> unsigned element_cnt = 1;<br>
> + struct clk *clk;<br>
><br>
> /* general line settings */<br>
> spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);<br>
> @@ -396,12 +427,60 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,<br>
> davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);<br>
> }<br>
><br>
> - i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);<br>
> - srgr = DAVINCI_MCBSP_SRGR_FSGM;<br>
> - srgr |= DAVINCI_MCBSP_SRGR_FWID(snd_interval_value(i) - 1);<br>
> + master = mcbsp_data.fmt & SND_SOC_DAIFMT_MASTER_MASK;<br>
> + fmt = params_format(params);<br>
> + mcbsp_word_length = asp_word_length[fmt];<br>
><br>
> - i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_FRAME_BITS);<br>
> - srgr |= DAVINCI_MCBSP_SRGR_FPER(snd_interval_value(i) - 1);<br>
> + if (master == SND_SOC_DAIFMT_CBS_CFS) {<br>
> + clk = clk_get(NULL, "pll1_sysclk6");<br>
> + if (clk)<br>
> + freq = clk_get_rate(clk);<br>
> + freq = 122000000; /* FIXME ask to Texas */<br>
<br>
</div></div>This frequency should either be passed in as platform data or by your<br>
machine driver calling snd_soc_dai_set_sysclk().<br></blockquote><div><br>First I need to understand its meaning.<br>Reading sprufi3a-1.pdf (TMS320DM36x DMSoC Multichannel Buffered Serial Port (McBSP) Interface) at "2.5.3 Data Clock Generation" paragraph this info not appears really clear if we are in the case of "McBSP internal input clock".<br>
I hope some Ti developer can help me.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + if (dev->i2s_fast_clock) {<br>
> + clk_div = 256;<br>
> + do {<br>
> + framesize = (freq / (--clk_div)) /<br>
> + params->rate_num *<br>
> + params->rate_den;<br>
> + } while (((framesize < 33) || (framesize > 4095)) &&<br>
> + (clk_div));<br>
> + clk_div--;<br>
> + srgr = DAVINCI_MCBSP_SRGR_FSGM |<br>
> + DAVINCI_MCBSP_SRGR_CLKSM;<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FWID(mcbsp_word_length *<br>
> + 8 - 1);<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FPER(framesize - 1);<br>
> + } else {<br>
> + /* simmetric waveforms */<br>
<br>
</div>symmetric<br></blockquote><div><br>thx<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> + srgr = DAVINCI_MCBSP_SRGR_FSGM |<br>
> + DAVINCI_MCBSP_SRGR_CLKSM;<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FWID(mcbsp_word_length *<br>
> + 8 - 1);<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FPER(mcbsp_word_length *<br>
> + 16 - 1);<br>
> + clk_div = freq / (mcbsp_word_length * 16) /<br>
> + params->rate_num * params->rate_den;<br>
> + }<br>
> + clk_div &= 0xFF;<br>
> + srgr |= clk_div;<br>
> + } else if (master == SND_SOC_DAIFMT_CBS_CFM) {<br>
> + /* Clock given on CLKS */<br>
> + srgr = DAVINCI_MCBSP_SRGR_FSGM;<br>
> + clk_div = mcbsp_data.clk_div - 1;<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FWID(mcbsp_word_length * 8 - 1);<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FPER(mcbsp_word_length * 16 - 1);<br>
> + clk_div &= 0xFF;<br>
> + srgr |= clk_div;<br>
> + } else {<br>
> + i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);<br>
> + srgr = DAVINCI_MCBSP_SRGR_FSGM;<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FWID(snd_interval_value(i) - 1);<br>
> + pr_debug("%s - %d FWID set: re-read srgr = %X\n",<br>
> + __func__, __LINE__, snd_interval_value(i) - 1);<br>
> +<br>
> + i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_FRAME_BITS);<br>
> + srgr |= DAVINCI_MCBSP_SRGR_FPER(snd_interval_value(i) - 1);<br>
> + }<br>
> davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SRGR_REG, srgr);<br>
><br>
> rcr = DAVINCI_MCBSP_RCR_RFIG;<br>
> @@ -426,22 +505,45 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,<br>
> element_cnt = 1;<br>
> fmt = double_fmt[fmt];<br>
> }<br>
> + if (master == SND_SOC_DAIFMT_CBS_CFS ||<br>
> + master == SND_SOC_DAIFMT_CBS_CFM) {<br>
> + rcr |= DAVINCI_MCBSP_RCR_RFRLEN2(0);<br>
> + xcr |= DAVINCI_MCBSP_XCR_XFRLEN2(0);<br>
> + rcr |= DAVINCI_MCBSP_RCR_RPHASE;<br>
> + xcr |= DAVINCI_MCBSP_XCR_XPHASE;<br>
> + } else {<br>
> + /* FIXME ask to Texas */<br>
> + rcr |= DAVINCI_MCBSP_RCR_RFRLEN2(element_cnt - 1);<br>
> + xcr |= DAVINCI_MCBSP_XCR_XFRLEN2(element_cnt - 1);<br>
> + }<br>
> }<br>
> dma_params->acnt = dma_params->data_type = data_type[fmt];<br>
> dma_params->fifo_level = 0;<br>
> mcbsp_word_length = asp_word_length[fmt];<br>
> - rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(element_cnt - 1);<br>
> - xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(element_cnt - 1);<br>
> +<br>
> + if (master == SND_SOC_DAIFMT_CBS_CFS ||<br>
> + master == SND_SOC_DAIFMT_CBS_CFM) {<br>
> + rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);<br>
> + xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);<br>
> + } else {<br>
> + /* FIXME ask to Texas */<br>
> + rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(element_cnt - 1);<br>
> + xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(element_cnt - 1);<br>
> + }<br>
><br>
> rcr |= DAVINCI_MCBSP_RCR_RWDLEN1(mcbsp_word_length) |<br>
> - DAVINCI_MCBSP_RCR_RWDLEN2(mcbsp_word_length);<br>
> + DAVINCI_MCBSP_RCR_RWDLEN2(mcbsp_word_length);<br>
<br>
</div></div>more unintended formatting ?<br>
<div class="im"><br>
> xcr |= DAVINCI_MCBSP_XCR_XWDLEN1(mcbsp_word_length) |<br>
> - DAVINCI_MCBSP_XCR_XWDLEN2(mcbsp_word_length);<br>
> + DAVINCI_MCBSP_XCR_XWDLEN2(mcbsp_word_length);<br>
<br>
</div>ditto<br></blockquote><div><br><br>I'll remove these changes.<br><br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
><br>
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)<br>
> davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_XCR_REG, xcr);<br>
> else<br>
> davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_RCR_REG, rcr);<br>
> +<br>
> + pr_debug("%s - %d srgr=%X\n", __func__, __LINE__, srgr);<br>
> + pr_debug("%s - %d xcr=%X\n", __func__, __LINE__, xcr);<br>
> + pr_debug("%s - %d rcr=%X\n", __func__, __LINE__, rcr);<br>
> return 0;<br>
> }<br>
><br>
> @@ -500,7 +602,7 @@ static struct snd_soc_dai_ops davinci_i2s_dai_ops = {<br>
> .trigger = davinci_i2s_trigger,<br>
> .hw_params = davinci_i2s_hw_params,<br>
> .set_fmt = davinci_i2s_set_dai_fmt,<br>
> -<br>
> + .set_clkdiv = davinci_i2s_dai_set_clkdiv,<br>
<br>
</div>the formatting is different to rest of struct here.<br></blockquote><div><br>ok. fixing<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> };<br>
><br>
> struct snd_soc_dai davinci_i2s_dai = {<br>
> @@ -548,6 +650,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)<br>
> }<br>
> if (pdata) {<br>
> dev->enable_channel_combine = pdata->enable_channel_combine;<br>
> + dev->i2s_fast_clock = pdata->i2s_fast_clock;<br>
> dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK].sram_size =<br>
> pdata->sram_size_playback;<br>
> dev->dma_params[SNDRV_PCM_STREAM_CAPTURE].sram_size =<br>
<br>
</div>The subject of the patch looks wrong as I can't really see where you are<br>
adding stereo support to the DAI. This patch looks likes it adds more<br>
clocking options only,<br></blockquote><div><br>there are these supports added.<br><br>1. SND_SOC_DAIFMT_CBS_CFS (the cpu generates clock and frame sync)<br>I need to clarify how is clocked McBSP interface internally.<br>
i2s_fast_clock switch can be used to have better approximate or symmetric waveforms.<br><br>2. SND_SOC_DAIFMT_CBS_CFM (the cpu get clock from external pin and generates frame sync)<br>this is important for uda1345.<br><br>
3. We haven't changed the evmdm365 support (due also to CPLD that doesn't help to understand)<br>We don't know in this mode if audio stereo works on evmdm365.<br>Probably it does.<br><br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Thanks<br>
<br>
Liam<br>
<font color="#888888">--<br>
Freelance Developer, SlimLogic Ltd<br>
ASoC and Voltage Regulator Maintainer.<br>
<a href="http://www.slimlogic.co.uk" target="_blank">http://www.slimlogic.co.uk</a><br>
<br>
</font></blockquote></div><br><br>The better (I hope) patch we'll come today.<br><br>Bye,<br>Raffaele and Davide.<br><br>