On 06/14/2012 06:48 AM, Arnd Bergmann wrote:

On Wednesday 13 June 2012, Jon Hunter wrote:

quoted

quoted

As I said previously, I think just encoding the direction but not
the client specific ID (meaning we would have to disambiguate
the more complex cases that Stephen mentioned using a dma-names
property) would be the best because it keeps the common case simple,
but I could live with other things going in there too.

Ok, so you are saying that there are some DMA controllers where there is
no channel/request ID and only a direction is needed? So an even simpler
case than what I had imagined.

No, that was not the case I was thinking about.

Ok, good because that would be the most simple dma controller I have
heard about ;-)

quoted

So in that case, I don't see why the first cell after the phandle could
not be an index which could be either a direction or request-id and then
the next cell after that be a secondary match variable.

So simple case with just a index (either req-id or direction) ...

dmas = <&dmac0 index>

More complex case ...

dmas = <&dmac0 index match>

For example, for OMAP index = req-id and match = direction ...

dmas = <&dmac0 req-id direction>

Or am I way off the page?

The intention was instead to remove the need for the /index/ in those
cases, because having a client-specific index in here makes it inconsistent
with other similar bindings (reg, interrupts, gpios, ...) that people
are familiar with. They use an implicit index by counting the
fields in the respective properties.

So maybe "index" was not the right term to use here. What I meant was
that this is really the req/chan-id associated with this device. It is
not an arbitrary index that in turn gets resolved into the req-id. So in
other words, just like gpio where you have the gpio number, here you
have the dma req-id.

The existing method we have for avoiding index numbers is to use
named fields, like

	dmas = <&dmac0 matchA>, <dmac1 matchB>, <dmac2 matchC>;
	dma-names = "rx", "rx", "tx";

This is similar to how we use named interrupt and mmio resources, but
it requires that we always request the dma lines by name, which is
slightly more complex than we might want it to be.

Ok, but how do you get the req-id from the above binding? Doesn't it
need to be stored there somewhere even for the most simplest case? Or
are you saying that in this case you are just returning a name and the
dma driver resolves that into a req-id?

Because the vast majority of cases just use a single channel, or one
channel per direction, my idea was to encode the direction in the
dmas property, which lets us request a dma channel by direction from
the driver side, without explicitly encoding the name.

Yes, thats fine and so the direction is really the match criteria in
this case.

This would let us handle the following cases very easily:

1. one read-write channel

	dmas = <&dmac 0x3 match>;

Where 0x3 is the req-id? Just to confirm ;-)

Why not have match after the phandle to be consistent with the simple
example using name fields above?

2. a choice of two read-write channels:

	dmas = <&dmacA 0x3 matchA>, <&dmacB 0x3 matchB>;

3. one read-channel, one write channel:

	dmas = <&dmac 0x1 match-read>, <&dmac 0x2 match-write>;

4. a choice of two read channels and one write channel:

	dmas = <&dmacA 0x1 match-readA>, <&dmacA 0x2 match-write> 
			<&dmacB match-readB>;

And only the cases where we have more multiple channels that differ
in more aspects would require named properties:

5. two different channels

	dmas = <&dmac 0x3 match-rwdata>, <&dmac 0x1 match-status>;
	dma-names = "rwdata", "status";

6. same as 5, but with a choice of channels:

	dmas = <&dmacA 0x3 match-rwdataA>, <&dmacA 0x1 match-status>;
		<dmacB 0x3 match-rwdataB>;
	dma-names = "rwdata", "status", "rwdata";


With a definition like that, we can implement a very simple device
driver interface for the common cases, and a slightly more complex
one for the more complex cases:

1. chan = of_dma_request_channel(dev->of_node, 0);
2. chan = of_dma_request_channel(dev->of_node, 0);
3. rxchan = of_dma_request_channel(dev->of_node, DMA_MEM_TO_DEV);
   txchan = of_dma_request_channel(dev->of_node, DMA_DEV_TO_MEM);
4. rxchan = of_dma_request_channel(dev->of_node, DMA_MEM_TO_DEV);
   txchan = of_dma_request_channel(dev->of_node, DMA_DEV_TO_MEM);
5. chan = of_dma_request_named_channel(dev->of_node, "rwdata", 0);
   auxchan = of_dma_request_named_channel(dev->of_node, "status", DMA_DEV_TO_MEM);
6. chan = of_dma_request_named_channel(dev->of_node, "rwdata", 0);
   auxchan = of_dma_request_named_channel(dev->of_node, "status", DMA_DEV_TO_MEM);

Yes, that looks good to me.

Cheers
Jon