| Message ID | 1370533645-23690-3-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
On Thursday 06 June 2013, Guennadi Liakhovetski wrote: > Documentation/devicetree/bindings/dma/dma.txt | 44 +++++++++++++++++++++++++ > drivers/dma/of-dma.c | 31 +++++++++++++---- > 2 files changed, 67 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt > index 8f504e6..a861298 100644 > --- a/Documentation/devicetree/bindings/dma/dma.txt > +++ b/Documentation/devicetree/bindings/dma/dma.txt > @@ -31,6 +31,50 @@ Example: > dma-requests = <127>; > }; > > +* DMA multiplexer > + > +Several DMA controllers with the same #dma-cells number and the same request > +line IDs can be grouped under a DMA multiplexer super-node, if slaves can use > +DMA channels on any of them. > + > +Required property: > +- compatible: Must include "dma-mux". > + > +Some standard optional properties can be helpful: > + > +Optional properties: > +- compatible: You will probably also want to include compatibility > + with "simple-bus" to automatically create platform > + devices from subnodes. > +- ranges: Map DMA controller memory areas in the parent address > + space. > +- #address-cells: Number of address cells in case automatic propagation > + with the help of "ranges" doesn't work. > +- #size-cells: Number of size cells. > + > +Example: > + > + dmac: dma-mux { > + compatible = "simple-bus", "dma-mux"; > + ranges; > + > + dma0: dma@10000000 { > + #dma-cells = <1>; > + ... > + }; > + > + dma1: dma@20000000 { > + #dma-cells = <1>; > + ... > + }; > + }; > + > + mmc0: mmc@30000000 { > + dmas = <&dmac 1 > + &dmac 2>; > + dma-names = "tx", "rx"; > + ... > + }; > > * DMA client Hmm, you've clearly shown that this can work, but it feels like a really odd way to do this. I don't think we should do it this way, because it tries to be really generic but then cannot some of the interesting cases, e.g. 1. you have multiplexed dma engines, but they need different #dma-cells. 1. you have multiplexed dma engines, but they need different dma specifiers. 2. The mux is between devices that are not on the same parent bus. I think this should either not try to be fully generic and just limited to the case you care about, i.e. shdma, or it should be more abstract and multiplex between the individual channels. In either case, I guess it would not need a change like this to the of_dma_request_slave_channel() function, and the node pointer used by the slave would be the same node that defines the address space for the channel descriptions with #dma-cells. I think the easiest way would be the first of those two, so it would essentially look like dmac: dma-mux { compatible = "renesas,shdma-mux"; /* not simple-bus! */ #dma-cells = <1>; #address-cells = <1>; #size-cells = <1>; dma@10000000 { compatible = "renesas,shdma"; reg = <0x10000000 0x1000>; interrupts = <10>; }; dma@20000000 { compatible = "renesas,shdma"; reg = <0x10000000 0x1000>; interrupts = <10>; }; } You then register a device driver for the master device, which will call of_dma_controller_register() for itself and create its child devices. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd Thanks for your comments On Mon, 17 Jun 2013, Arnd Bergmann wrote: > On Thursday 06 June 2013, Guennadi Liakhovetski wrote: > > Documentation/devicetree/bindings/dma/dma.txt | 44 +++++++++++++++++++++++++ > > drivers/dma/of-dma.c | 31 +++++++++++++---- > > 2 files changed, 67 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt > > index 8f504e6..a861298 100644 > > --- a/Documentation/devicetree/bindings/dma/dma.txt > > +++ b/Documentation/devicetree/bindings/dma/dma.txt > > @@ -31,6 +31,50 @@ Example: > > dma-requests = <127>; > > }; > > > > +* DMA multiplexer > > + > > +Several DMA controllers with the same #dma-cells number and the same request > > +line IDs can be grouped under a DMA multiplexer super-node, if slaves can use > > +DMA channels on any of them. > > + > > +Required property: > > +- compatible: Must include "dma-mux". > > + > > +Some standard optional properties can be helpful: > > + > > +Optional properties: > > +- compatible: You will probably also want to include compatibility > > + with "simple-bus" to automatically create platform > > + devices from subnodes. > > +- ranges: Map DMA controller memory areas in the parent address > > + space. > > +- #address-cells: Number of address cells in case automatic propagation > > + with the help of "ranges" doesn't work. > > +- #size-cells: Number of size cells. > > + > > +Example: > > + > > + dmac: dma-mux { > > + compatible = "simple-bus", "dma-mux"; > > + ranges; > > + > > + dma0: dma@10000000 { > > + #dma-cells = <1>; > > + ... > > + }; > > + > > + dma1: dma@20000000 { > > + #dma-cells = <1>; > > + ... > > + }; > > + }; > > + > > + mmc0: mmc@30000000 { > > + dmas = <&dmac 1 > > + &dmac 2>; > > + dma-names = "tx", "rx"; > > + ... > > + }; > > > > * DMA client > > Hmm, you've clearly shown that this can work, but it feels like a really odd way to > do this. I don't think we should do it this way, because it tries to be really > generic but then cannot some of the interesting cases, e.g. > > 1. you have multiplexed dma engines, but they need different #dma-cells. > 1. you have multiplexed dma engines, but they need different dma specifiers. > 2. The mux is between devices that are not on the same parent bus. Yes, I know about these restrictions. I'm not sure whether we really ever will get DMA multiplexers with different #dma-cells or DMA specifiers, but in any case we can make this less generic - either keep it as a generic "uniform multiplexer" or making it shdma specific - I'm fine either way. > I think this should either not try to be fully generic and just limited to > the case you care about, i.e. shdma, or it should be more abstract and > multiplex between the individual channels. In either case, I guess > it would not need a change like this to the of_dma_request_slave_channel() > function, and the node pointer used by the slave would be the same node > that defines the address space for the channel descriptions with #dma-cells. > > I think the easiest way would be the first of those two, so it would > essentially look like > > dmac: dma-mux { > compatible = "renesas,shdma-mux"; /* not simple-bus! */ > #dma-cells = <1>; > #address-cells = <1>; > #size-cells = <1>; > > dma@10000000 { > compatible = "renesas,shdma"; > reg = <0x10000000 0x1000>; > interrupts = <10>; > }; > dma@20000000 { > compatible = "renesas,shdma"; > reg = <0x10000000 0x1000>; > interrupts = <10>; > }; > } > > You then register a device driver for the master device, which > will call of_dma_controller_register() for itself and create > its child devices. Hmm, it is an interesting idea to only register one struct of_dma instance for all compatible shdma instances instead of one per shdma controller, and then call of_platform_populate() to instantiate all shdma instances. A couple of drawbacks: 1. we'll always have to put a mux DT node in .dts, even if there's just one DMAC instance on the system. 2. as AUXDATA for the new wrapper device we'll have to use an AUXDATA array for all child nodes, to be passed to of_platform_populate(). 3. it seems confusing to me - having one ofdma instance for multiple dmaengine devices. The advantage is, of course, that we don't need to extend existing OF and dmaengine APIs. So, I think, it can be done this way, but do you really think it'd be an improvement over my original proposal? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 18 June 2013, Guennadi Liakhovetski wrote: > > > > Hmm, you've clearly shown that this can work, but it feels like a really odd way to > > do this. I don't think we should do it this way, because it tries to be really > > generic but then cannot some of the interesting cases, e.g. > > > > 1. you have multiplexed dma engines, but they need different #dma-cells. > > 1. you have multiplexed dma engines, but they need different dma specifiers. > > 2. The mux is between devices that are not on the same parent bus. > > Yes, I know about these restrictions. I'm not sure whether we really ever > will get DMA multiplexers with different #dma-cells or DMA specifiers, but > in any case we can make this less generic - either keep it as a generic > "uniform multiplexer" or making it shdma specific - I'm fine either way. Ok, let's make it shdma specific then. > > I think this should either not try to be fully generic and just limited to > > the case you care about, i.e. shdma, or it should be more abstract and > > multiplex between the individual channels. In either case, I guess > > it would not need a change like this to the of_dma_request_slave_channel() > > function, and the node pointer used by the slave would be the same node > > that defines the address space for the channel descriptions with #dma-cells. > > > > I think the easiest way would be the first of those two, so it would > > essentially look like > > > > dmac: dma-mux { > > compatible = "renesas,shdma-mux"; /* not simple-bus! */ > > #dma-cells = <1>; > > #address-cells = <1>; > > #size-cells = <1>; > > > > dma@10000000 { > > compatible = "renesas,shdma"; > > reg = <0x10000000 0x1000>; > > interrupts = <10>; > > }; > > dma@20000000 { > > compatible = "renesas,shdma"; > > reg = <0x10000000 0x1000>; > > interrupts = <10>; > > }; > > } > > > > You then register a device driver for the master device, which > > will call of_dma_controller_register() for itself and create > > its child devices. > > Hmm, it is an interesting idea to only register one struct of_dma instance > for all compatible shdma instances instead of one per shdma controller, > and then call of_platform_populate() to instantiate all shdma instances. A > couple of drawbacks: > > 1. we'll always have to put a mux DT node in .dts, even if there's just > one DMAC instance on the system. > > 2. as AUXDATA for the new wrapper device we'll have to use an AUXDATA > array for all child nodes, to be passed to of_platform_populate(). My suggestion above is just one of the possible ways to do it, and I'm less concerned about it if you make it specific to shdma. Other options would be: 1 using phandles from the mux to the individual dma engine instances, but have them as independent nodes. 2 same as 1, but using phandles from the individual instances to the mux 3 Keep using the simple-bus. 4 Have just one combined device node for all shdma instances, with multiple 'reg' and 'interrupts' properties, and handle the muxing in the shdma driver. You could create platform_device_create_resndata from the top-level driver to reuse most of the existing code. In any of these cases you should be able to support both muxed and non-muxed operation in the shdma driver if you want to. You'd just have two separate ofdma registrations. > 3. it seems confusing to me - having one ofdma instance for multiple > dmaengine devices. I don't see a better way around this one, but I also don't see it as problematic. Wiht a mux, you always end up having just one node that the slaves point to, but multiple dma_device structures in the kernel. struct ofdma really refers to the first one. > The advantage is, of course, that we don't need to extend existing OF and > dmaengine APIs. > > So, I think, it can be done this way, but do you really think it'd be an > improvement over my original proposal? My main interest is to keep the shdma specifics out of the generic dmaengine binding document, where it just complicates the common case. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt index 8f504e6..a861298 100644 --- a/Documentation/devicetree/bindings/dma/dma.txt +++ b/Documentation/devicetree/bindings/dma/dma.txt @@ -31,6 +31,50 @@ Example: dma-requests = <127>; }; +* DMA multiplexer + +Several DMA controllers with the same #dma-cells number and the same request +line IDs can be grouped under a DMA multiplexer super-node, if slaves can use +DMA channels on any of them. + +Required property: +- compatible: Must include "dma-mux". + +Some standard optional properties can be helpful: + +Optional properties: +- compatible: You will probably also want to include compatibility + with "simple-bus" to automatically create platform + devices from subnodes. +- ranges: Map DMA controller memory areas in the parent address + space. +- #address-cells: Number of address cells in case automatic propagation + with the help of "ranges" doesn't work. +- #size-cells: Number of size cells. + +Example: + + dmac: dma-mux { + compatible = "simple-bus", "dma-mux"; + ranges; + + dma0: dma@10000000 { + #dma-cells = <1>; + ... + }; + + dma1: dma@20000000 { + #dma-cells = <1>; + ... + }; + }; + + mmc0: mmc@30000000 { + dmas = <&dmac 1 + &dmac 2>; + dma-names = "tx", "rx"; + ... + }; * DMA client diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 7aa0864..ec58022 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -146,8 +146,8 @@ static int of_dma_match_channel(struct device_node *np, const char *name, if (strcmp(name, s)) return -ENODEV; - if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index, - dma_spec)) + if (of_parse_phandle_with_child_args(np, "dmas", "#dma-cells", index, + dma_spec, "dma-mux")) return -ENODEV; return 0; @@ -165,7 +165,7 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, { struct of_phandle_args dma_spec; struct of_dma *ofdma; - struct dma_chan *chan; + struct dma_chan *chan = NULL; int count, i; if (!np || !name) { @@ -180,20 +180,35 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, } for (i = 0; i < count; i++) { + struct device_node *dma = NULL, *parent; + bool is_mux; + if (of_dma_match_channel(np, name, i, &dma_spec)) continue; mutex_lock(&of_dma_lock); - ofdma = of_dma_find_controller(&dma_spec); + parent = of_node_get(dma_spec.np->parent); + is_mux = of_device_is_compatible(parent, "dma-mux"); + + do { + /* If we're in a mux, try all nodes */ + if (is_mux) { + dma = of_get_next_available_child(parent, dma); + if (!dma) + break; + dma_spec.np = dma; + } + + ofdma = of_dma_find_controller(&dma_spec); - if (ofdma) - chan = ofdma->of_dma_xlate(&dma_spec, ofdma); - else - chan = NULL; + if (ofdma) + chan = ofdma->of_dma_xlate(&dma_spec, ofdma); + } while (dma && !chan); mutex_unlock(&of_dma_lock); of_node_put(dma_spec.np); + of_node_put(parent); if (chan) return chan;
If a slave can use any of several DMA controllers on the system, multiple DMA descriptors can be listed in its "dmas" DT property with the same channel name and different DMA controller phandles. However, if multiple such slaves can use any of a set of DMA controllers on the system, listing them all in each slave's "dmas" property becomes counterproductive. This patch adds support for a "dma-mux" DT node, whose sole purpose is to group such DMA controller DT nodes. Slaves can then specify the group's phandle in their "dmas" property. DMA controllers, belonging to the same group must have the same #dma-cells number and use the same slave IDs. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> --- Documentation/devicetree/bindings/dma/dma.txt | 44 +++++++++++++++++++++++++ drivers/dma/of-dma.c | 31 +++++++++++++---- 2 files changed, 67 insertions(+), 8 deletions(-)