| Message ID | 1410507442-32451-1-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Friday 12 September 2014, Linus Walleij wrote: > This introduces device tree bindings for the PL08x DMA controllers > when used with fixed signal assignment per channel, i.e. if each > channel on the PL08x is assigned precisely one burst/single signal > set. > > In many incarnations that exist in the wild, a mux had been put > in front of the signals so that the system has to select a subset > of signals to handle from a larger set. This is not described in > the current binding: instead this is assumed to be handled with > a more elaborate binding especially for muxed signal cases. > > I imagine things like adding the property dma-mux = <&phandle>; > for the DMA controller in such cases, and not specifying any > signals for the channels, and provide a separate binding for > the mux to enlist its signals. If the mux handling is really simple, we could it part of the pl08 driver and key it off the compatible string -- for any of the known variants that use a mux, we then have the driver implement the muxing directly. Or it could be done the other way round: Have a binding for the mux that implements the generic dma binding, and a driver for it that registers with of_dma and forwards any dma_request_slave_channel() call to the underlying driver. That would even make the mux driver independent of pl08. I'm not worried about it at all, I'm sure we can do it when we want to. > +Required properties: > +- compatible: "arm,pl080", "arm,pl081", "arm,primecell" I don't think you'd want both pl080 and pl081 in the same device, so it should be one of the two, plus the primecell one. > +- reg: Address range of the PL08x registers > +- interrupt: The PL08x interrupt number > +- clocks: The clock running the IP core clock > +- clock-names: A list with one element with the name of the core clock This needs to list the required clock names. > +Optional sub-nodes: > +The slave transfer channels are assigned in consecutive order and > +identified by one child node per channel, assuming a fixed-signal > +per channel assignment and each with the following properties: > + > +Required properties: > +- signal: the name of the on-chip signal line handled by this channel > +- bus-interface-ahb1 or bus-interface-ahb2: tells the driver which > + bus interface(s) that is eligible for this specific channel. At least > + one of the interfaces must be specified, it is perfectly legal to > + specify both if the hardware supports using either interface. I'd really like to avoid this and move all of it into the dma specifier. I understand where you are coming from with this given the existing code, but I'd rather change the driver code to allow a simpler binding. > + saa0@dmac0 { > + signal = "saa0"; > + bus-interface-ahb1; > + }; The main value-add this gives you is a name of the signal, but nobody else does this, and it seems this is only an artifact of the way the driver today matches devices that come from platform data. If you want it just for migration purposes, we can probably put the names in the platform, but the bus-interface-* should IMHO be expressed in the dma specifier, the same way we do for the designware part. Arnd -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 16, 2014 at 6:21 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 12 September 2014, Linus Walleij wrote: >> This introduces device tree bindings for the PL08x DMA controllers >> when used with fixed signal assignment per channel, i.e. if each >> channel on the PL08x is assigned precisely one burst/single signal >> set. (...) > If the mux handling is really simple, we could it part of the > pl08 driver and key it off the compatible string -- for any > of the known variants that use a mux, we then have the driver > implement the muxing directly. I am also leaning toward this solution. Probably moving the mux handling to a separate file and Kconfig symbol with some header stubs that get compiled out unless explicitly enabled. > Or it could be done the other way round: Have a binding for the > mux that implements the generic dma binding, and a driver for > it that registers with of_dma and forwards any > dma_request_slave_channel() call to the underlying driver. > That would even make the mux driver independent of pl08. Yeah that is the big hammer. This is what we did for IRQ line muxes in drivers/irqchip/irq-crossbar.c, but maybe it's overkill. >> +Optional sub-nodes: >> +The slave transfer channels are assigned in consecutive order and >> +identified by one child node per channel, assuming a fixed-signal >> +per channel assignment and each with the following properties: >> + >> +Required properties: >> +- signal: the name of the on-chip signal line handled by this channel >> +- bus-interface-ahb1 or bus-interface-ahb2: tells the driver which >> + bus interface(s) that is eligible for this specific channel. At least >> + one of the interfaces must be specified, it is perfectly legal to >> + specify both if the hardware supports using either interface. > > I'd really like to avoid this and move all of it into the dma specifier. > I understand where you are coming from with this given the existing > code, but I'd rather change the driver code to allow a simpler binding. > >> + saa0@dmac0 { >> + signal = "saa0"; >> + bus-interface-ahb1; >> + }; > > The main value-add this gives you is a name of the signal, but nobody else > does this, and it seems this is only an artifact of the way the driver today > matches devices that come from platform data. > > If you want it just for migration purposes, we can probably put the names > in the platform, but the bus-interface-* should IMHO be expressed in the > dma specifier, the same way we do for the designware part. The main requirement is telling which master to use for each channel, really, I can do without the naming (though it is very helpful for debugging). I don't know how to provide a specifier for eligible bus interfaces in some elegant way with the DMA specifier, I could of course use two cells containing a plain number, specifying 0 for any interface, 1 for ahb1 and 2 for ahb2 like that: #define BUS_AHB_ANY 0 #define BUS_AHB1 1 #define BUS_AHB2 2 uart1: uart@101fb000 { compatible = "arm,pl011", "arm,primecell"; (...) dmas = <&dmac1 22 BUS_AHB1>, <&dmac1 23 BUS_AHB1>; dma-names = "rx", "tx"; }; The problem is that this is just so wrong: the bus master arbitration is a property of the DMA controller, not the consuming device. The DMA controller side is where the lines to the external buses are connected, and it is performing the work on behalf of the device. This way the knowledge is put in the totally wrong place. People get the impression that this is a property of the consumer... When it comes to the actual DMA signal line that is more natural to have in the specifier. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 06 November 2014 10:11:58 Linus Walleij wrote: > >> + saa0@dmac0 { > >> + signal = "saa0"; > >> + bus-interface-ahb1; > >> + }; > > > > The main value-add this gives you is a name of the signal, but nobody else > > does this, and it seems this is only an artifact of the way the driver today > > matches devices that come from platform data. > > > > If you want it just for migration purposes, we can probably put the names > > in the platform, but the bus-interface-* should IMHO be expressed in the > > dma specifier, the same way we do for the designware part. > > The main requirement is telling which master to use for each > channel, really, I can do without the naming (though it is very helpful > for debugging). > > I don't know how to provide a specifier for eligible bus interfaces > in some elegant way with the DMA specifier, I could of course use > two cells containing a plain number, specifying 0 for any interface, > 1 for ahb1 and 2 for ahb2 like that: > > #define BUS_AHB_ANY 0 > #define BUS_AHB1 1 > #define BUS_AHB2 2 > > uart1: uart@101fb000 { > compatible = "arm,pl011", "arm,primecell"; > (...) > dmas = <&dmac1 22 BUS_AHB1>, > <&dmac1 23 BUS_AHB1>; > dma-names = "rx", "tx"; > }; > > The problem is that this is just so wrong: the bus master arbitration > is a property of the DMA controller, not the consuming device. The DMA > controller side is where the lines to the external buses are connected, > and it is performing the work on behalf of the device. > > This way the knowledge is put in the totally wrong place. People get > the impression that this is a property of the consumer... > > When it comes to the actual DMA signal line that is more natural to > have in the specifier. Not sure about that, the approach you describe there sounds entirely reasonable to me. The DMA specifier is not just a feature of the consumer (the DMA slave) but of the connection between the consumer and the controller (slave and master). This connection consists of the combination of a request line and a bus master that is able to access the fifo register, so it makes sense to list both here. As far as I understand, the controller has to pick two master numbers here: one for the memory side and one for the fifo side. The memory side is a property of the dma engine, and that should always be the same one, while the fifo side depends on the slave that is connected. Arnd -- To unsubscribe from this list: send the line "unsubscribe dmaengine" 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/arm-pl08x.txt b/Documentation/devicetree/bindings/dma/arm-pl08x.txt new file mode 100644 index 000000000000..5e0aca09b56b --- /dev/null +++ b/Documentation/devicetree/bindings/dma/arm-pl08x.txt @@ -0,0 +1,182 @@ +* ARM PrimeCells PL080 and PL081 and derivatives DMA controller + +Required properties: +- compatible: "arm,pl080", "arm,pl081", "arm,primecell" +- reg: Address range of the PL08x registers +- interrupt: The PL08x interrupt number +- clocks: The clock running the IP core clock +- clock-names: A list with one element with the name of the core clock +- lli-bus-interface-ahb1: if AHB master 1 is eligible for fetching LLIs +- lli-bus-interface-ahb2: if AHB master 2 is eligible for fetching LLIs +- mem-bus-interface-ahb1: if AHB master 1 is eligible for fetching memory contents +- mem-bus-interface-ahb2: if AHB master 2 is eligible for fetching memory contents +- #dma-cells: must be <3> + +Optional properties: +- memcpy-burst-size: the size of the bursts for memcpy: 1, 4, 8, 16, 32 + 64, 128 or 256 bytes are legal values +- memcpy-bus-width: the bus width used for memcpy: 8, 16 or 32 are legal + values + +Optional sub-nodes: +The slave transfer channels are assigned in consecutive order and +identified by one child node per channel, assuming a fixed-signal +per channel assignment and each with the following properties: + +Required properties: +- signal: the name of the on-chip signal line handled by this channel +- bus-interface-ahb1 or bus-interface-ahb2: tells the driver which + bus interface(s) that is eligible for this specific channel. At least + one of the interfaces must be specified, it is perfectly legal to + specify both if the hardware supports using either interface. + +Clients +Required properties: +- dmas: Comma separated list of dma channel requests +- dma-names: Names of the aforementioned requested channels + +Example: + +dmac0: dma-controller@10130000 { + compatible = "arm,pl080", "arm,primecell"; + reg = <0x10130000 0x1000>; + interrupt-parent = <&vica>; + interrupts = <15>; + clocks = <&hclkdma0>; + clock-names = "apb_pclk"; + lli-bus-interface-ahb1; + lli-bus-interface-ahb2; + mem-bus-interface-ahb2; + memcpy-burst-size = <256>; + memcpy-bus-width = <32>; + #dma-cells = <1>; + /* Assignments for the 32 channels */ + saa0@dmac0 { + signal = "saa0"; + bus-interface-ahb1; + }; + saa1@dmac0 { + signal = "saa1"; + bus-interface-ahb1; + }; + saa2@dmac0 { + signal = "saa2"; + bus-interface-ahb1; + }; + saa3@dmac0 { + signal = "saa3"; + bus-interface-ahb1; + }; + saa4@dmac0 { + signal = "saa4"; + bus-interface-ahb1; + }; + saa5@dmac0 { + signal = "saa5"; + bus-interface-ahb1; + }; + saa6@dmac0 { + signal = "saa6"; + bus-interface-ahb1; + }; + saa7@dmac0 { + signal = "saa7"; + bus-interface-ahb1; + }; + unused@dmac0 { + signal = "unused"; + bus-interface-ahb1; + }; + fir@dmac0 { + signal = "firdatxrx"; + bus-interface-ahb1; + }; + msp0rx@dmac0 { + signal = "msp0rx"; + bus-interface-ahb1; + }; + msp0tx@dmac0 { + signal = "msp0tx"; + bus-interface-ahb1; + }; + ssprx@dmac0 { + signal = "ssprx"; + bus-interface-ahb1; + }; + ssptx@dmac0 { + signal = "ssptx"; + bus-interface-ahb1; + }; + uart0rx@dmac0 { + signal = "uart0rx"; + bus-interface-ahb1; + }; + uart0tx@dmac0 { + signal = "uart0tx"; + bus-interface-ahb1; + }; + hsirxch0@dmac0 { + signal = "hsirxch0"; + bus-interface-ahb1; + }; + hsirxch1@dmac0 { + signal = "hsirxch1"; + bus-interface-ahb1; + }; + hsirxch2@dmac0 { + signal = "hsirxch2"; + bus-interface-ahb1; + }; + hsirxch3@dmac0 { + signal = "hsirxch3"; + bus-interface-ahb1; + }; + hsirxch4@dmac0 { + signal = "hsirxch4"; + bus-interface-ahb1; + }; + hsirxch5@dmac0 { + signal = "hsirxch5"; + bus-interface-ahb1; + }; + hsirxch6@dmac0 { + signal = "hsirxch6"; + bus-interface-ahb1; + }; + hsirxch7@dmac0 { + signal = "hsirxch7"; + bus-interface-ahb1; + }; + hsitxch0@dmac0 { + signal = "hsitxch0"; + bus-interface-ahb1; + }; + hsitxch1@dmac0 { + signal = "hsitxch1"; + bus-interface-ahb1; + }; + hsitxch2@dmac0 { + signal = "hsitxch2"; + bus-interface-ahb1; + }; + hsitxch3@dmac0 { + signal = "hsitxch3"; + bus-interface-ahb1; + }; + hsitxch4@dmac0 { + signal = "hsitxch4"; + bus-interface-ahb1; + }; + hsitxch5@dmac0 { + signal = "hsitxch5"; + bus-interface-ahb1; + }; + hsitxch6@dmac0 { + signal = "hsitxch6"; + bus-interface-ahb1; + }; + hsitxch7@dmac0 { + signal = "hsitxch7"; + bus-interface-ahb1; + }; +};
This introduces device tree bindings for the PL08x DMA controllers when used with fixed signal assignment per channel, i.e. if each channel on the PL08x is assigned precisely one burst/single signal set. In many incarnations that exist in the wild, a mux had been put in front of the signals so that the system has to select a subset of signals to handle from a larger set. This is not described in the current binding: instead this is assumed to be handled with a more elaborate binding especially for muxed signal cases. I imagine things like adding the property dma-mux = <&phandle>; for the DMA controller in such cases, and not specifying any signals for the channels, and provide a separate binding for the mux to enlist its signals. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- .../devicetree/bindings/dma/arm-pl08x.txt | 182 +++++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/arm-pl08x.txt