On Thursday 01 May 2014 12:11:25 Mark Brown wrote:

On Tue, Apr 29, 2014 at 07:18:34PM +0800, Xia Kaixu wrote:

...

From: Arnd Bergmann arnd@arndb.de

SND_S3C_DMA_LEGACY can only be set on S3C24xx, which does not (yet) support the dmaengine framework, so samsung_dma_get_ops() fails to link if S3C24XX_DMA is disabled:

Why is the fix for this not to ensure that s3c24xx always enables the DMA controller - how likely is it that it would be sane to build a kernel without DMA after all?

S3C24XX_DMA is only needed for ASoC and for s3cmci. The latter uses 'depends on S3C24XX_DMA'. arch/arm/configs/tct_hammer_defconfig is an example of a configuration that does not include DMA because it uses neither of the two drivers.

On a related topic, I looked at the overall DMA situation for plat-samsung again now, and it looks much nicer now than it used to, with mach-s3c64xx fully migrated to dmaengine, so there might be an even better way to deal with this.

From all I can tell, sound/soc/samsung is the only remaining user

of the plat/dma.h interfaces, and there is only one other driver using the s3c24xx_dma interface directly, drivers/mmc/host/s3cmci.c.

How about the patch below? 8<------- ASoC: s3c24xx: use legacy DMA interface directly

The samsung platform code has a wrapper around the legacy s3c24xx DMA API as an alternative to the dmaengine API. This is only used by the legacy s3c24xx sound support, which is never used in combination with the dmaengine API.

We can simplify the ASoC code significantly, and thereby completely obsoleting arch/arm/plat-samsung/dma-ops.c, arch/arm/plat-samsung/s3c-dma-ops.c and arch/arm/plat-samsung/include/plat/dma-ops.h.

This patch achieves this by open-coding the implementation of this wrapper, which ends up saving us more code than we have to add. s3c24xx does not support cyclic DMA, so anything referring to that gets deleted in the process.

The plat-samsung dma wrapper code is now dead code and can be removed subsequently.

Signed-off--by: Arnd Bergmann arnd@arndb.de ---

sound/soc/samsung/ac97.c | 10 ++---- sound/soc/samsung/dma.c | 79 +++++++++++++++--------------------------------- sound/soc/samsung/dma.h | 2 -- 3 files changed, 27 insertions(+), 64 deletions(-)

diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c index 76b072b..28fe097 100644 --- a/sound/soc/samsung/ac97.c +++ b/sound/soc/samsung/ac97.c @@ -253,10 +253,7 @@ static int s3c_ac97_trigger(struct snd_pcm_substream *substream, int cmd,

writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);

- if (!dma_data->ops) - dma_data->ops = samsung_dma_get_ops(); - - dma_data->ops->started(dma_data->channel); + s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED);

return 0; } @@ -287,10 +284,7 @@ static int s3c_ac97_mic_trigger(struct snd_pcm_substream *substream,

writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);

- if (!dma_data->ops) - dma_data->ops = samsung_dma_get_ops(); - - dma_data->ops->started(dma_data->channel); + s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED);

return 0; } diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c index dc09b71..3c6e2f2 100644 --- a/sound/soc/samsung/dma.c +++ b/sound/soc/samsung/dma.c @@ -54,8 +54,6 @@ struct runtime_data { struct s3c_dma_params *params; };

-static void audio_buffdone(void *data); - /* dma_enqueue * * place a dma buffer onto the queue for the dma system @@ -66,7 +64,6 @@ static void dma_enqueue(struct snd_pcm_substream *substream) struct runtime_data *prtd = substream->runtime->private_data; dma_addr_t pos = prtd->dma_pos; unsigned int limit; - struct samsung_dma_prep dma_info;

pr_debug("Entered %s\n", __func__);

@@ -75,33 +72,11 @@ static void dma_enqueue(struct snd_pcm_substream *substream) pr_debug("%s: loaded %d, limit %d\n", __func__, prtd->dma_loaded, limit);

- dma_info.cap = (samsung_dma_has_circular() ? DMA_CYCLIC : DMA_SLAVE); - dma_info.direction = - (substream->stream == SNDRV_PCM_STREAM_PLAYBACK - ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM); - dma_info.fp = audio_buffdone; - dma_info.fp_param = substream; - dma_info.period = prtd->dma_period; - dma_info.len = prtd->dma_period*limit; - - if (dma_info.cap == DMA_CYCLIC) { - dma_info.buf = pos; - prtd->params->ops->prepare(prtd->params->ch, &dma_info); - prtd->dma_loaded += limit; - return; - } - while (prtd->dma_loaded < limit) { pr_debug("dma_loaded: %d\n", prtd->dma_loaded);

- if ((pos + dma_info.period) > prtd->dma_end) { - dma_info.period = prtd->dma_end - pos; - pr_debug("%s: corrected dma len %ld\n", - __func__, dma_info.period); - } - - dma_info.buf = pos; - prtd->params->ops->prepare(prtd->params->ch, &dma_info); + s3c2410_dma_enqueue(prtd->params->channel, substream, pos, + prtd->dma_period * limit);

prtd->dma_loaded++; pos += prtd->dma_period; @@ -112,7 +87,8 @@ static void dma_enqueue(struct snd_pcm_substream *substream) prtd->dma_pos = pos; }

-static void audio_buffdone(void *data) +static void audio_buffdone(struct s3c2410_dma_chan *channel, void *data, + int size, enum s3c2410_dma_buffresult res) { struct snd_pcm_substream *substream = data; struct runtime_data *prtd = substream->runtime->private_data; @@ -128,10 +104,8 @@ static void audio_buffdone(void *data) snd_pcm_period_elapsed(substream);

spin_lock(&prtd->lock); - if (!samsung_dma_has_circular()) { - prtd->dma_loaded--; - dma_enqueue(substream); - } + prtd->dma_loaded--; + dma_enqueue(substream); spin_unlock(&prtd->lock); } } @@ -145,8 +119,7 @@ static int dma_hw_params(struct snd_pcm_substream *substream, unsigned long totbytes = params_buffer_bytes(params); struct s3c_dma_params *dma = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); - struct samsung_dma_req req; - struct samsung_dma_config config; + enum dma_transfer_direction direction;

pr_debug("Entered %s\n", __func__);

@@ -164,24 +137,22 @@ static int dma_hw_params(struct snd_pcm_substream *substream, pr_debug("params %p, client %p, channel %d\n", prtd->params, prtd->params->client, prtd->params->channel);

- prtd->params->ops = samsung_dma_get_ops(); - - req.cap = (samsung_dma_has_circular() ? - DMA_CYCLIC : DMA_SLAVE); - req.client = prtd->params->client; - config.direction = - (substream->stream == SNDRV_PCM_STREAM_PLAYBACK + direction = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM); - config.width = prtd->params->dma_size; - config.fifo = prtd->params->dma_addr; - prtd->params->ch = prtd->params->ops->request( - prtd->params->channel, &req, rtd->cpu_dai->dev, - prtd->params->ch_name); - if (!prtd->params->ch) { + + if (s3c2410_dma_request(prtd->params->channel, + (void*)prtd->params->client, NULL) < 0) { pr_err("Failed to allocate DMA channel\n"); + s3c2410_dma_free(prtd->params->channel, + (void*)prtd->params->client); return -ENXIO; } - prtd->params->ops->config(prtd->params->ch, &config); + s3c2410_dma_devconfig(prtd->params->channel, direction, + prtd->params->dma_addr); + s3c2410_dma_config(prtd->params->channel, + prtd->params->dma_size); + s3c2410_dma_set_buffdone_fn(prtd->params->channel, + audio_buffdone); }

snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); @@ -208,9 +179,9 @@ static int dma_hw_free(struct snd_pcm_substream *substream) snd_pcm_set_runtime_buffer(substream, NULL);

if (prtd->params) { - prtd->params->ops->flush(prtd->params->ch); - prtd->params->ops->release(prtd->params->ch, - prtd->params->client); + s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_FLUSH); + s3c2410_dma_free(prtd->params->channel, + (void*)prtd->params->client); prtd->params = NULL; }

@@ -230,7 +201,7 @@ static int dma_prepare(struct snd_pcm_substream *substream) return 0;

/* flush the DMA channel */ - prtd->params->ops->flush(prtd->params->ch); + s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_FLUSH);

prtd->dma_loaded = 0; prtd->dma_pos = prtd->dma_start; @@ -253,12 +224,12 @@ static int dma_trigger(struct snd_pcm_substream *substream, int cmd) switch (cmd) { case SNDRV_PCM_TRIGGER_START: prtd->state |= ST_RUNNING; - prtd->params->ops->trigger(prtd->params->ch); + s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_START); break;

case SNDRV_PCM_TRIGGER_STOP: prtd->state &= ~ST_RUNNING; - prtd->params->ops->stop(prtd->params->ch); + s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STOP); break;

default: diff --git a/sound/soc/samsung/dma.h b/sound/soc/samsung/dma.h index ad7c0f0..c52fa86 100644 --- a/sound/soc/samsung/dma.h +++ b/sound/soc/samsung/dma.h @@ -23,8 +23,6 @@ struct s3c_dma_params { int channel; /* Channel ID */ dma_addr_t dma_addr; int dma_size; /* Size of the DMA transfer */ - unsigned ch; - struct samsung_dma_ops *ops; char *ch_name; struct snd_dmaengine_dai_dma_data dma_data; };

On Friday 02 May 2014 12:25:58 Heiko Stübner wrote:

Just to mention, we have a dmaengine driver for s3c24xx . Correct platform-data is present for all s3c24xx socs (in mach-s3c24xx/common.c) .

Mark already removed support for the legacy API from the s3c64xx spi driver (used by s3c2416 and s3c2443), so I guess to way forward would be to "simply" convert asoc and s3cmci to dmaengine and get rid of it altogether.

For asoc, this is probably very simple: the dma.c gets replaced with the dmaengine.c, and the three drivers calling s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED) get changed to do something equivalent.

I just never had hardware using the old mci driver or with any previously working sound and didn't trust my experience to be enough to be able to do such a conversion "on the fly" like you below in the opposite direction

What I did was much easier: the dma.c file was already using a wrapper around both interfaces, I just unwrapped it. For mci, you'd have to replace one API with another incompatible one. I've tried a conversion in the patch below, but I also don't enough confidence in this to submit it.

What is interesting however is that none of the three defconfigs we have for S3C24xx actually uses DMA wiht MMC_S3C: tct_hammer doesn't have any MMC support, and mini2440_defonfig as well as s3c2410_defconfig enable the MMC driver in PIO-only mode.

When Ben Dooks did all the changes to the DMA code in that driver, but he also marked DMA support as 'EXPERIMENTAL'. Maybe we can just get away with deprecating that?

Arnd

8<------ Subject: convert s3cmci to use dmaengine

This is an attempt to migrate s3cmci from the samsung-proprietary DMA interface to the generic dmaengine API. I don't really understand what I'm doing here, and this is not tested at all. Somebody who has the hardware and understands this code can probably get it to work, but I would expect a couple of small bugs to get introduced.

Signed-off-by: Arnd Bergmann arnd@arndb.de

diff --git a/drivers/mmc/host/s3cmci.c b/drivers/mmc/host/s3cmci.c index f237826..02847d3 100644 --- a/drivers/mmc/host/s3cmci.c +++ b/drivers/mmc/host/s3cmci.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/dma-mapping.h> #include <linux/clk.h> +#include <linux/dmaengine.h> #include <linux/mmc/host.h> #include <linux/platform_device.h> #include <linux/cpufreq.h> @@ -28,6 +29,7 @@ #include <mach/gpio-samsung.h>

#include <linux/platform_data/mmc-s3cmci.h> +#include <linux/platform_data/dma-s3c24xx.h>

#include "s3cmci.h"

@@ -140,10 +142,6 @@ static const int dbgmap_debug = dbg_err | dbg_debug; dev_dbg(&host->pdev->dev, args); \ } while (0)

-static struct s3c2410_dma_client s3cmci_dma_client = { - .name = "s3c-mci", -}; - static void finalize_request(struct s3cmci_host *host); static void s3cmci_send_request(struct mmc_host *mmc); static void s3cmci_reset(struct s3cmci_host *host); @@ -841,9 +839,7 @@ static irqreturn_t s3cmci_irq_cd(int irq, void *dev_id) return IRQ_HANDLED; }

-static void s3cmci_dma_done_callback(struct s3c2410_dma_chan *dma_ch, - void *buf_id, int size, - enum s3c2410_dma_buffresult result) +static void s3cmci_dma_done_callback(void *buf_id) { struct s3cmci_host *host = buf_id; unsigned long iflags; @@ -856,45 +852,18 @@ static void s3cmci_dma_done_callback(struct s3c2410_dma_chan *dma_ch,

BUG_ON(!host->mrq); BUG_ON(!host->mrq->data); - BUG_ON(!host->dmatogo);

spin_lock_irqsave(&host->complete_lock, iflags);

- if (result != S3C2410_RES_OK) { - dbg(host, dbg_fail, "DMA FAILED: csta=0x%08x dsta=0x%08x " - "fsta=0x%08x dcnt:0x%08x result:0x%08x toGo:%u\n", - mci_csta, mci_dsta, mci_fsta, - mci_dcnt, result, host->dmatogo); - - goto fail_request; - } - - host->dmatogo--; - if (host->dmatogo) { - dbg(host, dbg_dma, "DMA DONE Size:%i DSTA:[%08x] " - "DCNT:[%08x] toGo:%u\n", - size, mci_dsta, mci_dcnt, host->dmatogo); - - goto out; - } - - dbg(host, dbg_dma, "DMA FINISHED Size:%i DSTA:%08x DCNT:%08x\n", - size, mci_dsta, mci_dcnt); + dbg(host, dbg_dma, "DMA FINISHED DSTA:%08x DCNT:%08x\n", + mci_dsta, mci_dcnt);

host->dma_complete = 1; host->complete_what = COMPLETION_FINALIZE;

-out: tasklet_schedule(&host->pio_tasklet); spin_unlock_irqrestore(&host->complete_lock, iflags); return; - -fail_request: - host->mrq->data->error = -EINVAL; - host->complete_what = COMPLETION_FINALIZE; - clear_imask(host); - - goto out; }

static void finalize_request(struct s3cmci_host *host) @@ -966,7 +935,7 @@ static void finalize_request(struct s3cmci_host *host) * DMA channel and the fifo to clear out any garbage. */ if (mrq->data->error != 0) { if (s3cmci_host_usedma(host)) - s3c2410_dma_ctrl(host->dma, S3C2410_DMAOP_FLUSH); + dmaengine_terminate_all(host->dma);

if (host->is2440) { /* Clear failure register and reset fifo. */ @@ -993,26 +962,27 @@ request_done: }

static void s3cmci_dma_setup(struct s3cmci_host *host, - enum dma_data_direction source) + enum dma_transfer_direction source) { - static enum dma_data_direction last_source = -1; - static int setup_ok; + static enum dma_transfer_direction last_source = -1; + struct dma_slave_config slave_config;

if (last_source == source) return;

- last_source = source; - - s3c2410_dma_devconfig(host->dma, source, - host->mem->start + host->sdidata); - - if (!setup_ok) { - s3c2410_dma_config(host->dma, 4); - s3c2410_dma_set_buffdone_fn(host->dma, - s3cmci_dma_done_callback); - s3c2410_dma_setflags(host->dma, S3C2410_DMAF_AUTOSTART); - setup_ok = 1; + memset(&slave_config, 0, sizeof(struct dma_slave_config)); + slave_config.direction = source; + if (source == DMA_DEV_TO_MEM) { + slave_config.src_addr = host->mem->start + host->sdidata; + slave_config.src_addr_width = 4; + slave_config.src_maxburst = 1; + } else { + slave_config.dst_addr = host->mem->start + host->sdidata; + slave_config.dst_addr_width = 4; + slave_config.dst_maxburst = 1; } + last_source = source; + dmaengine_slave_config(host->dma, &slave_config); }

static void s3cmci_send_command(struct s3cmci_host *host, @@ -1162,41 +1132,33 @@ static int s3cmci_prepare_pio(struct s3cmci_host *host, struct mmc_data *data)

static int s3cmci_prepare_dma(struct s3cmci_host *host, struct mmc_data *data) { - int dma_len, i; - int rw = data->flags & MMC_DATA_WRITE; + int dma_len; + int dir = data->flags & MMC_DATA_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE; + struct dma_async_tx_descriptor *desc;

BUG_ON((data->flags & BOTH_DIR) == BOTH_DIR);

- s3cmci_dma_setup(host, rw ? DMA_TO_DEVICE : DMA_FROM_DEVICE); - s3c2410_dma_ctrl(host->dma, S3C2410_DMAOP_FLUSH); - - dma_len = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len, - rw ? DMA_TO_DEVICE : DMA_FROM_DEVICE); + s3cmci_dma_setup(host, dir);

+ dma_len = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len, dir); if (dma_len == 0) return -ENOMEM;

- host->dma_complete = 0; - host->dmatogo = dma_len; - - for (i = 0; i < dma_len; i++) { - int res; + desc = dmaengine_prep_slave_sg(host->dma, data->sg, dma_len, + dir, DMA_CTRL_ACK);

- dbg(host, dbg_dma, "enqueue %i: %08x@%u\n", i, - sg_dma_address(&data->sg[i]), - sg_dma_len(&data->sg[i])); + if (!desc) { + dma_unmap_sg(mmc_dev(host->mmc), data->sg, dma_len, dir); + return -EIO; + }

- res = s3c2410_dma_enqueue(host->dma, host, - sg_dma_address(&data->sg[i]), - sg_dma_len(&data->sg[i])); + host->dma_complete = 0;

- if (res) { - s3c2410_dma_ctrl(host->dma, S3C2410_DMAOP_FLUSH); - return -EBUSY; - } - } + desc->callback = s3cmci_dma_done_callback; + desc->callback_param = host;

- s3c2410_dma_ctrl(host->dma, S3C2410_DMAOP_START); + dmaengine_submit(desc); + dma_async_issue_pending(host->dma);

return 0; } @@ -1765,9 +1727,14 @@ static int s3cmci_probe(struct platform_device *pdev) /* depending on the dma state, get a dma channel to use. */

if (s3cmci_host_usedma(host)) { - host->dma = s3c2410_dma_request(DMACH_SDI, &s3cmci_dma_client, - host); - if (host->dma < 0) { + dma_cap_mask_t mask; + + dma_cap_zero(mask); + dma_cap_set(DMA_SLAVE, mask); + + host->dma = dma_request_channel(mask, s3c24xx_dma_filter, (void *)DMACH_SDI); + + if (!host->dma) { dev_err(&pdev->dev, "cannot get DMA channel.\n"); if (!s3cmci_host_canpio()) { ret = -EBUSY; @@ -1816,9 +1783,9 @@ static int s3cmci_probe(struct platform_device *pdev) mmc->max_segs = 128;

dbg(host, dbg_debug, - "probe: mode:%s mapped mci_base:%p irq:%u irq_cd:%u dma:%u.\n", + "probe: mode:%s mapped mci_base:%p irq:%u irq_cd:%u.\n", (host->is2440?"2440":""), - host->base, host->irq, host->irq_cd, host->dma); + host->base, host->irq, host->irq_cd);

ret = s3cmci_cpufreq_register(host); if (ret) { @@ -1852,7 +1819,7 @@ static int s3cmci_probe(struct platform_device *pdev)

probe_free_dma: if (s3cmci_host_usedma(host)) - s3c2410_dma_free(host->dma, &s3cmci_dma_client); + dma_release_channel(host->dma);

probe_free_gpio_wp: if (!host->pdata->no_wprotect) @@ -1914,7 +1881,7 @@ static int s3cmci_remove(struct platform_device *pdev) tasklet_disable(&host->pio_tasklet);

if (s3cmci_host_usedma(host)) - s3c2410_dma_free(host->dma, &s3cmci_dma_client); + dma_release_channel(host->dma);

free_irq(host->irq, host);

On Fri, May 02, 2014 at 12:35:21AM +0200, Arnd Bergmann wrote:

On Thursday 01 May 2014 12:11:25 Mark Brown wrote:

...

Why is the fix for this not to ensure that s3c24xx always enables the DMA controller - how likely is it that it would be sane to build a kernel without DMA after all?

S3C24XX_DMA is only needed for ASoC and for s3cmci. The latter uses 'depends on S3C24XX_DMA'. arch/arm/configs/tct_hammer_defconfig is an example of a configuration that does not include DMA because it uses neither of the two drivers.

I'm having a hard time caring about that, the arch code can always do an "if whatever". If that board is what I think it actually does have audio on it, either the defconfig was never updated or the driver wasn't merged into mainline.

How about the patch below?

I guess (modulo the issues below). Killing the wrapper and making the existing selection of DMA support actually select DMA support does avoid the redundancy.

diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c index 76b072b..28fe097 100644 --- a/sound/soc/samsung/ac97.c +++ b/sound/soc/samsung/ac97.c @@ -253,10 +253,7 @@ static int s3c_ac97_trigger(struct snd_pcm_substream *substream, int cmd,

writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);

• if (!dma_data->ops)

• dma_data->ops = samsung_dma_get_ops();
  

• dma_data->ops->started(dma_data->channel);

• s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED);

  return 0;

}

This code is broken in general - at least s3c64xx has AC'97. That's broken anyway though.