On Wed, Jun 26, 2013 at 04:35:45PM +0200, Arnd Bergmann wrote:

On Wednesday 26 June 2013, Guennadi Liakhovetski wrote:

quoted

quoted

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

quoted

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