[PATCHv2 0/3] Devicetree bindings for Ion

[PATCHv2 0/3] Devicetree bindings for Ion

[Laura Abbott]

Hi,

This is another attempt at devicetree bindings for Ion. The big complaint from
v1 was that too much unnecessary data was being pushed into devicetree.
v2 takes a different approach of using just compatbile strings for the heaps.
This gets closer to the devicetree philosophy of describing just the hardware.
Each heap ultimately represents some kind of memory that exists in the system.
The compatible string indicates the match for how to handle the type of memory
on this system. The details like heap-id, name etc. are now pushed out to C
files. This makes Ion heaps look closer to something like a quirks framework.
(I'd argue this isn't a complete mischaracterization given the type of setup
Ion gets used for...) The one downside here is that this leads to more new
bindings for each board that gets added.

This version also includes a sample C file which shows what the structure
might look like. As always, your comments and reviews are appreciated.

Thanks,
Laura

Laura Abbott (3):
  ion: Devicetree bindings for Ion
  staging: ion: Add files for parsing the devicetree (WIP)
  NOMERGE: Sample driver

 drivers/staging/android/ion/Kconfig         |  16 +++
 drivers/staging/android/ion/Makefile        |   2 +
 drivers/staging/android/ion/devicetree.txt  |  50 +++++++++
 drivers/staging/android/ion/ion_of.c        | 168 ++++++++++++++++++++++++++++
 drivers/staging/android/ion/ion_of.h        |  35 ++++++
 drivers/staging/android/ion/ion_of_sample.c |  96 ++++++++++++++++
 6 files changed, 367 insertions(+)
 create mode 100644 drivers/staging/android/ion/devicetree.txt
 create mode 100644 drivers/staging/android/ion/ion_of.c
 create mode 100644 drivers/staging/android/ion/ion_of.h
 create mode 100644 drivers/staging/android/ion/ion_of_sample.c

-- 
2.5.0

[PATCHv2 2/3] staging: ion: Add files for parsing the devicetree

[Laura Abbott]

Devicetree is the preferred mechanism for platform definition
these days. Add a set of files for supporting Ion with devicetree.
This is a set of functions for drivers to call to parse the Ion
devicetree along with definitons for defining heaps.

Signed-off-by: Laura Abbott <laura@labbott.name>
---
 drivers/staging/android/ion/Kconfig  |  10 +++
 drivers/staging/android/ion/Makefile |   1 +
 drivers/staging/android/ion/ion_of.c | 168 +++++++++++++++++++++++++++++++++++
 drivers/staging/android/ion/ion_of.h |  35 ++++++++
 4 files changed, 214 insertions(+)
 create mode 100644 drivers/staging/android/ion/ion_of.c
 create mode 100644 drivers/staging/android/ion/ion_of.h

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..9b6d65d 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,13 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_OF
+	bool "Devicetree support for Ion"
+	depends on ION && OF
+	help
+	  Provides base support for defining Ion heaps in devicetree
+	  and setting them up. Also includes functions for platforms
+	  to parse the devicetree and expand for their own custom
+	  extensions
+
+	  If using Ion and devicetree, you should say Y here
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..a77417b 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -7,4 +7,5 @@ endif
 
 obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
 obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_OF) += ion_of.o ion_physmem.o
 
diff --git a/drivers/staging/android/ion/ion_of.c b/drivers/staging/android/ion/ion_of.c
new file mode 100644
index 0000000..9a56194
--- /dev/null
+++ b/drivers/staging/android/ion/ion_of.c
@@ -0,0 +1,168 @@
+/*
+ * Based on work from:
+ *   Andrew Andrianov <andrew@ncrmnt.org>
+ *   Google
+ *   The Linux Foundation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/cma.h>
+#include <linux/dma-contiguous.h>
+#include <linux/io.h>
+#include <linux/of_reserved_mem.h>
+#include "ion.h"
+#include "ion_priv.h"
+#include "ion_of.h"
+
+int ion_parse_dt_heap_common(struct device_node *heap_node,
+			struct ion_platform_heap *heap,
+			struct ion_of_heap *compatible)
+{
+	int i;
+
+	for (i = 0; compatible[i].name != NULL; i++) {
+		if (of_device_is_compatible(heap_node, compatible[i].compat))
+			break;
+	}
+
+	if (compatible[i].name == NULL)
+		return -ENODEV;
+
+	heap->id = compatible[i].heap_id;
+	heap->type = compatible[i].type;
+	heap->name = compatible[i].name;
+	heap->align = compatible[i].align;
+
+	/* Some kind of callback function pointer? */
+
+	pr_info("%s: id %d type %d name %s align %lx\n", __func__,
+			heap->id, heap->type, heap->name, heap->align);
+	return 0;
+}
+
+int ion_setup_heap_common(struct platform_device *parent,
+			struct device_node *heap_node,
+			struct ion_platform_heap *heap)
+{
+	int ret = 0;
+
+	switch (heap->type) {
+		case ION_HEAP_TYPE_CARVEOUT:
+		case ION_HEAP_TYPE_CHUNK:
+			if (heap->base && heap->size)
+				return 0;
+
+			ret = of_reserved_mem_device_init(heap->priv);
+			break;
+		default:
+			break;
+	}
+
+	return ret;
+}
+
+struct ion_platform_data *ion_parse_dt(struct platform_device *pdev,
+					struct ion_of_heap *compatible)
+{
+	int num_heaps, ret;
+	const struct device_node *dt_node = pdev->dev.of_node;
+	struct device_node *node;
+	struct ion_platform_heap *heaps;
+	struct ion_platform_data *data;
+	int i = 0;
+
+	num_heaps = of_get_available_child_count(dt_node);
+
+	if (!num_heaps)
+		return ERR_PTR(-EINVAL);
+
+	heaps = devm_kzalloc(&pdev->dev,
+				sizeof(struct ion_platform_heap)*num_heaps,
+				GFP_KERNEL);
+	if (!heaps)
+		return ERR_PTR(-ENOMEM);
+
+	data = devm_kzalloc(&pdev->dev, sizeof(struct ion_platform_data),
+				GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	for_each_available_child_of_node(dt_node, node) {
+		struct platform_device *heap_pdev;
+
+		ret = ion_parse_dt_heap_common(node, &heaps[i], compatible);
+		if (ret)
+			return ERR_PTR(ret);
+
+		heap_pdev = of_platform_device_create(node, heaps[i].name,
+							&pdev->dev);
+		if (!pdev)
+			return ERR_PTR(-ENOMEM);
+		heap_pdev->dev.platform_data = &heaps[i];
+
+		heaps[i].priv = &heap_pdev->dev;
+
+		ret = ion_setup_heap_common(pdev, node, &heaps[i]);
+		if (ret)
+			return ERR_PTR(ret);
+		i++;
+	}
+
+
+	data->heaps = heaps;
+	data->nr = num_heaps;
+	return data;
+}
+
+#ifdef CONFIG_OF_RESERVED_MEM
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/of_reserved_mem.h>
+
+static int rmem_ion_device_init(struct reserved_mem *rmem, struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ion_platform_heap *heap = pdev->dev.platform_data;
+
+	heap->base = rmem->base;
+	heap->base = rmem->size;
+	pr_debug("%s: heap %s base %pa size %pa dev %p\n", __func__,
+			heap->name, &rmem->base, &rmem->size, dev);
+	return 0;
+}
+
+static void rmem_ion_device_release(struct reserved_mem *rmem,
+					struct device *dev)
+{
+	return;
+}
+
+static const struct reserved_mem_ops rmem_dma_ops = {
+	.device_init	= rmem_ion_device_init,
+	.device_release	= rmem_ion_device_release,
+};
+
+static int __init rmem_ion_setup(struct reserved_mem *rmem)
+{
+	phys_addr_t size = rmem->size;
+
+	size = size / 1024;
+
+	pr_info("Ion memory setup at %pa size %pa MiB\n",
+		&rmem->base, &size);
+	rmem->ops = &rmem_dma_ops;
+	return 0;
+}
+RESERVEDMEM_OF_DECLARE(ion, "ion-region", rmem_ion_setup);
+#endif
diff --git a/drivers/staging/android/ion/ion_of.h b/drivers/staging/android/ion/ion_of.h
new file mode 100644
index 0000000..6b767e7
--- /dev/null
+++ b/drivers/staging/android/ion/ion_of.h
@@ -0,0 +1,35 @@
+/*
+ * Based on work from:
+ *   Andrew Andrianov <andrew@ncrmnt.org>
+ *   Google
+ *   The Linux Foundation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _ION_OF_H
+#define _ION_OF_H
+
+struct ion_of_heap {
+	const char *compat;
+	int heap_id;
+	int type;
+	const char *name;
+	int align;
+};
+
+#define PLATFORM_HEAP(_compat, _id, _type, _name) \
+{ \
+	.compat = _compat, \
+	.heap_id = _id, \
+	.type = _type, \
+	.name = _name, \
+	.align = PAGE_SIZE, \
+}
+
+struct ion_platform_data *ion_parse_dt(struct platform_device *pdev,
+					struct ion_of_heap *compatible);
+
+#endif
-- 
2.5.0

Re: [PATCHv2 2/3] staging: ion: Add files for parsing the devicetree

[Dan Carpenter]

On Mon, Nov 16, 2015 at 04:57:34PM -0800, Laura Abbott wrote:
> +	for_each_available_child_of_node(dt_node, node) {
> +		struct platform_device *heap_pdev;
> +
> +		ret = ion_parse_dt_heap_common(node, &heaps[i], compatible);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		heap_pdev = of_platform_device_create(node, heaps[i].name,
> +							&pdev->dev);

We should free these if something fails later in the function.

> +		if (!pdev)
> +			return ERR_PTR(-ENOMEM);
> +		heap_pdev->dev.platform_data = &heaps[i];
> +
> +		heaps[i].priv = &heap_pdev->dev;
> +
> +		ret = ion_setup_heap_common(pdev, node, &heaps[i]);
> +		if (ret)
> +			return ERR_PTR(ret);


regards,
dan carpenter

[PATCHv2 3/3] NOMERGE: Sample driver

[Laura Abbott]

Small sample driver to show what setup would look like with the dt
bindings

Signed-off-by: Laura Abbott <laura@labbott.name>
---
 drivers/staging/android/ion/Kconfig         |  6 ++
 drivers/staging/android/ion/Makefile        |  1 +
 drivers/staging/android/ion/ion_of_sample.c | 96 +++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 drivers/staging/android/ion/ion_of_sample.c

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 9b6d65d..c2afb35 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -43,3 +43,9 @@ config ION_OF
 	  extensions
 
 	  If using Ion and devicetree, you should say Y here
+
+config ION_OF_SAMPLE
+	bool "Sample driver"
+	depends on ION_OF
+	help
+	  Small sample driver showing the Ion OF interface
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index a77417b..1309b91 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -8,4 +8,5 @@ endif
 obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
 obj-$(CONFIG_ION_TEGRA) += tegra/
 obj-$(CONFIG_ION_OF) += ion_of.o ion_physmem.o
+obj-$(CONFIG_ION_OF_SAMPLE) += ion_of_sample.o
 
diff --git a/drivers/staging/android/ion/ion_of_sample.c b/drivers/staging/android/ion/ion_of_sample.c
new file mode 100644
index 0000000..bbcdf4d
--- /dev/null
+++ b/drivers/staging/android/ion/ion_of_sample.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ * Also based on work from Google, The Linux Foundation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include "ion.h"
+#include "ion_priv.h"
+#include "ion_of.h"
+
+struct sample_ion_dev {
+        struct ion_heap         **heaps;
+        struct ion_device       *idev;
+};
+
+static struct ion_of_heap heaps[] = {
+	PLATFORM_HEAP("sample-system", 0, ION_HEAP_TYPE_SYSTEM, "system"),
+	PLATFORM_HEAP("sample-camera", 1, ION_HEAP_TYPE_DMA, "camera"),
+	PLATFORM_HEAP("sample-fb", 2, ION_HEAP_TYPE_DMA, "fb"),
+	{}
+};
+
+static int ion_sample_probe(struct platform_device *pdev)
+{
+	struct ion_platform_data *data;
+	struct sample_ion_dev *ipdev;
+	int i;
+
+	ipdev = devm_kzalloc(&pdev->dev, sizeof(*ipdev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	ipdev->idev = ion_device_create(NULL);
+	if (!ipdev->idev)
+		return -ENOMEM;
+
+	data = ion_parse_dt(pdev, heaps);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	ipdev->heaps = devm_kzalloc(&pdev->dev,
+				sizeof(struct ion_heap)*data->nr, GFP_KERNEL);
+
+	if (!ipdev->heaps)
+		return -ENOMEM;
+
+	for (i = 0; i < data->nr; i++) {
+		ipdev->heaps[i] = ion_heap_create(&data->heaps[i]);
+		if (!ipdev->heaps[i])
+			return -ENOMEM;
+		ion_device_add_heap(ipdev->idev, ipdev->heaps[i]);
+	}
+	return 0;
+}
+
+static int ion_sample_remove(struct platform_device *pdev)
+{
+	/* Everything should be devm */
+	return 0;
+}
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "sample-ion", },
+	{ /* end of list */ }
+};
+
+static struct platform_driver ion_sample_driver = {
+	.probe		= ion_sample_probe,
+	.remove		= ion_sample_remove,
+	.driver		= {
+		.name	= "ion-of",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+
+static int __init ion_sample_init(void)
+{
+	return platform_driver_register(&ion_sample_driver);
+}
+device_initcall(ion_sample_init);
-- 
2.5.0

[PATCHv2 1/3] ion: Devicetree bindings for Ion

[Laura Abbott]

This adds a base set of devicetree bindings for the Ion memory
manager. This supports setting up the generic set of heaps and
their properties.

Signed-off-by: Laura Abbott <laura-0PSzFVTn/CLa5EbDDlwbIw@public.gmane.org>
---
 drivers/staging/android/ion/devicetree.txt | 50 ++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 drivers/staging/android/ion/devicetree.txt

diff --git a/drivers/staging/android/ion/devicetree.txt b/drivers/staging/android/ion/devicetree.txt
new file mode 100644
index 0000000..e1ea537
--- /dev/null
+++ b/drivers/staging/android/ion/devicetree.txt
@@ -0,0 +1,50 @@
+Ion Memory Manager
+
+Ion is a memory manager that allows for sharing of buffers via dma-buf.
+Ion allows for different types of allocation via an abstraction called
+a 'heap'. A heap represents a specific type of memory. Each heap has
+a different type. There can be multiple instances of the same heap
+type.
+
+Required properties for Ion
+
+- compatible: "linux,ion" PLUS a compatible property for the device
+
+All child nodes of a linux,ion node are interpreted as heaps
+
+required properties for heaps
+
+- compatible: compatible string for a heap type PLUS a compatible property
+for the specific instance of the heap. Current heap types
+-- linux,ion-heap-system
+-- linux,ion-heap-system-contig
+-- linux,ion-heap-carveout
+-- linux,ion-heap-chunk
+-- linux,ion-heap-dma
+-- linux,ion-heap-custom
+
+Optional properties
+- memory-region: A phandle to a memory region. Required for DMA heap type
+(see reserved-memory.txt for details on the reservation)
+
+Example:
+
+	ion {
+		compatbile = "qcom,msm8916-ion", "linux,ion";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ion-system-heap {
+			compatbile = "qcom,system-heap", "linux,ion-heap-system"
+		};
+
+		ion-camera-region {
+			compatible = "qcom,camera-heap", "linux,ion-heap-dma"
+			memory-region = <&camera_region>;
+		};
+
+		ion-fb-region {
+			compatbile = "qcom,fb-heap", "linux,ion-heap-dma"
+			memory-region = <&fb_region>;
+		};
+	}
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 0/3] Devicetree bindings for Ion

[Arnd Bergmann]

On Monday 16 November 2015 16:57:32 Laura Abbott wrote:
> Hi,
> 
> This is another attempt at devicetree bindings for Ion. The big complaint from
> v1 was that too much unnecessary data was being pushed into devicetree.
> v2 takes a different approach of using just compatbile strings for the heaps.
> This gets closer to the devicetree philosophy of describing just the hardware.
> Each heap ultimately represents some kind of memory that exists in the system.
> The compatible string indicates the match for how to handle the type of memory
> on this system. The details like heap-id, name etc. are now pushed out to C
> files. This makes Ion heaps look closer to something like a quirks framework.
> (I'd argue this isn't a complete mischaracterization given the type of setup
> Ion gets used for...) The one downside here is that this leads to more new
> bindings for each board that gets added.
> 
> This version also includes a sample C file which shows what the structure
> might look like. As always, your comments and reviews are appreciated.

I'm still a bit unsure about the concept of hardwiring ion in the DT
bindings. It's not just Linux-specific, it's specific to the implementation
of one or two GPU drivers, and if we fix the bindings for Ion, we might
never be able to migrate them away from this framework.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 0/3] Devicetree bindings for Ion

[Laura Abbott]

On 11/17/15 7:15 AM, Arnd Bergmann wrote:
> On Monday 16 November 2015 16:57:32 Laura Abbott wrote:
>> Hi,
>>
>> This is another attempt at devicetree bindings for Ion. The big complaint from
>> v1 was that too much unnecessary data was being pushed into devicetree.
>> v2 takes a different approach of using just compatbile strings for the heaps.
>> This gets closer to the devicetree philosophy of describing just the hardware.
>> Each heap ultimately represents some kind of memory that exists in the system.
>> The compatible string indicates the match for how to handle the type of memory
>> on this system. The details like heap-id, name etc. are now pushed out to C
>> files. This makes Ion heaps look closer to something like a quirks framework.
>> (I'd argue this isn't a complete mischaracterization given the type of setup
>> Ion gets used for...) The one downside here is that this leads to more new
>> bindings for each board that gets added.
>>
>> This version also includes a sample C file which shows what the structure
>> might look like. As always, your comments and reviews are appreciated.
>
> I'm still a bit unsure about the concept of hardwiring ion in the DT
> bindings. It's not just Linux-specific, it's specific to the implementation
> of one or two GPU drivers, and if we fix the bindings for Ion, we might
> never be able to migrate them away from this framework.
>

Ion isn't just for GPU drivers. It certainly started out that way but
it still fills some gaps in APIs (e.g. the use cases mentioned by
Andrew Andrianov). The goal here was get those who are using Ion
standardized on something and possibly treat those bindings as unstable.
Devicetree bindings are probably going to be very low on the list of
things that will keep drivers from migrating away from Ion. There's
still an implicit assumption there that drivers will be migrating away
from Ion though. I think part of the problem here may be the lack of
direction right now for moving Ion forward. Progress has been slow.
The thought was to at least enable those who are using Ion now and
then revisit whatever we come up with later. If this isn't a good
approach, I'd rather hear consensus now to know where to put effort
and tell people to keep doing whatever outside the mainline kernel.

Thanks,
Laura