| Message ID | 1390890471-14882-3-git-send-email-agross@codeaurora.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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.
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.
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 --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"; + };
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