[RFC PATCH 0/5] OMAP/ASoC: McBSP: FIFO handling related fixes

[RFC PATCH 0/5] OMAP/ASoC: McBSP: FIFO handling related fixes

[Peter Ujfalusi]

Hello,

This series aims to correct how the McBSP FIFO is viewed, and handled.

Introduction of the problem:
OMAP McBSP FIFO is word structured:
McBSP2 has 1024 + 256 = 1280 word long buffer,
McBSP1,3,4,5 has 128 word long buffer

This means, that the size of the FIFO
depends on the McBSP word size configuration.
For example on McBSP3:
16bit samples: size is 128 * 2 = 256 bytes
32bit samples: size is 128 * 4 = 512 bytes
It is simpler to place constraint for buffer and period based on channels.
McBSP3 as example again (16 or 32 bit samples):
1 channel (mono): size is 128 frames (128 words)
2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
4 channels: size is 128 / 4 = 32 frames (4 * 32 words)

Since now the McBSP codec supports not only 16bit samples (32biut has been
recently added), the FIFO size handling is no longer correct, since it has
been hard wired for 16bit word length.

The series changes how the users of McBSP are configuring the FIFO:
It used to be 0 based (0 meant 1 word threshold). After this series users can
configure the threshold in 1 base mode (1 means 1 word threshold).
The platform code now provides the _full_ size of the FIFO in words, instead of
the already limited value used in the past.

In ASoC omap-mcbsp code hw_rule based constraint refinement is going to be used
instead of the hardwired static constraint, which was correct only in case of
16bit word length.

The hw_rule is refining the minimum buffer size based on the channel number
going to be used by the coming stream.
In case of threshold mode additional hw_rule refines the maximum allowed period
size.

The series are generated agains Takashi's sound-2.6: topic/asoc branch.

CCing also Eduardo, and Eero since they have worked on the original
FIFO/threshold implementation.

All commetns and testers are welcome!
Peter

---
Peter Ujfalusi (5):
  OMAP: McBSP: Function to query the FIFO size
  OMAP3: McBSP: Change the way how the FIFO is handled
  OMAP3: McBSP: Use the port's buffer_size when calculating tx delay
  ASoC: omap-mcbsp: Save, and use wlen for threshold configuration
  ASoC: omap-mcbsp: Place correct constraints for streams

 arch/arm/mach-omap2/mcbsp.c             |   10 ++--
 arch/arm/plat-omap/include/plat/mcbsp.h |    2 +
 arch/arm/plat-omap/mcbsp.c              |   31 ++++++---
 sound/soc/omap/omap-mcbsp.c             |  112 +++++++++++++++++++++++-------
 4 files changed, 114 insertions(+), 41 deletions(-)

[RFC PATCH 1/5] OMAP: McBSP: Function to query the FIFO size

[Peter Ujfalusi]

Users of McBSP can use the omap_mcbsp_get_fifo_size function to query
the size of the McBSP FIFO.
The function will return the FIFO size in words (the HW maximum).

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 arch/arm/plat-omap/include/plat/mcbsp.h |    2 ++
 arch/arm/plat-omap/mcbsp.c              |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mcbsp.h b/arch/arm/plat-omap/include/plat/mcbsp.h
index 1bd7021..e126951 100644
--- a/arch/arm/plat-omap/include/plat/mcbsp.h
+++ b/arch/arm/plat-omap/include/plat/mcbsp.h
@@ -473,6 +473,7 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold);
 void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold);
 u16 omap_mcbsp_get_max_tx_threshold(unsigned int id);
 u16 omap_mcbsp_get_max_rx_threshold(unsigned int id);
+u16 omap_mcbsp_get_fifo_size(unsigned int id);
 u16 omap_mcbsp_get_tx_delay(unsigned int id);
 u16 omap_mcbsp_get_rx_delay(unsigned int id);
 int omap_mcbsp_get_dma_op_mode(unsigned int id);
@@ -483,6 +484,7 @@ static inline void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold)
 { }
 static inline u16 omap_mcbsp_get_max_tx_threshold(unsigned int id) { return 0; }
 static inline u16 omap_mcbsp_get_max_rx_threshold(unsigned int id) { return 0; }
+static inline u16 omap_mcbsp_get_fifo_size(unsigned int id) { return 0; }
 static inline u16 omap_mcbsp_get_tx_delay(unsigned int id) { return 0; }
 static inline u16 omap_mcbsp_get_rx_delay(unsigned int id) { return 0; }
 static inline int omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; }
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 4820cab..51d8abf 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -559,6 +559,20 @@ u16 omap_mcbsp_get_max_rx_threshold(unsigned int id)
 }
 EXPORT_SYMBOL(omap_mcbsp_get_max_rx_threshold);
 
+u16 omap_mcbsp_get_fifo_size(unsigned int id)
+{
+	struct omap_mcbsp *mcbsp;
+
+	if (!omap_mcbsp_check_valid_id(id)) {
+		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
+		return -ENODEV;
+	}
+	mcbsp = id_to_mcbsp_ptr(id);
+
+	return mcbsp->pdata->buffer_size;
+}
+EXPORT_SYMBOL(omap_mcbsp_get_fifo_size);
+
 #define MCBSP2_FIFO_SIZE	0x500 /* 1024 + 256 locations */
 #define MCBSP1345_FIFO_SIZE	0x80  /* 128 locations */
 /*
-- 
1.7.1

[RFC PATCH 2/5] OMAP3: McBSP: Change the way how the FIFO is handled

[Peter Ujfalusi]

Use the actual FIFO size in words as buffer_size on OMAP2.
Change the threshold configuration to use 1 based numbering, when
specifying the allowed threshold maximum or the McBSP threshold value.
Set the default maximum threshold to (buffer_size - 0x10) intialy.
>From users of McBSP, now it is expected to use this method.
Asking for threshold 1 means that the value written to threshold registers
are going to be 0, which means 1 word threshold.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 arch/arm/mach-omap2/mcbsp.c |   10 +++++-----
 arch/arm/plat-omap/mcbsp.c  |   10 ++++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
index 016fe60..9139958 100644
--- a/arch/arm/mach-omap2/mcbsp.c
+++ b/arch/arm/mach-omap2/mcbsp.c
@@ -132,7 +132,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP1_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP1_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP2_BASE,
@@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP2_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP2_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x3FF,
+		.buffer_size	= 0x500,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP3_BASE,
@@ -152,7 +152,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP3_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP3_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP4_BASE,
@@ -161,7 +161,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP4_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP4_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP5_BASE,
@@ -170,7 +170,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP5_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP5_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 };
 #define OMAP34XX_MCBSP_PDATA_SZ		ARRAY_SIZE(omap34xx_mcbsp_pdata)
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 51d8abf..6462968 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -497,7 +497,8 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold)
 	}
 	mcbsp = id_to_mcbsp_ptr(id);
 
-	MCBSP_WRITE(mcbsp, THRSH2, threshold);
+	if (threshold && threshold <= mcbsp->max_tx_thres)
+		MCBSP_WRITE(mcbsp, THRSH2, threshold - 1);
 }
 EXPORT_SYMBOL(omap_mcbsp_set_tx_threshold);
 
@@ -519,7 +520,8 @@ void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold)
 	}
 	mcbsp = id_to_mcbsp_ptr(id);
 
-	MCBSP_WRITE(mcbsp, THRSH1, threshold);
+	if (threshold && threshold <= mcbsp->max_rx_thres)
+		MCBSP_WRITE(mcbsp, THRSH1, threshold - 1);
 }
 EXPORT_SYMBOL(omap_mcbsp_set_rx_threshold);
 
@@ -1696,8 +1698,8 @@ static inline void __devinit omap34xx_device_init(struct omap_mcbsp *mcbsp)
 {
 	mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT;
 	if (cpu_is_omap34xx()) {
-		mcbsp->max_tx_thres = max_thres(mcbsp);
-		mcbsp->max_rx_thres = max_thres(mcbsp);
+		mcbsp->max_tx_thres = max_thres(mcbsp) - 0x10;
+		mcbsp->max_rx_thres = max_thres(mcbsp) - 0x10;
 		/*
 		 * REVISIT: Set dmap_op_mode to THRESHOLD as default
 		 * for mcbsp2 instances.
-- 
1.7.1

[RFC PATCH 3/5] OMAP3: McBSP: Use the port's buffer_size when calculating tx delay

[Peter Ujfalusi]

Sicne the platform data's buffer_size now holds the full size
of the FIFO, there is no longer need to handle the ports
differently.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 arch/arm/plat-omap/mcbsp.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 6462968..f8ffeee 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -575,8 +575,6 @@ u16 omap_mcbsp_get_fifo_size(unsigned int id)
 }
 EXPORT_SYMBOL(omap_mcbsp_get_fifo_size);
 
-#define MCBSP2_FIFO_SIZE	0x500 /* 1024 + 256 locations */
-#define MCBSP1345_FIFO_SIZE	0x80  /* 128 locations */
 /*
  * omap_mcbsp_get_tx_delay returns the number of used slots in the McBSP FIFO
  */
@@ -595,10 +593,7 @@ u16 omap_mcbsp_get_tx_delay(unsigned int id)
 	buffstat = MCBSP_READ(mcbsp, XBUFFSTAT);
 
 	/* Number of slots are different in McBSP ports */
-	if (mcbsp->id == 2)
-		return MCBSP2_FIFO_SIZE - buffstat;
-	else
-		return MCBSP1345_FIFO_SIZE - buffstat;
+	return mcbsp->pdata->buffer_size - buffstat;
 }
 EXPORT_SYMBOL(omap_mcbsp_get_tx_delay);
 
-- 
1.7.1

[RFC PATCH 4/5] ASoC: omap-mcbsp: Save, and use wlen for threshold configuration

[Peter Ujfalusi]

Save the word length configuration of McBSP, and use that information
to calculate, and configure the threshold in McBSP.
Previously the calculation was only correct when the stream had 16bit
audio.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/omap/omap-mcbsp.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 6f44cb4..b06d8f1 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -59,6 +59,7 @@ struct omap_mcbsp_data {
 	int				configured;
 	unsigned int			in_freq;
 	int				clk_div;
+	int				wlen;
 };
 
 #define to_mcbsp(priv)	container_of((priv), struct omap_mcbsp_data, bus_id)
@@ -155,19 +156,21 @@ static void omap_mcbsp_set_threshold(struct snd_pcm_substream *substream)
 	struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
 	struct omap_mcbsp_data *mcbsp_data = to_mcbsp(cpu_dai->private_data);
 	int dma_op_mode = omap_mcbsp_get_dma_op_mode(mcbsp_data->bus_id);
-	int samples;
+	int words;
 
 	/* TODO: Currently, MODE_ELEMENT == MODE_FRAME */
 	if (dma_op_mode == MCBSP_DMA_MODE_THRESHOLD)
-		samples = snd_pcm_lib_period_bytes(substream) >> 1;
+		/* The FIFO size depends on the McBSP word configuration */
+		words = snd_pcm_lib_period_bytes(substream) /
+							(mcbsp_data->wlen / 8);
 	else
-		samples = 1;
+		words = 1;
 
 	/* Configure McBSP internal buffer usage */
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		omap_mcbsp_set_tx_threshold(mcbsp_data->bus_id, samples - 1);
+		omap_mcbsp_set_tx_threshold(mcbsp_data->bus_id, words);
 	else
-		omap_mcbsp_set_rx_threshold(mcbsp_data->bus_id, samples - 1);
+		omap_mcbsp_set_rx_threshold(mcbsp_data->bus_id, words);
 }
 
 static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
@@ -409,6 +412,7 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	omap_mcbsp_config(bus_id, &mcbsp_data->regs);
+	mcbsp_data->wlen = wlen;
 	mcbsp_data->configured = 1;
 
 	return 0;
-- 
1.7.1

[RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

OMAP McBSP FIFO is word structured:
McBSP2 has 1024 + 256 = 1280 word long buffer,
McBSP1,3,4,5 has 128 word long buffer

This means, that the size of the FIFO
depends on the McBSP word size configuration.
For example on McBSP3:
16bit samples: size is 128 * 2 = 256 bytes
32bit samples: size is 128 * 4 = 512 bytes
It is simpler to place constraint for buffer and period based on channels.
McBSP3 as example again (16 or 32 bit samples):
1 channel (mono): size is 128 frames (128 words)
2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
4 channels: size is 128 / 4 = 32 frames (4 * 32 words)

Use the second method to place hw_rule on buffer size, and in threshold
mode to period size.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/omap/omap-mcbsp.c |   98 +++++++++++++++++++++++++++++++++---------
 1 files changed, 77 insertions(+), 21 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index b06d8f1..d6ed761 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -173,6 +173,52 @@ static void omap_mcbsp_set_threshold(struct snd_pcm_substream *substream)
 		omap_mcbsp_set_rx_threshold(mcbsp_data->bus_id, words);
 }
 
+static int hw_rule_bsize_by_channels(struct snd_pcm_hw_params *params,
+				    struct snd_pcm_hw_rule *rule)
+{
+	struct snd_interval *bs = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_BUFFER_SIZE);
+	struct snd_interval *c = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_CHANNELS);
+	struct omap_mcbsp_data *mcbsp_data = rule->private;
+	struct snd_interval frames;
+	int size;
+
+	snd_interval_any(&frames);
+	size = omap_mcbsp_get_fifo_size(mcbsp_data->bus_id);
+
+	frames.min = size / c->min;
+	frames.integer = 1;
+	return snd_interval_refine(bs, &frames);
+
+}
+
+static int hw_rule_psize_by_channels(struct snd_pcm_hw_params *params,
+				    struct snd_pcm_hw_rule *rule)
+{
+	struct snd_interval *ps = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_PERIOD_SIZE);
+	struct snd_interval *c = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_CHANNELS);
+	struct snd_pcm_substream *substream = rule->private;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
+	struct omap_mcbsp_data *mcbsp_data = to_mcbsp(cpu_dai->private_data);
+	struct snd_interval frames;
+	int size;
+
+	snd_interval_any(&frames);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		size = omap_mcbsp_get_max_tx_threshold(mcbsp_data->bus_id);
+	else
+		size = omap_mcbsp_get_max_rx_threshold(mcbsp_data->bus_id);
+
+	frames.min = 8;
+	frames.max = size / c->min;
+	frames.integer = 1;
+	return snd_interval_refine(ps, &frames);
+}
+
 static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 				  struct snd_soc_dai *dai)
 {
@@ -185,33 +231,43 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 	if (!cpu_dai->active)
 		err = omap_mcbsp_request(bus_id);
 
+	/*
+	 * OMAP McBSP FIFO is word structured.
+	 * McBSP2 has 1024 + 256 = 1280 word long buffer,
+	 * McBSP1,3,4,5 has 128 word long buffer
+	 * This means that the size of the FIFO depends on the sample format.
+	 * For example on McBSP3:
+	 * 16bit samples: size is 128 * 2 = 256 bytes
+	 * 32bit samples: size is 128 * 4 = 512 bytes
+	 * It is simpler to place constraint for buffer and period based on
+	 * channels.
+	 * McBSP3 as example again (16 or 32 bit samples):
+	 * 1 channel (mono): size is 128 frames (128 words)
+	 * 2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
+	 * 4 channels: size is 128 / 4 = 32 frames (4 * 32 words)
+	 */
+
+	/*
+	 * The first rule is for the buffer size, we should not allow smaller
+	 * buffer than the FIFO size to avoid underruns
+	 */
+	snd_pcm_hw_rule_add(substream->runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
+				    hw_rule_bsize_by_channels, mcbsp_data,
+				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE, -1);
+
 	if (cpu_is_omap343x()) {
 		int dma_op_mode = omap_mcbsp_get_dma_op_mode(bus_id);
-		int max_period;
 
 		/*
-		 * McBSP2 in OMAP3 has 1024 * 32-bit internal audio buffer.
-		 * Set constraint for minimum buffer size to the same than FIFO
-		 * size in order to avoid underruns in playback startup because
-		 * HW is keeping the DMA request active until FIFO is filled.
+		 * In case os threshold mode, the rule will ensure, that the
+		 * period size is not bigger than the maximum allowed threshold
+		 * value.
 		 */
-		if (bus_id == 1)
-			snd_pcm_hw_constraint_minmax(substream->runtime,
-					SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
-					4096, UINT_MAX);
-
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			max_period = omap_mcbsp_get_max_tx_threshold(bus_id);
-		else
-			max_period = omap_mcbsp_get_max_rx_threshold(bus_id);
-
-		max_period++;
-		max_period <<= 1;
-
 		if (dma_op_mode == MCBSP_DMA_MODE_THRESHOLD)
-			snd_pcm_hw_constraint_minmax(substream->runtime,
-						SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
-						32, max_period);
+			snd_pcm_hw_rule_add(substream->runtime, 0,
+					SNDRV_PCM_HW_PARAM_CHANNELS,
+					hw_rule_psize_by_channels, substream,
+					SNDRV_PCM_HW_PARAM_PERIOD_SIZE, -1);
 	}
 
 	return err;
-- 
1.7.1

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

On Monday 31 May 2010 11:16:50 Ujfalusi Peter (Nokia-D/Tampere) wrote:
...

> @@ -185,33 +231,43 @@ static int omap_mcbsp_dai_startup(struct
> snd_pcm_substream *substream, if (!cpu_dai->active)
>  		err = omap_mcbsp_request(bus_id);
> 
> +	/*
> +	 * OMAP McBSP FIFO is word structured.
> +	 * McBSP2 has 1024 + 256 = 1280 word long buffer,
> +	 * McBSP1,3,4,5 has 128 word long buffer
> +	 * This means that the size of the FIFO depends on the sample format.
> +	 * For example on McBSP3:
> +	 * 16bit samples: size is 128 * 2 = 256 bytes
> +	 * 32bit samples: size is 128 * 4 = 512 bytes
> +	 * It is simpler to place constraint for buffer and period based on
> +	 * channels.
> +	 * McBSP3 as example again (16 or 32 bit samples):
> +	 * 1 channel (mono): size is 128 frames (128 words)
> +	 * 2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
> +	 * 4 channels: size is 128 / 4 = 32 frames (4 * 32 words)
> +	 */
> +
> +	/*
> +	 * The first rule is for the buffer size, we should not allow smaller
> +	 * buffer than the FIFO size to avoid underruns
> +	 */
> +	snd_pcm_hw_rule_add(substream->runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> +				    hw_rule_bsize_by_channels, mcbsp_data,
> +				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE, -1);
> +

Obviously this hw_rule must be moved after checking the CPU type, since it is 
only valid for OMAP3 class of devices.
I'll fix this in the v2 series.
I have been testing this on OMAP3...

>  	if (cpu_is_omap343x()) {
>  		int dma_op_mode = omap_mcbsp_get_dma_op_mode(bus_id);
> -		int max_period;
> 
>  		/*
> -		 * McBSP2 in OMAP3 has 1024 * 32-bit internal audio buffer.
> -		 * Set constraint for minimum buffer size to the same than FIFO
> -		 * size in order to avoid underruns in playback startup because
> -		 * HW is keeping the DMA request active until FIFO is filled.
> +		 * In case os threshold mode, the rule will ensure, that the
> +		 * period size is not bigger than the maximum allowed threshold
> +		 * value.
>  		 */
> -		if (bus_id == 1)
> -			snd_pcm_hw_constraint_minmax(substream->runtime,
> -					SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
> -					4096, UINT_MAX);
> -
> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> -			max_period = omap_mcbsp_get_max_tx_threshold(bus_id);
> -		else
> -			max_period = omap_mcbsp_get_max_rx_threshold(bus_id);
> -
> -		max_period++;
> -		max_period <<= 1;
> -
>  		if (dma_op_mode == MCBSP_DMA_MODE_THRESHOLD)
> -			snd_pcm_hw_constraint_minmax(substream->runtime,
> -						SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> -						32, max_period);
> +			snd_pcm_hw_rule_add(substream->runtime, 0,
> +					SNDRV_PCM_HW_PARAM_CHANNELS,
> +					hw_rule_psize_by_channels, substream,
> +					SNDRV_PCM_HW_PARAM_PERIOD_SIZE, -1);
>  	}
> 
>  	return err;

-- 
Péter

Re: [alsa-devel] [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Liam Girdwood]

On Mon, 2010-05-31 at 11:16 +0300, Peter Ujfalusi wrote:
> OMAP McBSP FIFO is word structured:
> McBSP2 has 1024 + 256 = 1280 word long buffer,
> McBSP1,3,4,5 has 128 word long buffer
> 
> This means, that the size of the FIFO
> depends on the McBSP word size configuration.
> For example on McBSP3:
> 16bit samples: size is 128 * 2 = 256 bytes
> 32bit samples: size is 128 * 4 = 512 bytes
> It is simpler to place constraint for buffer and period based on channels.
> McBSP3 as example again (16 or 32 bit samples):
> 1 channel (mono): size is 128 frames (128 words)
> 2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
> 4 channels: size is 128 / 4 = 32 frames (4 * 32 words)
> 
> Use the second method to place hw_rule on buffer size, and in threshold
> mode to period size.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> ---
>  sound/soc/omap/omap-mcbsp.c |   98 +++++++++++++++++++++++++++++++++---------
>  1 files changed, 77 insertions(+), 21 deletions(-)
> 
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> index b06d8f1..d6ed761 100644
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -173,6 +173,52 @@ static void omap_mcbsp_set_threshold(struct snd_pcm_substream *substream)
>  		omap_mcbsp_set_rx_threshold(mcbsp_data->bus_id, words);
>  }
>  
> +static int hw_rule_bsize_by_channels(struct snd_pcm_hw_params *params,
> +				    struct snd_pcm_hw_rule *rule)
> +{
> +	struct snd_interval *bs = hw_param_interval(params,
> +					SNDRV_PCM_HW_PARAM_BUFFER_SIZE);
> +	struct snd_interval *c = hw_param_interval(params,
> +					SNDRV_PCM_HW_PARAM_CHANNELS);

Best to make these variable names more meaningful.

> +	struct omap_mcbsp_data *mcbsp_data = rule->private;
> +	struct snd_interval frames;
> +	int size;
> +
> +	snd_interval_any(&frames);
> +	size = omap_mcbsp_get_fifo_size(mcbsp_data->bus_id);
> +
> +	frames.min = size / c->min;
> +	frames.integer = 1;
> +	return snd_interval_refine(bs, &frames);
> +
> +}
> +
> +static int hw_rule_psize_by_channels(struct snd_pcm_hw_params *params,
> +				    struct snd_pcm_hw_rule *rule)
> +{
> +	struct snd_interval *ps = hw_param_interval(params,
> +					SNDRV_PCM_HW_PARAM_PERIOD_SIZE);
> +	struct snd_interval *c = hw_param_interval(params,
> +					SNDRV_PCM_HW_PARAM_CHANNELS);

ditto

> +	struct snd_pcm_substream *substream = rule->private;
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> +	struct omap_mcbsp_data *mcbsp_data = to_mcbsp(cpu_dai->private_data);
> +	struct snd_interval frames;
> +	int size;
> +
> +	snd_interval_any(&frames);
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		size = omap_mcbsp_get_max_tx_threshold(mcbsp_data->bus_id);
> +	else
> +		size = omap_mcbsp_get_max_rx_threshold(mcbsp_data->bus_id);
> +
> +	frames.min = 8;
> +	frames.max = size / c->min;
> +	frames.integer = 1;
> +	return snd_interval_refine(ps, &frames);
> +}

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

On Monday 31 May 2010 13:00:21 ext Liam Girdwood wrote:

...

> > +static int hw_rule_bsize_by_channels(struct snd_pcm_hw_params *params,
> > +				    struct snd_pcm_hw_rule *rule)
> > +{
> > +	struct snd_interval *bs = hw_param_interval(params,
> > +					SNDRV_PCM_HW_PARAM_BUFFER_SIZE);
> > +	struct snd_interval *c = hw_param_interval(params,
> > +					SNDRV_PCM_HW_PARAM_CHANNELS);
> 
> Best to make these variable names more meaningful.

Sure, I can change that. I have picked these, since all code, which adds hw_rule 
(and the writing an ALSA driver manual) are using variables like this.
In Here I mean:
bs == Buffer Size
c == Channels

> 
> > +	struct omap_mcbsp_data *mcbsp_data = rule->private;
> > +	struct snd_interval frames;
> > +	int size;
> > +
> > +	snd_interval_any(&frames);
> > +	size = omap_mcbsp_get_fifo_size(mcbsp_data->bus_id);
> > +
> > +	frames.min = size / c->min;
> > +	frames.integer = 1;
> > +	return snd_interval_refine(bs, &frames);
> > +
> > +}
> > +
> > +static int hw_rule_psize_by_channels(struct snd_pcm_hw_params *params,
> > +				    struct snd_pcm_hw_rule *rule)
> > +{
> > +	struct snd_interval *ps = hw_param_interval(params,
> > +					SNDRV_PCM_HW_PARAM_PERIOD_SIZE);
> > +	struct snd_interval *c = hw_param_interval(params,
> > +					SNDRV_PCM_HW_PARAM_CHANNELS);
> 
> ditto

Same here:
I mean:
ps == Period Size
c == Channels

> 
> Thanks
> 
> Liam

Never the less, I can change them, 

Thanks,
Péter

Re: [RFC PATCH 2/5] OMAP3: McBSP: Change the way how the FIFO is handled

[Nishanth Menon]

On 05/31/2010 11:16 AM, Peter Ujfalusi wrote:
> Use the actual FIFO size in words as buffer_size on OMAP2.
> Change the threshold configuration to use 1 based numbering, when
> specifying the allowed threshold maximum or the McBSP threshold value.
> Set the default maximum threshold to (buffer_size - 0x10) intialy.
>> From users of McBSP, now it is expected to use this method.
> Asking for threshold 1 means that the value written to threshold registers
> are going to be 0, which means 1 word threshold.

just a 2cent minor comment: maybe omap_mcbsp_platform_data needs 
structure documentation.. it might be difficult for folks to figure that 
out from commit ID itself..
>
> Signed-off-by: Peter Ujfalusi<peter.ujfalusi@nokia.com>
> ---
>   arch/arm/mach-omap2/mcbsp.c |   10 +++++-----
>   arch/arm/plat-omap/mcbsp.c  |   10 ++++++----
>   2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
> index 016fe60..9139958 100644
> --- a/arch/arm/mach-omap2/mcbsp.c
> +++ b/arch/arm/mach-omap2/mcbsp.c
> @@ -132,7 +132,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
>   		.rx_irq		= INT_24XX_MCBSP1_IRQ_RX,
>   		.tx_irq		= INT_24XX_MCBSP1_IRQ_TX,
>   		.ops		=&omap2_mcbsp_ops,
> -		.buffer_size	= 0x6F,
> +		.buffer_size	= 0x80,
>   	},
>   	{
>   		.phys_base	= OMAP34XX_MCBSP2_BASE,
> @@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
>   		.rx_irq		= INT_24XX_MCBSP2_IRQ_RX,
>   		.tx_irq		= INT_24XX_MCBSP2_IRQ_TX,
>   		.ops		=&omap2_mcbsp_ops,
> -		.buffer_size	= 0x3FF,
> +		.buffer_size	= 0x500,
>   	},
>   	{
>   		.phys_base	= OMAP34XX_MCBSP3_BASE,
> @@ -152,7 +152,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
>   		.rx_irq		= INT_24XX_MCBSP3_IRQ_RX,
>   		.tx_irq		= INT_24XX_MCBSP3_IRQ_TX,
>   		.ops		=&omap2_mcbsp_ops,
> -		.buffer_size	= 0x6F,
> +		.buffer_size	= 0x80,
>   	},
>   	{
>   		.phys_base	= OMAP34XX_MCBSP4_BASE,
> @@ -161,7 +161,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
>   		.rx_irq		= INT_24XX_MCBSP4_IRQ_RX,
>   		.tx_irq		= INT_24XX_MCBSP4_IRQ_TX,
>   		.ops		=&omap2_mcbsp_ops,
> -		.buffer_size	= 0x6F,
> +		.buffer_size	= 0x80,
>   	},
>   	{
>   		.phys_base	= OMAP34XX_MCBSP5_BASE,
> @@ -170,7 +170,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
>   		.rx_irq		= INT_24XX_MCBSP5_IRQ_RX,
>   		.tx_irq		= INT_24XX_MCBSP5_IRQ_TX,
>   		.ops		=&omap2_mcbsp_ops,
> -		.buffer_size	= 0x6F,
> +		.buffer_size	= 0x80,
>   	},
>   };
>   #define OMAP34XX_MCBSP_PDATA_SZ		ARRAY_SIZE(omap34xx_mcbsp_pdata)
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index 51d8abf..6462968 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -497,7 +497,8 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold)
>   	}
>   	mcbsp = id_to_mcbsp_ptr(id);
>
> -	MCBSP_WRITE(mcbsp, THRSH2, threshold);
> +	if (threshold&&  threshold<= mcbsp->max_tx_thres)
> +		MCBSP_WRITE(mcbsp, THRSH2, threshold - 1);
>   }
>   EXPORT_SYMBOL(omap_mcbsp_set_tx_threshold);
>
> @@ -519,7 +520,8 @@ void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold)
>   	}
>   	mcbsp = id_to_mcbsp_ptr(id);
>
> -	MCBSP_WRITE(mcbsp, THRSH1, threshold);
> +	if (threshold&&  threshold<= mcbsp->max_rx_thres)
> +		MCBSP_WRITE(mcbsp, THRSH1, threshold - 1);
>   }
>   EXPORT_SYMBOL(omap_mcbsp_set_rx_threshold);
>
> @@ -1696,8 +1698,8 @@ static inline void __devinit omap34xx_device_init(struct omap_mcbsp *mcbsp)
>   {
>   	mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT;
>   	if (cpu_is_omap34xx()) {
> -		mcbsp->max_tx_thres = max_thres(mcbsp);
> -		mcbsp->max_rx_thres = max_thres(mcbsp);
> +		mcbsp->max_tx_thres = max_thres(mcbsp) - 0x10;
> +		mcbsp->max_rx_thres = max_thres(mcbsp) - 0x10;
>   		/*
>   		 * REVISIT: Set dmap_op_mode to THRESHOLD as default
>   		 * for mcbsp2 instances.

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Jarkko Nikula]

On Mon, 31 May 2010 14:57:22 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> On Monday 31 May 2010 13:00:21 ext Liam Girdwood wrote:
> 
> ...
> 
> > > +static int hw_rule_bsize_by_channels(struct snd_pcm_hw_params *params,
> > > +				    struct snd_pcm_hw_rule *rule)
> > > +{
> > > +	struct snd_interval *bs = hw_param_interval(params,
> > > +					SNDRV_PCM_HW_PARAM_BUFFER_SIZE);
> > > +	struct snd_interval *c = hw_param_interval(params,
> > > +					SNDRV_PCM_HW_PARAM_CHANNELS);
> > 
> > Best to make these variable names more meaningful.
> 
> Sure, I can change that. I have picked these, since all code, which adds hw_rule 
> (and the writing an ALSA driver manual) are using variables like this.
> In Here I mean:
> bs == Buffer Size
> c == Channels
> 
Change also function names. They should start with omap_mcbsp_.

hw_rule_bsize_by_channels
hw_rule_psize_by_channels
->
omap_mcbsp_(calc_max_bufsize, rule_max_buf, or something)
omap_mcbsp_(calc_max_periodsize)


-- 
Jarkko

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

On Tuesday 01 June 2010 09:38:07 ext Jarkko Nikula wrote:
> On Mon, 31 May 2010 14:57:22 +0300
> 
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > On Monday 31 May 2010 13:00:21 ext Liam Girdwood wrote:
> > 
> > ...
> > 
> > > > +static int hw_rule_bsize_by_channels(struct snd_pcm_hw_params
> > > > *params, +				    struct snd_pcm_hw_rule *rule)
> > > > +{
> > > > +	struct snd_interval *bs = hw_param_interval(params,
> > > > +					SNDRV_PCM_HW_PARAM_BUFFER_SIZE);
> > > > +	struct snd_interval *c = hw_param_interval(params,
> > > > +					SNDRV_PCM_HW_PARAM_CHANNELS);
> > > 
> > > Best to make these variable names more meaningful.
> > 
> > Sure, I can change that. I have picked these, since all code, which adds
> > hw_rule (and the writing an ALSA driver manual) are using variables like
> > this. In Here I mean:
> > bs == Buffer Size
> > c == Channels
> 
> Change also function names. They should start with omap_mcbsp_.
> 
> hw_rule_bsize_by_channels
> hw_rule_psize_by_channels
> ->
> omap_mcbsp_(calc_max_bufsize, rule_max_buf, or something)
> omap_mcbsp_(calc_max_periodsize)

Oh yes, they should start with omap_mcbsp_
I'll change them too.

I like the following naming:
omap_mcbsp_hwrule_min_buffersize()
omap_mcbsp_hwrule_max_periodsize()

Also, I think there is no point to limit the lower period size in threshold mode 
to 32, so I will remove that as well I think.

I'll wait couple of hours for comments, and resend the series.

Thanks,
Péter

Re: [RFC PATCH 2/5] OMAP3: McBSP: Change the way how the FIFO is handled

[Peter Ujfalusi]

On Monday 31 May 2010 20:41:12 ext Nishanth Menon wrote:
> On 05/31/2010 11:16 AM, Peter Ujfalusi wrote:
> > Use the actual FIFO size in words as buffer_size on OMAP2.
> > Change the threshold configuration to use 1 based numbering, when
> > specifying the allowed threshold maximum or the McBSP threshold value.
> > Set the default maximum threshold to (buffer_size - 0x10) intialy.
> > 
> >> From users of McBSP, now it is expected to use this method.
> > 
> > Asking for threshold 1 means that the value written to threshold
> > registers are going to be 0, which means 1 word threshold.
> 
> just a 2cent minor comment: maybe omap_mcbsp_platform_data needs
> structure documentation.. it might be difficult for folks to figure that
> out from commit ID itself..

I can add comments in the mach-omap2/mcbsp.c like this:

> 
> > Signed-off-by: Peter Ujfalusi<peter.ujfalusi@nokia.com>
> > ---
> > 
> >   arch/arm/mach-omap2/mcbsp.c |   10 +++++-----
> >   arch/arm/plat-omap/mcbsp.c  |   10 ++++++----
> >   2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
> > index 016fe60..9139958 100644
> > --- a/arch/arm/mach-omap2/mcbsp.c
> > +++ b/arch/arm/mach-omap2/mcbsp.c
> > @@ -132,7 +132,7 @@ static struct omap_mcbsp_platform_data
> > omap34xx_mcbsp_pdata[] = {
> > 
> >   		.rx_irq		= INT_24XX_MCBSP1_IRQ_RX,
> >   		.tx_irq		= INT_24XX_MCBSP1_IRQ_TX,
> >   		.ops		=&omap2_mcbsp_ops,
> > 
> > -		.buffer_size	= 0x6F,
> > +		.buffer_size	= 0x80,

 +		.buffer_size	= 0x80, /* The FIFO has 128 locations */

> > 
> >   	},
> >   	{
> >   	
> >   		.phys_base	= OMAP34XX_MCBSP2_BASE,
> > 
> > @@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data
> > omap34xx_mcbsp_pdata[] = {
> > 
> >   		.rx_irq		= INT_24XX_MCBSP2_IRQ_RX,
> >   		.tx_irq		= INT_24XX_MCBSP2_IRQ_TX,
> >   		.ops		=&omap2_mcbsp_ops,
> > 
> > -		.buffer_size	= 0x3FF,
> > +		.buffer_size	= 0x500,

 +		.buffer_size	= 0x500, /* The FIFO has 1024 + 256 locations */

> > 
> >   	},
> >   	{
> >   	
> >   		.phys_base	= OMAP34XX_MCBSP3_BASE,
> > 
> > @@ -152,7 +152,7 @@ static struct omap_mcbsp_platform_data
> > omap34xx_mcbsp_pdata[] = {
> > 
> >   		.rx_irq		= INT_24XX_MCBSP3_IRQ_RX,
> >   		.tx_irq		= INT_24XX_MCBSP3_IRQ_TX,
> >   		.ops		=&omap2_mcbsp_ops,
> > 
> > -		.buffer_size	= 0x6F,
> > +		.buffer_size	= 0x80,

 +		.buffer_size	= 0x80, /* The FIFO has 128 locations */

> > 
> >   	},
> >   	{
> >   	
> >   		.phys_base	= OMAP34XX_MCBSP4_BASE,
> > 
> > @@ -161,7 +161,7 @@ static struct omap_mcbsp_platform_data
> > omap34xx_mcbsp_pdata[] = {
> > 
> >   		.rx_irq		= INT_24XX_MCBSP4_IRQ_RX,
> >   		.tx_irq		= INT_24XX_MCBSP4_IRQ_TX,
> >   		.ops		=&omap2_mcbsp_ops,
> > 
> > -		.buffer_size	= 0x6F,
> > +		.buffer_size	= 0x80,
> > 
> >   	},
> >   	{
> >   	
> >   		.phys_base	= OMAP34XX_MCBSP5_BASE,
> > 
> > @@ -170,7 +170,7 @@ static struct omap_mcbsp_platform_data
> > omap34xx_mcbsp_pdata[] = {
> > 
> >   		.rx_irq		= INT_24XX_MCBSP5_IRQ_RX,
> >   		.tx_irq		= INT_24XX_MCBSP5_IRQ_TX,
> >   		.ops		=&omap2_mcbsp_ops,
> > 
> > -		.buffer_size	= 0x6F,
> > +		.buffer_size	= 0x80,
> > 
> >   	},
> >   
> >   };
> >   #define OMAP34XX_MCBSP_PDATA_SZ		ARRAY_SIZE(omap34xx_mcbsp_pdata)
> > 
> > diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> > index 51d8abf..6462968 100644
> > --- a/arch/arm/plat-omap/mcbsp.c
> > +++ b/arch/arm/plat-omap/mcbsp.c
> > @@ -497,7 +497,8 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16
> > threshold)
> > 
> >   	}
> >   	mcbsp = id_to_mcbsp_ptr(id);
> > 
> > -	MCBSP_WRITE(mcbsp, THRSH2, threshold);
> > +	if (threshold&&  threshold<= mcbsp->max_tx_thres)
> > +		MCBSP_WRITE(mcbsp, THRSH2, threshold - 1);
> > 
> >   }
> >   EXPORT_SYMBOL(omap_mcbsp_set_tx_threshold);
> > 
> > @@ -519,7 +520,8 @@ void omap_mcbsp_set_rx_threshold(unsigned int id, u16
> > threshold)
> > 
> >   	}
> >   	mcbsp = id_to_mcbsp_ptr(id);
> > 
> > -	MCBSP_WRITE(mcbsp, THRSH1, threshold);
> > +	if (threshold&&  threshold<= mcbsp->max_rx_thres)
> > +		MCBSP_WRITE(mcbsp, THRSH1, threshold - 1);
> > 
> >   }
> >   EXPORT_SYMBOL(omap_mcbsp_set_rx_threshold);
> > 
> > @@ -1696,8 +1698,8 @@ static inline void __devinit
> > omap34xx_device_init(struct omap_mcbsp *mcbsp)
> > 
> >   {
> >   
> >   	mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT;
> >   	if (cpu_is_omap34xx()) {
> > 
> > -		mcbsp->max_tx_thres = max_thres(mcbsp);
> > -		mcbsp->max_rx_thres = max_thres(mcbsp);
> > +		mcbsp->max_tx_thres = max_thres(mcbsp) - 0x10;
> > +		mcbsp->max_rx_thres = max_thres(mcbsp) - 0x10;
> > 
> >   		/*
> >   		
> >   		 * REVISIT: Set dmap_op_mode to THRESHOLD as default
> >   		 * for mcbsp2 instances.

Is this sufficient?

-- 
Péter

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Jarkko Nikula]

On Tue, 1 Jun 2010 09:47:09 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> I like the following naming:
> omap_mcbsp_hwrule_min_buffersize()
> omap_mcbsp_hwrule_max_periodsize()
> 
Looks clear to me.

> Also, I think there is no point to limit the lower period size in threshold mode 
> to 32, so I will remove that as well I think.
> 
What was the reason why period size cannot be bigger than threshold?
This constraint was there before your patch but I don't remember reason
for it.

Should it be opposite that period size cannot be smaller than threshold?


-- 
Jarkko

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

On Tuesday 01 June 2010 10:38:28 ext Jarkko Nikula wrote:
> On Tue, 1 Jun 2010 09:47:09 +0300
> 
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > I like the following naming:
> > omap_mcbsp_hwrule_min_buffersize()
> > omap_mcbsp_hwrule_max_periodsize()
> 
> Looks clear to me.
> 
> > Also, I think there is no point to limit the lower period size in
> > threshold mode to 32, so I will remove that as well I think.
> 
> What was the reason why period size cannot be bigger than threshold?
> This constraint was there before your patch but I don't remember reason
> for it.

When DMA is used to push the data to McBSP on OMAP3:
The McBSP threshold means, that if threshold amount of locations (words) are 
free in the buffer, than the DMA request line will be asserted, and McBSP 
expects that DMA will transfer _exactly_ threshold number of words in response 
to the DMA request.
So, if threshold is 1 (in register it is 0), than McBSP requests for new word, 
whenever a single location is free in the FIFO. The DMA should send 1 word per 
DMA request.

If threshold is configured to 100 (99 in register), than McBSP will asserts the 
DMA request line, when 100 locations are free. Than DMA has to send 100 words 
per DMA request.

So we need to limit the period size (which is used to configure the DMA's elem 
count - number of words per DMA request) that it shall never be bigger than the 
threshold.

> Should it be opposite that period size cannot be smaller than threshold?

No.

-- 
Péter

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

Hi

Resending...

On Tuesday 01 June 2010 10:38:28 ext Jarkko Nikula wrote:
> On Tue, 1 Jun 2010 09:47:09 +0300
> 
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > I like the following naming:
> > omap_mcbsp_hwrule_min_buffersize()
> > omap_mcbsp_hwrule_max_periodsize()
> 
> Looks clear to me.
> 
> > Also, I think there is no point to limit the lower period size in
> > threshold mode to 32, so I will remove that as well I think.
> 
> What was the reason why period size cannot be bigger than threshold?
> This constraint was there before your patch but I don't remember reason
> for it.

When DMA is used to push the data to McBSP on OMAP3:
The McBSP threshold means, that if threshold amount of locations (words) are 
free in the buffer, than the DMA request line will be asserted, and McBSP 
expects that DMA will transfer exactly threshold number of words in response 
to the DMA request.
So, if threshold is 1 (in register it is 0), than McBSP requests for new word, 
whenever a single location is free in the FIFO. The DMA should send 1 word per 
DMA request.

If threshold is configured to 100 (99 in register), than McBSP will asserts the 
DMA request line, when 100 locations are free. Than DMA has to send 100 words 
per DMA request.

So we need to limit the period size (which is used to configure the DMA's elem 
count - number of words per DMA request) that it shall never be bigger than the 
threshold.

> Should it be opposite that period size cannot be smaller than threshold?

No.

Well... One thing I was wondering about for a long time.
If we change the way how McBSP/DMA is used on OMAP3 (we could use the frame mode 
for that):
We place constraint on the period step to be the size of the threshold selected.
So userspace can have bigger periods than the McBSP FIFO and still gain benefit 
from the usage of the FIFO.
So period size is x * threshold.
We configure McBSP threshold
We configure DMA to push threshold amount of word per request.
In the DMA callback, we count, and when the x-th DMA request has been served, 
than we call elapsed.
If there is interest, I might look at this.
I guess this could be useful on McBSP1,3,4, and 5, which has small FIFO...

-- 
Péter

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Jarkko Nikula]

On Tue, 1 Jun 2010 11:19:30 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> If threshold is configured to 100 (99 in register), than McBSP will asserts the 
> DMA request line, when 100 locations are free. Than DMA has to send 100 words 
> per DMA request.
> 
> So we need to limit the period size (which is used to configure the DMA's elem 
> count - number of words per DMA request) that it shall never be bigger than the 
> threshold.
> 
Now I get it with some get hands dirty testing :-)

So this is a feature of SDMA that in threshold mode the transfer size
must equal or smaller than threshold (as says the TRM). Still don't
understand why the transfer size is dependent on peripheral FIFO
threshold size but that's the fact and we must obey it as Eduardo's
original patch and your's are doing.

> If there is interest, I might look at this.
> I guess this could be useful on McBSP1,3,4, and 5, which has small FIFO...
> 
Yes, have a packet based DMA transfer saving power and then we have
bunch of interrupts coming :-)


-- 
Jarkko

Re: [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

On Tuesday 01 June 2010 12:29:21 ext Jarkko Nikula wrote:
> On Tue, 1 Jun 2010 11:19:30 +0300
> 
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > If threshold is configured to 100 (99 in register), than McBSP will
> > asserts the DMA request line, when 100 locations are free. Than DMA has
> > to send 100 words per DMA request.
> > 
> > So we need to limit the period size (which is used to configure the DMA's
> > elem count - number of words per DMA request) that it shall never be
> > bigger than the threshold.
> 
> Now I get it with some get hands dirty testing :-)
> 
> So this is a feature of SDMA that in threshold mode the transfer size
> must equal or smaller than threshold (as says the TRM).

No exactly. This is the feature of McBSP, that the SDMA must transfer exactly 
the same number of words as the McBSP threshold on DMA request.

> Still don't understand why the transfer size is dependent on peripheral FIFO
> threshold size but that's the fact and we must obey it as Eduardo's
> original patch and your's are doing.

Because, if you want to transfer in one SDMA burst more than the space free in 
the McBSP FIFO, than where would the rest go?

> 
> > If there is interest, I might look at this.
> > I guess this could be useful on McBSP1,3,4, and 5, which has small
> > FIFO...
> 
> Yes, have a packet based DMA transfer saving power and then we have
> bunch of interrupts coming :-)

I guess it could be better than having 128 word long periods on McBSP1, 3, 4, 
and 5. With small period size the applications also need to be woken up, but if 
we silently handling the DMA IRQs, and the application is only woken up by every 
10. DMA IRQ, it might still save some power?



-- 
Péter

Re: [alsa-devel] [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Jarkko Nikula]

On Tue, 1 Jun 2010 13:30:10 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> Because, if you want to transfer in one SDMA burst more than the space free in 
> the McBSP FIFO, than where would the rest go?
> 
I would have expected peripheral to deassert the DMA request but I
haven't read the TRM so detail and experimenting with bigger period
sizes didn't work so some /dev/null effect was obviously happening :-)

> I guess it could be better than having 128 word long periods on McBSP1, 3, 4, 
> and 5. With small period size the applications also need to be woken up, but if 
> we silently handling the DMA IRQs, and the application is only woken up by every 
> 10. DMA IRQ, it might still save some power?
> 
This is worth to experiment. Probably more interrupts with or without
application wakeup reduction does not increase power as much as the
savings are from core clocks being more idle.


-- 
Jarkko

Re: [alsa-devel] [RFC PATCH 5/5] ASoC: omap-mcbsp: Place correct constraints for streams

[Peter Ujfalusi]

On Tuesday 01 June 2010 14:20:47 ext Jarkko Nikula wrote:
> On Tue, 1 Jun 2010 13:30:10 +0300
> 
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > Because, if you want to transfer in one SDMA burst more than the space
> > free in the McBSP FIFO, than where would the rest go?
> 
> I would have expected peripheral to deassert the DMA request but I
> haven't read the TRM so detail and experimenting with bigger period
> sizes didn't work so some /dev/null effect was obviously happening :-)

And for exchange I have tried the following:
in element mode (DMA is set to transfer 1 word on DMA request).
I have configured the McBSP threshold to max - 4 words.
It does not work either. Playback did not start.
I think McBSP is waiting to receive max - 4 samples, while only one word arrived
Or something, don't know...

> 
> > I guess it could be better than having 128 word long periods on McBSP1,
> > 3, 4, and 5. With small period size the applications also need to be
> > woken up, but if we silently handling the DMA IRQs, and the application
> > is only woken up by every 10. DMA IRQ, it might still save some power?
> 
> This is worth to experiment. Probably more interrupts with or without
> application wakeup reduction does not increase power as much as the
> savings are from core clocks being more idle.

Although application can also configure the avail_min, so it will be not woken 
up for every period....

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/5] OMAP3: McBSP: Change the way how the FIFO is handled

[Nishanth Menon]

On 06/01/2010 09:59 AM, Peter Ujfalusi wrote:
> On Monday 31 May 2010 20:41:12 ext Nishanth Menon wrote:
>> On 05/31/2010 11:16 AM, Peter Ujfalusi wrote:
>>> Use the actual FIFO size in words as buffer_size on OMAP2.
>>> Change the threshold configuration to use 1 based numbering, when
>>> specifying the allowed threshold maximum or the McBSP threshold value.
>>> Set the default maximum threshold to (buffer_size - 0x10) intialy.
>>>
>>>>  From users of McBSP, now it is expected to use this method.
>>>
>>> Asking for threshold 1 means that the value written to threshold
>>> registers are going to be 0, which means 1 word threshold.
>>
>> just a 2cent minor comment: maybe omap_mcbsp_platform_data needs
>> structure documentation.. it might be difficult for folks to figure that
>> out from commit ID itself..
>
> I can add comments in the mach-omap2/mcbsp.c like this:
>

I had meant more of
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/kernel-doc-nano-HOWTO.txt;h=27a52b35d55bf1b6009551c97f400da5c14f59bc;hb=HEAD#l84

but i suspect that it is out of the scope of this patch at least.. and 
the comments below might be helpful at the least.
>>
>>> Signed-off-by: Peter Ujfalusi<peter.ujfalusi@nokia.com>
>>> ---
>>>
>>>    arch/arm/mach-omap2/mcbsp.c |   10 +++++-----
>>>    arch/arm/plat-omap/mcbsp.c  |   10 ++++++----
>>>    2 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
>>> index 016fe60..9139958 100644
>>> --- a/arch/arm/mach-omap2/mcbsp.c
>>> +++ b/arch/arm/mach-omap2/mcbsp.c
>>> @@ -132,7 +132,7 @@ static struct omap_mcbsp_platform_data
>>> omap34xx_mcbsp_pdata[] = {
>>>
>>>    		.rx_irq		= INT_24XX_MCBSP1_IRQ_RX,
>>>    		.tx_irq		= INT_24XX_MCBSP1_IRQ_TX,
>>>    		.ops		=&omap2_mcbsp_ops,
>>>
>>> -		.buffer_size	= 0x6F,
>>> +		.buffer_size	= 0x80,
>
>   +		.buffer_size	= 0x80, /* The FIFO has 128 locations */
>
>>>
>>>    	},
>>>    	{
>>>    	
>>>    		.phys_base	= OMAP34XX_MCBSP2_BASE,
>>>
>>> @@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data
>>> omap34xx_mcbsp_pdata[] = {
>>>
>>>    		.rx_irq		= INT_24XX_MCBSP2_IRQ_RX,
>>>    		.tx_irq		= INT_24XX_MCBSP2_IRQ_TX,
>>>    		.ops		=&omap2_mcbsp_ops,
>>>
>>> -		.buffer_size	= 0x3FF,
>>> +		.buffer_size	= 0x500,
>
>   +		.buffer_size	= 0x500, /* The FIFO has 1024 + 256 locations */
>
>>>
>>>    	},
>>>    	{
>>>    	
>>>    		.phys_base	= OMAP34XX_MCBSP3_BASE,
>>>
>>> @@ -152,7 +152,7 @@ static struct omap_mcbsp_platform_data
>>> omap34xx_mcbsp_pdata[] = {
>>>
>>>    		.rx_irq		= INT_24XX_MCBSP3_IRQ_RX,
>>>    		.tx_irq		= INT_24XX_MCBSP3_IRQ_TX,
>>>    		.ops		=&omap2_mcbsp_ops,
>>>
>>> -		.buffer_size	= 0x6F,
>>> +		.buffer_size	= 0x80,
>
>   +		.buffer_size	= 0x80, /* The FIFO has 128 locations */
>
>>>
>>>    	},
>>>    	{
>>>    	
>>>    		.phys_base	= OMAP34XX_MCBSP4_BASE,
>>>
>>> @@ -161,7 +161,7 @@ static struct omap_mcbsp_platform_data
>>> omap34xx_mcbsp_pdata[] = {
>>>
>>>    		.rx_irq		= INT_24XX_MCBSP4_IRQ_RX,
>>>    		.tx_irq		= INT_24XX_MCBSP4_IRQ_TX,
>>>    		.ops		=&omap2_mcbsp_ops,
>>>
>>> -		.buffer_size	= 0x6F,
>>> +		.buffer_size	= 0x80,
>>>
>>>    	},
>>>    	{
>>>    	
>>>    		.phys_base	= OMAP34XX_MCBSP5_BASE,
>>>
>>> @@ -170,7 +170,7 @@ static struct omap_mcbsp_platform_data
>>> omap34xx_mcbsp_pdata[] = {
>>>
>>>    		.rx_irq		= INT_24XX_MCBSP5_IRQ_RX,
>>>    		.tx_irq		= INT_24XX_MCBSP5_IRQ_TX,
>>>    		.ops		=&omap2_mcbsp_ops,
>>>
>>> -		.buffer_size	= 0x6F,
>>> +		.buffer_size	= 0x80,
>>>
>>>    	},
>>>
>>>    };
>>>    #define OMAP34XX_MCBSP_PDATA_SZ		ARRAY_SIZE(omap34xx_mcbsp_pdata)
>>>
>>> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
>>> index 51d8abf..6462968 100644
>>> --- a/arch/arm/plat-omap/mcbsp.c
>>> +++ b/arch/arm/plat-omap/mcbsp.c
>>> @@ -497,7 +497,8 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16
>>> threshold)
>>>
>>>    	}
>>>    	mcbsp = id_to_mcbsp_ptr(id);
>>>
>>> -	MCBSP_WRITE(mcbsp, THRSH2, threshold);
>>> +	if (threshold&&   threshold<= mcbsp->max_tx_thres)
>>> +		MCBSP_WRITE(mcbsp, THRSH2, threshold - 1);
>>>
>>>    }
>>>    EXPORT_SYMBOL(omap_mcbsp_set_tx_threshold);
>>>
>>> @@ -519,7 +520,8 @@ void omap_mcbsp_set_rx_threshold(unsigned int id, u16
>>> threshold)
>>>
>>>    	}
>>>    	mcbsp = id_to_mcbsp_ptr(id);
>>>
>>> -	MCBSP_WRITE(mcbsp, THRSH1, threshold);
>>> +	if (threshold&&   threshold<= mcbsp->max_rx_thres)
>>> +		MCBSP_WRITE(mcbsp, THRSH1, threshold - 1);
>>>
>>>    }
>>>    EXPORT_SYMBOL(omap_mcbsp_set_rx_threshold);
>>>
>>> @@ -1696,8 +1698,8 @@ static inline void __devinit
>>> omap34xx_device_init(struct omap_mcbsp *mcbsp)
>>>
>>>    {
>>>
>>>    	mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT;
>>>    	if (cpu_is_omap34xx()) {
>>>
>>> -		mcbsp->max_tx_thres = max_thres(mcbsp);
>>> -		mcbsp->max_rx_thres = max_thres(mcbsp);
>>> +		mcbsp->max_tx_thres = max_thres(mcbsp) - 0x10;
>>> +		mcbsp->max_rx_thres = max_thres(mcbsp) - 0x10;
>>>
>>>    		/*
>>>    		
>>>    		 * REVISIT: Set dmap_op_mode to THRESHOLD as default
>>>    		 * for mcbsp2 instances.
>
> Is this sufficient?
>

Regards,
Nishanth Menon

[RFC PATCH 2/5] OMAP3: McBSP: Change the way how the FIFO is handled

[Peter Ujfalusi]

Use the actual FIFO size in words as buffer_size on OMAP2.
Change the threshold configuration to use 1 based numbering, when
specifying the allowed threshold maximum or the McBSP threshold value.
Set the default maximum threshold to (buffer_size - 0x10) intialy.
>From users of McBSP, now it is expected to use this method.
Asking for threshold 1 means that the value written to threshold registers
are going to be 0, which means 1 word threshold.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 arch/arm/mach-omap2/mcbsp.c |   10 +++++-----
 arch/arm/plat-omap/mcbsp.c  |   10 ++++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
index 016fe60..9139958 100644
--- a/arch/arm/mach-omap2/mcbsp.c
+++ b/arch/arm/mach-omap2/mcbsp.c
@@ -132,7 +132,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP1_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP1_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP2_BASE,
@@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP2_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP2_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x3FF,
+		.buffer_size	= 0x500,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP3_BASE,
@@ -152,7 +152,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP3_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP3_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP4_BASE,
@@ -161,7 +161,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP4_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP4_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 	{
 		.phys_base	= OMAP34XX_MCBSP5_BASE,
@@ -170,7 +170,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
 		.rx_irq		= INT_24XX_MCBSP5_IRQ_RX,
 		.tx_irq		= INT_24XX_MCBSP5_IRQ_TX,
 		.ops		= &omap2_mcbsp_ops,
-		.buffer_size	= 0x6F,
+		.buffer_size	= 0x80,
 	},
 };
 #define OMAP34XX_MCBSP_PDATA_SZ		ARRAY_SIZE(omap34xx_mcbsp_pdata)
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 51d8abf..6462968 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -497,7 +497,8 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold)
 	}
 	mcbsp = id_to_mcbsp_ptr(id);
 
-	MCBSP_WRITE(mcbsp, THRSH2, threshold);
+	if (threshold && threshold <= mcbsp->max_tx_thres)
+		MCBSP_WRITE(mcbsp, THRSH2, threshold - 1);
 }
 EXPORT_SYMBOL(omap_mcbsp_set_tx_threshold);
 
@@ -519,7 +520,8 @@ void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold)
 	}
 	mcbsp = id_to_mcbsp_ptr(id);
 
-	MCBSP_WRITE(mcbsp, THRSH1, threshold);
+	if (threshold && threshold <= mcbsp->max_rx_thres)
+		MCBSP_WRITE(mcbsp, THRSH1, threshold - 1);
 }
 EXPORT_SYMBOL(omap_mcbsp_set_rx_threshold);
 
@@ -1696,8 +1698,8 @@ static inline void __devinit omap34xx_device_init(struct omap_mcbsp *mcbsp)
 {
 	mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT;
 	if (cpu_is_omap34xx()) {
-		mcbsp->max_tx_thres = max_thres(mcbsp);
-		mcbsp->max_rx_thres = max_thres(mcbsp);
+		mcbsp->max_tx_thres = max_thres(mcbsp) - 0x10;
+		mcbsp->max_rx_thres = max_thres(mcbsp) - 0x10;
 		/*
 		 * REVISIT: Set dmap_op_mode to THRESHOLD as default
 		 * for mcbsp2 instances.
-- 
1.7.1