[PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Liam Girdwood]

From: Graeme Gregory <gg@slimlogic.co.uk>

This patch increases the number of supported audio channels from 4
to 16 and was sponsored by Shotspotter inc.

Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/omap/omap-mcbsp.c |   71 +++++++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 3341f49..290ef2f 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -49,6 +49,8 @@ struct omap_mcbsp_data {
 	 */
 	int				active;
 	int				configured;
+	unsigned int			in_freq;
+	int				clk_div;
 };
 
 #define to_mcbsp(priv)	container_of((priv), struct omap_mcbsp_data, bus_id)
@@ -257,7 +259,7 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 	int dma, bus_id = mcbsp_data->bus_id, id = cpu_dai->id;
 	int wlen, channels, wpf, sync_mode = OMAP_DMA_SYNC_ELEMENT;
 	unsigned long port;
-	unsigned int format;
+	unsigned int format, frame_size, div;
 
 	if (cpu_class_is_omap1()) {
 		dma = omap1_dma_reqs[bus_id][substream->stream];
@@ -294,27 +296,23 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 
 	format = mcbsp_data->fmt & SND_SOC_DAIFMT_FORMAT_MASK;
 	wpf = channels = params_channels(params);
-	switch (channels) {
-	case 2:
-		if (format == SND_SOC_DAIFMT_I2S) {
-			/* Use dual-phase frames */
-			regs->rcr2	|= RPHASE;
-			regs->xcr2	|= XPHASE;
-			/* Set 1 word per (McBSP) frame for phase1 and phase2 */
-			wpf--;
-			regs->rcr2	|= RFRLEN2(wpf - 1);
-			regs->xcr2	|= XFRLEN2(wpf - 1);
-		}
-	case 1:
-	case 4:
+	if (channels == 2 && format == SND_SOC_DAIFMT_I2S) {
+		/* Use dual-phase frames */
+		regs->rcr2	|= RPHASE;
+		regs->xcr2	|= XPHASE;
+		/* Set 1 word per (McBSP) frame for phase1 and phase2 */
+		wpf--;
+		regs->rcr2	|= RFRLEN2(wpf - 1);
+		regs->xcr2	|= XFRLEN2(wpf - 1);
 		/* Set word per (McBSP) frame for phase1 */
 		regs->rcr1	|= RFRLEN1(wpf - 1);
 		regs->xcr1	|= XFRLEN1(wpf - 1);
-		break;
-	default:
+	} else if (channels > 0 && channels < 17) {
+		regs->rcr1	|= RFRLEN1(wpf - 1);
+		regs->xcr1	|= XFRLEN1(wpf - 1);
+	} else
 		/* Unsupported number of channels */
 		return -EINVAL;
-	}
 
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
@@ -330,6 +328,34 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
+	/* Default div to 1 if it wasn't set by machine driver, otherwise
+	 * use set div as the maximum clock value
+	 */
+	div = mcbsp_data->clk_div ? mcbsp_data->clk_div : 1;
+
+	/* calc best frame size for rate and clock divider */
+	do {
+		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);
+		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
+				mcbsp_data->in_freq, params_rate(params), frame_size, div);
+
+		if (frame_size > 256)
+			div++;
+	} while (frame_size > 256);
+
+	/* Check we can fit the requested number of channels into our
+	 * calculated frame size
+	 */
+	if ((channels * wlen) > frame_size) {
+		printk(KERN_ERR
+			"OMAP-MCBSP: cannot fit channels in frame size\n");
+		return -EINVAL;
+	}
+
+	/* Set the actual clkdiv to use for this samplerate */
+	regs->srgr1 &= ~CLKGDV(0xFF);
+	regs->srgr1 |= CLKGDV(div - 1);
+
 	/* Set FS period and length in terms of bit clock periods */
 	switch (format) {
 	case SND_SOC_DAIFMT_I2S:
@@ -338,7 +364,7 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 		break;
 	case SND_SOC_DAIFMT_DSP_A:
 	case SND_SOC_DAIFMT_DSP_B:
-		regs->srgr2	|= FPER(wlen * channels - 1);
+		regs->srgr2	|= FPER(frame_size - 1);
 		regs->srgr1	|= FWID(0);
 		break;
 	}
@@ -449,12 +475,11 @@ static int omap_mcbsp_dai_set_clkdiv(struct snd_soc_dai *cpu_dai,
 				     int div_id, int div)
 {
 	struct omap_mcbsp_data *mcbsp_data = to_mcbsp(cpu_dai->private_data);
-	struct omap_mcbsp_reg_cfg *regs = &mcbsp_data->regs;
 
 	if (div_id != OMAP_MCBSP_CLKGDV)
 		return -ENODEV;
 
-	regs->srgr1	|= CLKGDV(div - 1);
+	mcbsp_data->clk_div = div;
 
 	return 0;
 }
@@ -554,6 +579,8 @@ static int omap_mcbsp_dai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
 	struct omap_mcbsp_reg_cfg *regs = &mcbsp_data->regs;
 	int err = 0;
 
+	mcbsp_data->in_freq = freq;
+
 	switch (clk_id) {
 	case OMAP_MCBSP_SYSCLK_CLK:
 		regs->srgr2	|= CLKSM;
@@ -598,13 +625,13 @@ static struct snd_soc_dai_ops omap_mcbsp_dai_ops = {
 	.id = (link_id),					\
 	.playback = {						\
 		.channels_min = 1,				\
-		.channels_max = 4,				\
+		.channels_max = 16,				\
 		.rates = OMAP_MCBSP_RATES,			\
 		.formats = SNDRV_PCM_FMTBIT_S16_LE,		\
 	},							\
 	.capture = {						\
 		.channels_min = 1,				\
-		.channels_max = 4,				\
+		.channels_max = 16,				\
 		.rates = OMAP_MCBSP_RATES,			\
 		.formats = SNDRV_PCM_FMTBIT_S16_LE,		\
 	},							\
-- 
1.6.3.3

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Mark Brown]

On Wed, Nov 04, 2009 at 05:53:55PM +0000, Liam Girdwood wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> This patch increases the number of supported audio channels from 4
> to 16 and was sponsored by Shotspotter inc.

I'm OK with this from an ASoC point of view (the provision of a default
for existing machine drivers is good BTW), also adding Jarkko as well as
Peter for the OMAP-specific review.

> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk>
> ---
>  sound/soc/omap/omap-mcbsp.c |   71 +++++++++++++++++++++++++++++-------------
>  1 files changed, 49 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> index 3341f49..290ef2f 100644
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -49,6 +49,8 @@ struct omap_mcbsp_data {
>  	 */
>  	int				active;
>  	int				configured;
> +	unsigned int			in_freq;
> +	int				clk_div;
>  };
>  
>  #define to_mcbsp(priv)	container_of((priv), struct omap_mcbsp_data, bus_id)
> @@ -257,7 +259,7 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
>  	int dma, bus_id = mcbsp_data->bus_id, id = cpu_dai->id;
>  	int wlen, channels, wpf, sync_mode = OMAP_DMA_SYNC_ELEMENT;
>  	unsigned long port;
> -	unsigned int format;
> +	unsigned int format, frame_size, div;
>  
>  	if (cpu_class_is_omap1()) {
>  		dma = omap1_dma_reqs[bus_id][substream->stream];
> @@ -294,27 +296,23 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
>  
>  	format = mcbsp_data->fmt & SND_SOC_DAIFMT_FORMAT_MASK;
>  	wpf = channels = params_channels(params);
> -	switch (channels) {
> -	case 2:
> -		if (format == SND_SOC_DAIFMT_I2S) {
> -			/* Use dual-phase frames */
> -			regs->rcr2	|= RPHASE;
> -			regs->xcr2	|= XPHASE;
> -			/* Set 1 word per (McBSP) frame for phase1 and phase2 */
> -			wpf--;
> -			regs->rcr2	|= RFRLEN2(wpf - 1);
> -			regs->xcr2	|= XFRLEN2(wpf - 1);
> -		}
> -	case 1:
> -	case 4:
> +	if (channels == 2 && format == SND_SOC_DAIFMT_I2S) {
> +		/* Use dual-phase frames */
> +		regs->rcr2	|= RPHASE;
> +		regs->xcr2	|= XPHASE;
> +		/* Set 1 word per (McBSP) frame for phase1 and phase2 */
> +		wpf--;
> +		regs->rcr2	|= RFRLEN2(wpf - 1);
> +		regs->xcr2	|= XFRLEN2(wpf - 1);
>  		/* Set word per (McBSP) frame for phase1 */
>  		regs->rcr1	|= RFRLEN1(wpf - 1);
>  		regs->xcr1	|= XFRLEN1(wpf - 1);
> -		break;
> -	default:
> +	} else if (channels > 0 && channels < 17) {
> +		regs->rcr1	|= RFRLEN1(wpf - 1);
> +		regs->xcr1	|= XFRLEN1(wpf - 1);
> +	} else
>  		/* Unsupported number of channels */
>  		return -EINVAL;
> -	}
>  
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
> @@ -330,6 +328,34 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
>  		return -EINVAL;
>  	}
>  
> +	/* Default div to 1 if it wasn't set by machine driver, otherwise
> +	 * use set div as the maximum clock value
> +	 */
> +	div = mcbsp_data->clk_div ? mcbsp_data->clk_div : 1;
> +
> +	/* calc best frame size for rate and clock divider */
> +	do {
> +		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);
> +		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
> +				mcbsp_data->in_freq, params_rate(params), frame_size, div);
> +
> +		if (frame_size > 256)
> +			div++;
> +	} while (frame_size > 256);
> +
> +	/* Check we can fit the requested number of channels into our
> +	 * calculated frame size
> +	 */
> +	if ((channels * wlen) > frame_size) {
> +		printk(KERN_ERR
> +			"OMAP-MCBSP: cannot fit channels in frame size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set the actual clkdiv to use for this samplerate */
> +	regs->srgr1 &= ~CLKGDV(0xFF);
> +	regs->srgr1 |= CLKGDV(div - 1);
> +
>  	/* Set FS period and length in terms of bit clock periods */
>  	switch (format) {
>  	case SND_SOC_DAIFMT_I2S:
> @@ -338,7 +364,7 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
>  		break;
>  	case SND_SOC_DAIFMT_DSP_A:
>  	case SND_SOC_DAIFMT_DSP_B:
> -		regs->srgr2	|= FPER(wlen * channels - 1);
> +		regs->srgr2	|= FPER(frame_size - 1);
>  		regs->srgr1	|= FWID(0);
>  		break;
>  	}
> @@ -449,12 +475,11 @@ static int omap_mcbsp_dai_set_clkdiv(struct snd_soc_dai *cpu_dai,
>  				     int div_id, int div)
>  {
>  	struct omap_mcbsp_data *mcbsp_data = to_mcbsp(cpu_dai->private_data);
> -	struct omap_mcbsp_reg_cfg *regs = &mcbsp_data->regs;
>  
>  	if (div_id != OMAP_MCBSP_CLKGDV)
>  		return -ENODEV;
>  
> -	regs->srgr1	|= CLKGDV(div - 1);
> +	mcbsp_data->clk_div = div;
>  
>  	return 0;
>  }
> @@ -554,6 +579,8 @@ static int omap_mcbsp_dai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
>  	struct omap_mcbsp_reg_cfg *regs = &mcbsp_data->regs;
>  	int err = 0;
>  
> +	mcbsp_data->in_freq = freq;
> +
>  	switch (clk_id) {
>  	case OMAP_MCBSP_SYSCLK_CLK:
>  		regs->srgr2	|= CLKSM;
> @@ -598,13 +625,13 @@ static struct snd_soc_dai_ops omap_mcbsp_dai_ops = {
>  	.id = (link_id),					\
>  	.playback = {						\
>  		.channels_min = 1,				\
> -		.channels_max = 4,				\
> +		.channels_max = 16,				\
>  		.rates = OMAP_MCBSP_RATES,			\
>  		.formats = SNDRV_PCM_FMTBIT_S16_LE,		\
>  	},							\
>  	.capture = {						\
>  		.channels_min = 1,				\
> -		.channels_max = 4,				\
> +		.channels_max = 16,				\
>  		.rates = OMAP_MCBSP_RATES,			\
>  		.formats = SNDRV_PCM_FMTBIT_S16_LE,		\
>  	},							\
> -- 
> 1.6.3.3
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

-- 
"You grabbed my hand and we fell into it, like a daydream - or a fever."

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Graeme Gregory]

On Wed, Nov 04, 2009 at 05:53:55PM +0000, Liam Girdwood wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> This patch increases the number of supported audio channels from 4
> to 16 and was sponsored by Shotspotter inc.
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk>
> +	/* Default div to 1 if it wasn't set by machine driver, otherwise
> +	 * use set div as the maximum clock value
> +	 */
> +	div = mcbsp_data->clk_div ? mcbsp_data->clk_div : 1;
> +
> +	/* calc best frame size for rate and clock divider */
> +	do {
> +		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);
> +		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
> +				mcbsp_data->in_freq, params_rate(params), frame_size, div);
> +
> +		if (frame_size > 256)
> +			div++;
> +	} while (frame_size > 256);
> +
> +	/* Check we can fit the requested number of channels into our
> +	 * calculated frame size
> +	 */
> +	if ((channels * wlen) > frame_size) {
> +		printk(KERN_ERR
> +			"OMAP-MCBSP: cannot fit channels in frame size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set the actual clkdiv to use for this samplerate */
> +	regs->srgr1 &= ~CLKGDV(0xFF);
> +	regs->srgr1 |= CLKGDV(div - 1);
> +

This chunk changes the semantics of other devices which I have never tested.

I also dont know how much damage it does if it does to slave mode. In fact
I think it might break it in cases which are actually allowable as it uses
the omap as its clock constraint and not the clock source.

Graeme

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Liam Girdwood]

On Wed, 2009-11-04 at 18:55 +0000, Graeme Gregory wrote:
> On Wed, Nov 04, 2009 at 05:53:55PM +0000, Liam Girdwood wrote:
> > From: Graeme Gregory <gg@slimlogic.co.uk>
> > 
> > This patch increases the number of supported audio channels from 4
> > to 16 and was sponsored by Shotspotter inc.
> > 
> > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> > Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk>
> > +	/* Default div to 1 if it wasn't set by machine driver, otherwise
> > +	 * use set div as the maximum clock value
> > +	 */
> > +	div = mcbsp_data->clk_div ? mcbsp_data->clk_div : 1;
> > +
> > +	/* calc best frame size for rate and clock divider */
> > +	do {
> > +		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);
> > +		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
> > +				mcbsp_data->in_freq, params_rate(params), frame_size, div);
> > +
> > +		if (frame_size > 256)
> > +			div++;
> > +	} while (frame_size > 256);
> > +
> > +	/* Check we can fit the requested number of channels into our
> > +	 * calculated frame size
> > +	 */
> > +	if ((channels * wlen) > frame_size) {
> > +		printk(KERN_ERR
> > +			"OMAP-MCBSP: cannot fit channels in frame size\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Set the actual clkdiv to use for this samplerate */
> > +	regs->srgr1 &= ~CLKGDV(0xFF);
> > +	regs->srgr1 |= CLKGDV(div - 1);
> > +
> 
> This chunk changes the semantics of other devices which I have never tested.
> 
> I also dont know how much damage it does if it does to slave mode. In fact
> I think it might break it in cases which are actually allowable as it uses
> the omap as its clock constraint and not the clock source.
> 

Btw, this has been reworked to avoid all I2S paths since you last worked
on it.

So it looks like we need a conditional around this block for when the
codec is mastering BCLK/FRM and MCBSP is slave (as you stated above).
This will then use the original SRGR2 values for SND_SOC_DAIFMT_DSP_A/B
in codec master mode.

V2 will follow.

Liam

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Jarkko Nikula]

On Wed, 04 Nov 2009 19:46:49 +0000
Liam Girdwood <lrg@slimlogic.co.uk> wrote:

> > > +	/* calc best frame size for rate and clock divider */
> > > +	do {
> > > +		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);
> > > +		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
> > > +				mcbsp_data->in_freq, params_rate(params), frame_size, div);
> > > +
> > > +		if (frame_size > 256)
> > > +			div++;
> > > +	} while (frame_size > 256);
> > > +

This would be better if it tries to calculate minimum frame size. Now
the algorithm stops when the frame_size is 256 and leads to higher
bit clock.

E.g. 4 * 16bits * 48 kHz and using 96 MHz internal clock:

Algorithm: div = 8, frame_size = 250 and bit clock = 12 MHz.

Possible dividers and frame_sizes:

25*80 (-> best, bit clock = 3.840 MHz)
20*100
16*125
10*200
8*250

> > This chunk changes the semantics of other devices which I have never tested.
> > 
> > I also dont know how much damage it does if it does to slave mode. In fact
> > I think it might break it in cases which are actually allowable as it uses
> > the omap as its clock constraint and not the clock source.
> > 
> 
IRCC, the CLKGDV doesn't have effect while OMAP is slave but I can test
that with Beagle.

> Btw, this has been reworked to avoid all I2S paths since you last worked
> on it.
> 
Probably that is not necessary if the algorithm above finds a frame
size (½ for dual-phase frames) which doesn't exceed I2S word size?
Uniform code is allways better :-)


-- 
Jarkko
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Liam Girdwood]

On Thu, 2009-11-05 at 09:51 +0200, Jarkko Nikula wrote:
> On Wed, 04 Nov 2009 19:46:49 +0000
> Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> 
> > > > +	/* calc best frame size for rate and clock divider */
> > > > +	do {
> > > > +		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);
> > > > +		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
> > > > +				mcbsp_data->in_freq, params_rate(params), frame_size, div);
> > > > +
> > > > +		if (frame_size > 256)
> > > > +			div++;
> > > > +	} while (frame_size > 256);
> > > > +
> 
> This would be better if it tries to calculate minimum frame size. Now
> the algorithm stops when the frame_size is 256 and leads to higher
> bit clock.
> 
> E.g. 4 * 16bits * 48 kHz and using 96 MHz internal clock:
> 
> Algorithm: div = 8, frame_size = 250 and bit clock = 12 MHz.
> 
> Possible dividers and frame_sizes:
> 
> 25*80 (-> best, bit clock = 3.840 MHz)
> 20*100
> 16*125
> 10*200
> 8*250
> 

I've reworked this myself now. It does appear that the current FPER
calculations assume BCLK scales with rate 

i.e. BCLK = rate * channels * word len

This is fine for when McBSP is FRM/BCLK slave (all users except pandora)
as FPER should be ignored internally.

However, when BCLK is constant (e.g. McBSP BCLK derived from constant
source) and we run McBSP as FRM/BCLK master we currently break our
sample rate generation.

Imho, it's better to generate FPER based upon BCLK and rate. e.g. we
calculate the frame size required for the given BCLK and rate.

/* In McBSP master modes, FRAME (i.e. sample rate) is generated
 * by _counting_ BCLKs. Calculate frame size in BCLKs */
div = mcbsp_data->clk_div ? mcbsp_data->clk_div : 1;
framesize = (mcbsp_data->in_freq / div) / params_rate(params);

if (framesize < wlen * channels) {
	printk(KERN_ERR "%s: not enough bandwidth for desired rate and channels\n",
			__func__);
	return -EINVAL;
}

/* Set FS period and length in terms of bit clock periods */
switch (format) {
case SND_SOC_DAIFMT_I2S:
	regs->srgr2	|= FPER(framesize - 1);
	regs->srgr1	|= FWID((framesize >> 1) - 1);
	break;
case SND_SOC_DAIFMT_DSP_A:
case SND_SOC_DAIFMT_DSP_B:
	regs->srgr2	|= FPER(framesize - 1);
	regs->srgr1	|= FWID(0);
	break;
}


I'm now slightly curious about how pandora handles different rates since
it uses the McBSP in master mode too. I guess they can only handle a
single sample rate ?

Liam

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Jarkko Nikula]

On Thu, 05 Nov 2009 14:55:30 +0000
Liam Girdwood <lrg@slimlogic.co.uk> wrote:

> I've reworked this myself now. It does appear that the current FPER
> calculations assume BCLK scales with rate 
> 
> i.e. BCLK = rate * channels * word len
> 
> This is fine for when McBSP is FRM/BCLK slave (all users except pandora)
> as FPER should be ignored internally.
> 
> However, when BCLK is constant (e.g. McBSP BCLK derived from constant
> source) and we run McBSP as FRM/BCLK master we currently break our
> sample rate generation.
> 
This is true, currently possibilities to get standard sample rates as
accurately as possible when using McBSP as a master and when using
fixed sample rate generator input clock (CLKSRG) are too much limited.
So the period size adjustment is a good improvement.

CLKSRG can take its input from internal fixed 96 MHz, CLKS pin, CLKR
pin, CLKX pin or internal interface clock (proprotional to CPU freq).

> Imho, it's better to generate FPER based upon BCLK and rate. e.g. we
> calculate the frame size required for the given BCLK and rate.
> 
Yep.

Actually BCLK doesn't have to be fixed as it is derived by dividing the
CLKSRG with CLKGDV. This can allow to optimize the BCLK and frame size
to be smaller when CLKSRG >> BCLK.

Would be nice if both the divider and frame size are calculated
dynamically based on CLKSRG frequency and sample rate.

> I'm now slightly curious about how pandora handles different rates since
> it uses the McBSP in master mode too. I guess they can only handle a
> single sample rate ?
> 
IRCC correctly the external master clock used in Pandora was derived
from the codec and that clock was following the sample rate.


-- 
Jarkko

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Liam Girdwood]

On Thu, 2009-11-05 at 21:28 +0200, Jarkko Nikula wrote:

> 
> Actually BCLK doesn't have to be fixed as it is derived by dividing the
> CLKSRG with CLKGDV. This can allow to optimize the BCLK and frame size
> to be smaller when CLKSRG >> BCLK.
> 
> Would be nice if both the divider and frame size are calculated
> dynamically based on CLKSRG frequency and sample rate.
> 

This does sound like a useful feature and should probably exist in
soc-core for other platforms too.

> > I'm now slightly curious about how pandora handles different rates since
> > it uses the McBSP in master mode too. I guess they can only handle a
> > single sample rate ?
> > 
> IRCC correctly the external master clock used in Pandora was derived
> from the codec and that clock was following the sample rate.
> 

Makes sense now :)

Liam

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Mark Brown]

On Thu, Nov 05, 2009 at 08:08:30PM +0000, Liam Girdwood wrote:
> On Thu, 2009-11-05 at 21:28 +0200, Jarkko Nikula wrote:

> > Would be nice if both the divider and frame size are calculated
> > dynamically based on CLKSRG frequency and sample rate.

> This does sound like a useful feature and should probably exist in
> soc-core for other platforms too.

A lot of drivers would probably have trouble using this due to the
number of different constraints in the dividers that can be set up that
random hardware has - it'd probably take more effort to finesse things
than it's worth - but a utility function for mostly unconstrained
hardware would be good.

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Liam Girdwood]

On Fri, 2009-11-06 at 13:20 +0000, Mark Brown wrote:
> On Thu, Nov 05, 2009 at 08:08:30PM +0000, Liam Girdwood wrote:
> > On Thu, 2009-11-05 at 21:28 +0200, Jarkko Nikula wrote:
> 
> > > Would be nice if both the divider and frame size are calculated
> > > dynamically based on CLKSRG frequency and sample rate.
> 
> > This does sound like a useful feature and should probably exist in
> > soc-core for other platforms too.
> 
> A lot of drivers would probably have trouble using this due to the
> number of different constraints in the dividers that can be set up that
> random hardware has - it'd probably take more effort to finesse things
> than it's worth - but a utility function for mostly unconstrained
> hardware would be good.

I was thinking more generic here. e.g. additions to soc.h :-

static inline int snd_soc_get_framesize(int clock, struct snd_pcm_hw_params *params);

A caller could vary clock based upon it's divider options. Error would
be returned if there was not enough bandwidth.

and

static inline int snd_soc_get_divider(int clock, struct snd_pcm_hw_params *params);

This would return the integer divider required or bandwidth error.

and

static inline int snd_soc_get_min_clock(struct snd_pcm_hw_params *params);

return min bit clock required for params.

Liam

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Mark Brown]

On Fri, Nov 06, 2009 at 02:25:20PM +0000, Liam Girdwood wrote:

> static inline int snd_soc_get_framesize(int clock, struct snd_pcm_hw_params *params);

That's roughly what I'm thinking of too - it doesn't even need to take
the clock in since the frame size is unrelated to the clock in (and the
calculation is also useful for devices which have FLLs they can use to
vary the system clock to meet their bit clock requirements).  We'd also
want a versions that takes the TDM parameters in for use when
configuring that.

I'll pull the relevant code out of some of my drivers sometime over the
next few days.

Most of the devices I've seen that couldn't use this stuff at all
actually need to maintain some sample rate ratio for their master clock
and don't need to worry about BCLK since the fs restriction usually
takes care of it.

> static inline int snd_soc_get_divider(int clock, struct snd_pcm_hw_params *params);

This one I'm less sure about, partly due to naming but also because it
tends to be where you end up with restrictions that'd need handling in
drivers - I'm not sure that you'd save anything that wasn't already
saved with the above two functions.

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Peter Ujfalusi]

On Wednesday 04 November 2009 20:55:34 ext Graeme Gregory wrote:
> 
> This chunk changes the semantics of other devices which I have never
>  tested.
> 
> I also dont know how much damage it does if it does to slave mode. In fact
> I think it might break it in cases which are actually allowable as it uses
> the omap as its clock constraint and not the clock source.

For the first look this is going to break the slave mode, when McBSP is slave 
the data->in_freq is going to be 0 (not configured).
In McBSP slave mode the sample rate generator (SRG) is not is use.

> 
> Graeme

-- 
Péter

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Peter Ujfalusi]

On Wednesday 04 November 2009 19:53:55 ext Liam Girdwood wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> This patch increases the number of supported audio channels from 4
> to 16 and was sponsored by Shotspotter inc.
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk>
> ---
>  sound/soc/omap/omap-mcbsp.c |   71
>  +++++++++++++++++++++++++++++------------- 1 files changed, 49
>  insertions(+), 22 deletions(-)
> 
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> index 3341f49..290ef2f 100644
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -49,6 +49,8 @@ struct omap_mcbsp_data {
>  	 */
>  	int				active;
>  	int				configured;
> +	unsigned int			in_freq;
> +	int				clk_div;
>  };
> 
>  #define to_mcbsp(priv)	container_of((priv), struct omap_mcbsp_data,
>  bus_id) @@ -257,7 +259,7 @@ static int omap_mcbsp_dai_hw_params(struct
>  snd_pcm_substream *substream, int dma, bus_id = mcbsp_data->bus_id, id =
>  cpu_dai->id;
>  	int wlen, channels, wpf, sync_mode = OMAP_DMA_SYNC_ELEMENT;
>  	unsigned long port;
> -	unsigned int format;
> +	unsigned int format, frame_size, div;
> 
>  	if (cpu_class_is_omap1()) {
>  		dma = omap1_dma_reqs[bus_id][substream->stream];
> @@ -294,27 +296,23 @@ static int omap_mcbsp_dai_hw_params(struct
>  snd_pcm_substream *substream,
> 
>  	format = mcbsp_data->fmt & SND_SOC_DAIFMT_FORMAT_MASK;
>  	wpf = channels = params_channels(params);
> -	switch (channels) {
> -	case 2:
> -		if (format == SND_SOC_DAIFMT_I2S) {
> -			/* Use dual-phase frames */
> -			regs->rcr2	|= RPHASE;
> -			regs->xcr2	|= XPHASE;
> -			/* Set 1 word per (McBSP) frame for phase1 and phase2 */
> -			wpf--;
> -			regs->rcr2	|= RFRLEN2(wpf - 1);
> -			regs->xcr2	|= XFRLEN2(wpf - 1);
> -		}
> -	case 1:
> -	case 4:
> +	if (channels == 2 && format == SND_SOC_DAIFMT_I2S) {
> +		/* Use dual-phase frames */
> +		regs->rcr2	|= RPHASE;
> +		regs->xcr2	|= XPHASE;
> +		/* Set 1 word per (McBSP) frame for phase1 and phase2 */
> +		wpf--;
> +		regs->rcr2	|= RFRLEN2(wpf - 1);
> +		regs->xcr2	|= XFRLEN2(wpf - 1);
>  		/* Set word per (McBSP) frame for phase1 */
>  		regs->rcr1	|= RFRLEN1(wpf - 1);
>  		regs->xcr1	|= XFRLEN1(wpf - 1);
> -		break;
> -	default:
> +	} else if (channels > 0 && channels < 17) {
> +		regs->rcr1	|= RFRLEN1(wpf - 1);
> +		regs->xcr1	|= XFRLEN1(wpf - 1);
> +	} else
>  		/* Unsupported number of channels */
>  		return -EINVAL;
> -	}

I would have done this a bit differently:

...
	/* Check if the number of channels are valid */
	if (channels < 1 || channels > 16) {
		/* Unsupported number of channels */
		/* Probably would be good to have some error message about it? */
		return -EINVAL;
	}

	format = mcbsp_data->fmt & SND_SOC_DAIFMT_FORMAT_MASK;
	wpf = channels = params_channels(params);

	if (channels == 2 && format == SND_SOC_DAIFMT_I2S) {
		/* Use dual-phase frames */
		regs->rcr2      |= RPHASE;
		regs->xcr2      |= XPHASE;
		/* Set 1 word per (McBSP) frame for phase1 and phase2 */
		wpf--;
		regs->rcr2      |= RFRLEN2(wpf - 1);
		regs->xcr2      |= XFRLEN2(wpf - 1);
	}
	/* Set word per (McBSP) frame for phase1 */
	regs->rcr1      |= RFRLEN1(wpf - 1);
	regs->xcr1      |= XFRLEN1(wpf - 1);

> 
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
> @@ -330,6 +328,34 @@ static int omap_mcbsp_dai_hw_params(struct
>  snd_pcm_substream *substream, return -EINVAL;
>  	}
> 
> +	/* Default div to 1 if it wasn't set by machine driver, otherwise
> +	 * use set div as the maximum clock value
> +	 */
> +	div = mcbsp_data->clk_div ? mcbsp_data->clk_div : 1;
> +
> +	/* calc best frame size for rate and clock divider */
> +	do {
> +		frame_size = (mcbsp_data->in_freq / div) / params_rate(params);

I think this as it is now will not work when McBSP is a slave.
mcbsp_data->in_freq is going to be 0, since the sample rate generator is not in 
use.


> +		pr_debug("freq %d, rate %d, frame size %d, div %d\n",
> +				mcbsp_data->in_freq, params_rate(params), frame_size, div);
> +
> +		if (frame_size > 256)
> +			div++;
> +	} while (frame_size > 256);
> +
> +	/* Check we can fit the requested number of channels into our
> +	 * calculated frame size
> +	 */
> +	if ((channels * wlen) > frame_size) {
> +		printk(KERN_ERR
> +			"OMAP-MCBSP: cannot fit channels in frame size\n");
> +		return -EINVAL;
> +	}

-- 
Péter

Re: [PATCH] ASoC - Add support for upto 16 channels on OMAP MCBSP

[Liam Girdwood]

On Thu, 2009-11-05 at 10:26 +0200, Peter Ujfalusi wrote:
> On Wednesday 04 November 2009 19:53:55 ext Liam Girdwood wrote:
> > From: Graeme Gregory <gg@slimlogic.co.uk>
> > 
> > This patch increases the number of supported audio channels from 4
> > to 16 and was sponsored by Shotspotter inc.
> > 
> > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> > Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk>
> > ---
> >  sound/soc/omap/omap-mcbsp.c |   71
> >  +++++++++++++++++++++++++++++------------- 1 files changed, 49
> >  insertions(+), 22 deletions(-)
> > 

snip

> 
> I would have done this a bit differently:
> 
> ...
> 	/* Check if the number of channels are valid */
> 	if (channels < 1 || channels > 16) {
> 		/* Unsupported number of channels */
> 		/* Probably would be good to have some error message about it? */
> 		return -EINVAL;
> 	}
> 

Not really required as soc-core takes care of this for us :)


> 	format = mcbsp_data->fmt & SND_SOC_DAIFMT_FORMAT_MASK;
> 	wpf = channels = params_channels(params);
> 
> 	if (channels == 2 && format == SND_SOC_DAIFMT_I2S) {
> 		/* Use dual-phase frames */
> 		regs->rcr2      |= RPHASE;
> 		regs->xcr2      |= XPHASE;
> 		/* Set 1 word per (McBSP) frame for phase1 and phase2 */
> 		wpf--;
> 		regs->rcr2      |= RFRLEN2(wpf - 1);
> 		regs->xcr2      |= XFRLEN2(wpf - 1);
> 	}
> 	/* Set word per (McBSP) frame for phase1 */
> 	regs->rcr1      |= RFRLEN1(wpf - 1);
> 	regs->xcr1      |= XFRLEN1(wpf - 1);
> 

I prefer this too.

Liam