| Message ID | 1370008605-3745603-2-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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/
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
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/
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
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/
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
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/
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
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/
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
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 --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;
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(-)