| Message ID | 22569458.nE7JkNNnz3@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/05/2015 11:39 PM, Arnd Bergmann wrote: > As discussed on the topic of shmobile DMA today, jz4740 is the only > user of the slave_id field in dma_slave_config besides shmobile. This > use is really incompatible with the way that other drivers use the > dmaengine API, so we should get rid of it. Do you have a link to that discussion? > > This adds a trivial filter function that uses the filter param to > pass the dma type, and uses that in both drivers. In my opinion that's just from bad to worse. Using filter functions isn't that great in the first place. And using them to pass data from the consumer to the DMA provider is just a horrible abuse of the API. The patch also adds a platform dependency, so the drivers won't built with COMPILE_TEST anymore. - Lars
Lars-Peter Clausen <lars@metafoo.de> writes: > On 01/05/2015 11:39 PM, Arnd Bergmann wrote: >> As discussed on the topic of shmobile DMA today, jz4740 is the only >> user of the slave_id field in dma_slave_config besides shmobile. This >> use is really incompatible with the way that other drivers use the >> dmaengine API, so we should get rid of it. > > Do you have a link to that discussion? > >> >> This adds a trivial filter function that uses the filter param to >> pass the dma type, and uses that in both drivers. > > In my opinion that's just from bad to worse. Using filter functions > isn't that great in the first place. And using them to pass data from > the consumer to the DMA provider is just a horrible abuse of the API. It seems to me the only sane way to use the dmaengine API is in conjunction with DT.
On 01/06/2015 01:47 PM, Måns Rullgård wrote: > Lars-Peter Clausen <lars@metafoo.de> writes: > >> On 01/05/2015 11:39 PM, Arnd Bergmann wrote: >>> As discussed on the topic of shmobile DMA today, jz4740 is the only >>> user of the slave_id field in dma_slave_config besides shmobile. This >>> use is really incompatible with the way that other drivers use the >>> dmaengine API, so we should get rid of it. >> >> Do you have a link to that discussion? >> >>> >>> This adds a trivial filter function that uses the filter param to >>> pass the dma type, and uses that in both drivers. >> >> In my opinion that's just from bad to worse. Using filter functions >> isn't that great in the first place. And using them to pass data from >> the consumer to the DMA provider is just a horrible abuse of the API. > > It seems to me the only sane way to use the dmaengine API is in > conjunction with DT. At the moment yes. For non DT we need something like the gpiod lookup tables that allow you to specify the assignment of the DMA channel in the machine driver. - Lars
On Tuesday 06 January 2015 11:45:58 Lars-Peter Clausen wrote: > On 01/05/2015 11:39 PM, Arnd Bergmann wrote: > > As discussed on the topic of shmobile DMA today, jz4740 is the only > > user of the slave_id field in dma_slave_config besides shmobile. This > > use is really incompatible with the way that other drivers use the > > dmaengine API, so we should get rid of it. > > Do you have a link to that discussion? http://www.spinics.net/lists/linux-mmc/msg30069.html > > This adds a trivial filter function that uses the filter param to > > pass the dma type, and uses that in both drivers. > > In my opinion that's just from bad to worse. Using filter functions isn't > that great in the first place. And using them to pass data from the consumer > to the DMA provider is just a horrible abuse of the API. I agree that it is quite horrible, but it is currently the only portable way to use the API without DT. The solution to this is to use the dma_request_slave_channel API, which was intentionally done in a way to allow extending it in a nice way by passing just "rx" and "tx" (or whichever string you choose) into the API and get back a channel that was connected to the device by the platform. This already works with all DT based systems and (to a limited extent) with ACPI, but nobody so far has implemented it for devices that are instantiated through board files. Anybody on ARM and PowerPC that is cleaning up their platform moves to DT, so there hasn't been a need for that so far. My patch is the simplest workaround, to make jz4740 work like all (legacy) ARM platforms. If you plan to use DT for jz4740 in the long run, that would solve it nicely, and as an alternative, one could implement the equivalent of clk_register_clkdevs/regulator_consumer_supplies/... and hook that up into dma_request_slave_channel_reason() as a third option. > The patch also adds a platform dependency, so the drivers won't built with > COMPILE_TEST anymore. That would be fixed by using proper platform_data to pass the filter and arguments, which I hinted in my patch description. This is how drivers are normally connected to a DMA engine. Arnd
On 01/06/2015 02:48 PM, Arnd Bergmann wrote: > On Tuesday 06 January 2015 11:45:58 Lars-Peter Clausen wrote: >> On 01/05/2015 11:39 PM, Arnd Bergmann wrote: >>> As discussed on the topic of shmobile DMA today, jz4740 is the only >>> user of the slave_id field in dma_slave_config besides shmobile. This >>> use is really incompatible with the way that other drivers use the >>> dmaengine API, so we should get rid of it. >> >> Do you have a link to that discussion? > > http://www.spinics.net/lists/linux-mmc/msg30069.html I'm really not comfortable with this patch, since it is a step back. But I guess the end justify the means. So if it helps to get rid of slave_id I'm ok with it, sooner or later jz4740 will be converted to DT so once that's done the filter function can be removed again. But please put the filter function in a non arch specific header so we can still compile test things. And maybe note in the commit message that this is meant as a temporary hack. - Lars
On Wednesday 07 January 2015 15:29:36 Lars-Peter Clausen wrote: > On 01/06/2015 02:48 PM, Arnd Bergmann wrote: > > On Tuesday 06 January 2015 11:45:58 Lars-Peter Clausen wrote: > >> On 01/05/2015 11:39 PM, Arnd Bergmann wrote: > >>> As discussed on the topic of shmobile DMA today, jz4740 is the only > >>> user of the slave_id field in dma_slave_config besides shmobile. This > >>> use is really incompatible with the way that other drivers use the > >>> dmaengine API, so we should get rid of it. > >> > >> Do you have a link to that discussion? > > > > http://www.spinics.net/lists/linux-mmc/msg30069.html > > I'm really not comfortable with this patch, since it is a step back. But I > guess the end justify the means. So if it helps to get rid of slave_id I'm > ok with it, sooner or later jz4740 will be converted to DT so once that's > done the filter function can be removed again. But please put the filter > function in a non arch specific header so we can still compile test things. > And maybe note in the commit message that this is meant as a temporary hack. > Do you have a timeline for DT support? Maybe it's easier to just wait for the problem to solve itself. Arnd
On 01/07/2015 05:13 PM, Arnd Bergmann wrote: > On Wednesday 07 January 2015 15:29:36 Lars-Peter Clausen wrote: >> On 01/06/2015 02:48 PM, Arnd Bergmann wrote: >>> On Tuesday 06 January 2015 11:45:58 Lars-Peter Clausen wrote: >>>> On 01/05/2015 11:39 PM, Arnd Bergmann wrote: >>>>> As discussed on the topic of shmobile DMA today, jz4740 is the only >>>>> user of the slave_id field in dma_slave_config besides shmobile. This >>>>> use is really incompatible with the way that other drivers use the >>>>> dmaengine API, so we should get rid of it. >>>> >>>> Do you have a link to that discussion? >>> >>> http://www.spinics.net/lists/linux-mmc/msg30069.html >> >> I'm really not comfortable with this patch, since it is a step back. But I >> guess the end justify the means. So if it helps to get rid of slave_id I'm >> ok with it, sooner or later jz4740 will be converted to DT so once that's >> done the filter function can be removed again. But please put the filter >> function in a non arch specific header so we can still compile test things. >> And maybe note in the commit message that this is meant as a temporary hack. >> > > Do you have a timeline for DT support? Maybe it's easier to just > wait for the problem to solve itself. 2 to 3 upstream releases. I have the code and it is working, but the migration path has a lot of inter dependencies between different frameworks, so it is going to take a while until all is upstream. - Lars
diff --git a/arch/mips/include/asm/mach-jz4740/dma.h b/arch/mips/include/asm/mach-jz4740/dma.h index 14ecc5313d2d..ece7e39cdacb 100644 --- a/arch/mips/include/asm/mach-jz4740/dma.h +++ b/arch/mips/include/asm/mach-jz4740/dma.h @@ -16,6 +16,13 @@ #ifndef __ASM_MACH_JZ4740_DMA_H__ #define __ASM_MACH_JZ4740_DMA_H__ +#include <linux/dmaengine.h> + +static inline bool jz4740_dma_filter_fn(struct dma_chan *chan, void *filter_param) +{ + chan->private = filter_param; +} + enum jz4740_dma_request_type { JZ4740_DMA_TYPE_AUTO_REQUEST = 8, JZ4740_DMA_TYPE_UART_TRANSMIT = 20, diff --git a/drivers/dma/dma-jz4740.c b/drivers/dma/dma-jz4740.c index bdeafeefa5f6..f02e60e8e5cc 100644 --- a/drivers/dma/dma-jz4740.c +++ b/drivers/dma/dma-jz4740.c @@ -119,6 +119,7 @@ struct jz4740_dma_desc { struct jz4740_dmaengine_chan { struct virt_dma_chan vchan; unsigned int id; + unsigned int slave; dma_addr_t fifo_addr; unsigned int transfer_shift; @@ -220,6 +221,11 @@ static int jz4740_dma_slave_config(struct dma_chan *c, enum jz4740_dma_flags flags; uint32_t cmd; + /* deprecated: all drivers should request the channel with + * the correct slave argument */ + if (!chan->slave) + chan->slave = config->slave_id; + switch (config->direction) { case DMA_MEM_TO_DEV: flags = JZ4740_DMA_SRC_AUTOINC; @@ -265,8 +271,7 @@ static int jz4740_dma_slave_config(struct dma_chan *c, jz4740_dma_write(dmadev, JZ_REG_DMA_CMD(chan->id), cmd); jz4740_dma_write(dmadev, JZ_REG_DMA_STATUS_CTRL(chan->id), 0); - jz4740_dma_write(dmadev, JZ_REG_DMA_REQ_TYPE(chan->id), - config->slave_id); + jz4740_dma_write(dmadev, JZ_REG_DMA_REQ_TYPE(chan->id), chan->slave); return 0; } @@ -513,6 +518,8 @@ static enum dma_status jz4740_dma_tx_status(struct dma_chan *c, static int jz4740_dma_alloc_chan_resources(struct dma_chan *c) { + struct jz4740_dmaengine_chan *chan = to_jz4740_dma_chan(c); + chan->slave = (unsigned long)c->private; return 0; } diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c index 76e8bce6f46e..a6680959ce83 100644 --- a/drivers/mmc/host/jz4740_mmc.c +++ b/drivers/mmc/host/jz4740_mmc.c @@ -178,13 +178,15 @@ static int jz4740_mmc_acquire_dma_channels(struct jz4740_mmc_host *host) dma_cap_zero(mask); dma_cap_set(DMA_SLAVE, mask); - host->dma_tx = dma_request_channel(mask, NULL, host); + host->dma_tx = dma_request_channel(mask, jz4740_dma_filter_fn, + (void*)JZ4740_DMA_TYPE_MMC_TRANSMIT); if (!host->dma_tx) { dev_err(mmc_dev(host->mmc), "Failed to get dma_tx channel\n"); return -ENODEV; } - host->dma_rx = dma_request_channel(mask, NULL, host); + host->dma_rx = dma_request_channel(mask, jz4740_dma_filter_fn, + (void*)JZ4740_DMA_TYPE_MMC_RECEIVE); if (!host->dma_rx) { dev_err(mmc_dev(host->mmc), "Failed to get dma_rx channel\n"); goto free_master_write; @@ -283,12 +285,10 @@ static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host, if (data->flags & MMC_DATA_WRITE) { conf.direction = DMA_MEM_TO_DEV; conf.dst_addr = host->mem_res->start + JZ_REG_MMC_TXFIFO; - conf.slave_id = JZ4740_DMA_TYPE_MMC_TRANSMIT; chan = host->dma_tx; } else { conf.direction = DMA_DEV_TO_MEM; conf.src_addr = host->mem_res->start + JZ_REG_MMC_RXFIFO; - conf.slave_id = JZ4740_DMA_TYPE_MMC_RECEIVE; chan = host->dma_rx; } diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c index d3d45c6f064f..7a60c075bd81 100644 --- a/sound/soc/jz4740/jz4740-i2s.c +++ b/sound/soc/jz4740/jz4740-i2s.c @@ -31,10 +31,12 @@ #include <sound/initval.h> #include <sound/dmaengine_pcm.h> +#include <asm/mach-jz4740/dma.h> + #include "jz4740-i2s.h" -#define JZ4740_DMA_TYPE_AIC_TRANSMIT 24 -#define JZ4740_DMA_TYPE_AIC_RECEIVE 25 +#define JZ4740_DMA_TYPE_AIC_TRANSMIT (void *)24ul +#define JZ4740_DMA_TYPE_AIC_RECEIVE (void *)25ul #define JZ_REG_AIC_CONF 0x00 #define JZ_REG_AIC_CTRL 0x04 @@ -337,13 +339,13 @@ static void jz4740_i2c_init_pcm_config(struct jz4740_i2s *i2s) /* Playback */ dma_data = &i2s->playback_dma_data; dma_data->maxburst = 16; - dma_data->slave_id = JZ4740_DMA_TYPE_AIC_TRANSMIT; + dma_data->filter_data = JZ4740_DMA_TYPE_AIC_TRANSMIT; dma_data->addr = i2s->phys_base + JZ_REG_AIC_FIFO; /* Capture */ dma_data = &i2s->capture_dma_data; dma_data->maxburst = 16; - dma_data->slave_id = JZ4740_DMA_TYPE_AIC_RECEIVE; + dma_data->filter_data = JZ4740_DMA_TYPE_AIC_RECEIVE; dma_data->addr = i2s->phys_base + JZ_REG_AIC_FIFO; } @@ -415,6 +417,10 @@ static const struct snd_soc_component_driver jz4740_i2s_component = { .name = "jz4740-i2s", }; +static struct snd_dmaengine_pcm_config jz4740_i2s_pcm_config = { + .filter_fn = jz4740_dma_filter_fn, +}; + static int jz4740_i2s_dev_probe(struct platform_device *pdev) { struct jz4740_i2s *i2s; @@ -447,8 +453,8 @@ static int jz4740_i2s_dev_probe(struct platform_device *pdev) if (ret) return ret; - return devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, - SND_DMAENGINE_PCM_FLAG_COMPAT); + return devm_snd_dmaengine_pcm_register(&pdev->dev, + jz4740_i2s_pcm_config, SND_DMAENGINE_PCM_FLAG_COMPAT); } static struct platform_driver jz4740_i2s_driver = {
As discussed on the topic of shmobile DMA today, jz4740 is the only user of the slave_id field in dma_slave_config besides shmobile. This use is really incompatible with the way that other drivers use the dmaengine API, so we should get rid of it. This adds a trivial filter function that uses the filter param to pass the dma type, and uses that in both drivers. With this patch and the respective shmobile change, we can remove the slave_id field from dma_slave_config. To make the drivers more generic, another patch would be required that passes the filter function and the argument through platform_data, but that can be done later. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/mips/include/asm/mach-jz4740/dma.h | 7 +++++++ drivers/dma/dma-jz4740.c | 11 +++++++++-- drivers/mmc/host/jz4740_mmc.c | 8 ++++---- sound/soc/jz4740/jz4740-i2s.c | 18 ++++++++++++------ 4 files changed, 32 insertions(+), 12 deletions(-) completely untested