diff mbox

[2/2] ARM: shmobile: sdhi: remove DMA hardware dependencies

Message ID 1370008605-3745603-2-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
The MMC driver should not need to care what the dma engine
is that it is using, so it must not rely on the argument
to the filter function to be a 'slave_id' value.

Passing the slave id in dmaengine_slave_config is not
portable, and does not work with DT-enabled systems,
so this turns the filter argument into a void pointer
that gets set by the platform code and gets passed
to the dmaengine code as an opaque value.

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          |  8 ++++----
 arch/arm/mach-shmobile/board-armadillo800eva.c |  8 ++++----
 arch/arm/mach-shmobile/board-kzm9g.c           |  8 ++++----
 arch/arm/mach-shmobile/board-mackerel.c        | 12 ++++++------
 drivers/mmc/host/sh_mobile_sdhi.c              | 20 +++-----------------
 drivers/mmc/host/tmio_mmc_dma.c                |  6 ++----
 include/linux/mfd/tmio.h                       |  2 --
 include/linux/mmc/sh_mobile_sdhi.h             |  5 +++--
 9 files changed, 28 insertions(+), 45 deletions(-)

Comments

Arnd Bergmann May 31, 2013, 1:58 p.m. UTC | #1
On Friday 31 May 2013 15:56:45 Arnd Bergmann wrote:
> --- a/include/linux/mmc/sh_mobile_sdhi.h
> +++ b/include/linux/mmc/sh_mobile_sdhi.h
> @@ -2,6 +2,7 @@
>  #define LINUX_MMC_SH_MOBILE_SDHI_H
>  
>  #include <linux/types.h>
> +#include <linux/dmaengine.h>
>  
>  struct platform_device;
>  

This hunk should have been in the first patch. I'll
wait for comments before re-sending. Don't apply as-is.

	Arnd
Guennadi Liakhovetski June 7, 2013, 10:22 a.m. UTC | #2
Hi Arnd

Unrelated to the original trigger, that prompted you to look at this, I 
don't think this would work:

On Fri, 31 May 2013, Arnd Bergmann wrote:

> The MMC driver should not need to care what the dma engine
> is that it is using, so it must not rely on the argument
> to the filter function to be a 'slave_id' value.
> 
> Passing the slave id in dmaengine_slave_config is not
> portable, and does not work with DT-enabled systems,
> so this turns the filter argument into a void pointer
> that gets set by the platform code and gets passed
> to the dmaengine code as an opaque value.
> 
> 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          |  8 ++++----
>  arch/arm/mach-shmobile/board-armadillo800eva.c |  8 ++++----
>  arch/arm/mach-shmobile/board-kzm9g.c           |  8 ++++----
>  arch/arm/mach-shmobile/board-mackerel.c        | 12 ++++++------
>  drivers/mmc/host/sh_mobile_sdhi.c              | 20 +++-----------------
>  drivers/mmc/host/tmio_mmc_dma.c                |  6 ++----
>  include/linux/mfd/tmio.h                       |  2 --
>  include/linux/mmc/sh_mobile_sdhi.h             |  5 +++--
>  9 files changed, 28 insertions(+), 45 deletions(-)

Aren't you forgetting about arch/sh?

> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index efe3386..c4a0aab 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -185,23 +185,9 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>  		if (p->get_cd)
>  			mmc_data->get_cd = sh_mobile_sdhi_get_cd;
>  
> -		if (p->dma_slave_tx > 0 && p->dma_slave_rx > 0) {
> -			/*
> -			 * Yes, we have to provide slave IDs twice to TMIO:
> -			 * once as a filter parameter and once for channel
> -			 * configuration as an explicit slave ID
> -			 */
> -			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;
> +		if (p->dma_slave_tx && p->dma_slave_rx) {
> +			dma_priv->chan_priv_tx = p->dma_slave_tx;
> +			dma_priv->chan_priv_rx = p->dma_slave_rx;
>  			dma_priv->filter = p->dma_filter;
>  		}
>  	}
> diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c
> index 47bdb8f..caad28e 100644
> --- a/drivers/mmc/host/tmio_mmc_dma.c
> +++ b/drivers/mmc/host/tmio_mmc_dma.c
> @@ -290,8 +290,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat
>  		if (!host->chan_tx)
>  			return;
>  
> -		if (pdata->dma->chan_priv_tx)
> -			cfg.slave_id = pdata->dma->slave_id_tx;
> +		cfg.slave_id = 0; /* already set */
>  		cfg.direction = DMA_MEM_TO_DEV;
>  		cfg.dst_addr = res->start + (CTL_SD_DATA_PORT << host->bus_shift);
>  		cfg.src_addr = 0;
> @@ -308,8 +307,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat
>  		if (!host->chan_rx)
>  			goto ereqrx;
>  
> -		if (pdata->dma->chan_priv_rx)
> -			cfg.slave_id = pdata->dma->slave_id_rx;
> +		cfg.slave_id = 0;

slave_id is needed for the DMAC driver to configure slave DMA. And this 
_is_ the current mainstreem method (in the non-DT case) to configure slave 
DMA, AFAIK. In the non-DT case the filter function is used to verify, 
whether this slave can be served with this channel, and 
dmaengine_slave_config() is used to actually configure the channel for 
slave DMA.

>  		cfg.direction = DMA_DEV_TO_MEM;
>  		cfg.src_addr = cfg.dst_addr;
>  		cfg.dst_addr = 0;
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index ce35113..0990d8a 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -86,8 +86,6 @@ struct dma_chan;
>  struct tmio_mmc_dma {
>  	void *chan_priv_tx;
>  	void *chan_priv_rx;
> -	int slave_id_tx;
> -	int slave_id_rx;
>  	int alignment_shift;
>  	bool (*filter)(struct dma_chan *chan, void *arg);
>  };
> diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
> index 342f07b..66a7d1c 100644
> --- a/include/linux/mmc/sh_mobile_sdhi.h
> +++ b/include/linux/mmc/sh_mobile_sdhi.h
> @@ -2,6 +2,7 @@
>  #define LINUX_MMC_SH_MOBILE_SDHI_H
>  
>  #include <linux/types.h>
> +#include <linux/dmaengine.h>
>  
>  struct platform_device;
>  
> @@ -18,8 +19,8 @@ struct sh_mobile_sdhi_ops {
>  };
>  
>  struct sh_mobile_sdhi_info {
> -	int dma_slave_tx;
> -	int dma_slave_rx;
> +	void *dma_slave_tx;
> +	void *dma_slave_rx;
>  	dma_filter_fn dma_filter;
>  	unsigned long tmio_flags;
>  	unsigned long tmio_caps;
> -- 
> 1.8.1.2
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann June 7, 2013, 12:59 p.m. UTC | #3
On Friday 07 June 2013 12:22:15 Guennadi Liakhovetski wrote:

> >  arch/arm/mach-shmobile/board-ag5evm.c          |  4 ++--
> >  arch/arm/mach-shmobile/board-ap4evb.c          |  8 ++++----
> >  arch/arm/mach-shmobile/board-armadillo800eva.c |  8 ++++----
> >  arch/arm/mach-shmobile/board-kzm9g.c           |  8 ++++----
> >  arch/arm/mach-shmobile/board-mackerel.c        | 12 ++++++------
> >  drivers/mmc/host/sh_mobile_sdhi.c              | 20 +++-----------------
> >  drivers/mmc/host/tmio_mmc_dma.c                |  6 ++----
> >  include/linux/mfd/tmio.h                       |  2 --
> >  include/linux/mmc/sh_mobile_sdhi.h             |  5 +++--
> >  9 files changed, 28 insertions(+), 45 deletions(-)
> 
> Aren't you forgetting about arch/sh?

Right, sorry about that. I was assuming that sh_mobile_sdhi is only used on
ARM, which was clearly incorrect.

> > diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c
> > index 47bdb8f..caad28e 100644
> > --- a/drivers/mmc/host/tmio_mmc_dma.c
> > +++ b/drivers/mmc/host/tmio_mmc_dma.c
> > @@ -290,8 +290,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat
> >  		if (!host->chan_tx)
> >  			return;
> >  
> > -		if (pdata->dma->chan_priv_tx)
> > -			cfg.slave_id = pdata->dma->slave_id_tx;
> > +		cfg.slave_id = 0; /* already set */
> >  		cfg.direction = DMA_MEM_TO_DEV;
> >  		cfg.dst_addr = res->start + (CTL_SD_DATA_PORT << host->bus_shift);
> >  		cfg.src_addr = 0;
> > @@ -308,8 +307,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat
> >  		if (!host->chan_rx)
> >  			goto ereqrx;
> >  
> > -		if (pdata->dma->chan_priv_rx)
> > -			cfg.slave_id = pdata->dma->slave_id_rx;
> > +		cfg.slave_id = 0;
> 
> slave_id is needed for the DMAC driver to configure slave DMA. And this 
> _is_ the current mainstreem method (in the non-DT case) to configure slave 
> DMA, AFAIK. In the non-DT case the filter function is used to verify, 
> whether this slave can be served with this channel, and 
> dmaengine_slave_config() is used to actually configure the channel for 
> slave DMA.

I think that is a flaw in the dmaengine driver. I don't actually know how
any dma-engine driver could use dmaengine_slave_config to pick the request
line, since that is something the driver should not know at the time
it calls dmaengine_slave_config().

It's probably best to change the shdma driver to just ignore the slave_id
value in slave_config, like most other drivers to. I don't see how that
could possibly work when used from a portable slave driver.

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

[snip]

> I think that is a flaw in the dmaengine driver. I don't actually know how
> any dma-engine driver could use dmaengine_slave_config to pick the request
> line, since that is something the driver should not know at the time
> it calls dmaengine_slave_config().
> 
> It's probably best to change the shdma driver to just ignore the slave_id
> value in slave_config, like most other drivers to.

This is how shdma driver used to work. We used to pass the slave ID (DMA 
request line ID) to shdma from clients from the filter. This was 
considered bad and we were asked to switch to using 
dmaengine_slave_config(), which is the current preferred API, IIUC.

> I don't see how that
> could possibly work when used from a portable slave driver.

A DMA slave ID (DMA request line) is considered to be a portable 
parameter. I.e. on every platform your DMA slave has to request a DMA 
request line to the DMAC and that line can be coded with an int. That's 
the current assumption, I think.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann June 7, 2013, 2:53 p.m. UTC | #5
On Friday 07 June 2013, Guennadi Liakhovetski wrote:
> > I think that is a flaw in the dmaengine driver. I don't actually know how
> > any dma-engine driver could use dmaengine_slave_config to pick the request
> > line, since that is something the driver should not know at the time
> > it calls dmaengine_slave_config().
> > 
> > It's probably best to change the shdma driver to just ignore the slave_id
> > value in slave_config, like most other drivers to.
> 
> This is how shdma driver used to work. We used to pass the slave ID (DMA 
> request line ID) to shdma from clients from the filter. This was 
> considered bad and we were asked to switch to using 
> dmaengine_slave_config(), which is the current preferred API, IIUC.

I think you misunderstood the requirement. All other configuration,
i.e. slave port address, burst size, bus width, really should be
configured using dmaengine_slave_config, since this is data that
is known by the /driver/, independent of the DMA engine in use and
unknown to the platform. The request line number is something that
the driver cannot know, since it is a property of the DMA engine.

> > I don't see how that
> > could possibly work when used from a portable slave driver.
> 
> A DMA slave ID (DMA request line) is considered to be a portable 
> parameter. I.e. on every platform your DMA slave has to request a DMA 
> request line to the DMAC and that line can be coded with an int. That's 
> the current assumption, I think.

In a lot of cases you actually need more than one number here, e.g.
to configure which master port of the DMA engine is being used.
That's why the dmaengine DT binding needs a variable #dma-cells
property that is specific to the dma engine being used.

	Arnd
Guennadi Liakhovetski June 7, 2013, 3:32 p.m. UTC | #6
On Fri, 7 Jun 2013, Arnd Bergmann wrote:

> On Friday 07 June 2013, Guennadi Liakhovetski wrote:
> > > I think that is a flaw in the dmaengine driver. I don't actually know how
> > > any dma-engine driver could use dmaengine_slave_config to pick the request
> > > line, since that is something the driver should not know at the time
> > > it calls dmaengine_slave_config().
> > > 
> > > It's probably best to change the shdma driver to just ignore the slave_id
> > > value in slave_config, like most other drivers to.
> > 
> > This is how shdma driver used to work. We used to pass the slave ID (DMA 
> > request line ID) to shdma from clients from the filter. This was 
> > considered bad and we were asked to switch to using 
> > dmaengine_slave_config(), which is the current preferred API, IIUC.
> 
> I think you misunderstood the requirement. All other configuration,
> i.e. slave port address, burst size, bus width, really should be
> configured using dmaengine_slave_config, since this is data that
> is known by the /driver/, independent of the DMA engine in use and
> unknown to the platform. The request line number is something that
> the driver cannot know, since it is a property of the DMA engine.

Isn't it a board property? In case of external devices, or an SoC property 
in case of integrated DMA slave and controller? Let me try to explain. The 
DMAC has to configure the controller to "link" a specific DMA channel to a 
slave request line. E.g. to link an internal channel #0 to SDHI0 tx. To do 
this it has to write specific "magic" values in certain DMAC registers. 
Those values aren't known to the DMAC driver, they are a property of the 
SoC. Let's say, the SoC has 3 DMAC instances, each of them can serve any 
of compatible DMA slaves, including SDHI0, on each of their 6 channels. To 
configure a channel on a DMAC instance for SDHI0 tx you have to write to 
that channel's config register a certain value, that cannot be calculated. 

Those values are only known to the SoC code, so, they are passed as 
platform parameters to the DMAC driver. Now, when SDHI0 asks DMAC to set 
up a DMA channel for its tx, the DMAC driver has to find that value in its 
platform data. In most cases you could use the client address (e.g. FIFO 
register) and direction to find that value. But, I think, that might not 
always be enough. So, we use unique enum values to find those values in 
DMAC platform data. The SDHI driver gets that enum value in its platform 
data, passes it to the DMAC driver as a slave ID, and the DMAC driver uses 
it to find the value(s), it needs to set up its DMA channel. Do you see a 
better way to do the same?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann June 7, 2013, 4:06 p.m. UTC | #7
On Friday 07 June 2013, Guennadi Liakhovetski wrote:
> Isn't it a board property? In case of external devices, or an SoC property 
> in case of integrated DMA slave and controller? 

Correct.

> Let me try to explain. The 
> DMAC has to configure the controller to "link" a specific DMA channel to a 
> slave request line. E.g. to link an internal channel #0 to SDHI0 tx. To do 
> this it has to write specific "magic" values in certain DMAC registers. 
> Those values aren't known to the DMAC driver, they are a property of the 
> SoC. Let's say, the SoC has 3 DMAC instances, each of them can serve any 
> of compatible DMA slaves, including SDHI0, on each of their 6 channels. To 
> configure a channel on a DMAC instance for SDHI0 tx you have to write to 
> that channel's config register a certain value, that cannot be calculated. 

Right. Or multiple values.

> Those values are only known to the SoC code, so, they are passed as 
> platform parameters to the DMAC driver. Now, when SDHI0 asks DMAC to set 
> up a DMA channel for its tx, the DMAC driver has to find that value in its 
> platform data. In most cases you could use the client address (e.g. FIFO 
> register) and direction to find that value. But, I think, that might not 
> always be enough. So, we use unique enum values to find those values in 
> DMAC platform data. The SDHI driver gets that enum value in its platform 
> data, passes it to the DMAC driver as a slave ID, and the DMAC driver uses 
> it to find the value(s), it needs to set up its DMA channel. Do you see a 
> better way to do the same?

Ah, so you have multiple values, too, and you just abstract them by using
an enum to index an array of platform_data in the dma engine.

This obviously works as well, and lets you get away with a single 32-bit
enum to use as the slave-id. However, I think slave drivers should not
be written with the assumption that all dma-engine drivers do this.
In particular, you seem to assume that the argument to the filter function
is not a pointer but this enum.

It also gets more complicated with the DT binding, since that should
reflect the hardware settings, i.e. not an arbitrarily defined enum
but the data that you have in your array. To keep the DT and platform_data
cases similar, I would suggest you actually remove the array listing
the per-slave data, and move that data instead as an opaque pointer into
the platform data. You can take a look at drivers/mmc/host/mmci.c for
an example of a driver that does this and that is portable across
multiple DMA engine drivers.

Essentially we pass the dma_filter function and dma_param struct pointers
to dma_request_channel directly from the platform_data, without trying
to interpret them. In case of stedma40, the dma_param contains a complex
data structure, while for others it may contain a single integer.

If you do the same, you would essentially pass the mid_rid and chcr values
of your sh_dmae_slave_config as in the pointer that gets passed from
the platform_data down to the filter function, get rid of the slave_id
number and pass the addr value through slave_config.

	Arnd
Guennadi Liakhovetski June 19, 2013, 7:51 p.m. UTC | #8
Hi Vinod

Sorry, originally the 2 patches from this thread only touched files under 
mmc and ARM, so, you were not CCed, but eventually this turned into a 
dmaengine API discussion, so, I'm adding you to CC and asking about your 
opinion.

On Fri, 7 Jun 2013, Arnd Bergmann wrote:

> On Friday 07 June 2013, Guennadi Liakhovetski wrote:
> > Isn't it a board property? In case of external devices, or an SoC property 
> > in case of integrated DMA slave and controller? 
> 
> Correct.
> 
> > Let me try to explain. The 
> > DMAC has to configure the controller to "link" a specific DMA channel to a 
> > slave request line. E.g. to link an internal channel #0 to SDHI0 tx. To do 
> > this it has to write specific "magic" values in certain DMAC registers. 
> > Those values aren't known to the DMAC driver, they are a property of the 
> > SoC. Let's say, the SoC has 3 DMAC instances, each of them can serve any 
> > of compatible DMA slaves, including SDHI0, on each of their 6 channels. To 
> > configure a channel on a DMAC instance for SDHI0 tx you have to write to 
> > that channel's config register a certain value, that cannot be calculated. 
> 
> Right. Or multiple values.
> 
> > Those values are only known to the SoC code, so, they are passed as 
> > platform parameters to the DMAC driver. Now, when SDHI0 asks DMAC to set 
> > up a DMA channel for its tx, the DMAC driver has to find that value in its 
> > platform data. In most cases you could use the client address (e.g. FIFO 
> > register) and direction to find that value. But, I think, that might not 
> > always be enough. So, we use unique enum values to find those values in 
> > DMAC platform data. The SDHI driver gets that enum value in its platform 
> > data, passes it to the DMAC driver as a slave ID, and the DMAC driver uses 
> > it to find the value(s), it needs to set up its DMA channel. Do you see a 
> > better way to do the same?
> 
> Ah, so you have multiple values, too, and you just abstract them by using
> an enum to index an array of platform_data in the dma engine.
> 
> This obviously works as well, and lets you get away with a single 32-bit
> enum to use as the slave-id. However, I think slave drivers should not
> be written with the assumption that all dma-engine drivers do this.
> In particular, you seem to assume that the argument to the filter function
> is not a pointer but this enum.
> 
> It also gets more complicated with the DT binding, since that should
> reflect the hardware settings, i.e. not an arbitrarily defined enum
> but the data that you have in your array. To keep the DT and platform_data
> cases similar, I would suggest you actually remove the array listing
> the per-slave data, and move that data instead as an opaque pointer into
> the platform data. You can take a look at drivers/mmc/host/mmci.c for
> an example of a driver that does this and that is portable across
> multiple DMA engine drivers.
> 
> Essentially we pass the dma_filter function and dma_param struct pointers
> to dma_request_channel directly from the platform_data, without trying
> to interpret them. In case of stedma40, the dma_param contains a complex
> data structure, while for others it may contain a single integer.

And what happens then? How do those parameters get used by the stedma40 
driver to configure the hardware? I see, in its filter function the 
stedma40 driver is storing the configuration in the driver private channel 
struct for later use. SHDMA used to do the same (ok, we used the .private 
pointer for that, but essentially it was the same), but we were told to 
stop doing that. The filter function should only verify, whether a channel 
is suitable, and not set any configuration. That's what the 
dmaengine_slave_config() function is for, including the .slave_id 
parameter in struct dma_slave_config.

> If you do the same, you would essentially pass the mid_rid and chcr values
> of your sh_dmae_slave_config as in the pointer that gets passed from
> the platform_data down to the filter function, get rid of the slave_id
> number and pass the addr value through slave_config.

As explained in an earlier email, my opinion is, that my simple header 
patch is a corrent fix for the currently present build failure. As for 
this discussion, what is your opinion on this? Is the new use of the 
.slave_id field in struct dma_slave_config correct in the driver and its 
users or not? 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann June 19, 2013, 9:51 p.m. UTC | #9
On Wednesday 19 June 2013, Guennadi Liakhovetski wrote:
> > Ah, so you have multiple values, too, and you just abstract them by using
> > an enum to index an array of platform_data in the dma engine.
> > 
> > This obviously works as well, and lets you get away with a single 32-bit
> > enum to use as the slave-id. However, I think slave drivers should not
> > be written with the assumption that all dma-engine drivers do this.
> > In particular, you seem to assume that the argument to the filter function
> > is not a pointer but this enum.
> > 
> > It also gets more complicated with the DT binding, since that should
> > reflect the hardware settings, i.e. not an arbitrarily defined enum
> > but the data that you have in your array. To keep the DT and platform_data
> > cases similar, I would suggest you actually remove the array listing
> > the per-slave data, and move that data instead as an opaque pointer into
> > the platform data. You can take a look at drivers/mmc/host/mmci.c for
> > an example of a driver that does this and that is portable across
> > multiple DMA engine drivers.
> > 
> > Essentially we pass the dma_filter function and dma_param struct pointers
> > to dma_request_channel directly from the platform_data, without trying
> > to interpret them. In case of stedma40, the dma_param contains a complex
> > data structure, while for others it may contain a single integer.
> 
> And what happens then? How do those parameters get used by the stedma40 
> driver to configure the hardware? I see, in its filter function the 
> stedma40 driver is storing the configuration in the driver private channel 
> struct for later use. SHDMA used to do the same (ok, we used the .private 
> pointer for that, but essentially it was the same), but we were told to 
> stop doing that. The filter function should only verify, whether a channel 
> is suitable, and not set any configuration. That's what the 
> dmaengine_slave_config() function is for, including the .slave_id 
> parameter in struct dma_slave_config.

The problem with this is that on a lot of dma engines you have one channel
per slave (in particular with virt-dma.c), so the slave-id has to be
part of the filter data. Since the slave driver cannot know what kind
of DMA engine it is connected to, it has to assume that the slave-id
is inherent to the channel an cannot change.
Also, the DT binding is built around the idea that you identify the
channel or request line with the specifier in whichever form the
dma engine requires, and there is no general way for the slave driver
to find out a slave id. Setting it with dma_slave_config is inherently
nonportable, and we should stop doing that.

	Arnd
Guennadi Liakhovetski June 26, 2013, 10:10 a.m. UTC | #10
Hi Arnd

I think it would be good to come to a certain conclusion regarding the 
current dmaengine channel selection and configuration API and its possibly 
desired amendments.

On Wed, 19 Jun 2013, Arnd Bergmann wrote:

[snip]

> The problem with this is that on a lot of dma engines you have one channel
> per slave (in particular with virt-dma.c), so the slave-id has to be
> part of the filter data. Since the slave driver cannot know what kind
> of DMA engine it is connected to, it has to assume that the slave-id
> is inherent to the channel an cannot change.
> Also, the DT binding is built around the idea that you identify the
> channel or request line with the specifier in whichever form the
> dma engine requires, and there is no general way for the slave driver
> to find out a slave id. Setting it with dma_slave_config is inherently
> nonportable, and we should stop doing that.

Don't you think, that this situation, where without DT a filter function 
has to be used, which has to be provided by the platform and is just a 
kind of a platform callback; and with DT channels are selected and 
configured by a reference to suitable DMAC instances and a hardware- 
specific data set is suboptimal? Wouldn't it be better to unify it?

If yes, the unification would go in the direction of DT compatibility, 
i.e. dropping filter functions and just directly referencing required 
DMACs and using DMAC-specific configuration? Wouldn't a channel requesting 
API, similar to that for requesting clocks, pinmux settings be a better 
option, than the current filtering procedure? Something like

	dma_request_slave_channel(dev, "tx", "dmac0", config);

in a simple case of just one suitable DMAC, or a NULL if DMACs should be 
able to figure out themselves, whether they can serve this slave, or 
something like "dmamux0" if there is a multiplexer, for which a driver has 
to be available, and in which case "config" would be that multiplexer 
driver's configuration?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Arnd Bergmann June 26, 2013, 2:35 p.m. UTC | #11
On Wednesday 26 June 2013, Guennadi Liakhovetski wrote:
> > The problem with this is that on a lot of dma engines you have one channel
> > per slave (in particular with virt-dma.c), so the slave-id has to be
> > part of the filter data. Since the slave driver cannot know what kind
> > of DMA engine it is connected to, it has to assume that the slave-id
> > is inherent to the channel an cannot change.
> > Also, the DT binding is built around the idea that you identify the
> > channel or request line with the specifier in whichever form the
> > dma engine requires, and there is no general way for the slave driver
> > to find out a slave id. Setting it with dma_slave_config is inherently
> > nonportable, and we should stop doing that.
> 
> Don't you think, that this situation, where without DT a filter function 
> has to be used, which has to be provided by the platform and is just a 
> kind of a platform callback; and with DT channels are selected and 
> configured by a reference to suitable DMAC instances and a hardware- 
> specific data set is suboptimal? Wouldn't it be better to unify it?
> 
> If yes, the unification would go in the direction of DT compatibility, 
> i.e. dropping filter functions and just directly referencing required 
> DMACs and using DMAC-specific configuration?

With the introduction of dma_request_slave_channel() we tried to
only change the cases going forward with DT, SFI and ACPI but
without changing the interface for the existing drivers, which
are highly inconsistent at the moment (filter function being defined
in the slave driver /or/ the platform /or/ the dmaengine driver).

> Wouldn't a channel requesting 
> API, similar to that for requesting clocks, pinmux settings be a better 
> option, than the current filtering procedure? Something like
> 
>         dma_request_slave_channel(dev, "tx", "dmac0", config);
> 
> in a simple case of just one suitable DMAC, or a NULL if DMACs should be 
> able to figure out themselves, whether they can serve this slave, or 
> something like "dmamux0" if there is a multiplexer, for which a driver has 
> to be available, and in which case "config" would be that multiplexer 
> driver's configuration?

I think what you suggest here is very similar to the existing
dma_request_slave_channel_compat() function, except that you
pass a string ("dmac0") instead of the filter function pointer,
and that string presumably also has to come from the platform,
since there is no other way for the driver to know it, right?

One idea that Vinod suggested was that the platform could register
a table of DMA configurations with the dmaengine core to do the
lookup by device and string. That would actually help unify the
API to the current dma_request_slave_channel(dev, name) form
for all the possible cases (DT, ACPI, SFI, platform_data).

In essence, the platform would have a table like clk_lookup
but also containing the config:

struct dma_lookup {
	struct list_head node;
	const char *dev_id;	/* the slave device */
	const char *con_id;	/* request line of the slave, e.g. "rx" */
	const char *dmadev_id;	/* master device name */
	void *slave_id;		/* data passed to the master */
};

	Arnd
Vinod Koul June 26, 2013, 3:48 p.m. UTC | #12
On Wed, Jun 26, 2013 at 04:35:45PM +0200, Arnd Bergmann wrote:
> On Wednesday 26 June 2013, Guennadi Liakhovetski wrote:
> > > The problem with this is that on a lot of dma engines you have one channel
> > > per slave (in particular with virt-dma.c), so the slave-id has to be
> > > part of the filter data. Since the slave driver cannot know what kind
> > > of DMA engine it is connected to, it has to assume that the slave-id
> > > is inherent to the channel an cannot change.
> > > Also, the DT binding is built around the idea that you identify the
> > > channel or request line with the specifier in whichever form the
> > > dma engine requires, and there is no general way for the slave driver
> > > to find out a slave id. Setting it with dma_slave_config is inherently
> > > nonportable, and we should stop doing that.
> > 
> > Don't you think, that this situation, where without DT a filter function 
> > has to be used, which has to be provided by the platform and is just a 
> > kind of a platform callback; and with DT channels are selected and 
> > configured by a reference to suitable DMAC instances and a hardware- 
> > specific data set is suboptimal? Wouldn't it be better to unify it?
> > 
> > If yes, the unification would go in the direction of DT compatibility, 
> > i.e. dropping filter functions and just directly referencing required 
> > DMACs and using DMAC-specific configuration?
> 
> With the introduction of dma_request_slave_channel() we tried to
> only change the cases going forward with DT, SFI and ACPI but
> without changing the interface for the existing drivers, which
> are highly inconsistent at the moment (filter function being defined
> in the slave driver /or/ the platform /or/ the dmaengine driver).
> 
> > Wouldn't a channel requesting 
> > API, similar to that for requesting clocks, pinmux settings be a better 
> > option, than the current filtering procedure? Something like
> > 
> >         dma_request_slave_channel(dev, "tx", "dmac0", config);
> > 
> > in a simple case of just one suitable DMAC, or a NULL if DMACs should be 
> > able to figure out themselves, whether they can serve this slave, or 
> > something like "dmamux0" if there is a multiplexer, for which a driver has 
> > to be available, and in which case "config" would be that multiplexer 
> > driver's configuration?
> 
> I think what you suggest here is very similar to the existing
> dma_request_slave_channel_compat() function, except that you
> pass a string ("dmac0") instead of the filter function pointer,
> and that string presumably also has to come from the platform,
> since there is no other way for the driver to know it, right?
> 
> One idea that Vinod suggested was that the platform could register
> a table of DMA configurations with the dmaengine core to do the
> lookup by device and string. That would actually help unify the
> API to the current dma_request_slave_channel(dev, name) form
> for all the possible cases (DT, ACPI, SFI, platform_data).
> 
> In essence, the platform would have a table like clk_lookup
> but also containing the config:
> 
> struct dma_lookup {
> 	struct list_head node;
> 	const char *dev_id;	/* the slave device */
> 	const char *con_id;	/* request line of the slave, e.g. "rx" */
> 	const char *dmadev_id;	/* master device name */
> 	void *slave_id;		/* data passed to the master */
> };
Let me describes the scenarios we want to solve:
1. dma controllers with SW mux:
	In this case the dma controller has SW mux to connect to periphrals.
The mux is programmed to talk to a slave controller. SO we can grab any channel
from that controller, we just need to pass the mux value.
Ideally, in this scenario virtual channels should be described by driver (not
one per physical channel though) and driver uses mux values to transfer to
periphrals

2. dma controllers with hardwired channels:
	In this case you need a channel from a controller A and also channel X,
as it is hard wired to periphral. This is quite common in bunch of drivers.
In this case we just have to look for this channel.

3. MIX
	Yes, I believe we have controllers where they have SW mux as well as
hard wired.

In case of 1 and 3a(sw), we just need controller info
In 2 and3b we need both info

SO the idea was that DT, ACPI pdata etc just tells me that how the assignments
should be done and at runtime dmanegine just does that.

This is what I had in mind (more on same lines as above)
struct dmaengine_slave_map {
	char *dma;	/* dma controller */
	char *client;	/* client */
	int ch;		/* specfic channel id, NULL for SW muxes */
	int slave_id;	/* request line */
	struct dmaengine_slave_map_entries *entry;
};
https://lkml.org/lkml/2012/9/14/139

This way we just ask for a channel and dmaengine know what to do.

No filter functions, no multiple request methods etc.

--
~Vinod
diff mbox

Patch

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index ad01651..cb467fb 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -405,8 +405,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_USE_GPIO_CD,
diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
index c900e24..d7ccb2e 100644
--- a/arch/arm/mach-shmobile/board-ap4evb.c
+++ b/arch/arm/mach-shmobile/board-ap4evb.c
@@ -357,8 +357,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_caps	= MMC_CAP_SDIO_IRQ,
@@ -398,8 +398,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI1_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI1_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_ocr_mask	= MMC_VDD_165_195,
diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index e95e5dc..fcea414 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -690,8 +690,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
@@ -735,8 +735,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI1_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI1_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_caps	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
diff --git a/arch/arm/mach-shmobile/board-kzm9g.c b/arch/arm/mach-shmobile/board-kzm9g.c
index 0434b36..3f3092f 100644
--- a/arch/arm/mach-shmobile/board-kzm9g.c
+++ b/arch/arm/mach-shmobile/board-kzm9g.c
@@ -444,8 +444,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT,
@@ -489,8 +489,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI2_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI2_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT |
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 49755ff..4522b76 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -973,8 +973,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_flags	= TMIO_MMC_USE_GPIO_CD,
@@ -1015,8 +1015,8 @@  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_slave_tx	= (void *)SHDMA_SLAVE_SDHI1_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI1_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_USE_GPIO_CD,
@@ -1061,8 +1061,8 @@  static struct platform_device sdhi1_device = {
  */
 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_slave_tx	= (void *)SHDMA_SLAVE_SDHI2_TX,
+	.dma_slave_rx	= (void *)SHDMA_SLAVE_SDHI2_RX,
 	.dma_filter	= shdma_chan_filter,
 #endif
 	.tmio_flags	= TMIO_MMC_WRPROTECT_DISABLE | TMIO_MMC_USE_GPIO_CD,
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index efe3386..c4a0aab 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -185,23 +185,9 @@  static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		if (p->get_cd)
 			mmc_data->get_cd = sh_mobile_sdhi_get_cd;
 
-		if (p->dma_slave_tx > 0 && p->dma_slave_rx > 0) {
-			/*
-			 * Yes, we have to provide slave IDs twice to TMIO:
-			 * once as a filter parameter and once for channel
-			 * configuration as an explicit slave ID
-			 */
-			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;
+		if (p->dma_slave_tx && p->dma_slave_rx) {
+			dma_priv->chan_priv_tx = p->dma_slave_tx;
+			dma_priv->chan_priv_rx = p->dma_slave_rx;
 			dma_priv->filter = p->dma_filter;
 		}
 	}
diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c
index 47bdb8f..caad28e 100644
--- a/drivers/mmc/host/tmio_mmc_dma.c
+++ b/drivers/mmc/host/tmio_mmc_dma.c
@@ -290,8 +290,7 @@  void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat
 		if (!host->chan_tx)
 			return;
 
-		if (pdata->dma->chan_priv_tx)
-			cfg.slave_id = pdata->dma->slave_id_tx;
+		cfg.slave_id = 0; /* already set */
 		cfg.direction = DMA_MEM_TO_DEV;
 		cfg.dst_addr = res->start + (CTL_SD_DATA_PORT << host->bus_shift);
 		cfg.src_addr = 0;
@@ -308,8 +307,7 @@  void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat
 		if (!host->chan_rx)
 			goto ereqrx;
 
-		if (pdata->dma->chan_priv_rx)
-			cfg.slave_id = pdata->dma->slave_id_rx;
+		cfg.slave_id = 0;
 		cfg.direction = DMA_DEV_TO_MEM;
 		cfg.src_addr = cfg.dst_addr;
 		cfg.dst_addr = 0;
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index ce35113..0990d8a 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -86,8 +86,6 @@  struct dma_chan;
 struct tmio_mmc_dma {
 	void *chan_priv_tx;
 	void *chan_priv_rx;
-	int slave_id_tx;
-	int slave_id_rx;
 	int alignment_shift;
 	bool (*filter)(struct dma_chan *chan, void *arg);
 };
diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
index 342f07b..66a7d1c 100644
--- a/include/linux/mmc/sh_mobile_sdhi.h
+++ b/include/linux/mmc/sh_mobile_sdhi.h
@@ -2,6 +2,7 @@ 
 #define LINUX_MMC_SH_MOBILE_SDHI_H
 
 #include <linux/types.h>
+#include <linux/dmaengine.h>
 
 struct platform_device;
 
@@ -18,8 +19,8 @@  struct sh_mobile_sdhi_ops {
 };
 
 struct sh_mobile_sdhi_info {
-	int dma_slave_tx;
-	int dma_slave_rx;
+	void *dma_slave_tx;
+	void *dma_slave_rx;
 	dma_filter_fn dma_filter;
 	unsigned long tmio_flags;
 	unsigned long tmio_caps;