diff mbox

[v3,2/2] dmaengine: qcom_bam_dma: Add device tree binding

Message ID 1390890471-14882-3-git-send-email-agross@codeaurora.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Andy Gross Jan. 28, 2014, 6:27 a.m. UTC
Add device tree binding support for the QCOM BAM DMA driver.

Signed-off-by: Andy Gross <agross@codeaurora.org>
---
 .../devicetree/bindings/dma/qcom_bam_dma.txt       |   52 ++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt

Comments

Lars-Peter Clausen Jan. 28, 2014, 9:05 a.m. UTC | #1
On 01/28/2014 07:27 AM, Andy Gross wrote:
> Add device tree binding support for the QCOM BAM DMA driver.
> 
> Signed-off-by: Andy Gross <agross@codeaurora.org>
> ---
>  .../devicetree/bindings/dma/qcom_bam_dma.txt       |   52 ++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> new file mode 100644
> index 0000000..53fd10a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> @@ -0,0 +1,52 @@
> +QCOM BAM DMA controller
> +
> +Required properties:
> +- compatible:	Must be "qcom,bam-v1.4.0" for MSM8974 V1
> +		Must be "qcom,bam-v1.4.1" for MSM8974 V2
> +- reg: Address range for DMA registers
> +- interrupts: single interrupt for this controller
> +- #dma-cells: must be <2>
> +- clocks: required clock
> +- clock-names: name of clock
> +- qcom,ee : indicates the active Execution Environment identifier (0-7)
> +
> +Example:
> +
> +	uart-bam: dma@f9984000 = {
> +		compatible = "qcom,bam-v1.4.1";
> +		reg = <0xf9984000 0x15000>;
> +		interrupts = <0 94 0>;
> +		clocks = <&gcc GCC_BAM_DMA_AHB_CLK>;
> +		clock-names = "bam_clk";
> +		#dma-cells = <2>;
> +		qcom,ee = <0>;
> +	};
> +
> +Client:
> +Required properties:
> +- dmas: List of dma channel requests
> +- dma-names: Names of aforementioned requested channels
> +
> +Clients must use the format described in the dma.txt file, using a three cell
> +specifier for each channel.
> +
> +The three cells in order are:
> +  1. A phandle pointing to the DMA controller
> +  2. The channel number
> +  3. Direction of the fixed unidirectional channel
> +     0 - Memory to Device
> +     1 - Device to Memory
> +     2 - Device to Device
> +

Why does the direction needs to be specified in specifier? I see two
options, either the direction per is fixed in hardware. In that case the DMA
controller node should describe which channel is which direction. Or the
direction is not fixed in hardware and can be changed at runtime in which
case it should be set on a per descriptor basis.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Jan. 28, 2014, 9:16 a.m. UTC | #2
On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > +
> > +Clients must use the format described in the dma.txt file, using a three cell
> > +specifier for each channel.
> > +
> > +The three cells in order are:
> > +  1. A phandle pointing to the DMA controller
> > +  2. The channel number
> > +  3. Direction of the fixed unidirectional channel
> > +     0 - Memory to Device
> > +     1 - Device to Memory
> > +     2 - Device to Device
> > +
> 
> Why does the direction needs to be specified in specifier? I see two
> options, either the direction per is fixed in hardware. In that case the DMA
> controller node should describe which channel is which direction. Or the
> direction is not fixed in hardware and can be changed at runtime in which
> case it should be set on a per descriptor basis.

Normally the direction is implied by dmaengine_slave_config().
Note that neither the dma slave API nor the generic DT binding
can actually support device-to-device transfers, since this
normally implies using two dma-request lines rather than one.

There might be a case where the direction is required in order
to allocate a channel, because the engine has specialized channels
per direction, and might connect any of them to any dma request
line. This does not seem to be the case for "bam", because
the DMA specifier already contains a specific channel number, not
a request line or slave ID number.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 28, 2014, 11:17 a.m. UTC | #3
On Tue, Jan 28, 2014 at 10:16:53AM +0100, Arnd Bergmann wrote:
> On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > Why does the direction needs to be specified in specifier? I see two
> > options, either the direction per is fixed in hardware. In that case the DMA
> > controller node should describe which channel is which direction. Or the
> > direction is not fixed in hardware and can be changed at runtime in which
> > case it should be set on a per descriptor basis.
> 
> Normally the direction is implied by dmaengine_slave_config().

No.  The direction argument in there is deprecated - we've been talking
about removing it for some time.

DMA engine drivers should store all parameters of the configuration, and
then select the appropriate ones when preparing a transfer (which itself
involves a direction.)

Not doing this implies that if you have a half-duplex device, you have to
repeatedly issue a dmaengine_slave_config() call, a prepare call, and a
submit call to the DMA engine code for every segment you want to transfer.
We don't need that kind of DMA engine specific behaviour in DMA engine
users.
Vinod Koul Jan. 28, 2014, 11:32 a.m. UTC | #4
On Tue, Jan 28, 2014 at 11:17:57AM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 28, 2014 at 10:16:53AM +0100, Arnd Bergmann wrote:
> > On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > > Why does the direction needs to be specified in specifier? I see two
> > > options, either the direction per is fixed in hardware. In that case the DMA
> > > controller node should describe which channel is which direction. Or the
> > > direction is not fixed in hardware and can be changed at runtime in which
> > > case it should be set on a per descriptor basis.
> > 
> > Normally the direction is implied by dmaengine_slave_config().
> 
> No.  The direction argument in there is deprecated - we've been talking
> about removing it for some time.
> 
> DMA engine drivers should store all parameters of the configuration, and
> then select the appropriate ones when preparing a transfer (which itself
> involves a direction.)

Right all the prep_ calls for slave cases have explcit direction argument so
sending it using slave config makes no sense. So will remove it after the merge
window closes and fix :)

--
~Vinod
> 
> Not doing this implies that if you have a half-duplex device, you have to
> repeatedly issue a dmaengine_slave_config() call, a prepare call, and a
> submit call to the DMA engine code for every segment you want to transfer.
> We don't need that kind of DMA engine specific behaviour in DMA engine
> users.
> 
> -- 
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".
> --
> 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
Arnd Bergmann Jan. 28, 2014, 12:05 p.m. UTC | #5
On Tuesday 28 January 2014 17:02:42 Vinod Koul wrote:
> On Tue, Jan 28, 2014 at 11:17:57AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Jan 28, 2014 at 10:16:53AM +0100, Arnd Bergmann wrote:
> > > On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > > > Why does the direction needs to be specified in specifier? I see two
> > > > options, either the direction per is fixed in hardware. In that case the DMA
> > > > controller node should describe which channel is which direction. Or the
> > > > direction is not fixed in hardware and can be changed at runtime in which
> > > > case it should be set on a per descriptor basis.
> > > 
> > > Normally the direction is implied by dmaengine_slave_config().
> > 
> > No.  The direction argument in there is deprecated - we've been talking
> > about removing it for some time.
> > 
> > DMA engine drivers should store all parameters of the configuration, and
> > then select the appropriate ones when preparing a transfer (which itself
> > involves a direction.)
> 
> Right all the prep_ calls for slave cases have explcit direction argument so
> sending it using slave config makes no sense. So will remove it after the merge
> window closes and fix 

Ok, thanks for clearing up my mistake. However, the argument remains:
the direction doesn't need to be in the DT DMA descriptor since it
gets set by software anyway.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Jan. 28, 2014, 12:08 p.m. UTC | #6
On Tuesday 28 January 2014 13:05:10 Arnd Bergmann wrote:
> On Tuesday 28 January 2014 17:02:42 Vinod Koul wrote:
> > On Tue, Jan 28, 2014 at 11:17:57AM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Jan 28, 2014 at 10:16:53AM +0100, Arnd Bergmann wrote:
> > > > On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > > > > Why does the direction needs to be specified in specifier? I see two
> > > > > options, either the direction per is fixed in hardware. In that case the DMA
> > > > > controller node should describe which channel is which direction. Or the
> > > > > direction is not fixed in hardware and can be changed at runtime in which
> > > > > case it should be set on a per descriptor basis.
> > > > 
> > > > Normally the direction is implied by dmaengine_slave_config().
> > > 
> > > No.  The direction argument in there is deprecated - we've been talking
> > > about removing it for some time.
> > > 
> > > DMA engine drivers should store all parameters of the configuration, and
> > > then select the appropriate ones when preparing a transfer (which itself
> > > involves a direction.)
> > 
> > Right all the prep_ calls for slave cases have explcit direction argument so
> > sending it using slave config makes no sense. So will remove it after the merge
> > window closes and fix 
> 
> Ok, thanks for clearing up my mistake. However, the argument remains:
> the direction doesn't need to be in the DT DMA descriptor since it
> gets set by software anyway.

On a related note, should we try to remove the slave_id field from
the slave config structure as well? I believe it is still used by
the shmobile dma engine in non-DT mode, but that is inconsistent with
how all the others work, and with what the same driver does for DT.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 28, 2014, 12:16 p.m. UTC | #7
On Tue, Jan 28, 2014 at 01:08:47PM +0100, Arnd Bergmann wrote:
> On a related note, should we try to remove the slave_id field from
> the slave config structure as well? I believe it is still used by
> the shmobile dma engine in non-DT mode, but that is inconsistent with
> how all the others work, and with what the same driver does for DT.

I didn't see that appear, but now that it is there, I'm in two minds
about it.

The first is that the virtual channel approach is more flexible (one
virtual channel per DMA request line) since it allows users to hold on
to a DMA engine virtual channel and don't have the overhead of getting
that.  It also means that they have access to the DMA engine struct
device, which should be used with the DMA API for mapping/unmapping etc.

Another advantage is that it is possible (though we don't really do this
at present) to schedule a number of virtual channels onto the underlying
physical channels according to whatever algorithm(s) we decide.

The second point is that requesting a physical channel and then
configuring it seems more elegant from the DMA engine point of view - but
has the down-side that clients have to release the DMA engine channel
(and thus forget the struct device) as soon as possible to avoid starving
the system of physical DMA channels.

On balance, I think the virtual channel approach makes client drivers
more elegant and simpler, and makes the DMA engine API easier to use,
and gives greater flexibility for future improvements.  So, I wouldn't
miss the slave_id being removed.
Russell King - ARM Linux Jan. 28, 2014, 1:01 p.m. UTC | #8
On Tue, Jan 28, 2014 at 01:05:10PM +0100, Arnd Bergmann wrote:
> Ok, thanks for clearing up my mistake. However, the argument remains:
> the direction doesn't need to be in the DT DMA descriptor since it
> gets set by software anyway.

Yes - for full-duplex, it's implied, since you have one DMA request
(and therefore virtual channel) for memory-to-device and another for
device-to-memory.
Andy Gross Jan. 28, 2014, 7:47 p.m. UTC | #9
On Tue, Jan 28, 2014 at 10:05:35AM +0100, Lars-Peter Clausen wrote:
> On 01/28/2014 07:27 AM, Andy Gross wrote:
> > Add device tree binding support for the QCOM BAM DMA driver.
> > 
> > Signed-off-by: Andy Gross <agross@codeaurora.org>
> > ---
> >  .../devicetree/bindings/dma/qcom_bam_dma.txt       |   52 ++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> > new file mode 100644
> > index 0000000..53fd10a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> > @@ -0,0 +1,52 @@
> > +QCOM BAM DMA controller
> > +
> > +Required properties:
> > +- compatible:	Must be "qcom,bam-v1.4.0" for MSM8974 V1
> > +		Must be "qcom,bam-v1.4.1" for MSM8974 V2
> > +- reg: Address range for DMA registers
> > +- interrupts: single interrupt for this controller
> > +- #dma-cells: must be <2>
> > +- clocks: required clock
> > +- clock-names: name of clock
> > +- qcom,ee : indicates the active Execution Environment identifier (0-7)
> > +
> > +Example:
> > +
> > +	uart-bam: dma@f9984000 = {
> > +		compatible = "qcom,bam-v1.4.1";
> > +		reg = <0xf9984000 0x15000>;
> > +		interrupts = <0 94 0>;
> > +		clocks = <&gcc GCC_BAM_DMA_AHB_CLK>;
> > +		clock-names = "bam_clk";
> > +		#dma-cells = <2>;
> > +		qcom,ee = <0>;
> > +	};
> > +
> > +Client:
> > +Required properties:
> > +- dmas: List of dma channel requests
> > +- dma-names: Names of aforementioned requested channels
> > +
> > +Clients must use the format described in the dma.txt file, using a three cell
> > +specifier for each channel.
> > +
> > +The three cells in order are:
> > +  1. A phandle pointing to the DMA controller
> > +  2. The channel number
> > +  3. Direction of the fixed unidirectional channel
> > +     0 - Memory to Device
> > +     1 - Device to Memory
> > +     2 - Device to Device
> > +
> 
> Why does the direction needs to be specified in specifier? I see two
> options, either the direction per is fixed in hardware. In that case the DMA
> controller node should describe which channel is which direction. Or the
> direction is not fixed in hardware and can be changed at runtime in which
> case it should be set on a per descriptor basis.
> 

It is specified because the direction is physically set by the hardware.  And
this can change depending on the attached peripheral.  It probably does make
more sense to set the direction in the controller.
Andy Gross Jan. 28, 2014, 7:50 p.m. UTC | #10
On Tue, Jan 28, 2014 at 10:16:53AM +0100, Arnd Bergmann wrote:
> On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > > +
> > > +Clients must use the format described in the dma.txt file, using a three cell
> > > +specifier for each channel.
> > > +
> > > +The three cells in order are:
> > > +  1. A phandle pointing to the DMA controller
> > > +  2. The channel number
> > > +  3. Direction of the fixed unidirectional channel
> > > +     0 - Memory to Device
> > > +     1 - Device to Memory
> > > +     2 - Device to Device
> > > +
> > 
> > Why does the direction needs to be specified in specifier? I see two
> > options, either the direction per is fixed in hardware. In that case the DMA
> > controller node should describe which channel is which direction. Or the
> > direction is not fixed in hardware and can be changed at runtime in which
> > case it should be set on a per descriptor basis.
> 
> Normally the direction is implied by dmaengine_slave_config().
> Note that neither the dma slave API nor the generic DT binding
> can actually support device-to-device transfers, since this
> normally implies using two dma-request lines rather than one.
> 
> There might be a case where the direction is required in order
> to allocate a channel, because the engine has specialized channels
> per direction, and might connect any of them to any dma request
> line. This does not seem to be the case for "bam", because
> the DMA specifier already contains a specific channel number, not
> a request line or slave ID number.
> 

In the case of BAM, the channels are hardcoded based on the attached peripheral.
For instance, if the BAM is attached to the BLSP UART, channel 0 is uart0-RX,
channel 1 is uart0-TX, channel 2 is uart1-RX... etc, etc.  So not only is the
direction hardcoded, but also the function.
Arnd Bergmann Jan. 29, 2014, 3:05 p.m. UTC | #11
On Tuesday 28 January 2014 12:16:56 Russell King - ARM Linux wrote:
> On Tue, Jan 28, 2014 at 01:08:47PM +0100, Arnd Bergmann wrote:
> 
> On balance, I think the virtual channel approach makes client drivers
> more elegant and simpler, and makes the DMA engine API easier to use,
> and gives greater flexibility for future improvements.  So, I wouldn't
> miss the slave_id being removed.

Ok, good. There are some dmaengine drivers that actually behave
in hardware like the virtual-channel extension, i.e. they have
one physical channel per request line (qcom_bam_dma seems to be
one of them in fact), so they don't really have a choice.

The way that both the DT and ACPI bindings are structured,
the request ID is always known by the time the channel is
allocated to allow this model, and that means supporting both
approaches in the same master or slave driver is a mess.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross Jan. 30, 2014, 6:23 a.m. UTC | #12
On Tue, Jan 28, 2014 at 10:16:53AM +0100, Arnd Bergmann wrote:
> On Tuesday 28 January 2014 10:05:35 Lars-Peter Clausen wrote:
> > > +
> > > +Clients must use the format described in the dma.txt file, using a three cell
> > > +specifier for each channel.
> > > +
> > > +The three cells in order are:
> > > +  1. A phandle pointing to the DMA controller
> > > +  2. The channel number
> > > +  3. Direction of the fixed unidirectional channel
> > > +     0 - Memory to Device
> > > +     1 - Device to Memory
> > > +     2 - Device to Device
> > > +
> > 
> > Why does the direction needs to be specified in specifier? I see two
> > options, either the direction per is fixed in hardware. In that case the DMA
> > controller node should describe which channel is which direction. Or the
> > direction is not fixed in hardware and can be changed at runtime in which
> > case it should be set on a per descriptor basis.
> 
> Normally the direction is implied by dmaengine_slave_config().
> Note that neither the dma slave API nor the generic DT binding
> can actually support device-to-device transfers, since this
> normally implies using two dma-request lines rather than one.
> 
> There might be a case where the direction is required in order
> to allocate a channel, because the engine has specialized channels
> per direction, and might connect any of them to any dma request
> line. This does not seem to be the case for "bam", because
> the DMA specifier already contains a specific channel number, not
> a request line or slave ID number.

After some deliberation, I think the best solution is removing the direction
from the DT for now.  It doesn't add anything except some verification
of direction.

As for the device to device:
As I mentioned before, each bam dma node is attached to a specific peripheral
(with one exception, but lets skip over that).  The peripherals allow for more
than one execution environment to access the peripheral and attached bam.  2 bam
channels can be connected to form a unidirectional pipe from one execution
environment to another.  Once the pipe is configured, the actually transfer
resembles a cyclical dma transfer and continues until you explicitly stop it.

That functionality will come later.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
new file mode 100644
index 0000000..53fd10a
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
@@ -0,0 +1,52 @@ 
+QCOM BAM DMA controller
+
+Required properties:
+- compatible:	Must be "qcom,bam-v1.4.0" for MSM8974 V1
+		Must be "qcom,bam-v1.4.1" for MSM8974 V2
+- reg: Address range for DMA registers
+- interrupts: single interrupt for this controller
+- #dma-cells: must be <2>
+- clocks: required clock
+- clock-names: name of clock
+- qcom,ee : indicates the active Execution Environment identifier (0-7)
+
+Example:
+
+	uart-bam: dma@f9984000 = {
+		compatible = "qcom,bam-v1.4.1";
+		reg = <0xf9984000 0x15000>;
+		interrupts = <0 94 0>;
+		clocks = <&gcc GCC_BAM_DMA_AHB_CLK>;
+		clock-names = "bam_clk";
+		#dma-cells = <2>;
+		qcom,ee = <0>;
+	};
+
+Client:
+Required properties:
+- dmas: List of dma channel requests
+- dma-names: Names of aforementioned requested channels
+
+Clients must use the format described in the dma.txt file, using a three cell
+specifier for each channel.
+
+The three cells in order are:
+  1. A phandle pointing to the DMA controller
+  2. The channel number
+  3. Direction of the fixed unidirectional channel
+     0 - Memory to Device
+     1 - Device to Memory
+     2 - Device to Device
+
+Example:
+	serial@f991e000 {
+		compatible = "qcom,msm-uart";
+		reg = <0xf991e000 0x1000>
+			<0xf9944000 0x19000>;
+		interrupts = <0 108 0>;
+		clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>;
+		clock-names = "core", "iface";
+
+		dmas = <&uart-bam 0 1>, <&uart-bam 1 0>;
+		dma-names = "rx", "tx";
+	};