[PATCH 0/2] Exynos: MFC: clean up device tree bindings

[PATCH 0/2] Exynos: MFC: clean up device tree bindings

[Marek Szyprowski]

Hello,

This patch series are an attempt to cleanup the reserved memory device
tree bindings for MFC device. MFC device has two memory ports (AXI
masters), which are used to do DMA. Usually separate memory regions are
being defined for each of those memory ports to improve performance.
Some versions of MFC block have also significant limitation on the
possible address range available for each of those memory ports/banks.

In the board file era, there have been two additional platform devices
defined for each of memory ports (named "s5p-mfc-l" and "s5p-mfc-r") to
let the driver distinguish memory allocations done for each of them.
Each of those platform devices might have special DMA ops assigned to
fulfil specific memory requirements for a given memory port.

Later, when device tree binding was added for MFC device, those memory
ports were designed as two additional properties: "samsung,mfc-l" and
"samsung,mfc-r" for codec node. This approach however has some
significant limitation, so I propose to redesign it before the binding
become considered as stable.

The first problem with the proposed bindings is the fact that
"samsung,mfc-r"/"samsung,mfc-l" properties are tied only to "base
address + size" attributes of reserved memory and do not allow to assign
any other attributes to those memory ports. This limits using those
memory ports only to simple reserved memory.

The second issue with those attributes is the fact that they are very
specific to the MFC device, while reserved memory region is something
more generic, which can be used for other devices as well. For example
even on Exynos4 platform, similar reserved memory handling will be
needed for the FIMC ISP device.

For handling reserved memory regions and having a method to assign them
to particular device I have posted the patches [1], which add device
tree support to Contiguous Memory Allocator and simple reserved memory
allocator based on dma_declare_coherent() function.

This patch series is my proposal for replacing those custom bindings
with generic approach, proposed in [1]. To get it working we need
separate device node for each memory port, what has been achieved by
adding "simple-bus" compatibility entry to the main MFC device node and
adding two child nodes, which represent each memory port. Those child
nodes have compatible property set to "samsung,memport".

With such a structure "dma-memory-region" property with a phandle to
respective reserved region can be easily added to the child nodes of MFC
device. The advantage of such approach is the fact that those child
nodes can be also used for adding properties for IOMMU (SYSMMU)
controllers. This way also bindings for SYSMMU and the code, which
handles it can be simplified, because respective device tree part better
matches physical hardware design.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/257615/


Patch summary:

Marek Szyprowski (2):
  ARM: Exynos: replace custom MFC reserved memory handling with generic
    code
  media: s5p-mfc: remove DT hacks and simplify initialization code

 .../devicetree/bindings/media/s5p-mfc.txt          |   63 +++++++++++++---
 arch/arm/boot/dts/exynos4.dtsi                     |   10 ++-
 arch/arm/boot/dts/exynos4210-origen.dts            |   25 ++++++-
 arch/arm/boot/dts/exynos4210-smdkv310.dts          |   25 ++++++-
 arch/arm/boot/dts/exynos4412-origen.dts            |   25 ++++++-
 arch/arm/boot/dts/exynos4412-smdk4412.dts          |   25 ++++++-
 arch/arm/boot/dts/exynos5250-arndale.dts           |   26 ++++++-
 arch/arm/boot/dts/exynos5250-smdk5250.dts          |   26 ++++++-
 arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++-
 arch/arm/mach-exynos/mach-exynos4-dt.c             |   16 -----
 arch/arm/mach-exynos/mach-exynos5-dt.c             |   17 -----
 arch/arm/plat-samsung/include/plat/mfc.h           |   11 ---
 arch/arm/plat-samsung/s5p-dev-mfc.c                |   32 ---------
 drivers/media/platform/s5p-mfc/s5p_mfc.c           |   75 ++++----------------
 14 files changed, 227 insertions(+), 159 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code

[Marek Szyprowski]

MFC driver use custom bindings for managing reserved memory. Those bindings
are not really specific to MFC device and no even well discussed. They can
be easily replaced with generic, platform independent code for handling
reserved and contiguous memory.

Two additional child devices for each memory port (AXI master) are
introduced to let one assign some properties to each of them. Later one
can also use them to assign properties related to SYSMMU controllers,
which can be used to manage the limited dma window provided by those
memory ports.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 .../devicetree/bindings/media/s5p-mfc.txt          |   63 +++++++++++++++++---
 arch/arm/boot/dts/exynos4.dtsi                     |   10 +++-
 arch/arm/boot/dts/exynos4210-origen.dts            |   25 +++++++-
 arch/arm/boot/dts/exynos4210-smdkv310.dts          |   25 +++++++-
 arch/arm/boot/dts/exynos4412-origen.dts            |   25 +++++++-
 arch/arm/boot/dts/exynos4412-smdk4412.dts          |   25 +++++++-
 arch/arm/boot/dts/exynos5250-arndale.dts           |   26 +++++++-
 arch/arm/boot/dts/exynos5250-smdk5250.dts          |   26 +++++++-
 arch/arm/boot/dts/exynos5250.dtsi                  |   10 +++-
 arch/arm/mach-exynos/mach-exynos4-dt.c             |   16 -----
 arch/arm/mach-exynos/mach-exynos5-dt.c             |   17 ------
 arch/arm/plat-samsung/include/plat/mfc.h           |   11 ----
 arch/arm/plat-samsung/s5p-dev-mfc.c                |   32 ----------
 13 files changed, 212 insertions(+), 99 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
index df37b02..d9528d4 100644
--- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
+++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
@@ -6,10 +6,17 @@ The MFC device driver is a v4l2 driver which can encode/decode
 video raw/elementary streams and has support for all popular
 video codecs.
 
+The MFC device is connected to system bus with two memory ports (AXI
+masters) for better performance. Those memory ports are modelled as
+separate child devices, so one can assign some properties to them (like
+memory region for dma buffer allocation or sysmmu controller).
+
 Required properties:
   - compatible : value should be either one among the following
 	(a) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs
 	(b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs
+	and additionally "simple-bus" to correctly initialize child
+	devices for memory ports (AXI masters)
 
   - reg : Physical base address of the IP registers and length of memory
 	  mapped region.
@@ -19,31 +26,69 @@ Required properties:
   - clock-names : from common clock binding: must contain "sclk_mfc" and "mfc",
 		  corresponding to entries in the clocks property.
 
-  - samsung,mfc-r : Base address of the first memory bank used by MFC
-		    for DMA contiguous memory allocation and its size.
-
-  - samsung,mfc-l : Base address of the second memory bank used by MFC
-		    for DMA contiguous memory allocation and its size.
-
 Optional properties:
   - samsung,power-domain : power-domain property defined with a phandle
 			   to respective power domain.
 
+Two child nodes must be defined for MFC device. Their names must be
+following: "memport-r" and "memport-l" ("right" and "left"). Required
+properties:
+  - compatible : value should be "samsung,memport"
+  - dma-memory-region : optional property with a phandle to respective memory
+			region (see devicetree/bindings/memory.txt), if no region
+			is defined, sysmmu controller must be used for managing
+			limited dma window of each memory port.
+
+
 Example:
 SoC specific DT entry:
 
 mfc: codec at 13400000 {
-	compatible = "samsung,mfc-v5";
+	compatible = "samsung,mfc-v5", "simple-bus";
 	reg = <0x13400000 0x10000>;
 	interrupts = <0 94 0>;
 	samsung,power-domain = <&pd_mfc>;
 	clocks = <&clock 170>, <&clock 273>;
 	clock-names = "sclk_mfc", "mfc";
+	status = "disabled";
+
+	mfc_r: memport-r {
+		compatible = "samsung,memport";
+	};
+
+	mfc_l: memport-l {
+		compatible = "samsung,memport";
+	};
 };
 
 Board specific DT entry:
 
+memory {
+	/* ... */
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		mfc_l_mem: mfc_l_region at 43000000 {
+			compatible = "contiguous-memory-region", "reserved-memory-region";
+			reg = <0x43000000 0x1000000>;
+		};
+
+		mfc_r_mem: mfc_r_region at 52000000 {
+			compatible = "contiguous-memory-region", "reserved-memory-region";
+			reg = <0x52000000 0x1000000>;
+		};
+	};
+};
+
 codec at 13400000 {
-	samsung,mfc-r = <0x43000000 0x800000>;
-	samsung,mfc-l = <0x51000000 0x800000>;
+	status = "okay";
+
+	memport-r {
+		dma-memory-region = <&mfc_r_mem>;
+	};
+
+	memport-l {
+		dma-memory-region = <&mfc_l_mem>;
+	};
 };
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 3f94fe8..599637f 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -156,13 +156,21 @@
 	};
 
 	mfc: codec at 13400000 {
-		compatible = "samsung,mfc-v5";
+		compatible = "samsung,mfc-v5", "simple-bus";
 		reg = <0x13400000 0x10000>;
 		interrupts = <0 94 0>;
 		samsung,power-domain = <&pd_mfc>;
 		clocks = <&clock 170>, <&clock 273>;
 		clock-names = "sclk_mfc", "mfc";
 		status = "disabled";
+
+		mfc_r: memport-r {
+			compatible = "samsung,memport";
+		};
+
+		mfc_l: memport-l {
+			compatible = "samsung,memport";
+		};
 	};
 
 	serial at 13800000 {
diff --git a/arch/arm/boot/dts/exynos4210-origen.dts b/arch/arm/boot/dts/exynos4210-origen.dts
index 382d8c7..e80fe8a 100644
--- a/arch/arm/boot/dts/exynos4210-origen.dts
+++ b/arch/arm/boot/dts/exynos4210-origen.dts
@@ -26,6 +26,21 @@
 		       0x50000000 0x10000000
 		       0x60000000 0x10000000
 		       0x70000000 0x10000000>;
+
+		reserved-memory {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			mfc_l_mem: mfc_l_region at 43000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x43000000 0x1000000>;
+			};
+
+			mfc_r_mem: mfc_r_region at 51000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x51000000 0x1000000>;
+			};
+		};
 	};
 
 	chosen {
@@ -66,9 +81,15 @@
 	};
 
 	codec at 13400000 {
-		samsung,mfc-r = <0x43000000 0x800000>;
-		samsung,mfc-l = <0x51000000 0x800000>;
 		status = "okay";
+
+		memport-r {
+			dma-memory-region = <&mfc_r_mem>;
+		};
+
+		memport-l {
+			dma-memory-region = <&mfc_l_mem>;
+		};
 	};
 
 	serial at 13800000 {
diff --git a/arch/arm/boot/dts/exynos4210-smdkv310.dts b/arch/arm/boot/dts/exynos4210-smdkv310.dts
index 9c01b71..07adb56 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts
@@ -23,6 +23,21 @@
 
 	memory {
 		reg = <0x40000000 0x80000000>;
+
+		reserved-memory {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			mfc_l_mem: mfc_l_region at 43000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x43000000 0x1000000>;
+			};
+
+			mfc_r_mem: mfc_r_region at 51000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x51000000 0x1000000>;
+			};
+		};
 	};
 
 	chosen {
@@ -41,9 +56,15 @@
 	};
 
 	codec at 13400000 {
-		samsung,mfc-r = <0x43000000 0x800000>;
-		samsung,mfc-l = <0x51000000 0x800000>;
 		status = "okay";
+
+		memport-r {
+			dma-memory-region = <&mfc_r_mem>;
+		};
+
+		memport-l {
+			dma-memory-region = <&mfc_l_mem>;
+		};
 	};
 
 	serial at 13800000 {
diff --git a/arch/arm/boot/dts/exynos4412-origen.dts b/arch/arm/boot/dts/exynos4412-origen.dts
index 7993641..1421070 100644
--- a/arch/arm/boot/dts/exynos4412-origen.dts
+++ b/arch/arm/boot/dts/exynos4412-origen.dts
@@ -21,6 +21,21 @@
 
 	memory {
 		reg = <0x40000000 0x40000000>;
+
+		reserved-memory {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			mfc_l_mem: mfc_l_region at 43000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x43000000 0x1000000>;
+			};
+
+			mfc_r_mem: mfc_r_region at 51000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x51000000 0x1000000>;
+			};
+		};
 	};
 
 	chosen {
@@ -133,9 +148,15 @@
 	};
 
 	codec at 13400000 {
-		samsung,mfc-r = <0x43000000 0x800000>;
-		samsung,mfc-l = <0x51000000 0x800000>;
 		status = "okay";
+
+		memport-r {
+			dma-memory-region = <&mfc_r_mem>;
+		};
+
+		memport-l {
+			dma-memory-region = <&mfc_l_mem>;
+		};
 	};
 
 	fimd at 11c00000 {
diff --git a/arch/arm/boot/dts/exynos4412-smdk4412.dts b/arch/arm/boot/dts/exynos4412-smdk4412.dts
index ad316a1..08a3735 100644
--- a/arch/arm/boot/dts/exynos4412-smdk4412.dts
+++ b/arch/arm/boot/dts/exynos4412-smdk4412.dts
@@ -21,6 +21,21 @@
 
 	memory {
 		reg = <0x40000000 0x40000000>;
+
+		reserved-memory {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			mfc_l_mem: mfc_l_region at 43000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x43000000 0x1000000>;
+			};
+
+			mfc_r_mem: mfc_r_region at 51000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x51000000 0x1000000>;
+			};
+		};
 	};
 
 	chosen {
@@ -126,9 +141,15 @@
 	};
 
 	codec at 13400000 {
-		samsung,mfc-r = <0x43000000 0x800000>;
-		samsung,mfc-l = <0x51000000 0x800000>;
 		status = "okay";
+
+		memport-r {
+			dma-memory-region = <&mfc_r_mem>;
+		};
+
+		memport-l {
+			dma-memory-region = <&mfc_l_mem>;
+		};
 	};
 
 	serial at 13800000 {
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index abc7272..ba4a533 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -18,6 +18,21 @@
 
 	memory {
 		reg = <0x40000000 0x80000000>;
+
+		reserved-memory {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			mfc_l_mem: mfc_l_region at 43000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x43000000 0x1000000>;
+			};
+
+			mfc_r_mem: mfc_r_region at 51000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x51000000 0x1000000>;
+			};
+		};
 	};
 
 	chosen {
@@ -25,8 +40,15 @@
 	};
 
 	codec at 11000000 {
-		samsung,mfc-r = <0x43000000 0x800000>;
-		samsung,mfc-l = <0x51000000 0x800000>;
+		status = "okay";
+
+		memport-r {
+			dma-memory-region = <&mfc_r_mem>;
+		};
+
+		memport-l {
+			dma-memory-region = <&mfc_l_mem>;
+		};
 	};
 
 	i2c at 12C60000 {
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 49f18c2..daed15e 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -21,6 +21,21 @@
 
 	memory {
 		reg = <0x40000000 0x80000000>;
+
+		reserved-memory {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			mfc_l_mem: mfc_l_region at 43000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x43000000 0x1000000>;
+			};
+
+			mfc_r_mem: mfc_r_region at 51000000 {
+				compatible = "contiguous-memory-region", "reserved-memory-region";
+				reg = <0x51000000 0x1000000>;
+			};
+		};
 	};
 
 	chosen {
@@ -223,8 +238,15 @@
 	};
 
 	codec at 11000000 {
-		samsung,mfc-r = <0x43000000 0x800000>;
-		samsung,mfc-l = <0x51000000 0x800000>;
+		status = "okay";
+
+		memport-r {
+			dma-memory-region = <&mfc_r_mem>;
+		};
+
+		memport-l {
+			dma-memory-region = <&mfc_l_mem>;
+		};
 	};
 
 	i2s0: i2s at 03830000 {
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index ef57277..6011518 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -164,10 +164,18 @@
 	};
 
 	codec at 11000000 {
-		compatible = "samsung,mfc-v6";
+		compatible = "samsung,mfc-v6", "simple-bus";
 		reg = <0x11000000 0x10000>;
 		interrupts = <0 96 0>;
 		samsung,power-domain = <&pd_mfc>;
+
+		mfc_r: memport-r {
+			compatible = "samsung,memport";
+		};
+
+		mfc_l: memport-l {
+			compatible = "samsung,memport";
+		};
 	};
 
 	rtc {
diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c
index 0099c6c..0b6da39 100644
--- a/arch/arm/mach-exynos/mach-exynos4-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
@@ -13,13 +13,10 @@
 
 #include <linux/kernel.h>
 #include <linux/of_platform.h>
-#include <linux/of_fdt.h>
 #include <linux/serial_core.h>
-#include <linux/memblock.h>
 #include <linux/clocksource.h>
 
 #include <asm/mach/arch.h>
-#include <plat/mfc.h>
 
 #include "common.h"
 
@@ -35,18 +32,6 @@ static char const *exynos4_dt_compat[] __initdata = {
 	NULL
 };
 
-static void __init exynos4_reserve(void)
-{
-#ifdef CONFIG_S5P_DEV_MFC
-	struct s5p_mfc_dt_meminfo mfc_mem;
-
-	/* Reserve memory for MFC only if it's available */
-	mfc_mem.compatible = "samsung,mfc-v5";
-	if (of_scan_flat_dt(s5p_fdt_find_mfc_mem, &mfc_mem))
-		s5p_mfc_reserve_mem(mfc_mem.roff, mfc_mem.rsize, mfc_mem.loff,
-				mfc_mem.lsize);
-#endif
-}
 DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
 	/* Maintainer: Thomas Abraham <thomas.abraham@linaro.org> */
 	.smp		= smp_ops(exynos_smp_ops),
@@ -57,5 +42,4 @@ DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
 	.init_time	= exynos_init_time,
 	.dt_compat	= exynos4_dt_compat,
 	.restart        = exynos4_restart,
-	.reserve	= exynos4_reserve,
 MACHINE_END
diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
index f874b77..b5b0571 100644
--- a/arch/arm/mach-exynos/mach-exynos5-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
@@ -10,16 +10,13 @@
 */
 
 #include <linux/of_platform.h>
-#include <linux/of_fdt.h>
 #include <linux/memblock.h>
-#include <linux/io.h>
 #include <linux/clocksource.h>
 
 #include <asm/mach/arch.h>
 #include <mach/regs-pmu.h>
 
 #include <plat/cpu.h>
-#include <plat/mfc.h>
 
 #include "common.h"
 
@@ -57,19 +54,6 @@ static char const *exynos5_dt_compat[] __initdata = {
 	NULL
 };
 
-static void __init exynos5_reserve(void)
-{
-#ifdef CONFIG_S5P_DEV_MFC
-	struct s5p_mfc_dt_meminfo mfc_mem;
-
-	/* Reserve memory for MFC only if it's available */
-	mfc_mem.compatible = "samsung,mfc-v6";
-	if (of_scan_flat_dt(s5p_fdt_find_mfc_mem, &mfc_mem))
-		s5p_mfc_reserve_mem(mfc_mem.roff, mfc_mem.rsize, mfc_mem.loff,
-				mfc_mem.lsize);
-#endif
-}
-
 DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
 	/* Maintainer: Kukjin Kim <kgene.kim@samsung.com> */
 	.smp		= smp_ops(exynos_smp_ops),
@@ -79,5 +63,4 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
 	.init_time	= exynos_init_time,
 	.dt_compat	= exynos5_dt_compat,
 	.restart        = exynos5_restart,
-	.reserve	= exynos5_reserve,
 MACHINE_END
diff --git a/arch/arm/plat-samsung/include/plat/mfc.h b/arch/arm/plat-samsung/include/plat/mfc.h
index e6d7c42..ac13227 100644
--- a/arch/arm/plat-samsung/include/plat/mfc.h
+++ b/arch/arm/plat-samsung/include/plat/mfc.h
@@ -10,14 +10,6 @@
 #ifndef __PLAT_SAMSUNG_MFC_H
 #define __PLAT_SAMSUNG_MFC_H __FILE__
 
-struct s5p_mfc_dt_meminfo {
-	unsigned long	loff;
-	unsigned long	lsize;
-	unsigned long	roff;
-	unsigned long	rsize;
-	char		*compatible;
-};
-
 /**
  * s5p_mfc_reserve_mem - function to early reserve memory for MFC driver
  * @rbase:	base address for MFC 'right' memory interface
@@ -32,7 +24,4 @@ struct s5p_mfc_dt_meminfo {
 void __init s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
 				phys_addr_t lbase, unsigned int lsize);
 
-int __init s5p_fdt_find_mfc_mem(unsigned long node, const char *uname,
-				int depth, void *data);
-
 #endif /* __PLAT_SAMSUNG_MFC_H */
diff --git a/arch/arm/plat-samsung/s5p-dev-mfc.c b/arch/arm/plat-samsung/s5p-dev-mfc.c
index ad51f85..f83be72 100644
--- a/arch/arm/plat-samsung/s5p-dev-mfc.c
+++ b/arch/arm/plat-samsung/s5p-dev-mfc.c
@@ -120,35 +120,3 @@ static int __init s5p_mfc_memory_init(void)
 }
 device_initcall(s5p_mfc_memory_init);
 #endif
-
-#ifdef CONFIG_OF
-int __init s5p_fdt_find_mfc_mem(unsigned long node, const char *uname,
-				int depth, void *data)
-{
-	__be32 *prop;
-	unsigned long len;
-	struct s5p_mfc_dt_meminfo *mfc_mem = data;
-
-	if (!data)
-		return 0;
-
-	if (!of_flat_dt_is_compatible(node, mfc_mem->compatible))
-		return 0;
-
-	prop = of_get_flat_dt_prop(node, "samsung,mfc-l", &len);
-	if (!prop || (len != 2 * sizeof(unsigned long)))
-		return 0;
-
-	mfc_mem->loff = be32_to_cpu(prop[0]);
-	mfc_mem->lsize = be32_to_cpu(prop[1]);
-
-	prop = of_get_flat_dt_prop(node, "samsung,mfc-r", &len);
-	if (!prop || (len != 2 * sizeof(unsigned long)))
-		return 0;
-
-	mfc_mem->roff = be32_to_cpu(prop[0]);
-	mfc_mem->rsize = be32_to_cpu(prop[1]);
-
-	return 1;
-}
-#endif
-- 
1.7.9.5

[PATCH 2/2] media: s5p-mfc: remove DT hacks and simplify initialization code

[Marek Szyprowski]

This patch removes custom initialization of reserved memory regions from
s5p-mfc driver. Memory initialization can be now handled by generic
code.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |   75 ++++++------------------------
 1 file changed, 15 insertions(+), 60 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index a130dcd..696e0e0 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1011,51 +1011,11 @@ static int match_child(struct device *dev, void *data)
 {
 	if (!dev_name(dev))
 		return 0;
-	return !strcmp(dev_name(dev), (char *)data);
+	return !!strstr(dev_name(dev), (char *)data);
 }
 
 static void *mfc_get_drv_data(struct platform_device *pdev);
 
-static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev *dev)
-{
-	unsigned int mem_info[2] = { };
-
-	dev->mem_dev_l = devm_kzalloc(&dev->plat_dev->dev,
-			sizeof(struct device), GFP_KERNEL);
-	if (!dev->mem_dev_l) {
-		mfc_err("Not enough memory\n");
-		return -ENOMEM;
-	}
-	device_initialize(dev->mem_dev_l);
-	of_property_read_u32_array(dev->plat_dev->dev.of_node,
-			"samsung,mfc-l", mem_info, 2);
-	if (dma_declare_coherent_memory(dev->mem_dev_l, mem_info[0],
-				mem_info[0], mem_info[1],
-				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) == 0) {
-		mfc_err("Failed to declare coherent memory for\n"
-		"MFC device\n");
-		return -ENOMEM;
-	}
-
-	dev->mem_dev_r = devm_kzalloc(&dev->plat_dev->dev,
-			sizeof(struct device), GFP_KERNEL);
-	if (!dev->mem_dev_r) {
-		mfc_err("Not enough memory\n");
-		return -ENOMEM;
-	}
-	device_initialize(dev->mem_dev_r);
-	of_property_read_u32_array(dev->plat_dev->dev.of_node,
-			"samsung,mfc-r", mem_info, 2);
-	if (dma_declare_coherent_memory(dev->mem_dev_r, mem_info[0],
-				mem_info[0], mem_info[1],
-				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) == 0) {
-		pr_err("Failed to declare coherent memory for\n"
-		"MFC device\n");
-		return -ENOMEM;
-	}
-	return 0;
-}
-
 /* MFC probe function */
 static int s5p_mfc_probe(struct platform_device *pdev)
 {
@@ -1107,25 +1067,20 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 		goto err_res;
 	}
 
-	if (pdev->dev.of_node) {
-		ret = s5p_mfc_alloc_memdevs(dev);
-		if (ret < 0)
-			goto err_res;
-	} else {
-		dev->mem_dev_l = device_find_child(&dev->plat_dev->dev,
-				"s5p-mfc-l", match_child);
-		if (!dev->mem_dev_l) {
-			mfc_err("Mem child (L) device get failed\n");
-			ret = -ENODEV;
-			goto err_res;
-		}
-		dev->mem_dev_r = device_find_child(&dev->plat_dev->dev,
-				"s5p-mfc-r", match_child);
-		if (!dev->mem_dev_r) {
-			mfc_err("Mem child (R) device get failed\n");
-			ret = -ENODEV;
-			goto err_res;
-		}
+	dev->mem_dev_l = device_find_child(&dev->plat_dev->dev, "-l",
+					   match_child);
+	if (!dev->mem_dev_l) {
+		mfc_err("Mem child (L) device get failed\n");
+		ret = -ENODEV;
+		goto err_res;
+	}
+
+	dev->mem_dev_r = device_find_child(&dev->plat_dev->dev, "-r",
+					   match_child);
+	if (!dev->mem_dev_r) {
+		mfc_err("Mem child (R) device get failed\n");
+		ret = -ENODEV;
+		goto err_res;
 	}
 
 	dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l);
-- 
1.7.9.5

[PATCH 0/2] Exynos: MFC: clean up device tree bindings

[Kukjin Kim]

On 08/05/13 21:26, Marek Szyprowski wrote:
> Hello,
>
Hi,

> This patch series are an attempt to cleanup the reserved memory device
> tree bindings for MFC device. MFC device has two memory ports (AXI
> masters), which are used to do DMA. Usually separate memory regions are
> being defined for each of those memory ports to improve performance.
> Some versions of MFC block have also significant limitation on the
> possible address range available for each of those memory ports/banks.
>
> In the board file era, there have been two additional platform devices
> defined for each of memory ports (named "s5p-mfc-l" and "s5p-mfc-r") to
> let the driver distinguish memory allocations done for each of them.
> Each of those platform devices might have special DMA ops assigned to
> fulfil specific memory requirements for a given memory port.
>
> Later, when device tree binding was added for MFC device, those memory
> ports were designed as two additional properties: "samsung,mfc-l" and
> "samsung,mfc-r" for codec node. This approach however has some
> significant limitation, so I propose to redesign it before the binding
> become considered as stable.
>
> The first problem with the proposed bindings is the fact that
> "samsung,mfc-r"/"samsung,mfc-l" properties are tied only to "base
> address + size" attributes of reserved memory and do not allow to assign
> any other attributes to those memory ports. This limits using those
> memory ports only to simple reserved memory.
>
> The second issue with those attributes is the fact that they are very
> specific to the MFC device, while reserved memory region is something
> more generic, which can be used for other devices as well. For example
> even on Exynos4 platform, similar reserved memory handling will be
> needed for the FIMC ISP device.
>
> For handling reserved memory regions and having a method to assign them
> to particular device I have posted the patches [1], which add device
> tree support to Contiguous Memory Allocator and simple reserved memory
> allocator based on dma_declare_coherent() function.
>
> This patch series is my proposal for replacing those custom bindings
> with generic approach, proposed in [1]. To get it working we need
> separate device node for each memory port, what has been achieved by
> adding "simple-bus" compatibility entry to the main MFC device node and
> adding two child nodes, which represent each memory port. Those child
> nodes have compatible property set to "samsung,memport".
>
> With such a structure "dma-memory-region" property with a phandle to
> respective reserved region can be easily added to the child nodes of MFC
> device. The advantage of such approach is the fact that those child
> nodes can be also used for adding properties for IOMMU (SYSMMU)
> controllers. This way also bindings for SYSMMU and the code, which
> handles it can be simplified, because respective device tree part better
> matches physical hardware design.
>
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
>
> [1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/257615/
>
>
> Patch summary:
>
> Marek Szyprowski (2):
>    ARM: Exynos: replace custom MFC reserved memory handling with generic
>      code
>    media: s5p-mfc: remove DT hacks and simplify initialization code
>
>   .../devicetree/bindings/media/s5p-mfc.txt          |   63 +++++++++++++---
>   arch/arm/boot/dts/exynos4.dtsi                     |   10 ++-
>   arch/arm/boot/dts/exynos4210-origen.dts            |   25 ++++++-
>   arch/arm/boot/dts/exynos4210-smdkv310.dts          |   25 ++++++-
>   arch/arm/boot/dts/exynos4412-origen.dts            |   25 ++++++-
>   arch/arm/boot/dts/exynos4412-smdk4412.dts          |   25 ++++++-
>   arch/arm/boot/dts/exynos5250-arndale.dts           |   26 ++++++-
>   arch/arm/boot/dts/exynos5250-smdk5250.dts          |   26 ++++++-
>   arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++-
>   arch/arm/mach-exynos/mach-exynos4-dt.c             |   16 -----
>   arch/arm/mach-exynos/mach-exynos5-dt.c             |   17 -----
>   arch/arm/plat-samsung/include/plat/mfc.h           |   11 ---
>   arch/arm/plat-samsung/s5p-dev-mfc.c                |   32 ---------
>   drivers/media/platform/s5p-mfc/s5p_mfc.c           |   75 ++++----------------
>   14 files changed, 227 insertions(+), 159 deletions(-)
>
Nice cleanup MFC dt bindings, BTW, IMHO, how about keeping the reserved 
memory in exynos4.dtsi instead of adding them in each board dts files, 
it depends on board though...

Kamil, if you're OK on the 2nd patch, please let me know so that this 
could be merged into samsung tree for v3.12..

Thanks,
Kukjin

[PATCH 2/2] media: s5p-mfc: remove DT hacks and simplify initialization code

[Kamil Debski]

Hi Kukjin,

This patch looks good.

Best wishes,
Kamil Debski

> From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
> Sent: Monday, August 05, 2013 2:27 PM
> 
> This patch removes custom initialization of reserved memory regions
> from s5p-mfc driver. Memory initialization can be now handled by
> generic code.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c |   75 ++++++----------------
> --------
>  1 file changed, 15 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index a130dcd..696e0e0 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1011,51 +1011,11 @@ static int match_child(struct device *dev, void
> *data)  {
>  	if (!dev_name(dev))
>  		return 0;
> -	return !strcmp(dev_name(dev), (char *)data);
> +	return !!strstr(dev_name(dev), (char *)data);
>  }
> 
>  static void *mfc_get_drv_data(struct platform_device *pdev);
> 
> -static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev *dev) -{
> -	unsigned int mem_info[2] = { };
> -
> -	dev->mem_dev_l = devm_kzalloc(&dev->plat_dev->dev,
> -			sizeof(struct device), GFP_KERNEL);
> -	if (!dev->mem_dev_l) {
> -		mfc_err("Not enough memory\n");
> -		return -ENOMEM;
> -	}
> -	device_initialize(dev->mem_dev_l);
> -	of_property_read_u32_array(dev->plat_dev->dev.of_node,
> -			"samsung,mfc-l", mem_info, 2);
> -	if (dma_declare_coherent_memory(dev->mem_dev_l, mem_info[0],
> -				mem_info[0], mem_info[1],
> -				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) == 0)
{
> -		mfc_err("Failed to declare coherent memory for\n"
> -		"MFC device\n");
> -		return -ENOMEM;
> -	}
> -
> -	dev->mem_dev_r = devm_kzalloc(&dev->plat_dev->dev,
> -			sizeof(struct device), GFP_KERNEL);
> -	if (!dev->mem_dev_r) {
> -		mfc_err("Not enough memory\n");
> -		return -ENOMEM;
> -	}
> -	device_initialize(dev->mem_dev_r);
> -	of_property_read_u32_array(dev->plat_dev->dev.of_node,
> -			"samsung,mfc-r", mem_info, 2);
> -	if (dma_declare_coherent_memory(dev->mem_dev_r, mem_info[0],
> -				mem_info[0], mem_info[1],
> -				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) == 0)
{
> -		pr_err("Failed to declare coherent memory for\n"
> -		"MFC device\n");
> -		return -ENOMEM;
> -	}
> -	return 0;
> -}
> -
>  /* MFC probe function */
>  static int s5p_mfc_probe(struct platform_device *pdev)  { @@ -1107,25
> +1067,20 @@ static int s5p_mfc_probe(struct platform_device *pdev)
>  		goto err_res;
>  	}
> 
> -	if (pdev->dev.of_node) {
> -		ret = s5p_mfc_alloc_memdevs(dev);
> -		if (ret < 0)
> -			goto err_res;
> -	} else {
> -		dev->mem_dev_l = device_find_child(&dev->plat_dev->dev,
> -				"s5p-mfc-l", match_child);
> -		if (!dev->mem_dev_l) {
> -			mfc_err("Mem child (L) device get failed\n");
> -			ret = -ENODEV;
> -			goto err_res;
> -		}
> -		dev->mem_dev_r = device_find_child(&dev->plat_dev->dev,
> -				"s5p-mfc-r", match_child);
> -		if (!dev->mem_dev_r) {
> -			mfc_err("Mem child (R) device get failed\n");
> -			ret = -ENODEV;
> -			goto err_res;
> -		}
> +	dev->mem_dev_l = device_find_child(&dev->plat_dev->dev, "-l",
> +					   match_child);
> +	if (!dev->mem_dev_l) {
> +		mfc_err("Mem child (L) device get failed\n");
> +		ret = -ENODEV;
> +		goto err_res;
> +	}
> +
> +	dev->mem_dev_r = device_find_child(&dev->plat_dev->dev, "-r",
> +					   match_child);
> +	if (!dev->mem_dev_r) {
> +		mfc_err("Mem child (R) device get failed\n");
> +		ret = -ENODEV;
> +		goto err_res;
>  	}
> 
>  	dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l);
> --
> 1.7.9.5

[PATCH 2/2] media: s5p-mfc: remove DT hacks and simplify initialization code

[Kukjin Kim]

On 08/06/13 19:22, Kamil Debski wrote:
> Hi Kukjin,
>
> This patch looks good.
>
> Best wishes,
> Kamil Debski
>
>> From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
>> Sent: Monday, August 05, 2013 2:27 PM
>>
>> This patch removes custom initialization of reserved memory regions
>> from s5p-mfc driver. Memory initialization can be now handled by
>> generic code.
>>
>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
>
> Acked-by: Kamil Debski<k.debski@samsung.com>
>
Kamil, thanks for your ack.

Applied.
- Kukjin

[PATCH 2/2] media: s5p-mfc: remove DT hacks and simplify initialization code

[Tomasz Figa]

Hi Kukjin,

On Wednesday 07 of August 2013 07:13:09 Kukjin Kim wrote:
> On 08/06/13 19:22, Kamil Debski wrote:
> > Hi Kukjin,
> > 
> > This patch looks good.
> > 
> > Best wishes,
> > Kamil Debski
> > 
> >> From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
> >> Sent: Monday, August 05, 2013 2:27 PM
> >> 
> >> This patch removes custom initialization of reserved memory regions
> >> from s5p-mfc driver. Memory initialization can be now handled by
> >> generic code.
> >> 
> >> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
> > 
> > Acked-by: Kamil Debski<k.debski@samsung.com>
> 
> Kamil, thanks for your ack.
> 
> Applied.

This is a nice cleanup, but I don't think it should be applied yet, 
because it depends on patch 1/2, which needs an Ack from DT maintainers.

Best regards,
Tomasz

[PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code

[Stephen Warren]

On 08/05/2013 06:26 AM, Marek Szyprowski wrote:
> MFC driver use custom bindings for managing reserved memory. Those bindings
> are not really specific to MFC device and no even well discussed. They can
> be easily replaced with generic, platform independent code for handling
> reserved and contiguous memory.
> 
> Two additional child devices for each memory port (AXI master) are
> introduced to let one assign some properties to each of them. Later one
> can also use them to assign properties related to SYSMMU controllers,
> which can be used to manage the limited dma window provided by those
> memory ports.

> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt

> +The MFC device is connected to system bus with two memory ports (AXI
> +masters) for better performance. Those memory ports are modelled as
> +separate child devices, so one can assign some properties to them (like
> +memory region for dma buffer allocation or sysmmu controller).
> +
>  Required properties:
>    - compatible : value should be either one among the following
>  	(a) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs
>  	(b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs
> +	and additionally "simple-bus" to correctly initialize child
> +	devices for memory ports (AXI masters)

simple-bus is wrong here; the child nodes aren't independent entities
that can be instantiated separately from their parent and without
depending on their parent.

> -  - samsung,mfc-r : Base address of the first memory bank used by MFC
> -		    for DMA contiguous memory allocation and its size.
> -
> -  - samsung,mfc-l : Base address of the second memory bank used by MFC
> -		    for DMA contiguous memory allocation and its size.

These properties shouldn't be removed, but simply marked deprecated. The
driver will need to continue to support them so that old DTs work with
new kernels. The binding must therefore continue to document them so
that the old DT content still makes sense.

> +Two child nodes must be defined for MFC device. Their names must be
> +following: "memport-r" and "memport-l" ("right" and "left"). Required

Node names shouldn't have semantic meaning.

> +properties:
> +  - compatible : value should be "samsung,memport"

There's no need to define compatible properties for things which aren't
separate devices.

> +  - dma-memory-region : optional property with a phandle to respective memory
> +			region (see devicetree/bindings/memory.txt), if no region
> +			is defined, sysmmu controller must be used for managing
> +			limited dma window of each memory port.

What's the benefit of putting this property in a sub-node; surely you
can put the property in the main MFC node yet follow the same conceptual
schema for its content as a dma-memory-region node?

[PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code

[Tomasz Figa]

Hi Stephen,

On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:
> On 08/05/2013 06:26 AM, Marek Szyprowski wrote:
> > MFC driver use custom bindings for managing reserved memory. Those
> > bindings are not really specific to MFC device and no even well
> > discussed. They can be easily replaced with generic, platform
> > independent code for handling reserved and contiguous memory.
> > 
> > Two additional child devices for each memory port (AXI master) are
> > introduced to let one assign some properties to each of them. Later
> > one
> > can also use them to assign properties related to SYSMMU controllers,
> > which can be used to manage the limited dma window provided by those
> > memory ports.
> > 
> > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > 
> > +The MFC device is connected to system bus with two memory ports (AXI
> > +masters) for better performance. Those memory ports are modelled as
> > +separate child devices, so one can assign some properties to them
> > (like +memory region for dma buffer allocation or sysmmu controller).
> > +
> > 
> >  Required properties:
> >    - compatible : value should be either one among the following
> >  	
> >  	(a) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs
> >  	(b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs
> > 
> > +	and additionally "simple-bus" to correctly initialize child
> > +	devices for memory ports (AXI masters)
> 
> simple-bus is wrong here; the child nodes aren't independent entities
> that can be instantiated separately from their parent and without
> depending on their parent.

I fully agree, but I can't think of anything better. Could you suggest a 
solution that would cover our use case:

The MFC IP has two separate bus masters (called here and there memports). 
On some SoCs, each master must use different memory bank to meet memory 
bandwidth requirements for higher bitrate video streams. This can be seen 
as MFC having two DMA subdevices, which have different DMA windows.

On Linux, this is handled by binding two appropriate CMA memory regions to 
the memports, so the driver can do DMA allocations on behalf of particular 
memport and get appropriate memory for it.

> > -  - samsung,mfc-r : Base address of the first memory bank used by MFC
> > -		    for DMA contiguous memory allocation and its size.
> > -
> > -  - samsung,mfc-l : Base address of the second memory bank used by
> > MFC
> > -		    for DMA contiguous memory allocation and its size.
> 
> These properties shouldn't be removed, but simply marked deprecated. The
> driver will need to continue to support them so that old DTs work with
> new kernels. The binding must therefore continue to document them so
> that the old DT content still makes sense.

I tend to disagree on this. For Samsung platforms we've been trying to 
avoid DT bindings changes as much as possible, but I'd rather say that 
device tree was coupled with kernel version it came from, so Samsung-
specific bindings haven't been fully stabilized yet, especially since we 
are still at discussion stage when it's about defining processes for 
binding staging and stabilization.

I would rather see this patch as part of work on Samsung DT binding 
janitoring or maybe even sanitizing in some cases, like this one, when the 
old (and IMHO bad) MFC binding was introduced without proper review. I 
don't think we want to support this kind of brokenness anymore, especially 
considering the hacks which would be required by such support (see 
original implementation of this binding and code required in board file).

> > +Two child nodes must be defined for MFC device. Their names must be
> > +following: "memport-r" and "memport-l" ("right" and "left"). Required
> 
> Node names shouldn't have semantic meaning.

What about bus-master-0 and bus-master-1?

> > +properties:
> > +  - compatible : value should be "samsung,memport"
> 
> There's no need to define compatible properties for things which aren't
> separate devices.

I agree.

> > +  - dma-memory-region : optional property with a phandle to
> > respective memory +			region (see 
devicetree/bindings/memory.txt), if
> > no region
> > +			is defined, sysmmu controller must be used for 
managing
> > +			limited dma window of each memory port.
> 
> What's the benefit of putting this property in a sub-node; surely you
> can put the property in the main MFC node yet follow the same conceptual
> schema for its content as a dma-memory-region node?

See my use case description above.

Best regards,
Tomasz

[PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code

[Stephen Warren]

On 08/08/2013 03:19 PM, Tomasz Figa wrote:
> Hi Stephen,
> 
> On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:
>> On 08/05/2013 06:26 AM, Marek Szyprowski wrote:
>>> MFC driver use custom bindings for managing reserved memory. Those
>>> bindings are not really specific to MFC device and no even well
>>> discussed. They can be easily replaced with generic, platform
>>> independent code for handling reserved and contiguous memory.
>>>
>>> Two additional child devices for each memory port (AXI master) are
>>> introduced to let one assign some properties to each of them. Later
>>> one
>>> can also use them to assign properties related to SYSMMU controllers,
>>> which can be used to manage the limited dma window provided by those
>>> memory ports.
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>>
>>> +The MFC device is connected to system bus with two memory ports (AXI
>>> +masters) for better performance. Those memory ports are modelled as
>>> +separate child devices, so one can assign some properties to them
>>> (like +memory region for dma buffer allocation or sysmmu controller).
>>> +
>>>
>>>  Required properties:
>>>    - compatible : value should be either one among the following
>>>  	
>>>  	(a) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs
>>>  	(b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs
>>>
>>> +	and additionally "simple-bus" to correctly initialize child
>>> +	devices for memory ports (AXI masters)
>>
>> simple-bus is wrong here; the child nodes aren't independent entities
>> that can be instantiated separately from their parent and without
>> depending on their parent.
> 
> I fully agree, but I can't think of anything better. Could you suggest a 
> solution that would cover our use case:
> 
> The MFC IP has two separate bus masters (called here and there memports). 
> On some SoCs, each master must use different memory bank to meet memory 
> bandwidth requirements for higher bitrate video streams. This can be seen 
> as MFC having two DMA subdevices, which have different DMA windows.
> 
> On Linux, this is handled by binding two appropriate CMA memory regions to 
> the memports, so the driver can do DMA allocations on behalf of particular 
> memport and get appropriate memory for it.

I don't see what that has to do with simple-bus. Whatever parses the
node of the MFC can directly read from any contained property or child
node; there's no need to try and get the core DT tree parser to
enumerate the children.

If you actually need a struct platform_device for each of these child
nodes (which sounds wrong, but I'm not familiar with the code), then
simply have the driver call of_platform_populate() itself at the
appropriate time. But that's not going to work well unless the child
nodes have compatible values, which doesn't seem right given their purpose.

>>> -  - samsung,mfc-r : Base address of the first memory bank used by MFC
>>> -		    for DMA contiguous memory allocation and its size.
>>> -
>>> -  - samsung,mfc-l : Base address of the second memory bank used by
>>> MFC
>>> -		    for DMA contiguous memory allocation and its size.
>>
>> These properties shouldn't be removed, but simply marked deprecated. The
>> driver will need to continue to support them so that old DTs work with
>> new kernels. The binding must therefore continue to document them so
>> that the old DT content still makes sense.
> 
> I tend to disagree on this. For Samsung platforms we've been trying to 
> avoid DT bindings changes as much as possible, but I'd rather say that 
> device tree was coupled with kernel version it came from, so Samsung-
> specific bindings haven't been fully stabilized yet, especially since we 
> are still at discussion stage when it's about defining processes for 
> binding staging and stabilization.

Well, that's why everyone is shouting at ARM for abusing DT...

> I would rather see this patch as part of work on Samsung DT binding 
> janitoring or maybe even sanitizing in some cases, like this one, when the 
> old (and IMHO bad) MFC binding was introduced without proper review. I 
> don't think we want to support this kind of brokenness anymore, especially 
> considering the hacks which would be required by such support (see 
> original implementation of this binding and code required in board file).
> 
>>> +Two child nodes must be defined for MFC device. Their names must be
>>> +following: "memport-r" and "memport-l" ("right" and "left"). Required
>>
>> Node names shouldn't have semantic meaning.
> 
> What about bus-master-0 and bus-master-1?

Just "bus-master" for each might make sense. Use reg properties to
differentiate the two?

[PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code

[Tomasz Figa]

On Thursday 08 of August 2013 15:47:19 Stephen Warren wrote:
> On 08/08/2013 03:19 PM, Tomasz Figa wrote:
> > Hi Stephen,
> > 
> > On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:
> >> On 08/05/2013 06:26 AM, Marek Szyprowski wrote:
> >>> MFC driver use custom bindings for managing reserved memory. Those
> >>> bindings are not really specific to MFC device and no even well
> >>> discussed. They can be easily replaced with generic, platform
> >>> independent code for handling reserved and contiguous memory.
> >>> 
> >>> Two additional child devices for each memory port (AXI master) are
> >>> introduced to let one assign some properties to each of them. Later
> >>> one
> >>> can also use them to assign properties related to SYSMMU
> >>> controllers,
> >>> which can be used to manage the limited dma window provided by those
> >>> memory ports.
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> >>> b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> >>> 
> >>> +The MFC device is connected to system bus with two memory ports
> >>> (AXI
> >>> +masters) for better performance. Those memory ports are modelled as
> >>> +separate child devices, so one can assign some properties to them
> >>> (like +memory region for dma buffer allocation or sysmmu
> >>> controller).
> >>> +
> >>> 
> >>>  Required properties:
> >>>    - compatible : value should be either one among the following
> >>>  	
> >>>  	(a) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs
> >>>  	(b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs
> >>> 
> >>> +	and additionally "simple-bus" to correctly initialize child
> >>> +	devices for memory ports (AXI masters)
> >> 
> >> simple-bus is wrong here; the child nodes aren't independent entities
> >> that can be instantiated separately from their parent and without
> >> depending on their parent.
> > 
> > I fully agree, but I can't think of anything better. Could you suggest
> > a solution that would cover our use case:
> > 
> > The MFC IP has two separate bus masters (called here and there
> > memports). On some SoCs, each master must use different memory bank
> > to meet memory bandwidth requirements for higher bitrate video
> > streams. This can be seen as MFC having two DMA subdevices, which
> > have different DMA windows.
> > 
> > On Linux, this is handled by binding two appropriate CMA memory
> > regions to the memports, so the driver can do DMA allocations on
> > behalf of particular memport and get appropriate memory for it.
> 
> I don't see what that has to do with simple-bus.

Well, this is not the first binding doing things this way, unless I don't 
understand something. See the recently posted mvebu bindings. Using 
simple-bus for this has the nice property of allowing both non-DT and DT 
cases to be handled in exactly the same way in MFC driver.

> Whatever parses the
> node of the MFC can directly read from any contained property or child
> node; there's no need to try and get the core DT tree parser to
> enumerate the children.
> 
> If you actually need a struct platform_device for each of these child
> nodes (which sounds wrong, but I'm not familiar with the code)

We need struct device for each memport and CMA region bound to both of 
them. This is a requirement of the Linux DMA mapping API, and well, it 
represents real hardware structure anyway.

> , then
> simply have the driver call of_platform_populate() itself at the
> appropriate time.

This sounds fine to me. 

> But that's not going to work well unless the child
> nodes have compatible values, which doesn't seem right given their
> purpose.
> >>> -  - samsung,mfc-r : Base address of the first memory bank used by
> >>> MFC
> >>> -		    for DMA contiguous memory allocation and its size.
> >>> -
> >>> -  - samsung,mfc-l : Base address of the second memory bank used by
> >>> MFC
> >>> -		    for DMA contiguous memory allocation and its size.
> >> 
> >> These properties shouldn't be removed, but simply marked deprecated.
> >> The driver will need to continue to support them so that old DTs
> >> work with new kernels. The binding must therefore continue to
> >> document them so that the old DT content still makes sense.
> > 
> > I tend to disagree on this. For Samsung platforms we've been trying to
> > avoid DT bindings changes as much as possible, but I'd rather say that
> > device tree was coupled with kernel version it came from, so Samsung-
> > specific bindings haven't been fully stabilized yet, especially since
> > we are still at discussion stage when it's about defining processes
> > for binding staging and stabilization.
> 
> Well, that's why everyone is shouting at ARM for abusing DT...

IMHO this is not fully fair. We have a lot of development happenning on 
ARM. Things usually can't be done perfectly on first iteration, while we 
often want things working reasonably ASAP.

This is why I'm really all for staging/stable separation. I believe things 
need to be tested in practice before we say that they are good already and 
can't be redone, which is what this kind of process would allow.

> > I would rather see this patch as part of work on Samsung DT binding
> > janitoring or maybe even sanitizing in some cases, like this one, when
> > the old (and IMHO bad) MFC binding was introduced without proper
> > review. I don't think we want to support this kind of brokenness
> > anymore, especially considering the hacks which would be required by
> > such support (see original implementation of this binding and code
> > required in board file).> 
> >>> +Two child nodes must be defined for MFC device. Their names must be
> >>> +following: "memport-r" and "memport-l" ("right" and "left").
> >>> Required
> >> 
> >> Node names shouldn't have semantic meaning.
> > 
> > What about bus-master-0 and bus-master-1?
> 
> Just "bus-master" for each might make sense. Use reg properties to
> differentiate the two?

What this reg property would mean in this case?

My understanding of reg property was that it should be used for real 
addresses or IDs and for all other cases node names should be suffixed 
with "-ID".

Best regards,
Tomasz

[PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code

[Stephen Warren]

On 08/08/2013 04:10 PM, Tomasz Figa wrote:
> On Thursday 08 of August 2013 15:47:19 Stephen Warren wrote:
>> On 08/08/2013 03:19 PM, Tomasz Figa wrote:
>>> On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:
>>>> On 08/05/2013 06:26 AM, Marek Szyprowski wrote:
>>>>> MFC driver use custom bindings for managing reserved memory. Those
>>>>> bindings are not really specific to MFC device and no even well
>>>>> discussed. They can be easily replaced with generic, platform
>>>>> independent code for handling reserved and contiguous memory.
>>>>>
>>>>> Two additional child devices for each memory port (AXI master) are
>>>>> introduced to let one assign some properties to each of them. Later
>>>>> one
>>>>> can also use them to assign properties related to SYSMMU
>>>>> controllers,
>>>>> which can be used to manage the limited dma window provided by those
>>>>> memory ports.
...
>>>>> +Two child nodes must be defined for MFC device. Their names must be
>>>>> +following: "memport-r" and "memport-l" ("right" and "left").
>>>>> Required
>>>>
>>>> Node names shouldn't have semantic meaning.
>>>
>>> What about bus-master-0 and bus-master-1?
>>
>> Just "bus-master" for each might make sense. Use reg properties to
>> differentiate the two?
> 
> What this reg property would mean in this case?
> 
> My understanding of reg property was that it should be used for real 
> addresses or IDs and for all other cases node names should be suffixed 
> with "-ID".

Presumably it would hold the ID of the HW block as defined in the
documentation. Or, it could be somewhat arbitrary.