| Message ID | 8235185.7qUDHu9jso@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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?
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
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 >
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
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 --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);