diff mbox

[2/3] ARM: edma: Rename header file for dmaengine filter function definition

Message ID 8235185.7qUDHu9jso@wuerfel (mailing list archive)
State Not Applicable
Headers show

Commit Message

Arnd Bergmann Nov. 27, 2014, 11:14 a.m. UTC
On Thursday 27 November 2014 12:41:30 Peter Ujfalusi wrote:
> diff --git a/include/linux/edma-dmaengine.h b/include/linux/edma-dmaengine.h
> new file mode 100644
> index 000000000000..8a2602809a77
> --- /dev/null
> +++ b/include/linux/edma-dmaengine.h
> @@ -0,0 +1,29 @@
> +struct dma_chan;
> +
> +#if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE)
> +bool edma_filter_fn(struct dma_chan *, void *);
> +#else
> +static inline bool edma_filter_fn(struct dma_chan *chan, void *param)
> +{
> +	return false;
> +}
> +#endif
> +
> +#endif

It's better not to hardcode the name of the filter function in a driver,
better move the filter function into platform code and pass it down
to the driver in the platform_data pointer in davinci_mmc_config and
davinci_spi_platform_data to get rid of this dependency.

Something roughly like the patch below.

	Arnd


---
[DRAFT] ARM: davinci: pass dma config through platform data

DMA slave drivers are supposed to be written independent of the dma engine
driver, which means that the mmc and spi drivers on davinci should not
reference the edma_filter_fn or know about the specific format of the
data passed into it.

This changes the channel data from a resource into a void pointer and
passes it as platform data along with the filter function.

TODO: do the same thing for SPI, and adapt the filter function for the
new format.

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

 arch/arm/mach-davinci/devices-da8xx.c     | 26 ++++++--------------------
 arch/arm/mach-davinci/devices.c           | 22 ++++++----------------
 drivers/mmc/host/davinci_mmc.c            | 27 +++++++++++----------------
 include/linux/platform_data/mmc-davinci.h |  5 +++++
 4 files changed, 28 insertions(+), 52 deletions(-)


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

Peter Ujfalusi Nov. 27, 2014, 2:23 p.m. UTC | #1
On 11/27/2014 01:14 PM, Arnd Bergmann wrote:
> On Thursday 27 November 2014 12:41:30 Peter Ujfalusi wrote:
>> diff --git a/include/linux/edma-dmaengine.h b/include/linux/edma-dmaengine.h
>> new file mode 100644
>> index 000000000000..8a2602809a77
>> --- /dev/null
>> +++ b/include/linux/edma-dmaengine.h
>> @@ -0,0 +1,29 @@
>> +struct dma_chan;
>> +
>> +#if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE)
>> +bool edma_filter_fn(struct dma_chan *, void *);
>> +#else
>> +static inline bool edma_filter_fn(struct dma_chan *chan, void *param)
>> +{
>> +	return false;
>> +}
>> +#endif
>> +
>> +#endif
> 
> It's better not to hardcode the name of the filter function in a driver,
> better move the filter function into platform code and pass it down
> to the driver in the platform_data pointer in davinci_mmc_config and
> davinci_spi_platform_data to get rid of this dependency.
> 
> Something roughly like the patch below.

This will only work in case of legacy boot. When booting with DT we do not
have pdata and after this patch in dt boot we are not going to be able to get
the DMA resources either.

I think if we want to do something like this, it has to be done within the
dmaengine framework. The dma controller's of_dma_filter_info already have
.filter_fn which could be used by the framework.

On the other hand the davinci_mmc (and spi-davinci) is only going to work on
davinci devices, so I don't really see issue with 'hard coding' davinci
related callback in the code.
Arnd Bergmann Nov. 27, 2014, 2:50 p.m. UTC | #2
On Thursday 27 November 2014 16:23:31 Peter Ujfalusi wrote:
> This will only work in case of legacy boot. When booting with DT we do not
> have pdata and after this patch in dt boot we are not going to be able to get
> the DMA resources either.

No, when booting with DT, the filter_fn and data are not used at all,
we get the dma channel by parsing the DT instead.

> I think if we want to do something like this, it has to be done within the
> dmaengine framework. The dma controller's of_dma_filter_info already have
> .filter_fn which could be used by the framework.

No, of_dma_filter_info/of_dma_simple_xlate was a mistake, we should never
have even introduced that. All drivers that rely on this can simply
provide their own xlate function that calls of_dma_get_slave_channel()
or one of the related functions.

edma is particularly trivial, it can just use of_dma_xlate_by_chan_id()
instead of of_dma_simple_xlate, as it looks up the channel by its number.

	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
Peter Ujfalusi Nov. 27, 2014, 6:46 p.m. UTC | #3
On 11/27/2014 04:50 PM, Arnd Bergmann wrote:
> On Thursday 27 November 2014 16:23:31 Peter Ujfalusi wrote:
>> This will only work in case of legacy boot. When booting with DT we do not
>> have pdata and after this patch in dt boot we are not going to be able to get
>> the DMA resources either.
> 
> No, when booting with DT, the filter_fn and data are not used at all,
> we get the dma channel by parsing the DT instead.

Correct.

>> I think if we want to do something like this, it has to be done within the
>> dmaengine framework. The dma controller's of_dma_filter_info already have
>> .filter_fn which could be used by the framework.
> 
> No, of_dma_filter_info/of_dma_simple_xlate was a mistake, we should never
> have even introduced that. All drivers that rely on this can simply
> provide their own xlate function that calls of_dma_get_slave_channel()
> or one of the related functions.
> 
> edma is particularly trivial, it can just use of_dma_xlate_by_chan_id()
> instead of of_dma_simple_xlate, as it looks up the channel by its number.

I see. With this series I did not planed to fix all edma related issues, just
as a start clean up the related header files. I would rather not add fixes to
mmc, spi, etc drivers since while you have valid point it is not in the scope
of this series.
Can we do the changes you are suggesting in an incremental manner?
Arnd Bergmann Nov. 27, 2014, 9:52 p.m. UTC | #4
On Thursday 27 November 2014 20:46:12 Peter Ujfalusi wrote:
> 
> I see. With this series I did not planed to fix all edma related issues, just
> as a start clean up the related header files. I would rather not add fixes to
> mmc, spi, etc drivers since while you have valid point it is not in the scope
> of this series.
> Can we do the changes you are suggesting in an incremental manner?

Sure, but I'd leave the existing filter function declaration alone then
and not move it, since we wouldn't want to keep it in the long run.

	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
Peter Ujfalusi Nov. 28, 2014, 7:16 a.m. UTC | #5
On 11/27/2014 11:52 PM, Arnd Bergmann wrote:
> On Thursday 27 November 2014 20:46:12 Peter Ujfalusi wrote:
>>
>> I see. With this series I did not planed to fix all edma related issues, just
>> as a start clean up the related header files. I would rather not add fixes to
>> mmc, spi, etc drivers since while you have valid point it is not in the scope
>> of this series.
>> Can we do the changes you are suggesting in an incremental manner?
> 
> Sure, but I'd leave the existing filter function declaration alone then
> and not move it, since we wouldn't want to keep it in the long run.

but if you want to reference the filter function (which is in
drivers/dma/edma.c) in arch/arm/mach-davinci/ directory, we will need it.
Don't we?

If I leave the header as it is, then how would we clean up the edma headers? I
would not put the API definitions for the arch code into the same file as we
have the filter definition.


> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Arnd Bergmann Nov. 28, 2014, 10:51 a.m. UTC | #6
On Friday 28 November 2014 09:16:24 Peter Ujfalusi wrote:
> On 11/27/2014 11:52 PM, Arnd Bergmann wrote:
> > On Thursday 27 November 2014 20:46:12 Peter Ujfalusi wrote:
> >>
> >> I see. With this series I did not planed to fix all edma related issues, just
> >> as a start clean up the related header files. I would rather not add fixes to
> >> mmc, spi, etc drivers since while you have valid point it is not in the scope
> >> of this series.
> >> Can we do the changes you are suggesting in an incremental manner?
> > 
> > Sure, but I'd leave the existing filter function declaration alone then
> > and not move it, since we wouldn't want to keep it in the long run.
> 
> but if you want to reference the filter function (which is in
> drivers/dma/edma.c) in arch/arm/mach-davinci/ directory, we will need it.
> Don't we?

Yes, unless you move the definition of the filter function into
arch/arm/common/edma.c or arch/arm/mach-davinci/devices.c, but that
would require other changes.

> If I leave the header as it is, then how would we clean up the edma headers? I
> would not put the API definitions for the arch code into the same file as we
> have the filter definition.

Ok, just go ahead with your current patch then, we can always follow up.
The most important cleanup for edma is elsewhere anyway, so once the asoc
drivers can use the dmaengine interface, this should be easier.

	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
Peter Ujfalusi Nov. 28, 2014, 11:48 a.m. UTC | #7
On 11/28/2014 12:51 PM, Arnd Bergmann wrote:
> On Friday 28 November 2014 09:16:24 Peter Ujfalusi wrote:
>> On 11/27/2014 11:52 PM, Arnd Bergmann wrote:
>>> On Thursday 27 November 2014 20:46:12 Peter Ujfalusi wrote:
>>>>
>>>> I see. With this series I did not planed to fix all edma related issues, just
>>>> as a start clean up the related header files. I would rather not add fixes to
>>>> mmc, spi, etc drivers since while you have valid point it is not in the scope
>>>> of this series.
>>>> Can we do the changes you are suggesting in an incremental manner?
>>>
>>> Sure, but I'd leave the existing filter function declaration alone then
>>> and not move it, since we wouldn't want to keep it in the long run.
>>
>> but if you want to reference the filter function (which is in
>> drivers/dma/edma.c) in arch/arm/mach-davinci/ directory, we will need it.
>> Don't we?
> 
> Yes, unless you move the definition of the filter function into
> arch/arm/common/edma.c or arch/arm/mach-davinci/devices.c, but that
> would require other changes.

At the end the aim is to get rid of the edma code form arch/arm and have only
dmaengine API towards eDMA. The ASoC davinci-pcm is the only user of the
legacy API AFAIK. It has a mode called ping-pong which is not possible with
the dmaeingine at all. This is to overcome underflow situations on parts where
the audio IP does not have FIFO.
My edma-pcm (which is using dmaengine) should be able to handle this
situation, but I need to verify it before I can remove the davinci-pcm and
then we can get rid of the direct eDMA API and code.

>> If I leave the header as it is, then how would we clean up the edma headers? I
>> would not put the API definitions for the arch code into the same file as we
>> have the filter definition.
> 
> Ok, just go ahead with your current patch then, we can always follow up.
> The most important cleanup for edma is elsewhere anyway, so once the asoc
> drivers can use the dmaengine interface, this should be easier.
> 
> 	Arnd
>
diff mbox

Patch

diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index b85b781b05fd..4aa872dc6dc3 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -671,16 +671,6 @@  static struct resource da8xx_mmcsd0_resources[] = {
 		.end	= IRQ_DA8XX_MMCSDINT0,
 		.flags	= IORESOURCE_IRQ,
 	},
-	{		/* DMA RX */
-		.start	= DA8XX_DMA_MMCSD0_RX,
-		.end	= DA8XX_DMA_MMCSD0_RX,
-		.flags	= IORESOURCE_DMA,
-	},
-	{		/* DMA TX */
-		.start	= DA8XX_DMA_MMCSD0_TX,
-		.end	= DA8XX_DMA_MMCSD0_TX,
-		.flags	= IORESOURCE_DMA,
-	},
 };
 
 static struct platform_device da8xx_mmcsd0_device = {
@@ -693,6 +683,9 @@  static struct platform_device da8xx_mmcsd0_device = {
 int __init da8xx_register_mmcsd0(struct davinci_mmc_config *config)
 {
 	da8xx_mmcsd0_device.dev.platform_data = config;
+	config->filter = edma_filter_fn;
+	config->rxdma = (void *)DA8XX_DMA_MMCSD0_RX;
+	config->txdma = (void *)DA8XX_DMA_MMCSD0_TX;
 	return platform_device_register(&da8xx_mmcsd0_device);
 }
 
@@ -708,16 +701,6 @@  static struct resource da850_mmcsd1_resources[] = {
 		.end	= IRQ_DA850_MMCSDINT0_1,
 		.flags	= IORESOURCE_IRQ,
 	},
-	{		/* DMA RX */
-		.start	= DA850_DMA_MMCSD1_RX,
-		.end	= DA850_DMA_MMCSD1_RX,
-		.flags	= IORESOURCE_DMA,
-	},
-	{		/* DMA TX */
-		.start	= DA850_DMA_MMCSD1_TX,
-		.end	= DA850_DMA_MMCSD1_TX,
-		.flags	= IORESOURCE_DMA,
-	},
 };
 
 static struct platform_device da850_mmcsd1_device = {
@@ -730,6 +713,9 @@  static struct platform_device da850_mmcsd1_device = {
 int __init da850_register_mmcsd1(struct davinci_mmc_config *config)
 {
 	da850_mmcsd1_device.dev.platform_data = config;
+	config->filter = edma_filter_fn;
+	config->rxdma = (void *)DA8XX_DMA_MMCSD1_RX;
+	config->txdma = (void *)DA8XX_DMA_MMCSD1_TX;
 	return platform_device_register(&da850_mmcsd1_device);
 }
 #endif
diff --git a/arch/arm/mach-davinci/devices.c b/arch/arm/mach-davinci/devices.c
index 6257aa452568..7b7039d9b079 100644
--- a/arch/arm/mach-davinci/devices.c
+++ b/arch/arm/mach-davinci/devices.c
@@ -144,14 +144,6 @@  static struct resource mmcsd0_resources[] = {
 		.start = IRQ_SDIOINT,
 		.flags = IORESOURCE_IRQ,
 	},
-	/* DMA channels: RX, then TX */
-	{
-		.start = EDMA_CTLR_CHAN(0, DAVINCI_DMA_MMCRXEVT),
-		.flags = IORESOURCE_DMA,
-	}, {
-		.start = EDMA_CTLR_CHAN(0, DAVINCI_DMA_MMCTXEVT),
-		.flags = IORESOURCE_DMA,
-	},
 };
 
 static struct platform_device davinci_mmcsd0_device = {
@@ -181,14 +173,6 @@  static struct resource mmcsd1_resources[] = {
 		.start = IRQ_DM355_SDIOINT1,
 		.flags = IORESOURCE_IRQ,
 	},
-	/* DMA channels: RX, then TX */
-	{
-		.start = EDMA_CTLR_CHAN(0, 30),	/* rx */
-		.flags = IORESOURCE_DMA,
-	}, {
-		.start = EDMA_CTLR_CHAN(0, 31),	/* tx */
-		.flags = IORESOURCE_DMA,
-	},
 };
 
 static struct platform_device davinci_mmcsd1_device = {
@@ -218,6 +202,9 @@  void __init davinci_setup_mmc(int module, struct davinci_mmc_config *config)
 	 */
 	switch (module) {
 	case 1:
+		config->filter = edma_filter_fn;
+		config->rxdma = (void *)EDMA_CTLR_CHAN(0, DAVINCI_DMA_MMCRXEVT);
+		config->txdma = (void *)EDMA_CTLR_CHAN(0, DAVINCI_DMA_MMCTXEVT);
 		if (cpu_is_davinci_dm355()) {
 			/* REVISIT we may not need all these pins if e.g. this
 			 * is a hard-wired SDIO device...
@@ -247,6 +234,9 @@  void __init davinci_setup_mmc(int module, struct davinci_mmc_config *config)
 		pdev = &davinci_mmcsd1_device;
 		break;
 	case 0:
+		config->filter = edma_filter_fn;
+		config->rxdma = (void *)EDMA_CTLR_CHAN(0, 30);
+		config->txdma = (void *)EDMA_CTLR_CHAN(0, 31);
 		if (cpu_is_davinci_dm355()) {
 			mmcsd0_resources[0].start = DM355_MMCSD0_BASE;
 			mmcsd0_resources[0].end = DM355_MMCSD0_BASE + SZ_4K - 1;
diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 1625f908dc70..3f25be9942c8 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -202,7 +202,8 @@  struct mmc_davinci_host {
 	u32 buffer_bytes_left;
 	u32 bytes_left;
 
-	u32 rxdma, txdma;
+	dma_filter_fn filter;
+	void *rxdma, *txdma;
 	struct dma_chan *dma_tx;
 	struct dma_chan *dma_rx;
 	bool use_dma;
@@ -524,16 +525,16 @@  static int __init davinci_acquire_dma_channels(struct mmc_davinci_host *host)
 	dma_cap_set(DMA_SLAVE, mask);
 
 	host->dma_tx =
-		dma_request_slave_channel_compat(mask, edma_filter_fn,
-				&host->txdma, mmc_dev(host->mmc), "tx");
+		dma_request_slave_channel_compat(mask, host->filter,
+				host->txdma, mmc_dev(host->mmc), "tx");
 	if (!host->dma_tx) {
 		dev_err(mmc_dev(host->mmc), "Can't get dma_tx channel\n");
 		return -ENODEV;
 	}
 
 	host->dma_rx =
-		dma_request_slave_channel_compat(mask, edma_filter_fn,
-				&host->rxdma, mmc_dev(host->mmc), "rx");
+		dma_request_slave_channel_compat(mask, host->filter,
+				host->rxdma, mmc_dev(host->mmc), "rx");
 	if (!host->dma_rx) {
 		dev_err(mmc_dev(host->mmc), "Can't get dma_rx channel\n");
 		r = -ENODEV;
@@ -1262,17 +1263,11 @@  static int __init davinci_mmcsd_probe(struct platform_device *pdev)
 	host = mmc_priv(mmc);
 	host->mmc = mmc;	/* Important */
 
-	r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
-	if (!r)
-		dev_warn(&pdev->dev, "RX DMA resource not specified\n");
-	else
-		host->rxdma = r->start;
-
-	r = platform_get_resource(pdev, IORESOURCE_DMA, 1);
-	if (!r)
-		dev_warn(&pdev->dev, "TX DMA resource not specified\n");
-	else
-		host->txdma = r->start;
+	if (pdata) {
+		host->filter = pdata->filter;
+		host->rxdma  = pdata->rxdma;
+		host->txdma  = pdata->txdma;
+	}
 
 	host->mem_res = mem;
 	host->base = ioremap(mem->start, mem_size);
diff --git a/include/linux/platform_data/mmc-davinci.h b/include/linux/platform_data/mmc-davinci.h
index 9cea4ee377b5..0e5809bcaff0 100644
--- a/include/linux/platform_data/mmc-davinci.h
+++ b/include/linux/platform_data/mmc-davinci.h
@@ -25,6 +25,11 @@  struct davinci_mmc_config {
 
 	/* Number of sg segments */
 	u8	nr_sg;
+
+	/* DMA setup */
+	dma_filter_fn filter;
+	void	*rxdma;
+	void	*txdma;
 };
 void davinci_setup_mmc(int module, struct davinci_mmc_config *config);