[PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

s3c24xx dma has the auto reload feature, when the the trnasfer is done,
CURR_TC(DSTAT[19:0], current value of transfer count) reaches 0, and DMA
ACK becomes 1, and then, TC(DCON[19:0]) will be loaded into CURR_TC. So
the transmission is repeated.

IRQ is issued while auto reload occurs. We change the DISRC and
DCON[19:0] in the ISR, but at this time, the auto reload has been
performed already. The first block is being re-transmitted by the DMA.

So we need rewrite the DISRC and DCON[19:0] for the next block
immediatly after the this block has been started to be transported.

The function s3c2410_dma_started() is for this perpose, which is called
in the form of "s3c2410_dma_ctrl(prtd->params->channel,
S3C2410_DMAOP_STARTED);" in s3c24xx_pcm_trigger().

But it is not correct. DMA transmission won't start until DMA REQ signal
arrived, it is the time s3c24xx_snd_txctrl(1) or s3c24xx_snd_rxctrl(1)
is called in s3c24xx_i2s_trigger().

In the current framework, s3c24xx_pcm_trigger() is always called before
s3c24xx_pcm_trigger(). So the s3c2410_dma_started() should be called in
s3c24xx_pcm_trigger() after s3c24xx_snd_txctrl(1) or
s3c24xx_snd_rxctrl(1) is called in this function.

However, s3c2410_dma_started() is dma related, to call this function we
should provide the channel number, which is given by
substream->runtime->private_data->params->channel. The private_data
points to a struct s3c24xx_runtime_data object, which is define in
s3c24xx_pcm.c, so s3c2410_dma_started() can't be called in s3c24xx_i2s.c

To move the struct s3c24xx_runtime_data defination to s3c24xx-pcm.h is
one solution.

If not moved, the patch shold be like:


Signed-off-by: Shine Liu <shinel@foxmail.com>
----------------------------------------------------------------

--- a/sound/soc/s3c24xx/s3c24xx-pcm.h	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.h	2009-08-20 16:06:14.000000000 +0800
@@ -28,4 +28,6 @@
 extern struct snd_soc_platform s3c24xx_soc_platform;
 extern struct snd_ac97_bus_ops s3c24xx_ac97_ops;
 
+void s3c24xx_pcm_dma_started(struct snd_pcm_substream *substream);
+
 #endif
--- a/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-20 15:56:46.000000000 +0800
@@ -66,6 +66,28 @@
 	struct s3c24xx_pcm_dma_params *params;
 };
 
+/* s3c24xx_pcm_dma_started
+ *
+ * To load the second dma buffer immediately after the 
+ * first dma buffer has been started transferring, so that 
+ * in the dma autoreload situation, the first data block
+ * won't be re-transferred after the first irq issued.
+ * 
+ * This function is called in the s3c24xx-i2s-trigger() of 
+ * s3c24xx-i2s.c, just after the dma transfer is really started.
+ *
+ * It's ugly here. Need a elegant way to fix this bug.
+ * FIXME
+ *
+ */
+
+void s3c24xx_pcm_dma_started(struct snd_pcm_substream *substream)
+{
+	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
+	s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STARTED);
+
+}
+
 /* s3c24xx_pcm_enqueue
  *
  * place a dma buffer onto the queue for the dma system
@@ -255,7 +277,6 @@
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		prtd->state |= ST_RUNNING;
 		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_START);
-		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STARTED);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
--- a/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-20 16:08:06.000000000 +0800
@@ -296,6 +296,9 @@
 			s3c24xx_snd_rxctrl(1);
 		else
 			s3c24xx_snd_txctrl(1);
+
+		/* DMA transfer is really started, equeue the next buffer */
+		s3c24xx_pcm_dma_started(substream);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Mark Brown]

On Thu, Aug 20, 2009 at 05:42:11PM +0800, Shine Liu wrote:

> In the current framework, s3c24xx_pcm_trigger() is always called before
> s3c24xx_pcm_trigger(). So the s3c2410_dma_started() should be called in
> s3c24xx_pcm_trigger() after s3c24xx_snd_txctrl(1) or
> s3c24xx_snd_rxctrl(1) is called in this function.

I suspect some of the function names in your description here are
incorrect :) Another option is to provide a callback in the private data
passed to the DMA driver which the DMA driver can call at the
appropriate point during setup.

Someone will presumably also need to take care of the same things in the
other S3C DAI drivers too, though it should be possible to arrange
things so that can be done seperately.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

On Thu, 2009-08-20 at 11:15 +0100, Mark Brown wrote:
> On Thu, Aug 20, 2009 at 05:42:11PM +0800, Shine Liu wrote:
> 
> > In the current framework, s3c24xx_pcm_trigger() is always called before
> > s3c24xx_pcm_trigger(). So the s3c2410_dma_started() should be called in
> > s3c24xx_pcm_trigger() after s3c24xx_snd_txctrl(1) or
> > s3c24xx_snd_rxctrl(1) is called in this function.
> 
> I suspect some of the function names in your description here are
> incorrect :) Another option is to provide a callback in the private data
> passed to the DMA driver which the DMA driver can call at the
> appropriate point during setup.

Sorry for the mistake. Should be s3c24xx_pcm_trigger() is always called
before s3c24xx_i2s_trigger() in current framework :
s3c24xx_pcm_trigger() is called via platform->pcm_ops->trigger and
s3c24xx_i2s_trigger() is called via cpu_dai->ops->trigger.

The problem is that DMA driver doesn't know when the DMA transfer has
been really started. The DMA setup is done in s3c24xx_pcm_trigger(), in
which all DMA related registers are set. But it doesn't mean the DMA
transfer is started at this point because the DMA REQ signal has not
arrived which is generated by the IIS. DMA driver has no way to know
when the DMA REQ signal arrived. So the s3c24xx DMA driver provids a
s3c2410_dma_started() method to let the user call when he knows the DMA
transfer has been started.

> 
> Someone will presumably also need to take care of the same things in the
> other S3C DAI drivers too, though it should be possible to arrange
> things so that can be done seperately.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Mark Brown]

On Thu, Aug 20, 2009 at 07:59:48PM +0800, Shine Liu wrote:
> On Thu, 2009-08-20 at 11:15 +0100, Mark Brown wrote:

> > incorrect :) Another option is to provide a callback in the private data
> > passed to the DMA driver which the DMA driver can call at the
> > appropriate point during setup.

> arrived which is generated by the IIS. DMA driver has no way to know
> when the DMA REQ signal arrived. So the s3c24xx DMA driver provids a
> s3c2410_dma_started() method to let the user call when he knows the DMA
> transfer has been started.

Yes, I understand the problem.  The callback I suggest above ought to
allow the appropriate sequencing - the DMA driver can call the callback
to start the DAI in between starting the DMA and kicking the DMA API to
queue the next block.  That would avoid the abstraction problems.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

On Thu, 2009-08-20 at 13:18 +0100, Mark Brown wrote:
> 
> Yes, I understand the problem.  The callback I suggest above ought to
> allow the appropriate sequencing - the DMA driver can call the callback
> to start the DAI in between starting the DMA and kicking the DMA API to
> queue the next block.  That would avoid the abstraction problems.
> 

Callback is a elegant way to solve the problem. But I have a question
that when should the DMA driver call the callback funtion? There is no
event to tell it to call. Periodly checking? It will not only cause
latency but also increase the system load. 

Thanks.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Mark Brown]

On Thu, Aug 20, 2009 at 09:38:00PM +0800, Shine Liu wrote:

> Callback is a elegant way to solve the problem. But I have a question
> that when should the DMA driver call the callback funtion? There is no
> event to tell it to call. Periodly checking? It will not only cause
> latency but also increase the system load. 

The callback doesn't need to be set up within the trigger function - if
it's done before the trigger functions are called then the DMA trigger
can call it.  Off the top of my head I'd expect it's possible to set it
up when the device is opened.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

> The callback doesn't need to be set up within the trigger function - if
> it's done before the trigger functions are called then the DMA trigger
> can call it.  Off the top of my head I'd expect it's possible to set it
> up when the device is opened.

Who call the DMA trigger? When? In any of the platform/cpu_dai trigger
functions?

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Mark Brown]

On Thu, Aug 20, 2009 at 11:45:07PM +0800, Shine Liu wrote:

> > The callback doesn't need to be set up within the trigger function - if
> > it's done before the trigger functions are called then the DMA trigger
> > can call it.  Off the top of my head I'd expect it's possible to set it
> > up when the device is opened.

> Who call the DMA trigger? When? In any of the platform/cpu_dai trigger
> functions?

It's called ultimately by user space; within ASoC it's always called by
the core trigger function in the fixed order that it has.  The open()
callback would be a safe place to do the setup - that's called before
anything else.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

> 
> It's called ultimately by user space; within ASoC it's always called by
> the core trigger function in the fixed order that it has.  The open()
> callback would be a safe place to do the setup - that's called before
> anything else.
> 
> 

I've finished a draft patch in the callback manner. Waiting for any
suggestion. Thanks.


-----------------------------------------------------------------

diff -Nur a/arch/arm/plat-s3c24xx/include/plat/dma-plat.h b/arch/arm/plat-s3c24xx/include/plat/dma-plat.h
--- a/arch/arm/plat-s3c24xx/include/plat/dma-plat.h	2009-08-14 06:43:34.000000000 +0800
+++ b/arch/arm/plat-s3c24xx/include/plat/dma-plat.h	2009-08-21 14:14:12.000000000 +0800
@@ -76,6 +76,27 @@
 
 extern int s3c24xx_dma_order_set(struct s3c24xx_dma_order *map);
 
+enum s3c24xx_dma_channel_event {
+	CHANNEL_EVENT_START,
+	CHANNEL_EVENT_STOP,
+	CHANNEL_EVENT_MAX,
+};
+
+struct s3c24xx_dma_channel_trigger {
+	unsigned int channel;
+	/* trigger event */
+	enum s3c24xx_dma_channel_event event;
+	/* pravate date to the callback function */
+	void *private_data;
+	/* callback funtion to generate the DMA REQ signal */
+	void (*gen_request)(void *private_data);
+	/* callback funtion to end the DMA request */
+	void (*end_request)(void *private_data);
+};
+
+/* trigger the DMA engine to start, called from the client */
+extern void s3c24xx_dma_trigger(struct s3c24xx_dma_channel_trigger *client);
+
 /* DMA init code, called from the cpu support code */
 
 extern int s3c2410_dma_init(void);
diff -Nur a/arch/arm/plat-s3c24xx/dma.c b/arch/arm/plat-s3c24xx/dma.c
--- a/arch/arm/plat-s3c24xx/dma.c	2009-08-14 06:43:34.000000000 +0800
+++ b/arch/arm/plat-s3c24xx/dma.c	2009-08-21 14:30:41.000000000 +0800
@@ -1008,6 +1008,34 @@
 
 EXPORT_SYMBOL(s3c2410_dma_ctrl);
 
+void s3c24xx_dma_trigger(struct s3c24xx_dma_channel_trigger *client)
+{
+	if(!client || !(client->channel < DMACH_MAX)) {
+ 		pr_debug("%s: Invalid parameter\n", __func__);
+		return;
+	}
+
+	switch(client->event) {
+	case CHANNEL_EVENT_START:
+		s3c2410_dma_ctrl(client->channel, S3C2410_DMAOP_START);
+		if(client->gen_request)
+			client->gen_request(client->private_data);
+		s3c2410_dma_ctrl(client->channel, S3C2410_DMAOP_STARTED);
+		return;
+	case CHANNEL_EVENT_STOP:
+		s3c2410_dma_ctrl(client->channel, S3C2410_DMAOP_STOP);
+		if(client->end_request)
+			client->end_request(client->private_data);
+		/* should be set to a vaild value in the next request */
+		client->channel = DMACH_MAX;
+		return;
+	default:
+		pr_debug("%s: Invalid event\n", __func__);
+	}
+}
+
+EXPORT_SYMBOL(s3c24xx_dma_trigger);
+
 /* DMA configuration for each channel
  *
  * DISRCC -> source of the DMA (AHB,APB)
diff -Nur a/sound/soc/s3c24xx/s3c24xx-i2s.c b/sound/soc/s3c24xx/s3c24xx-i2s.c
--- a/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-21 15:08:07.000000000 +0800
@@ -38,6 +38,7 @@
 
 #include <plat/regs-iis.h>
 
+#define __USE_S3C24XX_PCM_RUNTIME
 #include "s3c24xx-pcm.h"
 #include "s3c24xx-i2s.h"
 
@@ -278,6 +279,7 @@
 static int s3c24xx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 			       struct snd_soc_dai *dai)
 {
+	struct s3c24xx_pcm_runtime_data *prtd = substream->runtime->private_data;
 	int ret = 0;
 
 	pr_debug("Entered %s\n", __func__);
@@ -292,24 +294,32 @@
 				goto exit_err;
 		}
 
+		spin_lock(&prtd->lock);
+		prtd->client.private_data= (void *)1;
 		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-			s3c24xx_snd_rxctrl(1);
+			prtd->client.gen_request = (void *)s3c24xx_snd_rxctrl;
 		else
-			s3c24xx_snd_txctrl(1);
+			prtd->client.gen_request = (void *)s3c24xx_snd_txctrl;
+		spin_unlock(&prtd->lock);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		spin_lock(&prtd->lock);
+		prtd->client.private_data= (void *)0;
 		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-			s3c24xx_snd_rxctrl(0);
+			prtd->client.end_request = (void *)s3c24xx_snd_rxctrl;
 		else
-			s3c24xx_snd_txctrl(0);
+			prtd->client.end_request = (void *)s3c24xx_snd_txctrl;
+		spin_unlock(&prtd->lock);
 		break;
 	default:
 		ret = -EINVAL;
-		break;
+		goto exit_err;
 	}
 
+	s3c24xx_dma_trigger(&prtd->client);
+
 exit_err:
 	return ret;
 }
diff -Nur a/sound/soc/s3c24xx/s3c24xx-pcm.h b/sound/soc/s3c24xx/s3c24xx-pcm.h
--- a/sound/soc/s3c24xx/s3c24xx-pcm.h	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.h	2009-08-21 14:40:05.000000000 +0800
@@ -22,6 +22,25 @@
 	int dma_size;			/* Size of the DMA transfer */
 };
 
+#ifdef __USE_S3C24XX_PCM_RUNTIME
+
+#include <plat/dma-plat.h>
+
+struct s3c24xx_pcm_runtime_data {
+	spinlock_t lock;
+	int state;
+	unsigned int dma_loaded;
+	unsigned int dma_limit;
+	unsigned int dma_period;
+	dma_addr_t dma_start;
+	dma_addr_t dma_pos;
+	dma_addr_t dma_end;
+	struct s3c24xx_pcm_dma_params *params;
+	struct s3c24xx_dma_channel_trigger client;
+};
+
+#endif
+
 #define S3C24XX_DAI_I2S			0
 
 /* platform data */
diff -Nur a/sound/soc/s3c24xx/s3c24xx-pcm.c b/sound/soc/s3c24xx/s3c24xx-pcm.c
--- a/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-21 14:47:29.000000000 +0800
@@ -31,6 +31,7 @@
 #include <mach/dma.h>
 #include <plat/audio.h>
 
+#define __USE_S3C24XX_PCM_RUNTIME
 #include "s3c24xx-pcm.h"
 
 static const struct snd_pcm_hardware s3c24xx_pcm_hardware = {
@@ -54,18 +55,6 @@
 	.fifo_size		= 32,
 };
 
-struct s3c24xx_runtime_data {
-	spinlock_t lock;
-	int state;
-	unsigned int dma_loaded;
-	unsigned int dma_limit;
-	unsigned int dma_period;
-	dma_addr_t dma_start;
-	dma_addr_t dma_pos;
-	dma_addr_t dma_end;
-	struct s3c24xx_pcm_dma_params *params;
-};
-
 /* s3c24xx_pcm_enqueue
  *
  * place a dma buffer onto the queue for the dma system
@@ -73,7 +62,7 @@
 */
 static void s3c24xx_pcm_enqueue(struct snd_pcm_substream *substream)
 {
-	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = substream->runtime->private_data;
 	dma_addr_t pos = prtd->dma_pos;
 	int ret;
 
@@ -110,7 +99,7 @@
 				enum s3c2410_dma_buffresult result)
 {
 	struct snd_pcm_substream *substream = dev_id;
-	struct s3c24xx_runtime_data *prtd;
+	struct s3c24xx_pcm_runtime_data *prtd;
 
 	pr_debug("Entered %s\n", __func__);
 
@@ -135,7 +124,7 @@
 	struct snd_pcm_hw_params *params)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct s3c24xx_runtime_data *prtd = runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = runtime->private_data;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct s3c24xx_pcm_dma_params *dma = rtd->dai->cpu_dai->dma_data;
 	unsigned long totbytes = params_buffer_bytes(params);
@@ -187,7 +176,7 @@
 
 static int s3c24xx_pcm_hw_free(struct snd_pcm_substream *substream)
 {
-	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = substream->runtime->private_data;
 
 	pr_debug("Entered %s\n", __func__);
 
@@ -204,7 +193,7 @@
 
 static int s3c24xx_pcm_prepare(struct snd_pcm_substream *substream)
 {
-	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = substream->runtime->private_data;
 	int ret = 0;
 
 	pr_debug("Entered %s\n", __func__);
@@ -242,7 +231,7 @@
 
 static int s3c24xx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
-	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = substream->runtime->private_data;
 	int ret = 0;
 
 	pr_debug("Entered %s\n", __func__);
@@ -254,15 +243,15 @@
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		prtd->state |= ST_RUNNING;
-		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_START);
-		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STARTED);
+		prtd->client.channel = prtd->params->channel;
+		prtd->client.event = CHANNEL_EVENT_START;
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		prtd->state &= ~ST_RUNNING;
-		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STOP);
+		prtd->client.event = CHANNEL_EVENT_STOP;
 		break;
 
 	default:
@@ -279,7 +268,7 @@
 s3c24xx_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct s3c24xx_runtime_data *prtd = runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = runtime->private_data;
 	unsigned long res;
 	dma_addr_t src, dst;
 
@@ -314,16 +303,18 @@
 static int s3c24xx_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct s3c24xx_runtime_data *prtd;
+	struct s3c24xx_pcm_runtime_data *prtd;
 
 	pr_debug("Entered %s\n", __func__);
 
 	snd_soc_set_runtime_hwparams(substream, &s3c24xx_pcm_hardware);
 
-	prtd = kzalloc(sizeof(struct s3c24xx_runtime_data), GFP_KERNEL);
+	prtd = kzalloc(sizeof(struct s3c24xx_pcm_runtime_data), GFP_KERNEL);
 	if (prtd == NULL)
 		return -ENOMEM;
 
+	prtd->client.channel = DMACH_MAX;
+
 	spin_lock_init(&prtd->lock);
 
 	runtime->private_data = prtd;
@@ -333,7 +324,7 @@
 static int s3c24xx_pcm_close(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct s3c24xx_runtime_data *prtd = runtime->private_data;
+	struct s3c24xx_pcm_runtime_data *prtd = runtime->private_data;
 
 	pr_debug("Entered %s\n", __func__);

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Mark Brown]

On Fri, Aug 21, 2009 at 06:00:39PM +0800, Shine Liu wrote:

> I've finished a draft patch in the callback manner. Waiting for any
> suggestion. Thanks.

This isn't quite what I was expecting - what I was expecting was more
that the I2S driver would set up a function on startup which would be
called by the ASoC DMA driver from its trigger function.  It does look
like a reasonable approach, though.

The way you've got things at the minute at least all the other S3C ASoC
drivers will need to be updated to match the change in the DMA driver.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

> 
> This isn't quite what I was expecting - what I was expecting was more
> that the I2S driver would set up a function on startup which would be
> called by the ASoC DMA driver from its trigger function.  It does look
> like a reasonable approach, though.

Yes, I understand what you exactly want. I was intend to register the
callback function on I2S startup, but finally I found it is not possible
because there are two callback functions for the DMA driver:
s3c24xx_snd_txctrl() and s3c24xx_snd_rxctrl(), however, there's only one
slot for the callback function. So I choose not to setup the callback
functions dynamicly, when the task is playback, the callback funtion is
s3c24xx_snd_txctrl() and when the task is record, the funtion is
s3c24xx_snd_rxctrl().

However, there's another way to archive what you expect: combine the
s3c24xx_snd_txctrl() and s3c24xx_snd_rxctrl() into one function. That
would be like:

struct s3c24xx_snd_ctrl_data {
	int event;	//start or stop
	int direction;	//playback or record
};

static void s3c24xx_snd_ctrl(struct s3c24xx_snd_ctrl_data* data)
{
	if(data->direction == SNDRV_PCM_STREAM_PLAYBACK)
		s3c24xx_snd_txctrl(data->event);
	eles
		s3c24xx_snd_rxctrl(data->event);
}

Then we can register this callback funtion on I2S startup.

> 
> The way you've got things at the minute at least all the other S3C ASoC
> drivers will need to be updated to match the change in the DMA driver.
> 

I have posted the DMA driver patch to linux-arm-kernel yesterday:
http://lists.arm.linux.org.uk/lurker/message/20090822.153940.e15e3bd7.en.html

Hope there is no problem to merge the patch.

If the dma patch is accepted, I will post a formal patch here for ASoC
S3C24XX.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

> ...but there's no need for the struct, just pass two arguments to the
> function, the event and either the substream or the direction.  The
> substream is probably easier since otherwise all the callers will need
> to parse the direction.

Finally, I decide to place the callback function in struct
s3c2410_dma_client instead of add a new struct:

 struct s3c2410_dma_client {

       ....

       /* callback funtion from client to generate the DMA REQ signal */
       int (*gen_request)(void *);

       /* callback funtion from client to end the DMA request */
       int (*end_request)(void *);

       /* argument for the callback function */
       void            *private_data;
 };

I think the callback function should have only one argument. If we have
more than one argument to pass, just embbed them into a internal struct
known to the callback funtions and pass the pointer to the object of
this struct to the callback function. 

If not, more filed will be needed in struct s3c2410_dma_client to place
the arguments. I think it's redundant for the generic DMA driver.

I've posted five new patches which regist the callback functions either
in the I2S/AC97 probe phase or by directly assiged in the source code
depended by whether the callback functions and the object of struct
s3c2410_dma_client are in the same source file.

Look forward for further comments on these patches. There should be some
defects in them.

Thanks.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Mark Brown]

On Mon, Aug 24, 2009 at 07:54:40PM +0800, Shine Liu wrote:

> If not, more filed will be needed in struct s3c2410_dma_client to place
> the arguments. I think it's redundant for the generic DMA driver.

But why are you changing the core DMA code in the first place?  The ASoC
drivers have split front end and DMA code but this is not the normal
case for a DMA client.

Re: [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time

[Shine Liu]

> But why are you changing the core DMA code in the first place?  The ASoC
> drivers have split front end and DMA code but this is not the normal
> case for a DMA client.

I think your idea of the callback is very elegant and I appreciate this
way to solve the problem so I try to add new API for the core DMA to
achive this goal.

Well, if there are two arguments for the callback function passed though
struct struct s3c2410_dma_client, it's also reasonable. If there are
more arguments to pass, maybe this is not the best solution. Use a
internal struct to wrapper more arguments into one object and pass the
pointer is just a candidate. 

[PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time

[Shine Liu]

Hi Mark,

    There's a candidate patch for the bug of s3c2410_dma_started()
called at improper time. This patch also covers all S3C I2C/AC97 ASoC
drivers affected by this bug.

    The previous patches try to fix this bug in the DMA callback way. It
's elegant and should be the proper way to fix the bug but the changes
is a little large and need be well discussed and tested.

     I think before we fix the bug in the callback way, we can use this
patch provisionally for a while though it seems a little ugly. 

     This patch is created against linux-2.6.31-rc7, and has been well
tested under s3c2440 platform with I2S DAI.

Best Regards,

Shine 



Signed-off-by: Shine Liu <liuxian@redflag-linux.com>
----------------------------------------------------


--- a/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-24 15:57:23.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-25 10:29:12.000000000 +0800
@@ -255,7 +255,6 @@
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		prtd->state |= ST_RUNNING;
 		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_START);
-		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STARTED);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
--- a/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-24 15:57:23.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-25 10:05:02.000000000 +0800
@@ -279,9 +279,14 @@
 			       struct snd_soc_dai *dai)
 {
 	int ret = 0;
+	int channel;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 
 	pr_debug("Entered %s\n", __func__);
 
+	channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
@@ -296,6 +301,13 @@
 			s3c24xx_snd_rxctrl(1);
 		else
 			s3c24xx_snd_txctrl(1);
+		/* DMA engine has just been started at this point,
+		 * load the next buffer to DMA to meet the reqirement 
+		 * of the auto reload mechanism.
+		 * Ugly here, but it is the simplest way.
+		 * Will be fixed if DMA callback API provided.
+		 */
+		s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
--- a/sound/soc/s3c24xx/s3c2443-ac97.c	2009-08-24 15:57:23.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c2443-ac97.c	2009-08-25 10:08:57.000000000 +0800
@@ -290,6 +290,9 @@
 				struct snd_soc_dai *dai)
 {
 	u32 ac_glbctrl;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 	switch (cmd) {
@@ -312,6 +315,14 @@
 	}
 	writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 
+	/* DMA engine has just been started at this point,
+	 * load the next buffer to DMA to meet the reqirement 
+	 * of the auto reload mechanism.
+	 * Ugly here, but it is the simplest way.
+	 * Will be fixed if DMA callback API provided.
+	 */
+	s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
+
 	return 0;
 }
 
@@ -334,6 +345,9 @@
 				    int cmd, struct snd_soc_dai *dai)
 {
 	u32 ac_glbctrl;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 	switch (cmd) {
@@ -349,6 +363,14 @@
 	}
 	writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 
+	/* DMA engine has just been started at this point,
+	 * load the next buffer to DMA to meet the reqirement 
+	 * of the auto reload mechanism.
+	 * Ugly here, but it is the simplest way.
+	 * Will be fixed if DMA callback API provided.
+	 */
+	s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
+
 	return 0;
 }
 
--- a/sound/soc/s3c24xx/s3c-i2s-v2.c	2009-08-25 10:25:40.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c-i2s-v2.c	2009-08-25 10:15:34.000000000 +0800
@@ -387,6 +387,8 @@
 	int capture = (substream->stream == SNDRV_PCM_STREAM_CAPTURE);
 	unsigned long irqs;
 	int ret = 0;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	pr_debug("Entered %s\n", __func__);
 
@@ -416,6 +418,18 @@
 			s3c2412_snd_txctrl(i2s, 1);
 
 		local_irq_restore(irqs);
+
+		/* DMA engine has just been started at this point,
+		 * load the next buffer to DMA to meet the reqirement 
+		 * of the auto reload mechanism.
+		 * Ugly here, but it is the simplest way.
+		 * Will be fixed if DMA callback API provided.
+		 * 
+		 * S3C64XX doesn't have the auto reload problem, 
+		 * only for S3C24XX. This call won't bother S3C64XX.
+		 */
+		s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
+
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:

Re: [PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time

[Mark Brown]

On Tue, Aug 25, 2009 at 11:04:26AM +0800, Shine Liu wrote:

>      I think before we fix the bug in the callback way, we can use this
> patch provisionally for a while though it seems a little ugly. 

I can't prevail on you to do this with a callback within the ASoC code
only?  This really does seem like something that's created by the way
ASoC is structured rather than a general S3C DMA issue so I'm not sure
that the arch/arm code needs to be changed at all.

Otherwise the patch looks OK, in fact I'd tone down the comments about
it being a hack to do this - ASoC DAI and PCM drivers are supposed to be
fairly closely tied to each other.

Re: [PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time

[Shine Liu]

> Otherwise the patch looks OK, in fact I'd tone down the comments about
> it being a hack to do this - ASoC DAI and PCM drivers are supposed to be
> fairly closely tied to each other.
> 

Yes, I agree with you. I have cut down the unnecessary comments in the
patch. Thank you for your appropriate suggestion.

Shine




Signed-off-by: Shine Liu <liuxian@redflag-linux.com>
Signed-off-by: Shine Liu <shinel@foxmail.com>


--- a/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-24 15:57:23.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-25 10:29:12.000000000 +0800
@@ -255,7 +255,6 @@
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		prtd->state |= ST_RUNNING;
 		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_START);
-		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STARTED);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
--- a/sound/soc/s3c24xx/s3c2443-ac97.c	2009-08-25 10:22:31.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c2443-ac97.c	2009-08-25 19:38:44.000000000 +0800
@@ -290,6 +290,9 @@
 				struct snd_soc_dai *dai)
 {
 	u32 ac_glbctrl;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 	switch (cmd) {
@@ -312,6 +315,8 @@
 	}
 	writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 
+	s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
+
 	return 0;
 }
 
@@ -334,6 +339,9 @@
 				    int cmd, struct snd_soc_dai *dai)
 {
 	u32 ac_glbctrl;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 	switch (cmd) {
@@ -349,6 +357,8 @@
 	}
 	writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
 
+	s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
+
 	return 0;
 }
 
--- a/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-25 10:25:40.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-i2s.c	2009-08-25 19:39:32.000000000 +0800
@@ -279,6 +279,9 @@
 			       struct snd_soc_dai *dai)
 {
 	int ret = 0;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	pr_debug("Entered %s\n", __func__);
 
@@ -296,6 +299,8 @@
 			s3c24xx_snd_rxctrl(1);
 		else
 			s3c24xx_snd_txctrl(1);
+
+		s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
--- a/sound/soc/s3c24xx/s3c-i2s-v2.c	2009-08-25 10:25:40.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c-i2s-v2.c	2009-08-25 19:38:35.000000000 +0800
@@ -387,6 +387,8 @@
 	int capture = (substream->stream == SNDRV_PCM_STREAM_CAPTURE);
 	unsigned long irqs;
 	int ret = 0;
+	int channel = ((struct s3c24xx_pcm_dma_params *)
+		  rtd->dai->cpu_dai->dma_data)->channel;
 
 	pr_debug("Entered %s\n", __func__);
 
@@ -416,6 +418,14 @@
 			s3c2412_snd_txctrl(i2s, 1);
 
 		local_irq_restore(irqs);
+
+		/*
+		 * Load the next buffer to DMA to meet the reqirement 
+		 * of the auto reload mechanism of S3C24XX.
+		 * This call won't bother S3C64XX.
+		 */
+		s3c2410_dma_ctrl(channel, S3C2410_DMAOP_STARTED);
+
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:

Re: [PATCH] ASoC: S3C platform: Fix s3c2410_dma_started() called at improper time

[Mark Brown]

On Tue, Aug 25, 2009 at 08:05:50PM +0800, Shine Liu wrote:

> Yes, I agree with you. I have cut down the unnecessary comments in the
> patch. Thank you for your appropriate suggestion.

I've applied this, taking the commit message from your original e-mail
with a slight update to reflect the final solution.  Thanks for all your
patient work on this.