diff mbox

[1/2] ARM: shmobile: sdhi: pass DMA filter from platform code

Message ID 1370008605-3745603-1-git-send-email-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann May 31, 2013, 1:56 p.m. UTC
Since eec95ee2261 "mmc: sdhi/tmio: switch to using
dmaengine_slave_config()", we are getting a link error with
a number of shmobile configurations that turn on the sdhi
driver but the not the SH_DMAE_BASE driver.

The reason is that the driver now incorrectly refers to
a global filter function that should be referenced only
by platform code:

drivers/built-in.o: In function `sh_mobile_sdhi_probe':
drivers/mmc/host/sh_mobile_sdhi.c:202: undefined reference to `shdma_chan_filter'
drivers/mmc/host/sh_mobile_sdhi.c:202: undefined reference to `shdma_chan_filter'

The fix is to move the pointer to the filter function out
of the driver, similar to how we do it for most other
platforms. This makes the driver almost independent
of the underlying dma engine, besides the fact that
it tries to pass the same number both into the filter
function and into slave config, which only works on
some DMA engine drivers.

Since the bug is only present on the mmc tree, I think
the fix should be applied there as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
---
 arch/arm/mach-shmobile/board-ag5evm.c          |  4 ++++
 arch/arm/mach-shmobile/board-ap4evb.c          |  7 +++++++
 arch/arm/mach-shmobile/board-armadillo800eva.c |  7 +++++++
 arch/arm/mach-shmobile/board-kzm9g.c           |  7 +++++++
 arch/arm/mach-shmobile/board-mackerel.c        | 10 ++++++++++
 drivers/mmc/host/sh_mobile_sdhi.c              |  9 ++++++++-
 include/linux/mmc/sh_mobile_sdhi.h             |  1 +
 7 files changed, 44 insertions(+), 1 deletion(-)

Comments

Guennadi Liakhovetski May 31, 2013, 2:52 p.m. UTC | #1
Hi Arnd

Thanks for the patches. How about this fix:

http://thread.gmane.org/gmane.linux.kernel.mmc/20619

I think it's a better fix. SHDMA users cannot use other DMA drivers, so, I 
don't think we need to pass the filter via the platform data.

Thanks
Guennadi

On Fri, 31 May 2013, Arnd Bergmann wrote:

> Since eec95ee2261 "mmc: sdhi/tmio: switch to using
> dmaengine_slave_config()", we are getting a link error with
> a number of shmobile configurations that turn on the sdhi
> driver but the not the SH_DMAE_BASE driver.
> 
> The reason is that the driver now incorrectly refers to
> a global filter function that should be referenced only
> by platform code:
> 
> drivers/built-in.o: In function `sh_mobile_sdhi_probe':
> drivers/mmc/host/sh_mobile_sdhi.c:202: undefined reference to `shdma_chan_filter'
> drivers/mmc/host/sh_mobile_sdhi.c:202: undefined reference to `shdma_chan_filter'
> 
> The fix is to move the pointer to the filter function out
> of the driver, similar to how we do it for most other
> platforms. This makes the driver almost independent
> of the underlying dma engine, besides the fact that
> it tries to pass the same number both into the filter
> function and into slave config, which only works on
> some DMA engine drivers.
> 
> Since the bug is only present on the mmc tree, I think
> the fix should be applied there as well.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> ---
>  arch/arm/mach-shmobile/board-ag5evm.c          |  4 ++++
>  arch/arm/mach-shmobile/board-ap4evb.c          |  7 +++++++
>  arch/arm/mach-shmobile/board-armadillo800eva.c |  7 +++++++
>  arch/arm/mach-shmobile/board-kzm9g.c           |  7 +++++++
>  arch/arm/mach-shmobile/board-mackerel.c        | 10 ++++++++++
>  drivers/mmc/host/sh_mobile_sdhi.c              |  9 ++++++++-
>  include/linux/mmc/sh_mobile_sdhi.h             |  1 +
>  7 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
> index c754071..ad01651 100644
> --- a/arch/arm/mach-shmobile/board-ag5evm.c
> +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> @@ -42,6 +42,7 @@
>  #include <linux/mmc/sh_mobile_sdhi.h>
>  #include <linux/mfd/tmio.h>
>  #include <linux/sh_clk.h>
> +#include <linux/sh_dma.h>
>  #include <linux/irqchip/arm-gic.h>
>  #include <video/sh_mobile_lcdc.h>
>  #include <video/sh_mipi_dsi.h>
> @@ -403,8 +404,11 @@ static struct regulator_consumer_supply fixed2v8_power_consumers[] =
>  
>  /* SDHI0 */
>  static struct sh_mobile_sdhi_info sdhi0_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED,
>  	.tmio_ocr_mask	= MMC_VDD_27_28 | MMC_VDD_28_29,
> diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
> index 297bf5e..c900e24 100644
> --- a/arch/arm/mach-shmobile/board-ap4evb.c
> +++ b/arch/arm/mach-shmobile/board-ap4evb.c
> @@ -38,6 +38,7 @@
>  #include <linux/regulator/fixed.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/smsc911x.h>
> +#include <linux/sh_dma.h>
>  #include <linux/sh_intc.h>
>  #include <linux/sh_clk.h>
>  #include <linux/gpio.h>
> @@ -355,8 +356,11 @@ static struct platform_device sh_mmcif_device = {
>  
>  /* SDHI0 */
>  static struct sh_mobile_sdhi_info sdhi0_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_caps	= MMC_CAP_SDIO_IRQ,
>  };
>  
> @@ -393,8 +397,11 @@ static struct platform_device sdhi0_device = {
>  
>  /* SDHI1 */
>  static struct sh_mobile_sdhi_info sdhi1_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI1_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI1_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_ocr_mask	= MMC_VDD_165_195,
>  	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE,
>  	.tmio_caps	= MMC_CAP_NEEDS_POLL | MMC_CAP_SDIO_IRQ,
> diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
> index 44a6215..e95e5dc 100644
> --- a/arch/arm/mach-shmobile/board-armadillo800eva.c
> +++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
> @@ -34,6 +34,7 @@
>  #include <linux/regulator/fixed.h>
>  #include <linux/regulator/gpio-regulator.h>
>  #include <linux/regulator/machine.h>
> +#include <linux/sh_dma.h>
>  #include <linux/sh_eth.h>
>  #include <linux/videodev2.h>
>  #include <linux/usb/renesas_usbhs.h>
> @@ -688,8 +689,11 @@ static struct platform_device vcc_sdhi1 = {
>   */
>  #define IRQ31	irq_pin(31)
>  static struct sh_mobile_sdhi_info sdhi0_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>  			  MMC_CAP_POWER_OFF_CARD,
>  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
> @@ -730,8 +734,11 @@ static struct platform_device sdhi0_device = {
>  
>  /* SDHI1 */
>  static struct sh_mobile_sdhi_info sdhi1_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI1_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI1_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>  			  MMC_CAP_POWER_OFF_CARD,
>  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
> diff --git a/arch/arm/mach-shmobile/board-kzm9g.c b/arch/arm/mach-shmobile/board-kzm9g.c
> index 165483c..0434b36 100644
> --- a/arch/arm/mach-shmobile/board-kzm9g.c
> +++ b/arch/arm/mach-shmobile/board-kzm9g.c
> @@ -37,6 +37,7 @@
>  #include <linux/regulator/fixed.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/smsc911x.h>
> +#include <linux/sh_dma.h>
>  #include <linux/usb/r8a66597.h>
>  #include <linux/usb/renesas_usbhs.h>
>  #include <linux/videodev2.h>
> @@ -442,8 +443,11 @@ static struct platform_device vcc_sdhi2 = {
>  
>  /* SDHI */
>  static struct sh_mobile_sdhi_info sdhi0_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT,
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>  			  MMC_CAP_POWER_OFF_CARD,
> @@ -484,8 +488,11 @@ static struct platform_device sdhi0_device = {
>  
>  /* Micro SD */
>  static struct sh_mobile_sdhi_info sdhi2_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI2_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI2_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT |
>  			  TMIO_MMC_USE_GPIO_CD |
>  			  TMIO_MMC_WRPROTECT_DISABLE,
> diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
> index 85f51a8..49755ff 100644
> --- a/arch/arm/mach-shmobile/board-mackerel.c
> +++ b/arch/arm/mach-shmobile/board-mackerel.c
> @@ -45,6 +45,7 @@
>  #include <linux/regulator/fixed.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/smsc911x.h>
> +#include <linux/sh_dma.h>
>  #include <linux/sh_intc.h>
>  #include <linux/tca6416_keypad.h>
>  #include <linux/usb/renesas_usbhs.h>
> @@ -971,8 +972,11 @@ static struct platform_device nand_flash_device = {
>  
>  /* SDHI0 */
>  static struct sh_mobile_sdhi_info sdhi0_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_USE_GPIO_CD,
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
>  	.cd_gpio	= 172,
> @@ -1010,8 +1014,11 @@ static struct platform_device sdhi0_device = {
>  
>  /* GPIO 41 can trigger IRQ8, but it is used by USBHS1, we have to poll */
>  static struct sh_mobile_sdhi_info sdhi1_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI1_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI1_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_USE_GPIO_CD,
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>  			  MMC_CAP_NEEDS_POLL,
> @@ -1053,8 +1060,11 @@ static struct platform_device sdhi1_device = {
>   * connected to GPIO SCIFB_SCK of SH7372 (GPIO 162).
>   */
>  static struct sh_mobile_sdhi_info sdhi2_info = {
> +#ifdef CONFIG_SH_DMAE_BASE
>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI2_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI2_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_USE_GPIO_CD,
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>  			  MMC_CAP_NEEDS_POLL,
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index cc4c872..efe3386 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -193,13 +193,20 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>  			 */
>  			dma_priv->chan_priv_tx = (void *)p->dma_slave_tx;
>  			dma_priv->chan_priv_rx = (void *)p->dma_slave_rx;
> +			/*
> +			 * This is a layering violation: the slave driver
> +			 * should not be aware that the chan_priv_* is the
> +			 * slave id.
> +			 * We should not really need to set the slave id
> +			 * here anyway. -arnd
> +			 */
>  			dma_priv->slave_id_tx = p->dma_slave_tx;
>  			dma_priv->slave_id_rx = p->dma_slave_rx;
> +			dma_priv->filter = p->dma_filter;
>  		}
>  	}
>  
>  	dma_priv->alignment_shift = 1; /* 2-byte alignment */
> -	dma_priv->filter = shdma_chan_filter;
>  
>  	mmc_data->dma = dma_priv;
>  
> diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
> index b76bcf0..342f07b 100644
> --- a/include/linux/mmc/sh_mobile_sdhi.h
> +++ b/include/linux/mmc/sh_mobile_sdhi.h
> @@ -20,6 +20,7 @@ struct sh_mobile_sdhi_ops {
>  struct sh_mobile_sdhi_info {
>  	int dma_slave_tx;
>  	int dma_slave_rx;
> +	dma_filter_fn dma_filter;
>  	unsigned long tmio_flags;
>  	unsigned long tmio_caps;
>  	unsigned long tmio_caps2;
> -- 
> 1.8.1.2
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann May 31, 2013, 3:11 p.m. UTC | #2
On Friday 31 May 2013 16:52:13 Guennadi Liakhovetski wrote:
> 
> Thanks for the patches. How about this fix:
> 
> http://thread.gmane.org/gmane.linux.kernel.mmc/20619
> 
> I think it's a better fix. SHDMA users cannot use other DMA drivers, so, I 
> don't think we need to pass the filter via the platform data.

I think it's more a matter of using the API correctly. The dmaengine
API is an abstraction to separate the slave driver from the master
through well-defined calls. If you make additional assumptions
in the slave driver about the master, that is a layering violation.

	Arnd
Guennadi Liakhovetski May 31, 2013, 3:30 p.m. UTC | #3
On Fri, 31 May 2013, Arnd Bergmann wrote:

> On Friday 31 May 2013 16:52:13 Guennadi Liakhovetski wrote:
> > 
> > Thanks for the patches. How about this fix:
> > 
> > http://thread.gmane.org/gmane.linux.kernel.mmc/20619
> > 
> > I think it's a better fix. SHDMA users cannot use other DMA drivers, so, I 
> > don't think we need to pass the filter via the platform data.
> 
> I think it's more a matter of using the API correctly. The dmaengine
> API is an abstraction to separate the slave driver from the master
> through well-defined calls. If you make additional assumptions
> in the slave driver about the master, that is a layering violation.

I think it is a common practice, see e.g.

drivers/mmc/host/omap_hsmmc.c
drivers/mmc/host/davinci_mmc.c

And what do you do for DT-based platforms?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann May 31, 2013, 4:02 p.m. UTC | #4
On Friday 31 May 2013 17:30:01 Guennadi Liakhovetski wrote:
> On Fri, 31 May 2013, Arnd Bergmann wrote:
> > On Friday 31 May 2013 16:52:13 Guennadi Liakhovetski wrote:

> > I think it's more a matter of using the API correctly. The dmaengine
> > API is an abstraction to separate the slave driver from the master
> > through well-defined calls. If you make additional assumptions
> > in the slave driver about the master, that is a layering violation.
> 
> I think it is a common practice, see e.g.
> 
> drivers/mmc/host/omap_hsmmc.c
> drivers/mmc/host/davinci_mmc.c

Yes, those should be fixed as well.

> And what do you do for DT-based platforms?

Once the driver is DT-enabled, we no longer have to pass
a filter function at all because the slave id and all settings
will get pulled out of the DT using the dma engine's xlate()
callback.

	Arnd
Guennadi Liakhovetski June 5, 2013, 8:28 a.m. UTC | #5
Hi Arnd

On Fri, 31 May 2013, Arnd Bergmann wrote:

> On Friday 31 May 2013 17:30:01 Guennadi Liakhovetski wrote:
> > On Fri, 31 May 2013, Arnd Bergmann wrote:
> > > On Friday 31 May 2013 16:52:13 Guennadi Liakhovetski wrote:
> 
> > > I think it's more a matter of using the API correctly. The dmaengine
> > > API is an abstraction to separate the slave driver from the master
> > > through well-defined calls. If you make additional assumptions
> > > in the slave driver about the master, that is a layering violation.
> > 
> > I think it is a common practice, see e.g.
> > 
> > drivers/mmc/host/omap_hsmmc.c
> > drivers/mmc/host/davinci_mmc.c
> 
> Yes, those should be fixed as well.

Then we'll have to fix quite a few of those - I only looked under 
drivers/mmc for now.

But isn't that a separate issue? The problem we have to address now is 
broken compilation, for which, I think, my patch is the simplest solution. 
DMA slave drivers being DMAC implementation agnostic is good, no doubt 
about that, even though many of them will ever only use one DMAC type, but 
isn't that a separate issue? Fixing it would require patching 3 locations: 
a header to add a callback field, arches to add filters and drivers to 
actually call them instead of hard-coded functions. In the worst case 
those changes would go via 3 different git-trees, so, might take 3 kernel 
releases... Ok, at least two if we puch the header change together with 
arch updates via the same tree with suitable acks, still, that's too long, 
IMHO.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Guennadi Liakhovetski June 7, 2013, 10:25 a.m. UTC | #6
Hi Arnd

On Fri, 31 May 2013, Arnd Bergmann wrote:

> Since eec95ee2261 "mmc: sdhi/tmio: switch to using
> dmaengine_slave_config()", we are getting a link error with
> a number of shmobile configurations that turn on the sdhi
> driver but the not the SH_DMAE_BASE driver.
> 
> The reason is that the driver now incorrectly refers to
> a global filter function that should be referenced only
> by platform code:
> 
> drivers/built-in.o: In function `sh_mobile_sdhi_probe':
> drivers/mmc/host/sh_mobile_sdhi.c:202: undefined reference to `shdma_chan_filter'
> drivers/mmc/host/sh_mobile_sdhi.c:202: undefined reference to `shdma_chan_filter'
> 
> The fix is to move the pointer to the filter function out
> of the driver, similar to how we do it for most other
> platforms. This makes the driver almost independent
> of the underlying dma engine, besides the fact that
> it tries to pass the same number both into the filter
> function and into slave config, which only works on
> some DMA engine drivers.
> 
> Since the bug is only present on the mmc tree, I think
> the fix should be applied there as well.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> ---
>  arch/arm/mach-shmobile/board-ag5evm.c          |  4 ++++
>  arch/arm/mach-shmobile/board-ap4evb.c          |  7 +++++++
>  arch/arm/mach-shmobile/board-armadillo800eva.c |  7 +++++++
>  arch/arm/mach-shmobile/board-kzm9g.c           |  7 +++++++
>  arch/arm/mach-shmobile/board-mackerel.c        | 10 ++++++++++
>  drivers/mmc/host/sh_mobile_sdhi.c              |  9 ++++++++-
>  include/linux/mmc/sh_mobile_sdhi.h             |  1 +
>  7 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
> index c754071..ad01651 100644
> --- a/arch/arm/mach-shmobile/board-ag5evm.c
> +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> @@ -42,6 +42,7 @@
>  #include <linux/mmc/sh_mobile_sdhi.h>
>  #include <linux/mfd/tmio.h>
>  #include <linux/sh_clk.h>
> +#include <linux/sh_dma.h>
>  #include <linux/irqchip/arm-gic.h>
>  #include <video/sh_mobile_lcdc.h>
>  #include <video/sh_mipi_dsi.h>
> @@ -403,8 +404,11 @@ static struct regulator_consumer_supply fixed2v8_power_consumers[] =
>  
>  /* SDHI0 */
>  static struct sh_mobile_sdhi_info sdhi0_info = {
> +#ifdef CONFIG_SH_DMAE_BASE

Right, that's the problem, I think. Don't think we want these #ifdefs in 
all shdma users - under arch/arm and arch/sh. That's why I think we need 
my fix in the first place to fix the compile breakage. After that we can 
think about improving DMA slave driver decoupling from specific DMAC 
drivers - if at all needed.

>  	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
>  	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
> +	.dma_filter	= shdma_chan_filter,
> +#endif
>  	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
>  	.tmio_caps	= MMC_CAP_SD_HIGHSPEED,
>  	.tmio_ocr_mask	= MMC_VDD_27_28 | MMC_VDD_28_29,

[snip]

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann June 7, 2013, 12:52 p.m. UTC | #7
On Friday 07 June 2013 12:25:11 Guennadi Liakhovetski wrote:
> >  /* SDHI0 */
> >  static struct sh_mobile_sdhi_info sdhi0_info = {
> > +#ifdef CONFIG_SH_DMAE_BASE
> 
> Right, that's the problem, I think. Don't think we want these #ifdefs in 
> all shdma users - under arch/arm and arch/sh. That's why I think we need 
> my fix in the first place to fix the compile breakage. After that we can 
> think about improving DMA slave driver decoupling from specific DMAC 
> drivers - if at all needed.
> 
> >       .dma_slave_tx   = SHDMA_SLAVE_SDHI0_TX,
> >       .dma_slave_rx   = SHDMA_SLAVE_SDHI0_RX,
> > +     .dma_filter     = shdma_chan_filter,
> > +#endif

Well, the problem is that you check the value of dma_slave_rx/tx in order
to find out whether you should do DMA in the driver. If the filter
function is NULL but the values are positive, I think the driver won't
actually fall back to PIO mode but just fail.

	Arnd
Guennadi Liakhovetski June 7, 2013, 1:01 p.m. UTC | #8
On Fri, 7 Jun 2013, Arnd Bergmann wrote:

> On Friday 07 June 2013 12:25:11 Guennadi Liakhovetski wrote:
> > >  /* SDHI0 */
> > >  static struct sh_mobile_sdhi_info sdhi0_info = {
> > > +#ifdef CONFIG_SH_DMAE_BASE
> > 
> > Right, that's the problem, I think. Don't think we want these #ifdefs in 
> > all shdma users - under arch/arm and arch/sh. That's why I think we need 
> > my fix in the first place to fix the compile breakage. After that we can 
> > think about improving DMA slave driver decoupling from specific DMAC 
> > drivers - if at all needed.
> > 
> > >       .dma_slave_tx   = SHDMA_SLAVE_SDHI0_TX,
> > >       .dma_slave_rx   = SHDMA_SLAVE_SDHI0_RX,
> > > +     .dma_filter     = shdma_chan_filter,
> > > +#endif
> 
> Well, the problem is that you check the value of dma_slave_rx/tx in order
> to find out whether you should do DMA in the driver. If the filter
> function is NULL but the values are positive, I think the driver won't
> actually fall back to PIO mode but just fail.

So far the tmio_mmc_request_dma() function has only one caller - SDHI. 
Even worse - tmio_mmc_dma.o is only built for SDHI. With SDHI the filter 
function cannot be NULL, it is hard-coded. When we change that, then we'll 
take care of the problem. So far we just have to fix the breakage.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
diff mbox

Patch

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index c754071..ad01651 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -42,6 +42,7 @@ 
 #include <linux/mmc/sh_mobile_sdhi.h>
 #include <linux/mfd/tmio.h>
 #include <linux/sh_clk.h>
+#include <linux/sh_dma.h>
 #include <linux/irqchip/arm-gic.h>
 #include <video/sh_mobile_lcdc.h>
 #include <video/sh_mipi_dsi.h>
@@ -403,8 +404,11 @@  static struct regulator_consumer_supply fixed2v8_power_consumers[] =
 
 /* SDHI0 */
 static struct sh_mobile_sdhi_info sdhi0_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED,
 	.tmio_ocr_mask	= MMC_VDD_27_28 | MMC_VDD_28_29,
diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
index 297bf5e..c900e24 100644
--- a/arch/arm/mach-shmobile/board-ap4evb.c
+++ b/arch/arm/mach-shmobile/board-ap4evb.c
@@ -38,6 +38,7 @@ 
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/smsc911x.h>
+#include <linux/sh_dma.h>
 #include <linux/sh_intc.h>
 #include <linux/sh_clk.h>
 #include <linux/gpio.h>
@@ -355,8 +356,11 @@  static struct platform_device sh_mmcif_device = {
 
 /* SDHI0 */
 static struct sh_mobile_sdhi_info sdhi0_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_caps	= MMC_CAP_SDIO_IRQ,
 };
 
@@ -393,8 +397,11 @@  static struct platform_device sdhi0_device = {
 
 /* SDHI1 */
 static struct sh_mobile_sdhi_info sdhi1_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI1_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI1_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_ocr_mask	= MMC_VDD_165_195,
 	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE,
 	.tmio_caps	= MMC_CAP_NEEDS_POLL | MMC_CAP_SDIO_IRQ,
diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index 44a6215..e95e5dc 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -34,6 +34,7 @@ 
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/gpio-regulator.h>
 #include <linux/regulator/machine.h>
+#include <linux/sh_dma.h>
 #include <linux/sh_eth.h>
 #include <linux/videodev2.h>
 #include <linux/usb/renesas_usbhs.h>
@@ -688,8 +689,11 @@  static struct platform_device vcc_sdhi1 = {
  */
 #define IRQ31	irq_pin(31)
 static struct sh_mobile_sdhi_info sdhi0_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_POWER_OFF_CARD,
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
@@ -730,8 +734,11 @@  static struct platform_device sdhi0_device = {
 
 /* SDHI1 */
 static struct sh_mobile_sdhi_info sdhi1_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI1_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI1_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_POWER_OFF_CARD,
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
diff --git a/arch/arm/mach-shmobile/board-kzm9g.c b/arch/arm/mach-shmobile/board-kzm9g.c
index 165483c..0434b36 100644
--- a/arch/arm/mach-shmobile/board-kzm9g.c
+++ b/arch/arm/mach-shmobile/board-kzm9g.c
@@ -37,6 +37,7 @@ 
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/smsc911x.h>
+#include <linux/sh_dma.h>
 #include <linux/usb/r8a66597.h>
 #include <linux/usb/renesas_usbhs.h>
 #include <linux/videodev2.h>
@@ -442,8 +443,11 @@  static struct platform_device vcc_sdhi2 = {
 
 /* SDHI */
 static struct sh_mobile_sdhi_info sdhi0_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT,
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_POWER_OFF_CARD,
@@ -484,8 +488,11 @@  static struct platform_device sdhi0_device = {
 
 /* Micro SD */
 static struct sh_mobile_sdhi_info sdhi2_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI2_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI2_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT |
 			  TMIO_MMC_USE_GPIO_CD |
 			  TMIO_MMC_WRPROTECT_DISABLE,
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 85f51a8..49755ff 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -45,6 +45,7 @@ 
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/smsc911x.h>
+#include <linux/sh_dma.h>
 #include <linux/sh_intc.h>
 #include <linux/tca6416_keypad.h>
 #include <linux/usb/renesas_usbhs.h>
@@ -971,8 +972,11 @@  static struct platform_device nand_flash_device = {
 
 /* SDHI0 */
 static struct sh_mobile_sdhi_info sdhi0_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI0_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI0_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_flags	= TMIO_MMC_USE_GPIO_CD,
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 	.cd_gpio	= 172,
@@ -1010,8 +1014,11 @@  static struct platform_device sdhi0_device = {
 
 /* GPIO 41 can trigger IRQ8, but it is used by USBHS1, we have to poll */
 static struct sh_mobile_sdhi_info sdhi1_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI1_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI1_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_USE_GPIO_CD,
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_NEEDS_POLL,
@@ -1053,8 +1060,11 @@  static struct platform_device sdhi1_device = {
  * connected to GPIO SCIFB_SCK of SH7372 (GPIO 162).
  */
 static struct sh_mobile_sdhi_info sdhi2_info = {
+#ifdef CONFIG_SH_DMAE_BASE
 	.dma_slave_tx	= SHDMA_SLAVE_SDHI2_TX,
 	.dma_slave_rx	= SHDMA_SLAVE_SDHI2_RX,
+	.dma_filter	= shdma_chan_filter,
+#endif
 	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_USE_GPIO_CD,
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_NEEDS_POLL,
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index cc4c872..efe3386 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -193,13 +193,20 @@  static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 			 */
 			dma_priv->chan_priv_tx = (void *)p->dma_slave_tx;
 			dma_priv->chan_priv_rx = (void *)p->dma_slave_rx;
+			/*
+			 * This is a layering violation: the slave driver
+			 * should not be aware that the chan_priv_* is the
+			 * slave id.
+			 * We should not really need to set the slave id
+			 * here anyway. -arnd
+			 */
 			dma_priv->slave_id_tx = p->dma_slave_tx;
 			dma_priv->slave_id_rx = p->dma_slave_rx;
+			dma_priv->filter = p->dma_filter;
 		}
 	}
 
 	dma_priv->alignment_shift = 1; /* 2-byte alignment */
-	dma_priv->filter = shdma_chan_filter;
 
 	mmc_data->dma = dma_priv;
 
diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
index b76bcf0..342f07b 100644
--- a/include/linux/mmc/sh_mobile_sdhi.h
+++ b/include/linux/mmc/sh_mobile_sdhi.h
@@ -20,6 +20,7 @@  struct sh_mobile_sdhi_ops {
 struct sh_mobile_sdhi_info {
 	int dma_slave_tx;
 	int dma_slave_rx;
+	dma_filter_fn dma_filter;
 	unsigned long tmio_flags;
 	unsigned long tmio_caps;
 	unsigned long tmio_caps2;