diff mbox

[RFC] MIPS: jz4740: use dma filter function

Message ID 22569458.nE7JkNNnz3@wuerfel (mailing list archive)
State RFC
Headers show

Commit Message

Arnd Bergmann Jan. 5, 2015, 10:39 p.m. UTC
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


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

Comments

Lars-Peter Clausen Jan. 6, 2015, 10:45 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Måns Rullgård Jan. 6, 2015, 12:47 p.m. UTC | #2
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.
Lars-Peter Clausen Jan. 6, 2015, 12:51 p.m. UTC | #3
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


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Jan. 6, 2015, 1:48 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Jan. 7, 2015, 2:29 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Jan. 7, 2015, 4:13 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Jan. 7, 2015, 4:17 p.m. UTC | #7
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

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 = {