[PATCH 2/4] dmaengine: device tree bindings for PL08x

[PATCH 2/4] dmaengine: device tree bindings for PL08x

[Linus Walleij]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/dma/arm-pl08x.txt          | 182 +++++++++++++++++++++
 1 file changed, 182 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/arm-pl08x.txt

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;
+	};
+};
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] dmaengine: device tree bindings for PL08x

[Arnd Bergmann]

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

Re: [PATCH 2/4] dmaengine: device tree bindings for PL08x

[Linus Walleij]

On Tue, Sep 16, 2014 at 6:21 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] dmaengine: device tree bindings for PL08x

[Arnd Bergmann]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html