[alsa-devel] [PATCH] ASoC: s3c24xx platform: Fix s3c2410_dma_started called at wrong time
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:
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.
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.
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.
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.
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.
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?
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.
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__);
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.
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.htm...
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.
On Sun, Aug 23, 2009 at 07:37:13PM +0800, Shine Liu wrote:
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
This is not a problem - just pass the substream into the callback and you can check to see if it's a playback or record stream. Kind of like what you have here...
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); }
...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.
...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.
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.
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.
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.
Hi Mark,
I have updated the patch. These series of patch covers all S3C I2C/AC97 ASoC drivers affected by the changes in the DMA driver.
Cheers,
Shine
DMA part: No old API dropped. Add the s3c2410_dma_trigger() API used for to start or to stop a DMA transfer. This API will reduce the miss use of s3c2410_dma_started() function. User should provide the callback functions, trigger event and the DMA channel number before calling this API. These private data is packed in struct s3c2410_dma_client. No new struct added.
--- a/arch/arm/plat-s3c/include/plat/dma.h 2009-08-24 13:29:03.000000000 +0800 +++ b/arch/arm/plat-s3c/include/plat/dma.h 2009-08-24 15:30:23.000000000 +0800 @@ -37,8 +37,23 @@ S3C2410_DMAOP_STARTED, /* indicate channel started */ };
+enum s3c2410_dma_trigger_event { + S3C2410_DMA_TRIGGER_START, /* start a channel */ + S3C2410_DMA_TRIGGER_STOP, /* stop a channel */ + S3C2410_DMA_TRIGGER_MAX, +}; + struct s3c2410_dma_client { - char *name; + char *name; + unsigned int channel; /* channel number */ + /* trigger event */ + enum s3c2410_dma_trigger_event event; + /* 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 *); + /* pravate data to the callback function */ + void *private_data; };
struct s3c2410_dma_chan; @@ -73,6 +88,13 @@
extern int s3c2410_dma_ctrl(unsigned int channel, enum s3c2410_chan_op op);
+/* s3c2410_dma_trigger + * + * trigger the DMA engine to start or stop +*/ + +extern int s3c2410_dma_trigger(struct s3c2410_dma_client *client); + /* s3c2410_dma_setflags * * set the channel's flags to a given state --- a/arch/arm/plat-s3c/dma.c 2009-08-24 15:26:07.000000000 +0800 +++ b/arch/arm/plat-s3c/dma.c 2009-08-24 15:27:09.000000000 +0800 @@ -84,3 +84,32 @@ return 0; } EXPORT_SYMBOL(s3c2410_dma_setflags); + +int s3c2410_dma_trigger(struct s3c2410_dma_client *client) +{ + int ret = 0; + if(!client || !(client->channel < DMACH_MAX)) { + pr_debug("%s: Invalid parameter\n", __func__); + return -EINVAL; + } + + switch(client->event) { + case S3C2410_DMA_TRIGGER_START: + s3c2410_dma_ctrl(client->channel, S3C2410_DMAOP_START); + if(client->gen_request) + ret = client->gen_request(client->private_data); + s3c2410_dma_ctrl(client->channel, S3C2410_DMAOP_STARTED); + return ret; + case S3C2410_DMA_TRIGGER_STOP: + s3c2410_dma_ctrl(client->channel, S3C2410_DMAOP_STOP); + if(client->end_request) + ret = client->end_request(client->private_data); + return ret; + default: + pr_debug("%s: Invalid event\n", __func__); + } + return -EINVAL; +} + +EXPORT_SYMBOL(s3c2410_dma_trigger); +
part 2: Drop s3c2410_dma_ctrl() related calls. These will be called automaticly in s3c2410_dma_trigger().
--- a/sound/soc/s3c24xx/s3c24xx-pcm.c 2009-08-24 13:28:58.000000000 +0800 +++ b/sound/soc/s3c24xx/s3c24xx-pcm.c 2009-08-24 14:46:32.000000000 +0800 @@ -254,15 +254,12 @@ 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); 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); break;
default:
Part 3: S3C ASoC I2S-V2 drivers, used by S3C2412-I2S and S3C64XX-I2S. Add the callback functions for the DMA drivers. These callback functions are registered in the probe phase.
--- a/sound/soc/s3c24xx/s3c-i2s-v2.c 2009-08-24 13:28:58.000000000 +0800 +++ b/sound/soc/s3c24xx/s3c-i2s-v2.c 2009-08-24 15:17:02.000000000 +0800 @@ -53,6 +53,12 @@
#define S3C2412_I2S_DEBUG_CON 0
+struct s3c2412_i2s_trigger_internal_data { + struct snd_pcm_substream *substream; + int cmd; + struct snd_soc_dai *dai; +}; + static inline struct s3c_i2sv2_info *to_info(struct snd_soc_dai *cpu_dai) { return cpu_dai->private_data; @@ -383,14 +389,45 @@ struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct s3c24xx_pcm_dma_params *dma_params = rtd->dai->cpu_dai->dma_data; + struct s3c2410_dma_client *client = dma_params->client; + struct s3c2412_i2s_trigger_internal_data callback_params; + int ret = 0; + + callback_params.substream = substream; + callback_params.cmd = cmd; + callback_params.dai = dai; + switch (cmd) { + case SNDRV_PCM_TRIGGER_START; + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + client->event = S3C2410_DMA_TRIGGER_START;; + break; + + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + client->event = S3C2410_DMA_TRIGGER_STOP; + break; + default: + return -EINVAL; + } + + client->private_data = &callback_params; + return s3c2410_dma_trigger(client); +} + +static int s3c2412_i2s_trigger_internal(struct s3c2412_i2s_trigger_internal_data *data) +{ + struct snd_soc_pcm_runtime *rtd = data->substream->private_data; struct s3c_i2sv2_info *i2s = to_info(rtd->dai->cpu_dai); - int capture = (substream->stream == SNDRV_PCM_STREAM_CAPTURE); + int capture = (data->substream->stream == SNDRV_PCM_STREAM_CAPTURE); unsigned long irqs; int ret = 0;
pr_debug("Entered %s\n", __func__);
- switch (cmd) { + switch (data->cmd) { case SNDRV_PCM_TRIGGER_START: /* On start, ensure that the FIFOs are cleared and reset. */
@@ -627,6 +664,14 @@ s3c2412_snd_txctrl(i2s, 0); s3c2412_snd_rxctrl(i2s, 0);
+ if(i2s->dma_playback && i2s->dma_playback->client) { + i2s->dma_playback->client->gen_request = (void*)s3c2412_i2s_trigger_internal; + i2s->dma_playback->client->end_request = (void*)s3c2412_i2s_trigger_internal; + } + if(i2s->dma_capture && i2s->dma_capture->client) { + i2s->dma_capture->client->gen_request = (void*)s3c2412_i2s_trigger_internal; + i2s->dma_capture->client->end_request = (void*)s3c2412_i2s_trigger_internal; + } return 0; } EXPORT_SYMBOL_GPL(s3c_i2sv2_probe); --- a/sound/soc/s3c24xx/s3c2412-i2s.c 2009-08-24 13:28:58.000000000 +0800 +++ b/sound/soc/s3c24xx/s3c2412-i2s.c 2009-08-24 15:02:32.000000000 +0800 @@ -44,11 +44,16 @@ #define S3C2412_I2S_DEBUG 0
static struct s3c2410_dma_client s3c2412_dma_client_out = { - .name = "I2S PCM Stereo out" + .name = "I2S PCM Stereo out", + .channel = DMACH_I2S_OUT, + /* callback functions will be set in s3c_i2sv2_probe */ + };
static struct s3c2410_dma_client s3c2412_dma_client_in = { - .name = "I2S PCM Stereo in" + .name = "I2S PCM Stereo in", + .channel = DMACH_I2S_IN, + /* callback functions will be set in s3c_i2sv2_probe */ };
static struct s3c24xx_pcm_dma_params s3c2412_i2s_pcm_stereo_out = { @@ -112,13 +117,14 @@
pr_debug("Entered %s\n", __func__);
+ /* s3c_i2sv2_probe will setup dma callback functions for them */ + s3c2412_i2s.dma_capture = &s3c2412_i2s_pcm_stereo_in; + s3c2412_i2s.dma_playback = &s3c2412_i2s_pcm_stereo_out; + ret = s3c_i2sv2_probe(pdev, dai, &s3c2412_i2s, S3C2410_PA_IIS); if (ret) return ret;
- s3c2412_i2s.dma_capture = &s3c2412_i2s_pcm_stereo_in; - s3c2412_i2s.dma_playback = &s3c2412_i2s_pcm_stereo_out; - s3c2412_i2s.iis_cclk = clk_get(&pdev->dev, "i2sclk"); if (s3c2412_i2s.iis_cclk == NULL) { pr_err("failed to get i2sclk clock\n"); --- a/sound/soc/s3c24xx/s3c64xx-i2s.c 2009-08-24 13:28:58.000000000 +0800 +++ b/sound/soc/s3c24xx/s3c64xx-i2s.c 2009-08-24 15:00:24.000000000 +0800 @@ -39,24 +39,42 @@ #include "s3c24xx-pcm.h" #include "s3c64xx-i2s.h"
-static struct s3c2410_dma_client s3c64xx_dma_client_out = { - .name = "I2S PCM Stereo out" +static struct s3c2410_dma_client s3c64xx_dma_client_out_0 = { + .name = "I2S PCM Stereo out #0", + .channel = DMACH_I2S0_OUT, + /* callback functions will be set in s3c_i2sv2_probe */ };
-static struct s3c2410_dma_client s3c64xx_dma_client_in = { - .name = "I2S PCM Stereo in" +static struct s3c2410_dma_client s3c64xx_dma_client_out_1 = { + .name = "I2S PCM Stereo out #1", + .channel = DMACH_I2S1_OUT, + /* callback functions will be set in s3c_i2sv2_probe */ +}; + +static struct s3c2410_dma_client s3c64xx_dma_client_in_0 = { + .name = "I2S PCM Stereo in #0", + .channel = DMACH_I2S0_IN, + /* callback functions will be set in s3c_i2sv2_probe */ + +}; + +static struct s3c2410_dma_client s3c64xx_dma_client_in_1 = { + .name = "I2S PCM Stereo in #1", + .channel = DMACH_I2S1_IN, + /* callback functions will be set in s3c_i2sv2_probe */ + };
static struct s3c24xx_pcm_dma_params s3c64xx_i2s_pcm_stereo_out[2] = { [0] = { .channel = DMACH_I2S0_OUT, - .client = &s3c64xx_dma_client_out, + .client = &s3c64xx_dma_client_out_0, .dma_addr = S3C64XX_PA_IIS0 + S3C2412_IISTXD, .dma_size = 4, }, [1] = { .channel = DMACH_I2S1_OUT, - .client = &s3c64xx_dma_client_out, + .client = &s3c64xx_dma_client_out_1, .dma_addr = S3C64XX_PA_IIS1 + S3C2412_IISTXD, .dma_size = 4, }, @@ -65,13 +83,13 @@ static struct s3c24xx_pcm_dma_params s3c64xx_i2s_pcm_stereo_in[2] = { [0] = { .channel = DMACH_I2S0_IN, - .client = &s3c64xx_dma_client_in, + .client = &s3c64xx_dma_client_in_0, .dma_addr = S3C64XX_PA_IIS0 + S3C2412_IISRXD, .dma_size = 4, }, [1] = { .channel = DMACH_I2S1_IN, - .client = &s3c64xx_dma_client_in, + .client = &s3c64xx_dma_client_in_1, .dma_addr = S3C64XX_PA_IIS1 + S3C2412_IISRXD, .dma_size = 4, },
Part 4: S3C ASoC 24XX-I2S drivers. Add the callback functions for the DMA drivers. These callback functions are determined in the compile time (no need to be registered in the probe phase).
--- a/sound/soc/s3c24xx/s3c24xx-i2s.c 2009-08-24 13:28:58.000000000 +0800 +++ b/sound/soc/s3c24xx/s3c24xx-i2s.c 2009-08-24 15:20:53.000000000 +0800 @@ -41,12 +41,21 @@ #include "s3c24xx-pcm.h" #include "s3c24xx-i2s.h"
+static int s3c24xx_snd_txctrl(int); +static int s3c24xx_snd_rxctrl(int); + static struct s3c2410_dma_client s3c24xx_dma_client_out = { - .name = "I2S PCM Stereo out" + .name = "I2S PCM Stereo out", + .channel = DMACH_I2S_OUT, + .gen_request = (void *)s3c24xx_snd_txctrl, + .end_request = (void *)s3c24xx_snd_txctrl, };
static struct s3c2410_dma_client s3c24xx_dma_client_in = { - .name = "I2S PCM Stereo in" + .name = "I2S PCM Stereo in", + .channel = DMACH_I2S_IN, + .gen_request = (void *)s3c24xx_snd_rxctrl, + .end_request = (void *)s3c24xx_snd_rxctrl, };
static struct s3c24xx_pcm_dma_params s3c24xx_i2s_pcm_stereo_out = { @@ -73,7 +82,7 @@ }; static struct s3c24xx_i2s_info s3c24xx_i2s;
-static void s3c24xx_snd_txctrl(int on) +static int s3c24xx_snd_txctrl(int on) { u32 iisfcon; u32 iiscon; @@ -116,9 +125,10 @@ }
pr_debug("w: IISCON: %x IISMOD: %x IISFCON: %x\n", iiscon, iismod, iisfcon); + return 0; }
-static void s3c24xx_snd_rxctrl(int on) +static int s3c24xx_snd_rxctrl(int on) { u32 iisfcon; u32 iiscon; @@ -161,6 +171,7 @@ }
pr_debug("w: IISCON: %x IISMOD: %x IISFCON: %x\n", iiscon, iismod, iisfcon); + return 0; }
/* @@ -279,6 +290,9 @@ struct snd_soc_dai *dai) { int ret = 0; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct s3c24xx_pcm_dma_params *dma_params = rtd->dai->cpu_dai->dma_data; + struct s3c2410_dma_client *client = dma_params->client;
pr_debug("Entered %s\n", __func__);
@@ -292,24 +306,23 @@ goto exit_err; }
- if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - s3c24xx_snd_rxctrl(1); - else - s3c24xx_snd_txctrl(1); + client->event= S3C2410_DMA_TRIGGER_START; + client->private_data = (void *)1; break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - s3c24xx_snd_rxctrl(0); - else - s3c24xx_snd_txctrl(0); + client->event= S3C2410_DMA_TRIGGER_STOP; + client->private_data = (void *)0; break; + default: ret = -EINVAL; - break; + goto exit_err; }
+ ret = s3c2410_dma_trigger(client); + exit_err: return ret; }
Part 5: S3C ASoC 24XX-AC97 drivers. Add the callback functions for the DMA drivers. These callback functions are determined in the compile time (no need to be registered in the probe phase).
--- a/sound/soc/s3c24xx/s3c2443-ac97.c 2009-08-24 13:28:58.000000000 +0800 +++ b/sound/soc/s3c24xx/s3c2443-ac97.c 2009-08-24 15:13:42.000000000 +0800 @@ -154,6 +154,39 @@ writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); }
+static int s3c2443_ac97_snd_ctrl_on(int stream_direction) +{ + u32 ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + if (stream_direction == SNDRV_PCM_STREAM_CAPTURE) + ac_glbctrl |= S3C_AC97_GLBCTRL_PCMINTM_DMA; + else + ac_glbctrl |= S3C_AC97_GLBCTRL_PCMOUTTM_DMA; + writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + return 0; +} + +static int s3c2443_ac97_snd_ctrl_off(int stream_direction) +{ + u32 ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + if (stream_direction == SNDRV_PCM_STREAM_CAPTURE) + ac_glbctrl &= ~S3C_AC97_GLBCTRL_PCMINTM_MASK; + else + ac_glbctrl &= ~S3C_AC97_GLBCTRL_PCMOUTTM_MASK; + writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + return 0; +} + +static int s3c2443_ac97_mic_ctrl(int cmd) +{ + u32 ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + if(cmd == 1) + ac_glbctrl |= S3C_AC97_GLBCTRL_PCMINTM_DMA; + else + ac_glbctrl &= ~S3C_AC97_GLBCTRL_PCMINTM_MASK; + writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + return 0; +} + static irqreturn_t s3c2443_ac97_irq(int irq, void *dev_id) { int status; @@ -178,15 +211,24 @@ };
static struct s3c2410_dma_client s3c2443_dma_client_out = { - .name = "AC97 PCM Stereo out" + .name = "AC97 PCM Stereo out", + .channel = DMACH_PCM_OUT, + .gen_request = (void *)s3c2443_ac97_snd_ctrl_on, + .end_request = (void *)s3c2443_ac97_snd_ctrl_off, };
static struct s3c2410_dma_client s3c2443_dma_client_in = { - .name = "AC97 PCM Stereo in" + .name = "AC97 PCM Stereo in", + .channel = DMACH_PCM_IN, + .gen_request = (void *)s3c2443_ac97_snd_ctrl_on, + .end_request = (void *)s3c2443_ac97_snd_ctrl_off, };
static struct s3c2410_dma_client s3c2443_dma_client_micin = { - .name = "AC97 Mic Mono in" + .name = "AC97 Mic Mono in", + .channel = DMACH_MIC_IN, + .gen_request = (void *)s3c2443_ac97_mic_ctrl, + .end_request = (void *)s3c2443_ac97_mic_ctrl, };
static struct s3c24xx_pcm_dma_params s3c2443_ac97_pcm_stereo_out = { @@ -289,30 +331,25 @@ static int s3c2443_ac97_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { - u32 ac_glbctrl; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct s3c24xx_pcm_dma_params *dma_params = rtd->dai->cpu_dai->dma_data; + struct s3c2410_dma_client *client = dma_params->client;
- ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); + client->private_data = (void *)substream->stream; switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - ac_glbctrl |= S3C_AC97_GLBCTRL_PCMINTM_DMA; - else - ac_glbctrl |= S3C_AC97_GLBCTRL_PCMOUTTM_DMA; + client->event = S3C2410_DMA_TRIGGER_START; break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - ac_glbctrl &= ~S3C_AC97_GLBCTRL_PCMINTM_MASK; - else - ac_glbctrl &= ~S3C_AC97_GLBCTRL_PCMOUTTM_MASK; + client->event = S3C2410_DMA_TRIGGER_STOP; break; } - writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
- return 0; + return s3c2410_dma_trigger(client); }
static int s3c2443_ac97_hw_mic_params(struct snd_pcm_substream *substream, @@ -333,23 +370,25 @@ static int s3c2443_ac97_mic_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { - u32 ac_glbctrl; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct s3c24xx_pcm_dma_params *dma_params = rtd->dai->cpu_dai->dma_data; + struct s3c2410_dma_client *client = dma_params->client;
- ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL); switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - ac_glbctrl |= S3C_AC97_GLBCTRL_PCMINTM_DMA; + client->private_data = (void *)1; + client->event = S3C2410_DMA_TRIGGER_START; break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - ac_glbctrl &= ~S3C_AC97_GLBCTRL_PCMINTM_MASK; + client->private_data = (void *)0; + client->event = S3C2410_DMA_TRIGGER_STOP; } - writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
- return 0; + return s3c2410_dma_trigger(client); }
#define s3c2443_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
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:
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 thispatch 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.
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:
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.
participants (2)
-
Mark Brown -
Shine Liu