Exynos: MFC: clean up device tree bindings

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.

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

 
+The MFC device is connected to system bus with two memory ports (AXI

Quoted 27 lines · 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.

Documentation/devicetree/bindings/media/s5p-mfc.txt +3 -0

 
+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).

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

+
 
 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

Documentation/devicetree/bindings/media/s5p-mfc.txt The MFC device driver is a v4l2 driver which can encode/decode · line 19

Quoted 2 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+	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.

Documentation/devicetree/bindings/media/s5p-mfc.txt

Quoted 6 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +0 -4

-  - 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

Documentation/devicetree/bindings/media/s5p-mfc.txt +0 -1

-		    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.

Documentation/devicetree/bindings/media/s5p-mfc.txt Required properties: · line 34

Quoted 2 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+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.

Documentation/devicetree/bindings/media/s5p-mfc.txt Required properties: · line 36

Quoted 2 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

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

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

Quoted 2 lines

+ - dma-memory-region : optional property with a phandle to
respective memory + region (see

Documentation/devicetree/bindings/media/s5p-mfc.txt

Quoted 2 lines

no region

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

+			is defined, sysmmu controller must be used for 

Documentation/devicetree/bindings/media/s5p-mfc.txt Required properties: · line 40

Quoted 1 line

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

+			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?

Hi Stephen,

Quoted 28 lines · On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:

Quoted 27 lines · 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.

Documentation/devicetree/bindings/media/s5p-mfc.txt +3 -0

 
+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).

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

+
 
 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

Quoted 6 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt The MFC device driver is a v4l2 driver which can encode/decode · line 19

Quoted 2 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+	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.

Quoted 11 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt

Quoted 6 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +0 -4

-  - 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

Documentation/devicetree/bindings/media/s5p-mfc.txt +0 -1

-		    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).

Quoted 4 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt Required properties: · line 34

Quoted 2 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+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?

Quoted 50 lines · Tomasz Figa

Hi Stephen,

Quoted 31 lines · On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:

Quoted 30 lines · 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.

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

 
+The MFC device is connected to system bus with two memory ports

(AXI

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+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).

Documentation/devicetree/bindings/media/s5p-mfc.txt +1 -0

+
 
 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

Quoted 6 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt The MFC device driver is a v4l2 driver which can encode/decode · line 19

Quoted 2 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+	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.

Quoted 3 lines

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.

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt

Quoted 7 lines

- - samsung,mfc-r : Base address of the first memory bank used by MFC

Documentation/devicetree/bindings/media/s5p-mfc.txt +0 -3

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

MFC

Documentation/devicetree/bindings/media/s5p-mfc.txt +0 -1

-		    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.

Quoted 6 lines

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...

Quoted 13 lines

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).>

Quoted 5 lines

Quoted 3 lines

Documentation/devicetree/bindings/media/s5p-mfc.txt +2 -0

+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.

Quoted 1 line

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

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

Quoted 15 lines · On Thursday 08 of August 2013 15:47:19 Stephen Warren wrote:

Quoted 14 lines · Tomasz Figa

Quoted 13 lines · On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote:

Quoted 12 lines · 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.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

(quoted diff) +2 -0

+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?

Quoted 2 lines

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".

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 <[email protected]>

---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |   75 ++++++----------------
--------
 1 file changed, 15 insertions(+), 60 deletions(-)

drivers/media/platform/s5p-mfc/s5p_mfc.c +1 -6

index a130dcd..696e0e0 100644
@@ -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);

drivers/media/platform/s5p-mfc/s5p_mfc.c +0 -7

-		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);

drivers/media/platform/s5p-mfc/s5p_mfc.c +15 -27

-		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

Hi Kukjin,

This patch looks good.

Best wishes,
Kamil Debski

Quoted 3 lines

From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
Sent: Monday, August 05, 2013 2:27 PM

Quoted 5 lines

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<[email protected]>

Acked-by Kamil Debski<[email protected]>

Quoted 14 lines · Kamil Debski

Hi Kukjin,

This patch looks good.

Best wishes,
Kamil Debski

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Marek Szyprowski<[email protected]>

Acked-by Kamil Debski<[email protected]>

Kamil, thanks for your ack.

Applied.

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(-)