LinuxLists.cc - [PATCH v7 0/3] DMA: Freescale: Add support for 8-channel DMA engine

[permalink] [raw]

Subject: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

From: Hongbo Zhang <[email protected]>

Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch adds
the device tree nodes for them.

Signed-off-by: Hongbo Zhang <[email protected]>
---
.../devicetree/bindings/powerpc/fsl/dma.txt | 66 ++++++++++++++++
arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81 ++++++++++++++++++++
arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81 ++++++++++++++++++++
arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
5 files changed, 232 insertions(+), 4 deletions(-)
create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
index 6e9384b..2e66c3d 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
@@ -126,6 +126,72 @@ Example:
};
};

+** Freescale Elo3 DMA Controller
+ This is EloPlus controller with 8 channels, used in Freescale Txxx and Bxxx
+ series chips, such as t1040, t4240, b4860.
+
+Required properties:
+
+- compatible : must include "fsl,elo3-dma"
+- reg : <registers mapping for DMA general status reg>
+- ranges : describes the mapping between the address space of the
+ DMA channels and the address space of the DMA controller
+
+- DMA channel nodes:
+ - compatible : must include "fsl,eloplus-dma-channel"
+ - reg : <registers mapping for channel>
+ - interrupts : <interrupt mapping for DMA channel IRQ>
+ - interrupt-parent : optional, if needed for interrupt mapping
+
+Example:
+dma@100300 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,elo3-dma";
+ reg = <0x100300 0x4 0x100600 0x4>;
+ ranges = <0x0 0x100100 0x500>;
+ dma-channel@0 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x0 0x80>;
+ interrupts = <28 2 0 0>;
+ };
+ dma-channel@80 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x80 0x80>;
+ interrupts = <29 2 0 0>;
+ };
+ dma-channel@100 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x100 0x80>;
+ interrupts = <30 2 0 0>;
+ };
+ dma-channel@180 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x180 0x80>;
+ interrupts = <31 2 0 0>;
+ };
+ dma-channel@300 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x300 0x80>;
+ interrupts = <76 2 0 0>;
+ };
+ dma-channel@380 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x380 0x80>;
+ interrupts = <77 2 0 0>;
+ };
+ dma-channel@400 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x400 0x80>;
+ interrupts = <78 2 0 0>;
+ };
+ dma-channel@480 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x480 0x80>;
+ interrupts = <79 2 0 0>;
+ };
+};
+
Note on DMA channel compatible properties: The compatible property must say
"fsl,elo-dma-channel" or "fsl,eloplus-dma-channel" to be used by the Elo DMA
driver (fsldma). Any DMA channel used by fsldma cannot be used by another
diff --git a/arch/powerpc/boot/dts/fsl/b4si-post.dtsi b/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
index 7399154..ea53ea1 100644
--- a/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
@@ -223,13 +223,13 @@
reg = <0xe2000 0x1000>;
};

-/include/ "qoriq-dma-0.dtsi"
+/include/ "elo3-dma-0.dtsi"
dma@100300 {
fsl,iommu-parent = <&pamu0>;
fsl,liodn-reg = <&guts 0x580>; /* DMA1LIODNR */
};

-/include/ "qoriq-dma-1.dtsi"
+/include/ "elo3-dma-1.dtsi"
dma@101300 {
fsl,iommu-parent = <&pamu0>;
fsl,liodn-reg = <&guts 0x584>; /* DMA2LIODNR */
diff --git a/arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi b/arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
new file mode 100644
index 0000000..69a3277
--- /dev/null
+++ b/arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
@@ -0,0 +1,81 @@
+/*
+ * QorIQ DMA device tree stub [ controller @ offset 0x100000 ]
+ *
+ * Copyright 2013 Freescale Semiconductor Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * * Neither the name of Freescale Semiconductor nor the
+ * names of its contributors may be used to endorse or promote products
+ * derived from this software without specific prior written permission.
+ *
+ *
+ * ALTERNATIVELY, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") as published by the Free Software
+ * Foundation, either version 2 of that License or (at your option) any
+ * later version.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+dma0: dma@100300 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,elo3-dma";
+ reg = <0x100300 0x4 0x100600 0x4>;
+ ranges = <0x0 0x100100 0x500>;
+ dma-channel@0 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x0 0x80>;
+ interrupts = <28 2 0 0>;
+ };
+ dma-channel@80 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x80 0x80>;
+ interrupts = <29 2 0 0>;
+ };
+ dma-channel@100 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x100 0x80>;
+ interrupts = <30 2 0 0>;
+ };
+ dma-channel@180 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x180 0x80>;
+ interrupts = <31 2 0 0>;
+ };
+ dma-channel@300 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x300 0x80>;
+ interrupts = <76 2 0 0>;
+ };
+ dma-channel@380 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x380 0x80>;
+ interrupts = <77 2 0 0>;
+ };
+ dma-channel@400 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x400 0x80>;
+ interrupts = <78 2 0 0>;
+ };
+ dma-channel@480 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x480 0x80>;
+ interrupts = <79 2 0 0>;
+ };
+};
diff --git a/arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi b/arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
new file mode 100644
index 0000000..d410948
--- /dev/null
+++ b/arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
@@ -0,0 +1,81 @@
+/*
+ * QorIQ DMA device tree stub [ controller @ offset 0x101000 ]
+ *
+ * Copyright 2013 Freescale Semiconductor Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * * Neither the name of Freescale Semiconductor nor the
+ * names of its contributors may be used to endorse or promote products
+ * derived from this software without specific prior written permission.
+ *
+ *
+ * ALTERNATIVELY, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") as published by the Free Software
+ * Foundation, either version 2 of that License or (at your option) any
+ * later version.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+dma1: dma@101300 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,elo3-dma";
+ reg = <0x101300 0x4 0x101600 0x4>;
+ ranges = <0x0 0x101100 0x500>;
+ dma-channel@0 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x0 0x80>;
+ interrupts = <32 2 0 0>;
+ };
+ dma-channel@80 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x80 0x80>;
+ interrupts = <33 2 0 0>;
+ };
+ dma-channel@100 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x100 0x80>;
+ interrupts = <34 2 0 0>;
+ };
+ dma-channel@180 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x180 0x80>;
+ interrupts = <35 2 0 0>;
+ };
+ dma-channel@300 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x300 0x80>;
+ interrupts = <80 2 0 0>;
+ };
+ dma-channel@380 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x380 0x80>;
+ interrupts = <81 2 0 0>;
+ };
+ dma-channel@400 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x400 0x80>;
+ interrupts = <82 2 0 0>;
+ };
+ dma-channel@480 {
+ compatible = "fsl,eloplus-dma-channel";
+ reg = <0x480 0x80>;
+ interrupts = <83 2 0 0>;
+ };
+};
diff --git a/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
index bd611a9..ec95c60 100644
--- a/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
@@ -387,8 +387,8 @@
reg = <0xea000 0x4000>;
};

-/include/ "qoriq-dma-0.dtsi"
-/include/ "qoriq-dma-1.dtsi"
+/include/ "elo3-dma-0.dtsi"
+/include/ "elo3-dma-1.dtsi"

/include/ "qoriq-espi-0.dtsi"
spi@110000 {
--
1.7.9.5

2013-07-29 10:50:00

[permalink] [raw]

Subject: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document

From: Hongbo Zhang <[email protected]>

This patch updates the discription of each type of DMA controller and its
channels, it is preparation for adding another new DMA controller binding, it
also fixes some defects of indent for text alignment at the same time.

Signed-off-by: Hongbo Zhang <[email protected]>
---
.../devicetree/bindings/powerpc/fsl/dma.txt | 48 ++++++++------------
1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
index 2a4b4bc..6e9384b 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
@@ -1,33 +1,29 @@
-* Freescale 83xx DMA Controller
+* Freescale DMA Controllers

-Freescale PowerPC 83xx have on chip general purpose DMA controllers.
+** Freescale Elo DMA Controller
+ This is a little-endian DMA controller, used in Freescale mpc83xx series
+ chips such as mpc8315, mpc8349, mpc8379 etc.

Required properties:

-- compatible : compatible list, contains 2 entries, first is
- "fsl,CHIP-dma", where CHIP is the processor
- (mpc8349, mpc8360, etc.) and the second is
- "fsl,elo-dma"
+- compatible : must include "fsl,elo-dma"
- reg : <registers mapping for DMA general status reg>
-- ranges : Should be defined as specified in 1) to describe the
- DMA controller channels.
+- ranges : describes the mapping between the address space of the
+ DMA channels and the address space of the DMA controller
- cell-index : controller index. 0 for controller @ 0x8100
- interrupts : <interrupt mapping for DMA IRQ>
- interrupt-parent : optional, if needed for interrupt mapping

-
- DMA channel nodes:
- - compatible : compatible list, contains 2 entries, first is
- "fsl,CHIP-dma-channel", where CHIP is the processor
- (mpc8349, mpc8350, etc.) and the second is
- "fsl,elo-dma-channel". However, see note below.
+ - compatible : must include "fsl,elo-dma-channel"
+ However, see note below.
- reg : <registers mapping for channel>
- cell-index : dma channel index starts at 0.

Optional properties:
- interrupts : <interrupt mapping for DMA channel IRQ>
- (on 83xx this is expected to be identical to
- the interrupts property of the parent node)
+ (on 83xx this is expected to be identical to
+ the interrupts property of the parent node)
- interrupt-parent : optional, if needed for interrupt mapping

Example:
@@ -70,27 +66,23 @@ Example:
};
};

-* Freescale 85xx/86xx DMA Controller
-
-Freescale PowerPC 85xx/86xx have on chip general purpose DMA controllers.
+** Freescale EloPlus DMA Controller
+ This is DMA controller with extended addresses and chaining, mainly used in
+ Freescale mpc85xx/86xx, Pxxx and BSC series chips, such as mpc8540, mpc8641
+ p4080, bsc9131 etc.

Required properties:

-- compatible : compatible list, contains 2 entries, first is
- "fsl,CHIP-dma", where CHIP is the processor
- (mpc8540, mpc8540, etc.) and the second is
- "fsl,eloplus-dma"
+- compatible : must include "fsl,eloplus-dma"
- reg : <registers mapping for DMA general status reg>
- cell-index : controller index. 0 for controller @ 0x21000,
1 for controller @ 0xc000
-- ranges : Should be defined as specified in 1) to describe the
- DMA controller channels.
+- ranges : describes the mapping between the address space of the
+ DMA channels and the address space of the DMA controller

- DMA channel nodes:
- - compatible : compatible list, contains 2 entries, first is
- "fsl,CHIP-dma-channel", where CHIP is the processor
- (mpc8540, mpc8560, etc.) and the second is
- "fsl,eloplus-dma-channel". However, see note below.
+ - compatible : must include "fsl,eloplus-dma-channel"
+ However, see note below.
- cell-index : dma channel index starts at 0.
- reg : <registers mapping for channel>
- interrupts : <interrupt mapping for DMA channel IRQ>
--
1.7.9.5

2013-07-29 11:05:18

[permalink] [raw]

Subject: [PATCH v7 3/3] DMA: Freescale: update driver to support 8-channel DMA engine

From: Hongbo Zhang <[email protected]>

This patch adds support to 8-channel DMA engine, thus the driver works for both
the new 8-channel and the legacy 4-channel DMA engines.

Signed-off-by: Hongbo Zhang <[email protected]>
---
drivers/dma/Kconfig | 9 +++++----
drivers/dma/fsldma.c | 9 ++++++---
drivers/dma/fsldma.h | 2 +-
3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 6825957..3979c65 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -89,14 +89,15 @@ config AT_HDMAC
Support the Atmel AHB DMA controller.

config FSL_DMA
- tristate "Freescale Elo and Elo Plus DMA support"
+ tristate "Freescale Elo series DMA support"
depends on FSL_SOC
select DMA_ENGINE
select ASYNC_TX_ENABLE_CHANNEL_SWITCH
---help---
- Enable support for the Freescale Elo and Elo Plus DMA controllers.
- The Elo is the DMA controller on some 82xx and 83xx parts, and the
- Elo Plus is the DMA controller on 85xx and 86xx parts.
+ Enable support for the Freescale Elo series DMA controllers.
+ The Elo is the DMA controller on some mpc82xx and mpc83xx parts, the
+ EloPlus is on mpc85xx and mpc86xx and Pxxx parts, and the Elo3 is on
+ some Txxx and Bxxx parts.

config MPC512X_DMA
tristate "Freescale MPC512x built-in DMA engine support"
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 49e8fbd..16a9a48 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1261,7 +1261,9 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
WARN_ON(fdev->feature != chan->feature);

chan->dev = fdev->dev;
- chan->id = ((res.start - 0x100) & 0xfff) >> 7;
+ chan->id = (res.start & 0xfff) < 0x300 ?
+ ((res.start - 0x100) & 0xfff) >> 7 :
+ ((res.start - 0x200) & 0xfff) >> 7;
if (chan->id >= FSL_DMA_MAX_CHANS_PER_DEVICE) {
dev_err(fdev->dev, "too many channels for device\n");
err = -EINVAL;
@@ -1434,6 +1436,7 @@ static int fsldma_of_remove(struct platform_device *op)
}

static const struct of_device_id fsldma_of_ids[] = {
+ { .compatible = "fsl,elo3-dma", },
{ .compatible = "fsl,eloplus-dma", },
{ .compatible = "fsl,elo-dma", },
{}
@@ -1455,7 +1458,7 @@ static struct platform_driver fsldma_of_driver = {

static __init int fsldma_init(void)
{
- pr_info("Freescale Elo / Elo Plus DMA driver\n");
+ pr_info("Freescale Elo series DMA driver\n");
return platform_driver_register(&fsldma_of_driver);
}

@@ -1467,5 +1470,5 @@ static void __exit fsldma_exit(void)
subsys_initcall(fsldma_init);
module_exit(fsldma_exit);

-MODULE_DESCRIPTION("Freescale Elo / Elo Plus DMA driver");
+MODULE_DESCRIPTION("Freescale Elo series DMA driver");
MODULE_LICENSE("GPL");
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index f5c3879..1ffc244 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -112,7 +112,7 @@ struct fsldma_chan_regs {
};

struct fsldma_chan;
-#define FSL_DMA_MAX_CHANS_PER_DEVICE 4
+#define FSL_DMA_MAX_CHANS_PER_DEVICE 8

struct fsldma_device {
void __iomem *regs; /* DGSR register base */
--
1.7.9.5

2013-07-29 11:38:10

by Vinod Koul

[permalink] [raw]

Subject: Re: [PATCH v7 0/3] DMA: Freescale: Add support for 8-channel DMA engine

On Mon, Jul 29, 2013 at 06:49:01PM +0800, [email protected] wrote:
> From: Hongbo Zhang <[email protected]>
>
> Hi Vinod, Dan, Scott and Leo, please have a look at these V7 patches.
The dma relates changes look okay to me.

I need someone to review and ACK the DT bindings.

~Vinod
>
> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch set
> adds support this DMA engine.
>
> V6->V7 changes:
> - only remove unnecessary "CHIP-dma" explanations in [1/3]
>
> V5->V6 changes:
> - minor updates of descriptions in binding document and Kconfig
> - remove [4/4], that should be another patch in future
>
> V4->V5 changes:
> - update description in the dt binding document, to make it more resonable
> - add new patch [4/4] to eliminate a compiling warning which already exists
> for a long time
>
> V3->V4 changes:
> - introduce new patch [1/3] to revise the legacy dma binding document
> - and then add new paragraph to describe new dt node binding in [2/3]
> - rebase to latest kernel v3.11-rc1
>
> V2->V3 changes:
> - edit Documentation/devicetree/bindings/powerpc/fsl/dma.txt
> - edit text string in Kconfig and the driver files, using "elo series" to
> mention all the current "elo*"
>
> V1->V2 changes:
> - removed the codes handling the register dgsr1, since it isn't used currently
> - renamed the DMA DT compatible to "fsl,elo3-dma"
> - renamed the new dts files to "elo3-dma-<n>.dtsi"
>
> Hongbo Zhang (3):
> DMA: Freescale: revise device tree binding document
> DMA: Freescale: Add new 8-channel DMA engine device tree nodes
> DMA: Freescale: update driver to support 8-channel DMA engine
>
> .../devicetree/bindings/powerpc/fsl/dma.txt | 114 +++++++++++++++-----
> arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
> arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81 ++++++++++++++
> arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81 ++++++++++++++
> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
> drivers/dma/Kconfig | 9 +-
> drivers/dma/fsldma.c | 9 +-
> drivers/dma/fsldma.h | 2 +-
> 8 files changed, 264 insertions(+), 40 deletions(-)
> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
>
> --
> 1.7.9.5
>
>
>

--

2013-07-29 22:09:38

[permalink] [raw]

Subject: Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document

On 07/29/2013 05:49:02 AM, [email protected] wrote:
> From: Hongbo Zhang <[email protected]>
>
> This patch updates the discription of each type of DMA controller and
> its
> channels, it is preparation for adding another new DMA controller
> binding, it
> also fixes some defects of indent for text alignment at the same time.
>
> Signed-off-by: Hongbo Zhang <[email protected]>

ACK, but next time please do the whitespace fixes as a separate patch
so that it's easier to see what's actually changing.

-Scott

2013-07-29 22:11:05

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 07/29/2013 05:49:03 AM, [email protected] wrote:
> From: Hongbo Zhang <[email protected]>
>
> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this
> patch adds
> the device tree nodes for them.
>
> Signed-off-by: Hongbo Zhang <[email protected]>
> ---
> .../devicetree/bindings/powerpc/fsl/dma.txt | 66
> ++++++++++++++++
> arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
> arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81
> ++++++++++++++++++++
> arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81
> ++++++++++++++++++++
> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
> 5 files changed, 232 insertions(+), 4 deletions(-)
> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi

ACK

-Scott

2013-07-30 02:08:33

[permalink] [raw]

Subject: Re: [PATCH v7 0/3] DMA: Freescale: Add support for 8-channel DMA engine

On 07/29/2013 06:59 PM, Vinod Koul wrote:
> On Mon, Jul 29, 2013 at 06:49:01PM +0800, [email protected] wrote:
>> From: Hongbo Zhang <[email protected]>
>>
>> Hi Vinod, Dan, Scott and Leo, please have a look at these V7 patches.
> The dma relates changes look okay to me.
>
> I need someone to review and ACK the DT bindings.
Scott Wood has ACKed the [1/3] and [2/3].
Thank you Vinod.
>
> ~Vinod
>> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch set
>> adds support this DMA engine.
>>
>> V6->V7 changes:
>> - only remove unnecessary "CHIP-dma" explanations in [1/3]
>>
>> V5->V6 changes:
>> - minor updates of descriptions in binding document and Kconfig
>> - remove [4/4], that should be another patch in future
>>
>> V4->V5 changes:
>> - update description in the dt binding document, to make it more resonable
>> - add new patch [4/4] to eliminate a compiling warning which already exists
>> for a long time
>>
>> V3->V4 changes:
>> - introduce new patch [1/3] to revise the legacy dma binding document
>> - and then add new paragraph to describe new dt node binding in [2/3]
>> - rebase to latest kernel v3.11-rc1
>>
>> V2->V3 changes:
>> - edit Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>> - edit text string in Kconfig and the driver files, using "elo series" to
>> mention all the current "elo*"
>>
>> V1->V2 changes:
>> - removed the codes handling the register dgsr1, since it isn't used currently
>> - renamed the DMA DT compatible to "fsl,elo3-dma"
>> - renamed the new dts files to "elo3-dma-<n>.dtsi"
>>
>> Hongbo Zhang (3):
>> DMA: Freescale: revise device tree binding document
>> DMA: Freescale: Add new 8-channel DMA engine device tree nodes
>> DMA: Freescale: update driver to support 8-channel DMA engine
>>
>> .../devicetree/bindings/powerpc/fsl/dma.txt | 114 +++++++++++++++-----
>> arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
>> arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81 ++++++++++++++
>> arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81 ++++++++++++++
>> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
>> drivers/dma/Kconfig | 9 +-
>> drivers/dma/fsldma.c | 9 +-
>> drivers/dma/fsldma.h | 2 +-
>> 8 files changed, 264 insertions(+), 40 deletions(-)
>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
>>
>> --
>> 1.7.9.5
>>
>>
>>

2013-07-30 02:10:11

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 07/30/2013 06:10 AM, Scott Wood wrote:
> On 07/29/2013 05:49:03 AM, [email protected] wrote:
>> From: Hongbo Zhang <[email protected]>
>>
>> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this
>> patch adds
>> the device tree nodes for them.
>>
>> Signed-off-by: Hongbo Zhang <[email protected]>
>> ---
>> .../devicetree/bindings/powerpc/fsl/dma.txt | 66
>> ++++++++++++++++
>> arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
>> arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81
>> ++++++++++++++++++++
>> arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81
>> ++++++++++++++++++++
>> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
>> 5 files changed, 232 insertions(+), 4 deletions(-)
>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
>
> ACK
Thank you Scott for all the review comments.
>
> -Scott

2013-08-20 08:34:01

[permalink] [raw]

Subject: Re: [PATCH v7 0/3] DMA: Freescale: Add support for 8-channel DMA engine

On 07/29/2013 06:59 PM, Vinod Koul wrote:
> On Mon, Jul 29, 2013 at 06:49:01PM +0800, [email protected] wrote:
>> From: Hongbo Zhang <[email protected]>
>>
>> Hi Vinod, Dan, Scott and Leo, please have a look at these V7 patches.
> The dma relates changes look okay to me.
>
> I need someone to review and ACK the DT bindings.
>
> ~Vinod
Vinod,
Are you using this tree?
http://git.infradead.org/users/vkoul/slave-dma.git
Did you merge these patches?
Thanks.
>> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch set
>> adds support this DMA engine.
>>
>> V6->V7 changes:
>> - only remove unnecessary "CHIP-dma" explanations in [1/3]
>>
>> V5->V6 changes:
>> - minor updates of descriptions in binding document and Kconfig
>> - remove [4/4], that should be another patch in future
>>
>> V4->V5 changes:
>> - update description in the dt binding document, to make it more resonable
>> - add new patch [4/4] to eliminate a compiling warning which already exists
>> for a long time
>>
>> V3->V4 changes:
>> - introduce new patch [1/3] to revise the legacy dma binding document
>> - and then add new paragraph to describe new dt node binding in [2/3]
>> - rebase to latest kernel v3.11-rc1
>>
>> V2->V3 changes:
>> - edit Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>> - edit text string in Kconfig and the driver files, using "elo series" to
>> mention all the current "elo*"
>>
>> V1->V2 changes:
>> - removed the codes handling the register dgsr1, since it isn't used currently
>> - renamed the DMA DT compatible to "fsl,elo3-dma"
>> - renamed the new dts files to "elo3-dma-<n>.dtsi"
>>
>> Hongbo Zhang (3):
>> DMA: Freescale: revise device tree binding document
>> DMA: Freescale: Add new 8-channel DMA engine device tree nodes
>> DMA: Freescale: update driver to support 8-channel DMA engine
>>
>> .../devicetree/bindings/powerpc/fsl/dma.txt | 114 +++++++++++++++-----
>> arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
>> arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81 ++++++++++++++
>> arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81 ++++++++++++++
>> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
>> drivers/dma/Kconfig | 9 +-
>> drivers/dma/fsldma.c | 9 +-
>> drivers/dma/fsldma.h | 2 +-
>> 8 files changed, 264 insertions(+), 40 deletions(-)
>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
>>
>> --
>> 1.7.9.5
>>
>>
>>

2013-08-20 08:58:53

by Vinod Koul

[permalink] [raw]

Subject: Re: [PATCH v7 0/3] DMA: Freescale: Add support for 8-channel DMA engine

On Tue, Aug 20, 2013 at 04:33:46PM +0800, Hongbo Zhang wrote:
> On 07/29/2013 06:59 PM, Vinod Koul wrote:
> >On Mon, Jul 29, 2013 at 06:49:01PM +0800, [email protected] wrote:
> >>From: Hongbo Zhang <[email protected]>
> >>
> >>Hi Vinod, Dan, Scott and Leo, please have a look at these V7 patches.
> >The dma relates changes look okay to me.
> >
> >I need someone to review and ACK the DT bindings.
> >
> >~Vinod
> Vinod,
> Are you using this tree?
> http://git.infradead.org/users/vkoul/slave-dma.git
Yes

> Did you merge these patches?
No

As I said I would like someone who know DT and dma binding to ack them. I see
devicetree ML has been cced, can Arnd or someone else review these...

~Vinod

2013-08-21 06:59:27

[permalink] [raw]

Subject: Re: [PATCH v7 0/3] DMA: Freescale: Add support for 8-channel DMA engine

Hi DT maintainers,
Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
could you please have a look at [1/3] and [2/3] of these patch set.
These patches have been fully reviewed by Scott Wood, and the DMA
maintainer Vinod needs a Acted-by: from DT maintainers.
Thanks.

On 08/20/2013 04:15 PM, Vinod Koul wrote:
> On Tue, Aug 20, 2013 at 04:33:46PM +0800, Hongbo Zhang wrote:
>> On 07/29/2013 06:59 PM, Vinod Koul wrote:
>>> On Mon, Jul 29, 2013 at 06:49:01PM +0800, [email protected] wrote:
>>>> From: Hongbo Zhang <[email protected]>
>>>>
>>>> Hi Vinod, Dan, Scott and Leo, please have a look at these V7 patches.
>>> The dma relates changes look okay to me.
>>>
>>> I need someone to review and ACK the DT bindings.
>>>
>>> ~Vinod
>> Vinod,
>> Are you using this tree?
>> http://git.infradead.org/users/vkoul/slave-dma.git
> Yes
>
>> Did you merge these patches?
> No
>
> As I said I would like someone who know DT and dma binding to ack them. I see
> devicetree ML has been cced, can Arnd or someone else review these...
>
> ~Vinod
>

2013-08-21 22:33:22

[permalink] [raw]

Subject: Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document

On 07/29/2013 04:49 AM, [email protected] wrote:
> From: Hongbo Zhang <[email protected]>
>
> This patch updates the discription of each type of DMA controller and its
> channels, it is preparation for adding another new DMA controller binding, it
> also fixes some defects of indent for text alignment at the same time.

> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt

> -- compatible : compatible list, contains 2 entries, first is
> - "fsl,CHIP-dma", where CHIP is the processor
> - (mpc8349, mpc8360, etc.) and the second is
> - "fsl,elo-dma"
> +- compatible : must include "fsl,elo-dma"

Why remove the list of supported compatible values. Lately it seems that
we're moving towards listing more/all the values rather than removing
their documentation...

> -- ranges : Should be defined as specified in 1) to describe the
> - DMA controller channels.
> +- ranges : describes the mapping between the address space of the
> + DMA channels and the address space of the DMA controller

What is "the address space of the DMA controller". Perhaps this should
say "the CPU-visible address space" instead?

2013-08-21 22:40:51

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 07/29/2013 04:49 AM, [email protected] wrote:
> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch adds
> the device tree nodes for them.

> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt

> +** Freescale Elo3 DMA Controller
> + This is EloPlus controller with 8 channels, used in Freescale Txxx and Bxxx
> + series chips, such as t1040, t4240, b4860.
> +
> +Required properties:
> +
> +- compatible : must include "fsl,elo3-dma"

This should probably list all the SoC-specific compatible values too.

> +- ranges : describes the mapping between the address space of the
> + DMA channels and the address space of the DMA controller

Oh, so looking at the example, this is simply about being able to write
the reg value in the child nodes more easily without having to write out
the full based address of the controller in each child node.

I don't think the binding document should require this; all the binding
document should care about is that the child nodes have a valid reg
value. Whether that reg value is <0x100100 0x80> without a ranges in the
top-level DMA nor or whether that reg value is <0x0 0x80> with a ranges
value in the top-level DMA node isn't something that the binding should
specify. Either way will work equally without affecting a driver for the
DMA controller; the parsing of reg with/without a ranges property is
more of a core part of DT than anything to do with this binding.

> +- DMA channel nodes:
> + - compatible : must include "fsl,eloplus-dma-channel"

Why do the channel nodes even need a compatible value? Presumably the
driver for the top-level DMA node will scan these dma-channel nodes to
extract the information it needs and will simply assume that all these
nodes are DMA channel nodes rather than something else? I suppose this
doesn't hurt, it just seems unnecessary unless you foresee other child
nodes types existing in the future and hence a need to differentiate
different types of nodes.

> + - reg : <registers mapping for channel>
> + - interrupts : <interrupt mapping for DMA channel IRQ>

s/interrupts/specifier/

> +Example:
> +dma@100300 {
> + #address-cells = <1>;
> + #size-cells = <1>;

Those weren't mentioned in the required properties list above. Perhaps
they're considered such a core part of DT functionality that it's not
necessary, yet some other binding documents do include these properties
in the list of required properties.

2013-08-21 22:46:09

[permalink] [raw]

Subject: Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document

On Wed, 2013-08-21 at 16:33 -0600, Stephen Warren wrote:
> On 07/29/2013 04:49 AM, [email protected] wrote:
> > From: Hongbo Zhang <[email protected]>
> >
> > This patch updates the discription of each type of DMA controller and its
> > channels, it is preparation for adding another new DMA controller binding, it
> > also fixes some defects of indent for text alignment at the same time.
>
> > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>
> > -- compatible : compatible list, contains 2 entries, first is
> > - "fsl,CHIP-dma", where CHIP is the processor
> > - (mpc8349, mpc8360, etc.) and the second is
> > - "fsl,elo-dma"
> > +- compatible : must include "fsl,elo-dma"
>
> Why remove the list of supported compatible values. Lately it seems that
> we're moving towards listing more/all the values rather than removing
> their documentation...

Previous versions had language that required fsl,CHIP-dma for 83xx (and
maybe 85xx?) but not the new chip. I asked for it to be consistent.
The reason that 83xx still has fsl,CHIP-dma is not because of anything
special to 83xx, but that most other chips with this device have been
converted to dtsi and it's much more of a pain to specify the specific
SoC in that context. The existing language does not match actual device
trees when it comes to 85xx.

Plus, the exact SoC name is of dubious value for integrated devices. It
doesn't uniquely identify the hardware because different versions of the
SoC could have different versions of the subdevice. As such, on our
chips we've been moving away from including a compatible that specifies
the exact SoC. If it turns out we made a mistake in naming different
versions of the device, or if there are errata, the exact SoC can still
be determined at runtime using SVR.

> > -- ranges : Should be defined as specified in 1) to describe the
> > - DMA controller channels.
> > +- ranges : describes the mapping between the address space of the
> > + DMA channels and the address space of the DMA controller
>
> What is "the address space of the DMA controller". Perhaps this should
> say "the CPU-visible address space" instead?

It's translating from the addresses used in the child nodes to a CCSR
offset. It's really just a convenience for the readability and
macro-ability of the device tree that we do this translation at all,
versus having an empty ranges and using CCSR offsets in the children.

It's not about translating between the DMA controller's view and the
CPU's view or anything like that.

-Scott

2013-08-21 22:57:21

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
> On 07/29/2013 04:49 AM, [email protected] wrote:
> > Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch adds
> > the device tree nodes for them.
>
> > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>
> > +** Freescale Elo3 DMA Controller
> > + This is EloPlus controller with 8 channels, used in Freescale Txxx and Bxxx
> > + series chips, such as t1040, t4240, b4860.
> > +
> > +Required properties:
> > +
> > +- compatible : must include "fsl,elo3-dma"
>
> This should probably list all the SoC-specific compatible values too.

We're not going to specify them in the device tree (see my comment on
patch 1/3), so we probably shouldn't lie about them in the binding like
eloplus currently does. :-)

> > +- ranges : describes the mapping between the address space of the
> > + DMA channels and the address space of the DMA controller
>
> Oh, so looking at the example, this is simply about being able to write
> the reg value in the child nodes more easily without having to write out
> the full based address of the controller in each child node.
>
> I don't think the binding document should require this;

It doesn't. It just requires that there be a mapping; it doesn't have
to be any particular mapping.

> all the binding document should care about is that the child nodes have a valid reg
> value. Whether that reg value is <0x100100 0x80> without a ranges in the
> top-level DMA

Without a ranges property there is no translation and the registers
would not be memory mappable. Linux may treat the absence of ranges as
an identity mapping for compatibility with some broken OF trees, but
it's not standard.

> nor or whether that reg value is <0x0 0x80> with a ranges
> value in the top-level DMA node isn't something that the binding should
> specify. Either way will work equally without affecting a driver for the
> DMA controller; the parsing of reg with/without a ranges property is
> more of a core part of DT than anything to do with this binding.
>
> > +- DMA channel nodes:
> > + - compatible : must include "fsl,eloplus-dma-channel"
>
> Why do the channel nodes even need a compatible value? Presumably the
> driver for the top-level DMA node will scan these dma-channel nodes to
> extract the information it needs and will simply assume that all these
> nodes are DMA channel nodes rather than something else? I suppose this
> doesn't hurt, it just seems unnecessary unless you foresee other child
> nodes types existing in the future and hence a need to differentiate
> different types of nodes.

Other than "this is how the existing binding works and we're not going
to break compatibility", it allows the OS more flexibility to choose
whether to bind to controllers or directly to the channels. Sometimes a
channel will be labelled with a different compatible if it has a fixed
purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts
where some channels are "fsl,ssi-dma-channel").

The channels are mostly independent. Only an interrupt is shared on
elo, and only an IOMMU device number on eloplus/elo3 -- plus a shared
status register that doesn't have to be used and doesn't make sense to
use without a shared interrupt.

> > + - reg : <registers mapping for channel>
> > + - interrupts : <interrupt mapping for DMA channel IRQ>
>
> s/interrupts/specifier/
>
> > +Example:
> > +dma@100300 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
>
> Those weren't mentioned in the required properties list above.

It's inherent in the existence of child nodes with reg (unless you rely
on the default of 2/1, which is discouraged). The binding should
mention it if it has particular requirements for the value of either
property, but I don't think we care here (much like we don't care what
sort of translation is used in ranges).

> Perhaps they're considered such a core part of DT functionality that it's not
> necessary, yet some other binding documents do include these properties
> in the list of required properties.

Some bindings even try to repeat the definition of standard properties
-- often incorrectly. We should avoid that.

-Scott

2013-08-21 23:00:38

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
> On 07/29/2013 04:49 AM, [email protected] wrote:
> > + - reg : <registers mapping for channel>
> > + - interrupts : <interrupt mapping for DMA channel IRQ>
>
> s/interrupts/specifier/

Do you mean s/interrupt mapping/interrupt specifier/?

And probably s/registers mapping/register specifier/ as well.

-Scott

2013-08-21 23:12:59

[permalink] [raw]

Subject: Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document

On 08/21/2013 04:45 PM, Scott Wood wrote:
> On Wed, 2013-08-21 at 16:33 -0600, Stephen Warren wrote:
>> On 07/29/2013 04:49 AM, [email protected] wrote:
>>> From: Hongbo Zhang <[email protected]>
>>>
>>> This patch updates the discription of each type of DMA controller and its
>>> channels, it is preparation for adding another new DMA controller binding, it
>>> also fixes some defects of indent for text alignment at the same time.
>>
>>> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>>
>>> -- compatible : compatible list, contains 2 entries, first is
>>> - "fsl,CHIP-dma", where CHIP is the processor
>>> - (mpc8349, mpc8360, etc.) and the second is
>>> - "fsl,elo-dma"
>>> +- compatible : must include "fsl,elo-dma"
>>
>> Why remove the list of supported compatible values. Lately it seems that
>> we're moving towards listing more/all the values rather than removing
>> their documentation...
>
> Previous versions had language that required fsl,CHIP-dma for 83xx (and
> maybe 85xx?) but not the new chip. I asked for it to be consistent.
> The reason that 83xx still has fsl,CHIP-dma is not because of anything
> special to 83xx, but that most other chips with this device have been
> converted to dtsi and it's much more of a pain to specify the specific
> SoC in that context. The existing language does not match actual device
> trees when it comes to 85xx.
>
> Plus, the exact SoC name is of dubious value for integrated devices. It
> doesn't uniquely identify the hardware because different versions of the
> SoC could have different versions of the subdevice. As such, on our
> chips we've been moving away from including a compatible that specifies
> the exact SoC. If it turns out we made a mistake in naming different
> versions of the device, or if there are errata, the exact SoC can still
> be determined at runtime using SVR.

OK, if there's some alternative run-time way of enabling chip-specific
quirking, it's probably fine to remove the extra compatible values.

Now, that does rather assume that this DMA IP block will only ever be
used within SoCs that have that SVR concept, but perhaps if that's ever
not the case, we can simply go back to requiring extra compatible values
in those specific cases?

2013-08-21 23:15:53

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 08/21/2013 04:57 PM, Scott Wood wrote:
> On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
>> On 07/29/2013 04:49 AM, [email protected] wrote:

>>> +- ranges : describes the mapping between the address space of the
>>> + DMA channels and the address space of the DMA controller
>>
>> Oh, so looking at the example, this is simply about being able to write
>> the reg value in the child nodes more easily without having to write out
>> the full based address of the controller in each child node.
>>
>> I don't think the binding document should require this;
>
> It doesn't. It just requires that there be a mapping; it doesn't have
> to be any particular mapping.
>
>> all the binding document should care about is that the child nodes have a valid reg
>> value. Whether that reg value is <0x100100 0x80> without a ranges in the
>> top-level DMA
>
> Without a ranges property there is no translation and the registers
> would not be memory mappable. Linux may treat the absence of ranges as
> an identity mapping for compatibility with some broken OF trees, but
> it's not standard.

I would argue that missing ranges meaning 1:1 translation is now a
standard, given that it must be true to support some DTs, it therefore
can now be assumed?

>> nor or whether that reg value is <0x0 0x80> with a ranges
>> value in the top-level DMA node isn't something that the binding should
>> specify. Either way will work equally without affecting a driver for the
>> DMA controller; the parsing of reg with/without a ranges property is
>> more of a core part of DT than anything to do with this binding.
>>
>>> +- DMA channel nodes:
>>> + - compatible : must include "fsl,eloplus-dma-channel"
>>
>> Why do the channel nodes even need a compatible value? Presumably the
>> driver for the top-level DMA node will scan these dma-channel nodes to
>> extract the information it needs and will simply assume that all these
>> nodes are DMA channel nodes rather than something else? I suppose this
>> doesn't hurt, it just seems unnecessary unless you foresee other child
>> nodes types existing in the future and hence a need to differentiate
>> different types of nodes.
>
> Other than "this is how the existing binding works and we're not going
> to break compatibility", it allows the OS more flexibility to choose
> whether to bind to controllers or directly to the channels. Sometimes a
> channel will be labelled with a different compatible if it has a fixed
> purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts
> where some channels are "fsl,ssi-dma-channel").

That sounds terribly like encoding policy into DT rather than it being a
HW description.

2013-08-21 23:16:10

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 08/21/2013 05:00 PM, Scott Wood wrote:
> On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
>> On 07/29/2013 04:49 AM, [email protected] wrote:
>>> + - reg : <registers mapping for channel>
>>> + - interrupts : <interrupt mapping for DMA channel IRQ>
>>
>> s/interrupts/specifier/
>
> Do you mean s/interrupt mapping/interrupt specifier/?
>
> And probably s/registers mapping/register specifier/ as well.

Yup.

2013-08-21 23:31:21

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On Wed, 2013-08-21 at 17:15 -0600, Stephen Warren wrote:
> On 08/21/2013 04:57 PM, Scott Wood wrote:
> > On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
> >> On 07/29/2013 04:49 AM, [email protected] wrote:
>
> >>> +- ranges : describes the mapping between the address space of the
> >>> + DMA channels and the address space of the DMA controller
> >>
> >> Oh, so looking at the example, this is simply about being able to write
> >> the reg value in the child nodes more easily without having to write out
> >> the full based address of the controller in each child node.
> >>
> >> I don't think the binding document should require this;
> >
> > It doesn't. It just requires that there be a mapping; it doesn't have
> > to be any particular mapping.
> >
> >> all the binding document should care about is that the child nodes have a valid reg
> >> value. Whether that reg value is <0x100100 0x80> without a ranges in the
> >> top-level DMA
> >
> > Without a ranges property there is no translation and the registers
> > would not be memory mappable. Linux may treat the absence of ranges as
> > an identity mapping for compatibility with some broken OF trees, but
> > it's not standard.
>
> I would argue that missing ranges meaning 1:1 translation is now a
> standard, given that it must be true to support some DTs, it therefore
> can now be assumed?

"Some broken tree does it therefore it's fine for everyone to do it" is
awful. We have standards documents for a reason.

Plus, not all OSes need to run on hardware with that broken firmware.
For example, the Freescale Embedded Hypervisor will not accept a missing
ranges in that way. U-Boot does accept it, but only because the code
was copied from Linux (including the comment that says it's not supposed
to work that way).

If anything, we should remove the hack from U-Boot, and fix Linux to
only apply it in situations where it's known to be needed.

> >> Why do the channel nodes even need a compatible value? Presumably the
> >> driver for the top-level DMA node will scan these dma-channel nodes to
> >> extract the information it needs and will simply assume that all these
> >> nodes are DMA channel nodes rather than something else? I suppose this
> >> doesn't hurt, it just seems unnecessary unless you foresee other child
> >> nodes types existing in the future and hence a need to differentiate
> >> different types of nodes.
> >
> > Other than "this is how the existing binding works and we're not going
> > to break compatibility", it allows the OS more flexibility to choose
> > whether to bind to controllers or directly to the channels. Sometimes a
> > channel will be labelled with a different compatible if it has a fixed
> > purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts
> > where some channels are "fsl,ssi-dma-channel").
>
> That sounds terribly like encoding policy into DT rather than it being a
> HW description.

It is hardware description. Those DMA channels are physically wired
into the audio hardware. Other DMA channels in the same system aren't.

The only thing that's even slightly policy is that it assumes you aren't
going to ignore the audio device altogether, and use it as an extra
generic DMA channel. I'm not sure that it's worthwhile to care in this
case. If you want to be 100% policy-free then we shouldn't be
specifying a lot of the addresses we currently do, since that's actually
just how U-Boot configured things. Sometimes simplifying assumptions
get made, when what the hardware people actually came up with is too
awkward to be worth describing directly. In any case, this is not new,
nor is it relevant to the hardware we're currently adding support for,
and we're not going to break compatibility now.

-Scott

2013-08-21 23:33:57

[permalink] [raw]

Subject: Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document

On Wed, 2013-08-21 at 17:12 -0600, Stephen Warren wrote:
> OK, if there's some alternative run-time way of enabling chip-specific
> quirking, it's probably fine to remove the extra compatible values.
>
> Now, that does rather assume that this DMA IP block will only ever be
> used within SoCs that have that SVR concept, but perhaps if that's ever
> not the case, we can simply go back to requiring extra compatible values
> in those specific cases?

The only situation I can see where SVR would be absent is if we were to
integrate this device into an ARM chip, in which case I'd expect there
to be some equivalent way to find the SoC identification. If the driver
knows what SoC version it expects, it will know the way that that SoC
advertises its version.

-Scott

2013-08-22 00:27:55

by Timur Tabi

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On Wed, Aug 21, 2013 at 6:31 PM, Scott Wood <[email protected]> wrote:
>
>> > Other than "this is how the existing binding works and we're not going
>> > to break compatibility", it allows the OS more flexibility to choose
>> > whether to bind to controllers or directly to the channels. Sometimes a
>> > channel will be labelled with a different compatible if it has a fixed
>> > purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts
>> > where some channels are "fsl,ssi-dma-channel").
>>
>> That sounds terribly like encoding policy into DT rather than it being a
>> HW description.
>
> It is hardware description. Those DMA channels are physically wired
> into the audio hardware. Other DMA channels in the same system aren't.

Well, not quite. Technically the DMA channel can be dynamically
assigned to the SSI, but there are limits. At the time the code was
written, there was no way to reserve a DMA channel from the generic
DMA driver, and I didn't want to have to depend on that driver either.
Using the device tree forced a specific pair of channels to be
assigned to each SSI. The audio driver has code to program the SoC to
route whichever DMA channels are assigned, but it assumes that the
device tree has a valid assignment.

I believe the generic DMA driver can now accept DMA channel
reservations, but I don't think it works both ways. That is, if the
audio driver loads first, I don't think there's a clean way to tell
the DMA driver which channels have already been taken by the audio
driver.

2013-08-23 03:18:12

[permalink] [raw]

Subject: Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 08/22/2013 07:16 AM, Stephen Warren wrote:
> On 08/21/2013 05:00 PM, Scott Wood wrote:
>> On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
>>> On 07/29/2013 04:49 AM, [email protected] wrote:
>>>> + - reg : <registers mapping for channel>
>>>> + - interrupts : <interrupt mapping for DMA channel IRQ>
>>> s/interrupts/specifier/
>> Do you mean s/interrupt mapping/interrupt specifier/?
>>
>> And probably s/registers mapping/register specifier/ as well.
> Yup.
>
OK, I will update these descriptions.

2013-08-26 10:33:54