[PATCH v2 0/2] staging: ion: Generic ion-physmem driver

[PATCH v2 0/2] staging: ion: Generic ion-physmem driver

[Andrew Andrianov]

Rebased against 4.0-rc7.
This is the resubmit of my previous patches with fixes based on reviews by 
Greg and Paul and some futher improvements.
Changes since previous submit:
* Take care to enable the clock, if clock's specified in devicetree 
  (just like drivers/misc/sram.c)
* Do not ever try to build it as a module - ION doesn't support it (Yet?)  
* More sanity in error-reporting
* Keep track of registered heap ids and complain about duplicates (if any)
* Cleanup

The following ion driver implements a simple way to define ION heaps from a 
devicetree description. e.g.

>From on-chip SRAM:
                ion_im0: ion@00100000 {
                     compatible = "ion,physmem";
                     reg = <0x00100000 0x40000>;
                     reg-names = "memory";
                     ion-heap-id   = <2>;
                     ion-heap-type = <ION_HEAP_TYPE_DMA>;
                     ion-heap-align = <0x10>;
                     ion-heap-name = "IM0";
                };

The same, but with clock gating (if really needed): 
                ion_im0: ion@00100000 {
                     compatible = "ion,physmem";
                     reg = <0x00100000 0x40000>;
                     reg-names = "memory";
                     ion-heap-id   = <2>;
                     ion-heap-type = <ION_HEAP_TYPE_DMA>;
                     ion-heap-align = <0x10>;
                     ion-heap-name = "IM0";
	             clocks = <&pclk>;
		     clock-names = "apb_pclk";
                };

or 
		ion_crv: ion@deadbeef {
		     compatible = "ion,physmem";
		     reg = <0x00000000 0x100000>;
		     reg-names = "memory";
		     ion-heap-id   = <3>;
		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
		     ion-heap-align = <0x10>;
		     ion-heap-name = "carveout";
		};

This way you can define any ION heap, so it pretty much supersedes the dummy 
driver that has everything hardcoded and is good for nothing but tests.
For ION_HEAP_TYPE_DMA, if 'reg' range is present in devicetree it does a 
proper declare_dma_coherent_memory() and parses reg field as a physical address 
range. E.g. this way you can pass on-chip SRAM or dedicated RAM banks
to ION to be used as a heap.
If reg is not present - behavior is identical to a DMA heap of ion_dummy driver.
 
For carveout and chunk heaps it behaves just like the dummy driver, allocating 
some free pages as a heap and freeing them when unloaded. reg range is used 
for size calculations via resource_size() only. 

For system/system_contig things stay pretty much the same, except for it 
doesn't do any page allocations by itself and just passes all
parameters down to ION

My use case: 
The SoC I'm making this for is K1879XB1YA (К1879ХБ1Я) / MB77.07:
Product page: http://www.module.ru/en/catalog/micro/micro_pc/
Hopefully I'll get basic support for this SoC ready for submission by
the next merge window. 
It has dedicated 128MiB 'multimedia' memory that ARM core can't execute
code from, but can write/read to and several huge (256KiB) banks of
on-chip SRAM. All mapped into physical address space.  
ION's job will be pretty much allocating from those banks, each making up it's 
own heap and sharing the buffers between DSP, ARM and a few multimedia devices. 


Andrew Andrianov (2):
  staging: ion: Add generic ion-physmem driver
  staging: ion: Add ion-physmem documentation

 Documentation/devicetree/bindings/ion,physmem.txt |   96 +++++++++
 drivers/staging/android/ion/Kconfig               |    7 +
 drivers/staging/android/ion/Makefile              |    5 +-
 drivers/staging/android/ion/ion_physmem.c         |  226 +++++++++++++++++++++
 include/dt-bindings/ion,physmem.h                 |   17 ++
 5 files changed, 349 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

-- 
1.7.10.4

[PATCH v2 1/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 drivers/staging/android/ion/Kconfig       |    7 +
 drivers/staging/android/ion/Makefile      |    5 +-
 drivers/staging/android/ion/ion_physmem.c |  226 +++++++++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h         |   17 +++
 4 files changed, 253 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..c558cf8 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,10 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_PHYSMEM
+	bool "Generic PhysMem ION driver"
+	depends on ION
+	help
+	  Provides a generic ION driver that registers the
+	  /dev/ion device with heaps from devicetree entries.
+	  This heaps are made of chunks of physical memory
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..ac9856d 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
 obj-$(CONFIG_ION) += compat_ion.o
 endif
 
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
 
diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
new file mode 100644
index 0000000..5ebcd85
--- /dev/null
+++ b/drivers/staging/android/ion/ion_physmem.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ *
+ * 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.
+ *
+ * Generic devicetree physmem driver for ION Memory Manager
+ *
+ */
+
+#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"
+
+#define DRVNAME "ion-physmem"
+
+static struct ion_device *idev;
+static int num_heaps_registered;
+static uint32_t claimed_heap_ids;
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "ion,physmem", },
+	{ /* end of list */ }
+};
+
+struct physmem_ion_dev {
+	struct ion_platform_heap  data;
+	struct ion_heap          *heap;
+	int                       need_free_coherent;
+	void                     *freepage_ptr;
+	struct clk               *clk;
+};
+
+static int ion_physmem_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
+	ion_phys_addr_t addr;
+	size_t size = 0;
+	const char *ion_heap_name = NULL;
+	struct resource *res;
+	struct physmem_ion_dev *ipdev;
+
+	/*
+	   Looks like we can only have one ION device in our system.
+	   Therefore we call ion_device_create on first probe and only
+	   add heaps to it on subsequent probe calls.
+	   FixMe:
+	   1. Do we need to hold a spinlock here?
+	   2. Can several probes race here on SMP?
+	*/
+
+	if (!idev) {
+		idev = ion_device_create(NULL);
+		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
+		if (!idev)
+			return -ENOMEM;
+	}
+
+	ipdev = kzalloc(sizeof(struct physmem_ion_dev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	/* Read out name first for the sake of sane error-reporting */
+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+				       &ion_heap_name);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+				    &ion_heap_id);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	/* Check id to be sane first */
+	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
+		dev_err(&pdev->dev, "bad heap id specified: %d\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	if ((1 << ion_heap_id) & claimed_heap_ids) {
+		dev_err(&pdev->dev, "heap id %d is already claimed\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+				    &ion_heap_type);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+				    &ion_heap_align);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	/* Not always needed, throw no error if missing */
+	if (res) {
+		/* Fill in some defaults */
+		addr = res->start;
+		size = resource_size(res);
+	}
+
+	switch (ion_heap_type) {
+	case ION_HEAP_TYPE_DMA:
+		if (res) {
+			ret = dma_declare_coherent_memory(&pdev->dev,
+							  res->start,
+							  res->start,
+							  resource_size(res),
+							  DMA_MEMORY_MAP |
+							  DMA_MEMORY_EXCLUSIVE);
+			if (ret == 0) {
+				ret = -ENODEV;
+				goto errfreeipdev;
+			}
+		}
+		/*
+		 *  If no memory region declared in dt we assume that
+		 *  the user is okay with plain dma_alloc_coherent.
+		 */
+		break;
+	case ION_HEAP_TYPE_CARVEOUT:
+	case ION_HEAP_TYPE_CHUNK:
+	{
+		if (size == 0) {
+			ret = -EIO;
+			goto errfreeipdev;
+		}
+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
+		if (ipdev->freepage_ptr) {
+			addr = virt_to_phys(ipdev->freepage_ptr);
+		} else {
+			ret = -ENOMEM;
+			goto errfreeipdev;
+		}
+		break;
+	}
+	}
+
+	ipdev->data.id    = ion_heap_id;
+	ipdev->data.type  = ion_heap_type;
+	ipdev->data.name  = ion_heap_name;
+	ipdev->data.align = ion_heap_align;
+	ipdev->data.base  = addr;
+	ipdev->data.size  = size;
+
+	/* This one make dma_declare_coherent_memory actually work */
+	ipdev->data.priv  = &pdev->dev;
+
+	ipdev->heap = ion_heap_create(&ipdev->data);
+	if (!ipdev->heap)
+		goto errfreeipdev;
+
+	/* If it's needed - take care enable clocks */
+	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ipdev->clk))
+		ipdev->clk = NULL;
+	else
+		clk_prepare_enable(ipdev->clk);
+
+	ion_device_add_heap(idev, ipdev->heap);
+	num_heaps_registered++;
+	claimed_heap_ids |= (1 << ion_heap_id);
+
+	dev_info(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
+	       ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
+	       (unsigned long int) addr, ((unsigned long int) size / 1024));
+	return 0;
+
+errfreeipdev:
+	kfree(ipdev);
+	dev_err(&pdev->dev, "Failed to register heap: %s\n",
+		ion_heap_name);
+	return -ENOMEM;
+}
+
+static int ion_physmem_remove(struct platform_device *pdev)
+{
+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
+
+	ion_heap_destroy(ipdev->heap);
+	if (ipdev->need_free_coherent)
+		dma_release_declared_memory(&pdev->dev);
+	if (ipdev->freepage_ptr)
+		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
+	kfree(ipdev->heap);
+	if (ipdev->clk)
+		clk_disable_unprepare(ipdev->clk);
+	kfree(ipdev);
+	num_heaps_registered--;
+	if (0 == num_heaps_registered) {
+		ion_device_destroy(idev);
+		idev = NULL;
+	}
+	return 0;
+}
+
+static struct platform_driver ion_physmem_driver = {
+	.probe		= ion_physmem_probe,
+	.remove		= ion_physmem_remove,
+	.driver		= {
+		.name	= "ion-physmem",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+
+static int __init ion_physmem_init(void)
+{
+	return platform_driver_register(&ion_physmem_driver);
+}
+device_initcall(ion_physmem_init);
diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
new file mode 100644
index 0000000..d26b742
--- /dev/null
+++ b/include/dt-bindings/ion,physmem.h
@@ -0,0 +1,17 @@
+/*
+ * This header provides constants for ION physmem driver.
+ *
+ */
+
+#ifndef _DT_BINDINGS_ION_PHYSMEM_H
+#define _DT_BINDINGS_ION_PHYSMEM_H
+
+#define ION_HEAP_TYPE_SYSTEM         0
+#define ION_HEAP_TYPE_SYSTEM_CONTIG  1
+#define	ION_HEAP_TYPE_CARVEOUT       2
+#define	ION_HEAP_TYPE_CHUNK          3
+#define	ION_HEAP_TYPE_DMA            4
+#define	ION_HEAP_TYPE_CUSTOM         5
+
+
+#endif
-- 
1.7.10.4

[PATCH v2 2/2] staging: ion: Add ion-physmem documentation

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 Documentation/devicetree/bindings/ion,physmem.txt |   96 +++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
new file mode 100644
index 0000000..b83ae22
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,96 @@
+ION PhysMem Driver
+#include <dt-bindings/ion,physmem.h>
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows you to 
+define ION Memory Manager heaps using device tree. This is mostly useful if 
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks, 
+etc) that are present in the physical memory map and you want to add them to 
+ION as heaps of memory. 
+
+
+Examples: 
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
+   address range
+
+	ion_im0: ion@0x00100000 { 
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "IM0";
+	};
+
+2. The same, but using system DMA memory. 
+
+	ion_dma: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "SYSDMA";
+	};
+
+2. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using 
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+		ion_crv: ion@deadbeef {
+		     compatible = "ion,physmem";
+		     reg = <0x00000000 0x100000>;
+		     reg-names = "memory";
+		     ion-heap-id   = <3>;
+		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
+		     ion-heap-align = <0x10>;
+		     ion-heap-name = "carveout";
+		};
+
+3. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using 
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "chunky";
+	};
+
+
+4. vmalloc();
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "sys";
+	};
+
+5. kmalloc();
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "syscont";
+	};
+
+If the underlying heap relies on some physical device that needs clock 
+gating, you can need to fill the clock field in. E.g.: 
+
+ 
+	ion_im0: ion@0x00100000 { 
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "IM0";
+	     clocks = <&oscillator_27m>;
+	     clock-names = "clk_27m";
+	};
-- 
1.7.10.4

Re: [PATCH v2 1/2] staging: ion: Add generic ion-physmem driver

[Greg Kroah-Hartman]

On Sat, Apr 11, 2015 at 12:12:59AM +0300, Andrew Andrianov wrote:
> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>

You need something here in the changelog entry, you just added a whole
new driver.  Please give some information as to what it does, why we
need it, and how to use it.

> ---
>  drivers/staging/android/ion/Kconfig       |    7 +
>  drivers/staging/android/ion/Makefile      |    5 +-
>  drivers/staging/android/ion/ion_physmem.c |  226 +++++++++++++++++++++++++++++
>  include/dt-bindings/ion,physmem.h         |   17 +++
>  4 files changed, 253 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/staging/android/ion/ion_physmem.c
>  create mode 100644 include/dt-bindings/ion,physmem.h
> 
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index 3452346..c558cf8 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -33,3 +33,10 @@ config ION_TEGRA
>  	help
>  	  Choose this option if you wish to use ion on an nVidia Tegra.
>  
> +config ION_PHYSMEM
> +	bool "Generic PhysMem ION driver"
> +	depends on ION
> +	help
> +	  Provides a generic ION driver that registers the
> +	  /dev/ion device with heaps from devicetree entries.
> +	  This heaps are made of chunks of physical memory
> diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
> index b56fd2b..ac9856d 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
>  obj-$(CONFIG_ION) += compat_ion.o
>  endif
>  
> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
> -obj-$(CONFIG_ION_TEGRA) += tegra/
> +obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
> +obj-$(CONFIG_ION_TEGRA)   += tegra/
>  
> diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
> new file mode 100644
> index 0000000..5ebcd85
> --- /dev/null
> +++ b/drivers/staging/android/ion/ion_physmem.c
> @@ -0,0 +1,226 @@
> +/*
> + * Copyright (C) 2015 RC Module
> + * Andrew Andrianov <andrew@ncrmnt.org>
> + *
> + * 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.
> + *
> + * Generic devicetree physmem driver for ION Memory Manager
> + *
> + */
> +
> +#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"
> +
> +#define DRVNAME "ion-physmem"
> +
> +static struct ion_device *idev;
> +static int num_heaps_registered;
> +static uint32_t claimed_heap_ids;
> +
> +static const struct of_device_id of_match_table[] = {
> +	{ .compatible = "ion,physmem", },
> +	{ /* end of list */ }
> +};
> +
> +struct physmem_ion_dev {
> +	struct ion_platform_heap  data;
> +	struct ion_heap          *heap;
> +	int                       need_free_coherent;
> +	void                     *freepage_ptr;
> +	struct clk               *clk;
> +};
> +
> +static int ion_physmem_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	u32 ion_heap_id, ion_heap_align, ion_heap_type;
> +	ion_phys_addr_t addr;
> +	size_t size = 0;
> +	const char *ion_heap_name = NULL;
> +	struct resource *res;
> +	struct physmem_ion_dev *ipdev;
> +
> +	/*
> +	   Looks like we can only have one ION device in our system.
> +	   Therefore we call ion_device_create on first probe and only
> +	   add heaps to it on subsequent probe calls.
> +	   FixMe:
> +	   1. Do we need to hold a spinlock here?
> +	   2. Can several probes race here on SMP?
> +	*/
> +
> +	if (!idev) {
> +		idev = ion_device_create(NULL);
> +		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
> +		if (!idev)
> +			return -ENOMEM;
> +	}
> +
> +	ipdev = kzalloc(sizeof(struct physmem_ion_dev), GFP_KERNEL);
> +	if (!ipdev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ipdev);
> +
> +	/* Read out name first for the sake of sane error-reporting */
> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
> +				       &ion_heap_name);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
> +				    &ion_heap_id);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	/* Check id to be sane first */
> +	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
> +		dev_err(&pdev->dev, "bad heap id specified: %d\n",
> +			ion_heap_id);
> +		goto errfreeipdev;
> +	}
> +
> +	if ((1 << ion_heap_id) & claimed_heap_ids) {
> +		dev_err(&pdev->dev, "heap id %d is already claimed\n",
> +			ion_heap_id);
> +		goto errfreeipdev;
> +	}
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
> +				    &ion_heap_type);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
> +				    &ion_heap_align);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
> +	/* Not always needed, throw no error if missing */
> +	if (res) {
> +		/* Fill in some defaults */
> +		addr = res->start;
> +		size = resource_size(res);
> +	}
> +
> +	switch (ion_heap_type) {
> +	case ION_HEAP_TYPE_DMA:
> +		if (res) {
> +			ret = dma_declare_coherent_memory(&pdev->dev,
> +							  res->start,
> +							  res->start,
> +							  resource_size(res),
> +							  DMA_MEMORY_MAP |
> +							  DMA_MEMORY_EXCLUSIVE);
> +			if (ret == 0) {
> +				ret = -ENODEV;
> +				goto errfreeipdev;
> +			}
> +		}
> +		/*
> +		 *  If no memory region declared in dt we assume that
> +		 *  the user is okay with plain dma_alloc_coherent.
> +		 */
> +		break;
> +	case ION_HEAP_TYPE_CARVEOUT:
> +	case ION_HEAP_TYPE_CHUNK:
> +	{

Why this unneeded brace?

> +		if (size == 0) {
> +			ret = -EIO;
> +			goto errfreeipdev;
> +		}
> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
> +		if (ipdev->freepage_ptr) {
> +			addr = virt_to_phys(ipdev->freepage_ptr);
> +		} else {
> +			ret = -ENOMEM;
> +			goto errfreeipdev;
> +		}
> +		break;
> +	}
> +	}
> +
> +	ipdev->data.id    = ion_heap_id;
> +	ipdev->data.type  = ion_heap_type;
> +	ipdev->data.name  = ion_heap_name;
> +	ipdev->data.align = ion_heap_align;
> +	ipdev->data.base  = addr;
> +	ipdev->data.size  = size;
> +
> +	/* This one make dma_declare_coherent_memory actually work */
> +	ipdev->data.priv  = &pdev->dev;
> +
> +	ipdev->heap = ion_heap_create(&ipdev->data);
> +	if (!ipdev->heap)
> +		goto errfreeipdev;
> +
> +	/* If it's needed - take care enable clocks */
> +	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ipdev->clk))
> +		ipdev->clk = NULL;
> +	else
> +		clk_prepare_enable(ipdev->clk);
> +
> +	ion_device_add_heap(idev, ipdev->heap);
> +	num_heaps_registered++;
> +	claimed_heap_ids |= (1 << ion_heap_id);
> +
> +	dev_info(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
> +	       ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
> +	       (unsigned long int) addr, ((unsigned long int) size / 1024));

Don't be noisy for "normal" operations, this should be debug only as no
one really cares.

> +	return 0;
> +
> +errfreeipdev:
> +	kfree(ipdev);
> +	dev_err(&pdev->dev, "Failed to register heap: %s\n",
> +		ion_heap_name);
> +	return -ENOMEM;
> +}
> +
> +static int ion_physmem_remove(struct platform_device *pdev)
> +{
> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
> +
> +	ion_heap_destroy(ipdev->heap);
> +	if (ipdev->need_free_coherent)
> +		dma_release_declared_memory(&pdev->dev);
> +	if (ipdev->freepage_ptr)
> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
> +	kfree(ipdev->heap);
> +	if (ipdev->clk)
> +		clk_disable_unprepare(ipdev->clk);
> +	kfree(ipdev);
> +	num_heaps_registered--;
> +	if (0 == num_heaps_registered) {
> +		ion_device_destroy(idev);
> +		idev = NULL;
> +	}

We really have no other way to do allocation/cleanup than a simple
integer value?  Shouldn't the ion device have reference counting in it?

thanks,

greg k-h

[PATCH v3 0/2] staging: ion: ion-physmem driver

[Andrew Andrianov]

Thanks for the review, Greg. I've fixed the things you've noted.

I have removed the call to ion_device_destroy() in the 
ion_physmem_remove() and added a comment about it. We won't unload
the driver anyway for now.
I admit, integer counting was a dirty hack there, 
but struct ion_device contents are private to ion.c, so ion 
drivers can't access them. 
Unless I'm missing something, settling on only removing 
heaps is the only right way to do it right now. Correct me 
if I'm wrong.  

Thanks, 
Andrew. 

Andrew 'Necromant' Andrianov (2):
  staging: ion: Add generic ion-physmem driver
  staging: ion: Add ion-physmem documentation

 Documentation/devicetree/bindings/ion,physmem.txt |  96 +++++++++
 drivers/staging/android/ion/Kconfig               |   7 +
 drivers/staging/android/ion/Makefile              |   5 +-
 drivers/staging/android/ion/ion_physmem.c         | 229 ++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h                 |  17 ++
 5 files changed, 352 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

-- 
2.1.4

[PATCH v3 1/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

From: Andrew 'Necromant' Andrianov <andrew@ncrmnt.org>

This patch adds a generic ion driver that allows
ion heaps to be added via devicetree. It provides
a simple and generic way to feed physical memory regions
to ion without writing a custom driver, e.g.

	ion_sram: ion@0x00100000 {
	     compatible = "ion,physmem";
	     reg = <0x00100000 0x40000>;
	     reg-names = "memory";
	     ion-heap-id   = <1>;
	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
	     ion-heap-align = <0x10>;
	     ion-heap-name = "SRAM";
	};

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 drivers/staging/android/ion/Kconfig       |   7 +
 drivers/staging/android/ion/Makefile      |   5 +-
 drivers/staging/android/ion/ion_physmem.c | 229 ++++++++++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h         |  17 +++
 4 files changed, 256 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..c558cf8 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,10 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_PHYSMEM
+	bool "Generic PhysMem ION driver"
+	depends on ION
+	help
+	  Provides a generic ION driver that registers the
+	  /dev/ion device with heaps from devicetree entries.
+	  This heaps are made of chunks of physical memory
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..ac9856d 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
 obj-$(CONFIG_ION) += compat_ion.o
 endif
 
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
 
diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
new file mode 100644
index 0000000..ce2d238
--- /dev/null
+++ b/drivers/staging/android/ion/ion_physmem.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ *
+ * 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.
+ *
+ * Generic devicetree physmem driver for ION Memory Manager
+ *
+ */
+
+#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"
+
+#define DRVNAME "ion-physmem"
+
+static struct ion_device *idev;
+static uint32_t claimed_heap_ids;
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "ion,physmem", },
+	{ /* end of list */ }
+};
+
+struct physmem_ion_dev {
+	struct ion_platform_heap  data;
+	struct ion_heap          *heap;
+	int                       need_free_coherent;
+	void                     *freepage_ptr;
+	struct clk               *clk;
+	uint32_t                  heap_id;
+};
+
+static int ion_physmem_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
+	ion_phys_addr_t addr;
+	size_t size = 0;
+	const char *ion_heap_name = NULL;
+	struct resource *res;
+	struct physmem_ion_dev *ipdev;
+
+	/*
+	   Looks like we can only have one ION device in our system.
+	   Therefore we call ion_device_create on first probe and only
+	   add heaps to it on subsequent probe calls.
+	   FixMe:
+	   1. Do we need to hold a spinlock here?
+	   2. Can several probes race here on SMP?
+	*/
+
+	if (!idev) {
+		idev = ion_device_create(NULL);
+		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
+		if (!idev)
+			return -ENOMEM;
+	}
+
+	ipdev = kzalloc(sizeof(struct physmem_ion_dev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	/* Read out name first for the sake of sane error-reporting */
+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+				       &ion_heap_name);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+				    &ion_heap_id);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	/* Check id to be sane first */
+	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
+		dev_err(&pdev->dev, "bad heap id specified: %d\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	if ((1 << ion_heap_id) & claimed_heap_ids) {
+		dev_err(&pdev->dev, "heap id %d is already claimed\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+				    &ion_heap_type);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+				    &ion_heap_align);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	/* Not always needed, throw no error if missing */
+	if (res) {
+		/* Fill in some defaults */
+		addr = res->start;
+		size = resource_size(res);
+	}
+
+	switch (ion_heap_type) {
+	case ION_HEAP_TYPE_DMA:
+		if (res) {
+			ret = dma_declare_coherent_memory(&pdev->dev,
+							  res->start,
+							  res->start,
+							  resource_size(res),
+							  DMA_MEMORY_MAP |
+							  DMA_MEMORY_EXCLUSIVE);
+			if (ret == 0) {
+				ret = -ENODEV;
+				goto errfreeipdev;
+			}
+		}
+		/*
+		 *  If no memory region declared in dt we assume that
+		 *  the user is okay with plain dma_alloc_coherent.
+		 */
+		break;
+	case ION_HEAP_TYPE_CARVEOUT:
+	case ION_HEAP_TYPE_CHUNK:
+		if (size == 0) {
+			ret = -EIO;
+			goto errfreeipdev;
+		}
+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
+		if (ipdev->freepage_ptr) {
+			addr = virt_to_phys(ipdev->freepage_ptr);
+		} else {
+			ret = -ENOMEM;
+			goto errfreeipdev;
+		}
+		break;
+	}
+
+	ipdev->data.id    = ion_heap_id;
+	ipdev->data.type  = ion_heap_type;
+	ipdev->data.name  = ion_heap_name;
+	ipdev->data.align = ion_heap_align;
+	ipdev->data.base  = addr;
+	ipdev->data.size  = size;
+
+	/* This one make dma_declare_coherent_memory actually work */
+	ipdev->data.priv  = &pdev->dev;
+
+	ipdev->heap = ion_heap_create(&ipdev->data);
+	if (!ipdev->heap)
+		goto errfreeipdev;
+
+	/* If it's needed - take care enable clocks */
+	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ipdev->clk))
+		ipdev->clk = NULL;
+	else
+		clk_prepare_enable(ipdev->clk);
+
+	ion_device_add_heap(idev, ipdev->heap);
+	claimed_heap_ids |= (1 << ion_heap_id);
+	ipdev->heap_id = ion_heap_id;
+
+	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
+	       ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
+	       (unsigned long int) addr, ((unsigned long int) size / 1024));
+	return 0;
+
+errfreeipdev:
+	kfree(ipdev);
+	dev_err(&pdev->dev, "Failed to register heap: %s\n",
+		ion_heap_name);
+	return -ENOMEM;
+}
+
+static int ion_physmem_remove(struct platform_device *pdev)
+{
+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
+
+	ion_heap_destroy(ipdev->heap);
+	claimed_heap_ids &= ~(1 << ipdev->heap_id);
+	if (ipdev->need_free_coherent)
+		dma_release_declared_memory(&pdev->dev);
+	if (ipdev->freepage_ptr)
+		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
+	kfree(ipdev->heap);
+	if (ipdev->clk)
+		clk_disable_unprepare(ipdev->clk);
+	kfree(ipdev);
+
+	/* We only remove heaps and disable clocks here.
+	 * There's no point in nuking the device itself, since:
+	 * a). ION driver modules can't be unloaded (yet?)
+	 * b). ion_device_destroy() looks like a stub and doesn't
+	 * take care to free heaps/clients.
+	 * c). I can't think of a scenario where it will be required
+	 */
+
+	return 0;
+}
+
+static struct platform_driver ion_physmem_driver = {
+	.probe		= ion_physmem_probe,
+	.remove		= ion_physmem_remove,
+	.driver		= {
+		.name	= "ion-physmem",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+
+static int __init ion_physmem_init(void)
+{
+	return platform_driver_register(&ion_physmem_driver);
+}
+device_initcall(ion_physmem_init);
diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
new file mode 100644
index 0000000..d26b742
--- /dev/null
+++ b/include/dt-bindings/ion,physmem.h
@@ -0,0 +1,17 @@
+/*
+ * This header provides constants for ION physmem driver.
+ *
+ */
+
+#ifndef _DT_BINDINGS_ION_PHYSMEM_H
+#define _DT_BINDINGS_ION_PHYSMEM_H
+
+#define ION_HEAP_TYPE_SYSTEM         0
+#define ION_HEAP_TYPE_SYSTEM_CONTIG  1
+#define	ION_HEAP_TYPE_CARVEOUT       2
+#define	ION_HEAP_TYPE_CHUNK          3
+#define	ION_HEAP_TYPE_DMA            4
+#define	ION_HEAP_TYPE_CUSTOM         5
+
+
+#endif
-- 
2.1.4

[PATCH v3 2/2] staging: ion: Add ion-physmem documentation

[Andrew Andrianov]

From: Andrew 'Necromant' Andrianov <andrew@ncrmnt.org>

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 Documentation/devicetree/bindings/ion,physmem.txt | 96 +++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
new file mode 100644
index 0000000..b83ae22
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,96 @@
+ION PhysMem Driver
+#include <dt-bindings/ion,physmem.h>
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows you to 
+define ION Memory Manager heaps using device tree. This is mostly useful if 
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks, 
+etc) that are present in the physical memory map and you want to add them to 
+ION as heaps of memory. 
+
+
+Examples: 
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
+   address range
+
+	ion_im0: ion@0x00100000 { 
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "IM0";
+	};
+
+2. The same, but using system DMA memory. 
+
+	ion_dma: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "SYSDMA";
+	};
+
+3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using 
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+		ion_crv: ion@deadbeef {
+		     compatible = "ion,physmem";
+		     reg = <0x00000000 0x100000>;
+		     reg-names = "memory";
+		     ion-heap-id   = <3>;
+		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
+		     ion-heap-align = <0x10>;
+		     ion-heap-name = "carveout";
+		};
+
+4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using 
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "chunky";
+	};
+
+
+5. vmalloc();
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "sys";
+	};
+
+6. kmalloc();
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "syscont";
+	};
+
+If the underlying heap relies on some physical device that needs clock 
+gating, you may need to fill the clock field in. E.g.: 
+
+ 
+	ion_im0: ion@0x00100000 { 
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "IM0";
+	     clocks = <&oscillator_27m>;
+	     clock-names = "clk_27m";
+	};
-- 
2.1.4

Re: [PATCH v3 1/2] staging: ion: Add generic ion-physmem driver

[Sudip Mukherjee]

On Tue, Jun 02, 2015 at 07:00:39PM +0300, Andrew Andrianov wrote:
> From: Andrew 'Necromant' Andrianov <andrew@ncrmnt.org>
> 
> This patch adds a generic ion driver that allows
> ion heaps to be added via devicetree. It provides
> a simple and generic way to feed physical memory regions
> to ion without writing a custom driver, e.g.
> 
> 	ion_sram: ion@0x00100000 {
> 	     compatible = "ion,physmem";
> 	     reg = <0x00100000 0x40000>;
> 	     reg-names = "memory";
> 	     ion-heap-id   = <1>;
> 	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
> 	     ion-heap-align = <0x10>;
> 	     ion-heap-name = "SRAM";
> 	};
> 
> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>

Your From: name and Signed-off-by: name is not matching.
But why you are using this extra From: line? you email header From:
is same as your Signed-off-by.

And since you are adding new files it would be better if you can fix
few checkpatch warnings it has. like:

CHECK: Prefer kzalloc(sizeof(*ipdev)...) over kzalloc(sizeof(struct physmem_ion_dev)...)
CHECK: Alignment should match open parenthesis
CHECK: No space is necessary after a cast
CHECK: Please don't use multiple blank lines

regards
sudip

Re: [PATCH v3 2/2] staging: ion: Add ion-physmem documentation

[Sudip Mukherjee]

On Tue, Jun 02, 2015 at 07:00:40PM +0300, Andrew Andrianov wrote:
> From: Andrew 'Necromant' Andrianov <andrew@ncrmnt.org>
> 
> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>

same problem of From: not matching with Signed-off-by:
and lots of trailing whitespace errors.

regards
sudip

[PATCH v4 0/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

Sudip Mukherjee писал 03.06.2015 09:15:
> On Tue, Jun 02, 2015 at 07:00:39PM +0300, Andrew Andrianov wrote:
>> From: Andrew 'Necromant' Andrianov <andrew@ncrmnt.org>
>>
>> This patch adds a generic ion driver that allows
>> ion heaps to be added via devicetree. It provides
>> a simple and generic way to feed physical memory regions
>> to ion without writing a custom driver, e.g.
>>
>> 	ion_sram: ion@0x00100000 {
>> 	     compatible = "ion,physmem";
>> 	     reg = <0x00100000 0x40000>;
>> 	     reg-names = "memory";
>> 	     ion-heap-id   = <1>;
>> 	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> 	     ion-heap-align = <0x10>;
>> 	     ion-heap-name = "SRAM";
>> 	};
>>
>> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
> 
> Your From: name and Signed-off-by: name is not matching.

Fixed, it's been due to different setups on my home 
and work PCs. I'm still playing with proper email setup for 
LKML.  

> But why you are using this extra From: line? you email header From:
> is same as your Signed-off-by.
> And since you are adding new files it would be better if you can fix
> few checkpatch warnings it has. like:
> 
> CHECK: Prefer kzalloc(sizeof(*ipdev)...) over kzalloc(sizeof(struct
> physmem_ion_dev)...)
> CHECK: Alignment should match open parenthesis
> CHECK: No space is necessary after a cast
> CHECK: Please don't use multiple blank lines
> regards
> sudip

I've ran checkpatch again, now with --strict, everything should be 
clean now. Thanks for your comments. 

Andrew Andrianov (2):
  staging: ion: Add generic ion-physmem driver
  staging: ion: Add ion-physmem documentation

 Documentation/devicetree/bindings/ion,physmem.txt |  98 +++++++++
 drivers/staging/android/ion/Kconfig               |   7 +
 drivers/staging/android/ion/Makefile              |   5 +-
 drivers/staging/android/ion/ion_physmem.c         | 229 ++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h                 |  17 ++
 5 files changed, 354 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

-- 
2.1.4

[PATCH v4 1/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

This patch adds a generic ion driver that allows
ion heaps to be added via devicetree. It provides
a simple and generic way to feed physical memory regions
to ion without writing a custom driver, e.g.

	ion_sram: ion@0x00100000 {
	     compatible = "ion,physmem";
	     reg = <0x00100000 0x40000>;
	     reg-names = "memory";
	     ion-heap-id   = <1>;
	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
	     ion-heap-align = <0x10>;
	     ion-heap-name = "SRAM";
	};

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 drivers/staging/android/ion/Kconfig       |   7 +
 drivers/staging/android/ion/Makefile      |   5 +-
 drivers/staging/android/ion/ion_physmem.c | 229 ++++++++++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h         |  17 +++
 4 files changed, 256 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..c558cf8 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,10 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_PHYSMEM
+	bool "Generic PhysMem ION driver"
+	depends on ION
+	help
+	  Provides a generic ION driver that registers the
+	  /dev/ion device with heaps from devicetree entries.
+	  This heaps are made of chunks of physical memory
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..ac9856d 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
 obj-$(CONFIG_ION) += compat_ion.o
 endif
 
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
 
diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
new file mode 100644
index 0000000..ba5135f
--- /dev/null
+++ b/drivers/staging/android/ion/ion_physmem.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ *
+ * 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.
+ *
+ * Generic devicetree physmem driver for ION Memory Manager
+ *
+ */
+
+#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"
+
+#define DRVNAME "ion-physmem"
+
+static struct ion_device *idev;
+static uint32_t claimed_heap_ids;
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "ion,physmem", },
+	{ /* end of list */ }
+};
+
+struct physmem_ion_dev {
+	struct ion_platform_heap  data;
+	struct ion_heap          *heap;
+	int                       need_free_coherent;
+	void                     *freepage_ptr;
+	struct clk               *clk;
+	uint32_t                  heap_id;
+};
+
+static int ion_physmem_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
+	ion_phys_addr_t addr;
+	size_t size = 0;
+	const char *ion_heap_name = NULL;
+	struct resource *res;
+	struct physmem_ion_dev *ipdev;
+
+	/*
+	   Looks like we can only have one ION device in our system.
+	   Therefore we call ion_device_create on first probe and only
+	   add heaps to it on subsequent probe calls.
+	   FixMe:
+	   1. Do we need to hold a spinlock here?
+	   2. Can several probes race here on SMP?
+	*/
+
+	if (!idev) {
+		idev = ion_device_create(NULL);
+		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
+		if (!idev)
+			return -ENOMEM;
+	}
+
+	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	/* Read out name first for the sake of sane error-reporting */
+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+				       &ion_heap_name);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+				    &ion_heap_id);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	/* Check id to be sane first */
+	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
+		dev_err(&pdev->dev, "bad heap id specified: %d\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	if ((1 << ion_heap_id) & claimed_heap_ids) {
+		dev_err(&pdev->dev, "heap id %d is already claimed\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+				    &ion_heap_type);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+				    &ion_heap_align);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	/* Not always needed, throw no error if missing */
+	if (res) {
+		/* Fill in some defaults */
+		addr = res->start;
+		size = resource_size(res);
+	}
+
+	switch (ion_heap_type) {
+	case ION_HEAP_TYPE_DMA:
+		if (res) {
+			ret = dma_declare_coherent_memory(&pdev->dev,
+							  res->start,
+							  res->start,
+							  resource_size(res),
+							  DMA_MEMORY_MAP |
+							  DMA_MEMORY_EXCLUSIVE);
+			if (ret == 0) {
+				ret = -ENODEV;
+				goto errfreeipdev;
+			}
+		}
+		/*
+		 *  If no memory region declared in dt we assume that
+		 *  the user is okay with plain dma_alloc_coherent.
+		 */
+		break;
+	case ION_HEAP_TYPE_CARVEOUT:
+	case ION_HEAP_TYPE_CHUNK:
+		if (size == 0) {
+			ret = -EIO;
+			goto errfreeipdev;
+		}
+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
+		if (ipdev->freepage_ptr) {
+			addr = virt_to_phys(ipdev->freepage_ptr);
+		} else {
+			ret = -ENOMEM;
+			goto errfreeipdev;
+		}
+		break;
+	}
+
+	ipdev->data.id    = ion_heap_id;
+	ipdev->data.type  = ion_heap_type;
+	ipdev->data.name  = ion_heap_name;
+	ipdev->data.align = ion_heap_align;
+	ipdev->data.base  = addr;
+	ipdev->data.size  = size;
+
+	/* This one make dma_declare_coherent_memory actually work */
+	ipdev->data.priv  = &pdev->dev;
+
+	ipdev->heap = ion_heap_create(&ipdev->data);
+	if (!ipdev->heap)
+		goto errfreeipdev;
+
+	/* If it's needed - take care enable clocks */
+	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ipdev->clk))
+		ipdev->clk = NULL;
+	else
+		clk_prepare_enable(ipdev->clk);
+
+	ion_device_add_heap(idev, ipdev->heap);
+	claimed_heap_ids |= (1 << ion_heap_id);
+	ipdev->heap_id = ion_heap_id;
+
+	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
+		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
+		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
+	return 0;
+
+errfreeipdev:
+	kfree(ipdev);
+	dev_err(&pdev->dev, "Failed to register heap: %s\n",
+		ion_heap_name);
+	return -ENOMEM;
+}
+
+static int ion_physmem_remove(struct platform_device *pdev)
+{
+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
+
+	ion_heap_destroy(ipdev->heap);
+	claimed_heap_ids &= ~(1 << ipdev->heap_id);
+	if (ipdev->need_free_coherent)
+		dma_release_declared_memory(&pdev->dev);
+	if (ipdev->freepage_ptr)
+		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
+	kfree(ipdev->heap);
+	if (ipdev->clk)
+		clk_disable_unprepare(ipdev->clk);
+	kfree(ipdev);
+
+	/* We only remove heaps and disable clocks here.
+	 * There's no point in nuking the device itself, since:
+	 * a). ION driver modules can't be unloaded (yet?)
+	 * b). ion_device_destroy() looks like a stub and doesn't
+	 * take care to free heaps/clients.
+	 * c). I can't think of a scenario where it will be required
+	 */
+
+	return 0;
+}
+
+static struct platform_driver ion_physmem_driver = {
+	.probe		= ion_physmem_probe,
+	.remove		= ion_physmem_remove,
+	.driver		= {
+		.name	= "ion-physmem",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+
+static int __init ion_physmem_init(void)
+{
+	return platform_driver_register(&ion_physmem_driver);
+}
+device_initcall(ion_physmem_init);
diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
new file mode 100644
index 0000000..d26b742
--- /dev/null
+++ b/include/dt-bindings/ion,physmem.h
@@ -0,0 +1,16 @@
+/*
+ * This header provides constants for ION physmem driver.
+ *
+ */
+
+#ifndef _DT_BINDINGS_ION_PHYSMEM_H
+#define _DT_BINDINGS_ION_PHYSMEM_H
+
+#define        ION_HEAP_TYPE_SYSTEM         0
+#define        ION_HEAP_TYPE_SYSTEM_CONTIG  1
+#define	ION_HEAP_TYPE_CARVEOUT       2
+#define	ION_HEAP_TYPE_CHUNK          3
+#define	ION_HEAP_TYPE_DMA            4
+#define	ION_HEAP_TYPE_CUSTOM         5
+
+#endif
-- 
2.1.4

[PATCH v4 2/2] staging: ion: Add ion-physmem documentation

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 Documentation/devicetree/bindings/ion,physmem.txt | 98 +++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
new file mode 100644
index 0000000..e8c64dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,98 @@
+ION PhysMem Driver
+#include <dt-bindings/ion,physmem.h>
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows you to
+define ION Memory Manager heaps using device tree. This is mostly useful if
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks,
+etc) that are present in the physical memory map and you want to add them to
+ION as heaps of memory.
+
+
+Examples:
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
+   address range
+
+	ion_im0: ion@0x00100000 {
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "IM0";
+	};
+
+2. The same, but using system DMA memory.
+
+	ion_dma: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "SYSDMA";
+	};
+
+3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+		ion_crv: ion@deadbeef {
+		     compatible = "ion,physmem";
+		     reg = <0x00000000 0x100000>;
+		     reg-names = "memory";
+		     ion-heap-id   = <3>;
+		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
+		     ion-heap-align = <0x10>;
+		     ion-heap-name = "carveout";
+		};
+
+4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "chunky";
+	};
+
+
+5. vmalloc();
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "sys";
+	};
+
+6. kmalloc();
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "syscont";
+	};
+
+If the underlying heap relies on some physical device that needs clock
+gating, you may need to fill the clocks field in. E.g.:
+
+
+	ion_im0: ion@0x00100000 {
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "IM0";
+	     clocks = <&oscillator_27m>;
+	     clock-names = "clk_27m";
+	};
+
+ion-physmem will do everything required to enable and disable the clock.
-- 
2.1.4

Re: [PATCH v4 1/2] staging: ion: Add generic ion-physmem driver

[Greg Kroah-Hartman]

On Tue, Jun 09, 2015 at 05:58:24PM +0300, Andrew Andrianov wrote:
> This patch adds a generic ion driver that allows
> ion heaps to be added via devicetree. It provides
> a simple and generic way to feed physical memory regions
> to ion without writing a custom driver, e.g.
> 
> 	ion_sram: ion@0x00100000 {
> 	     compatible = "ion,physmem";
> 	     reg = <0x00100000 0x40000>;
> 	     reg-names = "memory";
> 	     ion-heap-id   = <1>;
> 	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
> 	     ion-heap-align = <0x10>;
> 	     ion-heap-name = "SRAM";
> 	};
> 
> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
> ---
>  drivers/staging/android/ion/Kconfig       |   7 +
>  drivers/staging/android/ion/Makefile      |   5 +-
>  drivers/staging/android/ion/ion_physmem.c | 229 ++++++++++++++++++++++++++++++
>  include/dt-bindings/ion,physmem.h         |  17 +++
>  4 files changed, 256 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/staging/android/ion/ion_physmem.c
>  create mode 100644 include/dt-bindings/ion,physmem.h
> 
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index 3452346..c558cf8 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -33,3 +33,10 @@ config ION_TEGRA
>  	help
>  	  Choose this option if you wish to use ion on an nVidia Tegra.
>  
> +config ION_PHYSMEM
> +	bool "Generic PhysMem ION driver"
> +	depends on ION
> +	help
> +	  Provides a generic ION driver that registers the
> +	  /dev/ion device with heaps from devicetree entries.
> +	  This heaps are made of chunks of physical memory

That last sentance doesn't make sense to me :(

And have you tested this driver build on a non-DT build (like x86-64)?

> diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
> index b56fd2b..ac9856d 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
>  obj-$(CONFIG_ION) += compat_ion.o
>  endif
>  
> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
> -obj-$(CONFIG_ION_TEGRA) += tegra/
> +obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
> +obj-$(CONFIG_ION_TEGRA)   += tegra/
>  
> diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
> new file mode 100644
> index 0000000..ba5135f
> --- /dev/null
> +++ b/drivers/staging/android/ion/ion_physmem.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2015 RC Module
> + * Andrew Andrianov <andrew@ncrmnt.org>
> + *
> + * 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.
> + *
> + * Generic devicetree physmem driver for ION Memory Manager
> + *
> + */
> +
> +#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"
> +
> +#define DRVNAME "ion-physmem"
> +
> +static struct ion_device *idev;
> +static uint32_t claimed_heap_ids;
> +
> +static const struct of_device_id of_match_table[] = {
> +	{ .compatible = "ion,physmem", },
> +	{ /* end of list */ }
> +};
> +
> +struct physmem_ion_dev {
> +	struct ion_platform_heap  data;
> +	struct ion_heap          *heap;
> +	int                       need_free_coherent;
> +	void                     *freepage_ptr;
> +	struct clk               *clk;
> +	uint32_t                  heap_id;
> +};
> +
> +static int ion_physmem_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	u32 ion_heap_id, ion_heap_align, ion_heap_type;
> +	ion_phys_addr_t addr;
> +	size_t size = 0;
> +	const char *ion_heap_name = NULL;
> +	struct resource *res;
> +	struct physmem_ion_dev *ipdev;
> +
> +	/*
> +	   Looks like we can only have one ION device in our system.
> +	   Therefore we call ion_device_create on first probe and only
> +	   add heaps to it on subsequent probe calls.
> +	   FixMe:
> +	   1. Do we need to hold a spinlock here?
> +	   2. Can several probes race here on SMP?
> +	*/
> +
> +	if (!idev) {
> +		idev = ion_device_create(NULL);
> +		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
> +		if (!idev)
> +			return -ENOMEM;
> +	}
> +
> +	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
> +	if (!ipdev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ipdev);
> +
> +	/* Read out name first for the sake of sane error-reporting */
> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
> +				       &ion_heap_name);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
> +				    &ion_heap_id);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	/* Check id to be sane first */
> +	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
> +		dev_err(&pdev->dev, "bad heap id specified: %d\n",
> +			ion_heap_id);
> +		goto errfreeipdev;
> +	}
> +
> +	if ((1 << ion_heap_id) & claimed_heap_ids) {
> +		dev_err(&pdev->dev, "heap id %d is already claimed\n",
> +			ion_heap_id);
> +		goto errfreeipdev;
> +	}
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
> +				    &ion_heap_type);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
> +				    &ion_heap_align);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
> +	/* Not always needed, throw no error if missing */
> +	if (res) {
> +		/* Fill in some defaults */
> +		addr = res->start;
> +		size = resource_size(res);
> +	}
> +
> +	switch (ion_heap_type) {
> +	case ION_HEAP_TYPE_DMA:
> +		if (res) {
> +			ret = dma_declare_coherent_memory(&pdev->dev,
> +							  res->start,
> +							  res->start,
> +							  resource_size(res),
> +							  DMA_MEMORY_MAP |
> +							  DMA_MEMORY_EXCLUSIVE);
> +			if (ret == 0) {
> +				ret = -ENODEV;
> +				goto errfreeipdev;
> +			}
> +		}
> +		/*
> +		 *  If no memory region declared in dt we assume that
> +		 *  the user is okay with plain dma_alloc_coherent.
> +		 */
> +		break;
> +	case ION_HEAP_TYPE_CARVEOUT:
> +	case ION_HEAP_TYPE_CHUNK:
> +		if (size == 0) {
> +			ret = -EIO;
> +			goto errfreeipdev;
> +		}
> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
> +		if (ipdev->freepage_ptr) {
> +			addr = virt_to_phys(ipdev->freepage_ptr);
> +		} else {
> +			ret = -ENOMEM;
> +			goto errfreeipdev;
> +		}
> +		break;
> +	}
> +
> +	ipdev->data.id    = ion_heap_id;
> +	ipdev->data.type  = ion_heap_type;
> +	ipdev->data.name  = ion_heap_name;
> +	ipdev->data.align = ion_heap_align;
> +	ipdev->data.base  = addr;
> +	ipdev->data.size  = size;
> +
> +	/* This one make dma_declare_coherent_memory actually work */
> +	ipdev->data.priv  = &pdev->dev;
> +
> +	ipdev->heap = ion_heap_create(&ipdev->data);
> +	if (!ipdev->heap)
> +		goto errfreeipdev;
> +
> +	/* If it's needed - take care enable clocks */
> +	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ipdev->clk))
> +		ipdev->clk = NULL;
> +	else
> +		clk_prepare_enable(ipdev->clk);
> +
> +	ion_device_add_heap(idev, ipdev->heap);
> +	claimed_heap_ids |= (1 << ion_heap_id);
> +	ipdev->heap_id = ion_heap_id;
> +
> +	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
> +		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
> +		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
> +	return 0;
> +
> +errfreeipdev:
> +	kfree(ipdev);
> +	dev_err(&pdev->dev, "Failed to register heap: %s\n",
> +		ion_heap_name);
> +	return -ENOMEM;
> +}
> +
> +static int ion_physmem_remove(struct platform_device *pdev)
> +{
> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
> +
> +	ion_heap_destroy(ipdev->heap);
> +	claimed_heap_ids &= ~(1 << ipdev->heap_id);
> +	if (ipdev->need_free_coherent)
> +		dma_release_declared_memory(&pdev->dev);
> +	if (ipdev->freepage_ptr)
> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
> +	kfree(ipdev->heap);
> +	if (ipdev->clk)
> +		clk_disable_unprepare(ipdev->clk);
> +	kfree(ipdev);
> +
> +	/* We only remove heaps and disable clocks here.
> +	 * There's no point in nuking the device itself, since:
> +	 * a). ION driver modules can't be unloaded (yet?)
> +	 * b). ion_device_destroy() looks like a stub and doesn't
> +	 * take care to free heaps/clients.
> +	 * c). I can't think of a scenario where it will be required
> +	 */
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ion_physmem_driver = {
> +	.probe		= ion_physmem_probe,
> +	.remove		= ion_physmem_remove,
> +	.driver		= {
> +		.name	= "ion-physmem",
> +		.of_match_table = of_match_ptr(of_match_table),
> +	},
> +};
> +
> +static int __init ion_physmem_init(void)
> +{
> +	return platform_driver_register(&ion_physmem_driver);
> +}
> +device_initcall(ion_physmem_init);
> diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
> new file mode 100644
> index 0000000..d26b742
> --- /dev/null
> +++ b/include/dt-bindings/ion,physmem.h
> @@ -0,0 +1,16 @@
> +/*
> + * This header provides constants for ION physmem driver.
> + *
> + */
> +
> +#ifndef _DT_BINDINGS_ION_PHYSMEM_H
> +#define _DT_BINDINGS_ION_PHYSMEM_H
> +
> +#define        ION_HEAP_TYPE_SYSTEM         0
> +#define        ION_HEAP_TYPE_SYSTEM_CONTIG  1
> +#define	ION_HEAP_TYPE_CARVEOUT       2
> +#define	ION_HEAP_TYPE_CHUNK          3
> +#define	ION_HEAP_TYPE_DMA            4
> +#define	ION_HEAP_TYPE_CUSTOM         5

Odd indentation choice, pick one and stick with it, don't mix in the
same list.

thanks,

greg k-h

Re: [PATCH v4 1/2] staging: ion: Add generic ion-physmem driver

[Andrew]

Greg Kroah-Hartman писал 13.06.2015 03:16:
> On Tue, Jun 09, 2015 at 05:58:24PM +0300, Andrew Andrianov wrote:
>> This patch adds a generic ion driver that allows
>> ion heaps to be added via devicetree. It provides
>> a simple and generic way to feed physical memory regions
>> to ion without writing a custom driver, e.g.
>> 
>> 	ion_sram: ion@0x00100000 {
>> 	     compatible = "ion,physmem";
>> 	     reg = <0x00100000 0x40000>;
>> 	     reg-names = "memory";
>> 	     ion-heap-id   = <1>;
>> 	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> 	     ion-heap-align = <0x10>;
>> 	     ion-heap-name = "SRAM";
>> 	};
>> 
>> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
>> ---
>>  drivers/staging/android/ion/Kconfig       |   7 +
>>  drivers/staging/android/ion/Makefile      |   5 +-
>>  drivers/staging/android/ion/ion_physmem.c | 229 
>> ++++++++++++++++++++++++++++++
>>  include/dt-bindings/ion,physmem.h         |  17 +++
>>  4 files changed, 256 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/staging/android/ion/ion_physmem.c
>>  create mode 100644 include/dt-bindings/ion,physmem.h
>> 
>> diff --git a/drivers/staging/android/ion/Kconfig 
>> b/drivers/staging/android/ion/Kconfig
>> index 3452346..c558cf8 100644
>> --- a/drivers/staging/android/ion/Kconfig
>> +++ b/drivers/staging/android/ion/Kconfig
>> @@ -33,3 +33,10 @@ config ION_TEGRA
>>  	help
>>  	  Choose this option if you wish to use ion on an nVidia Tegra.
>> 
>> +config ION_PHYSMEM
>> +	bool "Generic PhysMem ION driver"
>> +	depends on ION
>> +	help
>> +	  Provides a generic ION driver that registers the
>> +	  /dev/ion device with heaps from devicetree entries.
>> +	  This heaps are made of chunks of physical memory
> 
> That last sentance doesn't make sense to me :(

Neither it does to me, will fix ASAP. Must some old relic that went 
unnoticed.

> 
> And have you tested this driver build on a non-DT build (like x86-64)?
> 

Nothing beyond just building it on x86_64_defconfig + CONFIG_ANDROID + 
CONFIG_ION*
and a few tests on private older branches for ARM with no devicetree 
(ARM Realview EB).


>> diff --git a/drivers/staging/android/ion/Makefile 
>> b/drivers/staging/android/ion/Makefile
>> index b56fd2b..ac9856d 100644
>> --- a/drivers/staging/android/ion/Makefile
>> +++ b/drivers/staging/android/ion/Makefile
>> @@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
>>  obj-$(CONFIG_ION) += compat_ion.o
>>  endif
>> 
>> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
>> -obj-$(CONFIG_ION_TEGRA) += tegra/
>> +obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
>> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
>> +obj-$(CONFIG_ION_TEGRA)   += tegra/
>> 
>> diff --git a/drivers/staging/android/ion/ion_physmem.c 
>> b/drivers/staging/android/ion/ion_physmem.c
>> new file mode 100644
>> index 0000000..ba5135f
>> --- /dev/null
>> +++ b/drivers/staging/android/ion/ion_physmem.c
>> @@ -0,0 +1,229 @@
>> +/*
>> + * Copyright (C) 2015 RC Module
>> + * Andrew Andrianov <andrew@ncrmnt.org>
>> + *
>> + * 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.
>> + *
>> + * Generic devicetree physmem driver for ION Memory Manager
>> + *
>> + */
>> +
>> +#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"
>> +
>> +#define DRVNAME "ion-physmem"
>> +
>> +static struct ion_device *idev;
>> +static uint32_t claimed_heap_ids;
>> +
>> +static const struct of_device_id of_match_table[] = {
>> +	{ .compatible = "ion,physmem", },
>> +	{ /* end of list */ }
>> +};
>> +
>> +struct physmem_ion_dev {
>> +	struct ion_platform_heap  data;
>> +	struct ion_heap          *heap;
>> +	int                       need_free_coherent;
>> +	void                     *freepage_ptr;
>> +	struct clk               *clk;
>> +	uint32_t                  heap_id;
>> +};
>> +
>> +static int ion_physmem_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	u32 ion_heap_id, ion_heap_align, ion_heap_type;
>> +	ion_phys_addr_t addr;
>> +	size_t size = 0;
>> +	const char *ion_heap_name = NULL;
>> +	struct resource *res;
>> +	struct physmem_ion_dev *ipdev;
>> +
>> +	/*
>> +	   Looks like we can only have one ION device in our system.
>> +	   Therefore we call ion_device_create on first probe and only
>> +	   add heaps to it on subsequent probe calls.
>> +	   FixMe:
>> +	   1. Do we need to hold a spinlock here?
>> +	   2. Can several probes race here on SMP?
>> +	*/
>> +
>> +	if (!idev) {
>> +		idev = ion_device_create(NULL);
>> +		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
>> +		if (!idev)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
>> +	if (!ipdev)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, ipdev);
>> +
>> +	/* Read out name first for the sake of sane error-reporting */
>> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
>> +				       &ion_heap_name);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
>> +				    &ion_heap_id);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	/* Check id to be sane first */
>> +	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
>> +		dev_err(&pdev->dev, "bad heap id specified: %d\n",
>> +			ion_heap_id);
>> +		goto errfreeipdev;
>> +	}
>> +
>> +	if ((1 << ion_heap_id) & claimed_heap_ids) {
>> +		dev_err(&pdev->dev, "heap id %d is already claimed\n",
>> +			ion_heap_id);
>> +		goto errfreeipdev;
>> +	}
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
>> +				    &ion_heap_type);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
>> +				    &ion_heap_align);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
>> +	/* Not always needed, throw no error if missing */
>> +	if (res) {
>> +		/* Fill in some defaults */
>> +		addr = res->start;
>> +		size = resource_size(res);
>> +	}
>> +
>> +	switch (ion_heap_type) {
>> +	case ION_HEAP_TYPE_DMA:
>> +		if (res) {
>> +			ret = dma_declare_coherent_memory(&pdev->dev,
>> +							  res->start,
>> +							  res->start,
>> +							  resource_size(res),
>> +							  DMA_MEMORY_MAP |
>> +							  DMA_MEMORY_EXCLUSIVE);
>> +			if (ret == 0) {
>> +				ret = -ENODEV;
>> +				goto errfreeipdev;
>> +			}
>> +		}
>> +		/*
>> +		 *  If no memory region declared in dt we assume that
>> +		 *  the user is okay with plain dma_alloc_coherent.
>> +		 */
>> +		break;
>> +	case ION_HEAP_TYPE_CARVEOUT:
>> +	case ION_HEAP_TYPE_CHUNK:
>> +		if (size == 0) {
>> +			ret = -EIO;
>> +			goto errfreeipdev;
>> +		}
>> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
>> +		if (ipdev->freepage_ptr) {
>> +			addr = virt_to_phys(ipdev->freepage_ptr);
>> +		} else {
>> +			ret = -ENOMEM;
>> +			goto errfreeipdev;
>> +		}
>> +		break;
>> +	}
>> +
>> +	ipdev->data.id    = ion_heap_id;
>> +	ipdev->data.type  = ion_heap_type;
>> +	ipdev->data.name  = ion_heap_name;
>> +	ipdev->data.align = ion_heap_align;
>> +	ipdev->data.base  = addr;
>> +	ipdev->data.size  = size;
>> +
>> +	/* This one make dma_declare_coherent_memory actually work */
>> +	ipdev->data.priv  = &pdev->dev;
>> +
>> +	ipdev->heap = ion_heap_create(&ipdev->data);
>> +	if (!ipdev->heap)
>> +		goto errfreeipdev;
>> +
>> +	/* If it's needed - take care enable clocks */
>> +	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(ipdev->clk))
>> +		ipdev->clk = NULL;
>> +	else
>> +		clk_prepare_enable(ipdev->clk);
>> +
>> +	ion_device_add_heap(idev, ipdev->heap);
>> +	claimed_heap_ids |= (1 << ion_heap_id);
>> +	ipdev->heap_id = ion_heap_id;
>> +
>> +	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx 
>> len %lu KiB\n",
>> +		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
>> +		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
>> +	return 0;
>> +
>> +errfreeipdev:
>> +	kfree(ipdev);
>> +	dev_err(&pdev->dev, "Failed to register heap: %s\n",
>> +		ion_heap_name);
>> +	return -ENOMEM;
>> +}
>> +
>> +static int ion_physmem_remove(struct platform_device *pdev)
>> +{
>> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
>> +
>> +	ion_heap_destroy(ipdev->heap);
>> +	claimed_heap_ids &= ~(1 << ipdev->heap_id);
>> +	if (ipdev->need_free_coherent)
>> +		dma_release_declared_memory(&pdev->dev);
>> +	if (ipdev->freepage_ptr)
>> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
>> +	kfree(ipdev->heap);
>> +	if (ipdev->clk)
>> +		clk_disable_unprepare(ipdev->clk);
>> +	kfree(ipdev);
>> +
>> +	/* We only remove heaps and disable clocks here.
>> +	 * There's no point in nuking the device itself, since:
>> +	 * a). ION driver modules can't be unloaded (yet?)
>> +	 * b). ion_device_destroy() looks like a stub and doesn't
>> +	 * take care to free heaps/clients.
>> +	 * c). I can't think of a scenario where it will be required
>> +	 */
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver ion_physmem_driver = {
>> +	.probe		= ion_physmem_probe,
>> +	.remove		= ion_physmem_remove,
>> +	.driver		= {
>> +		.name	= "ion-physmem",
>> +		.of_match_table = of_match_ptr(of_match_table),
>> +	},
>> +};
>> +
>> +static int __init ion_physmem_init(void)
>> +{
>> +	return platform_driver_register(&ion_physmem_driver);
>> +}
>> +device_initcall(ion_physmem_init);
>> diff --git a/include/dt-bindings/ion,physmem.h 
>> b/include/dt-bindings/ion,physmem.h
>> new file mode 100644
>> index 0000000..d26b742
>> --- /dev/null
>> +++ b/include/dt-bindings/ion,physmem.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * This header provides constants for ION physmem driver.
>> + *
>> + */
>> +
>> +#ifndef _DT_BINDINGS_ION_PHYSMEM_H
>> +#define _DT_BINDINGS_ION_PHYSMEM_H
>> +
>> +#define        ION_HEAP_TYPE_SYSTEM         0
>> +#define        ION_HEAP_TYPE_SYSTEM_CONTIG  1
>> +#define	ION_HEAP_TYPE_CARVEOUT       2
>> +#define	ION_HEAP_TYPE_CHUNK          3
>> +#define	ION_HEAP_TYPE_DMA            4
>> +#define	ION_HEAP_TYPE_CUSTOM         5
> 
> Odd indentation choice, pick one and stick with it, don't mix in the
> same list.

Got it, will fix and resend as soon as I get near the box with the 
actual code.

> 
> thanks,
> 
> greg k-h


-- 
Regards,
Andrew

[PATCH v5 0/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

Sorry for the delay, here goes yet another iteration of the patchset, 
fixed everything that Greg pointed out. 

Regards, 
Andrew


Andrew Andrianov (2):
  staging: ion: Add generic ion-physmem driver
  staging: ion: Add ion-physmem documentation

 Documentation/devicetree/bindings/ion,physmem.txt |  98 +++++++++
 drivers/staging/android/ion/Kconfig               |   7 +
 drivers/staging/android/ion/Makefile              |   5 +-
 drivers/staging/android/ion/ion_physmem.c         | 229 ++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h                 |  16 ++
 5 files changed, 353 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 1/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

This patch adds a generic ion driver that allows
ion heaps to be added via devicetree. It provides
a simple and generic way to feed physical memory regions
to ion without writing a custom driver, e.g.

	ion_sram: ion@0x00100000 {
	     compatible = "ion,physmem";
	     reg = <0x00100000 0x40000>;
	     reg-names = "memory";
	     ion-heap-id   = <1>;
	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
	     ion-heap-align = <0x10>;
	     ion-heap-name = "SRAM";
	};

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 drivers/staging/android/ion/Kconfig       |   7 +
 drivers/staging/android/ion/Makefile      |   5 +-
 drivers/staging/android/ion/ion_physmem.c | 229 ++++++++++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h         |  16 +++
 4 files changed, 255 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..102c924 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,10 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_PHYSMEM
+	bool "Generic PhysMem ION driver"
+	depends on ION
+	help
+	  Provides a generic ION driver that allows defining ION heaps
+	  via devicetree entries.
+
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..ac9856d 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
 obj-$(CONFIG_ION) += compat_ion.o
 endif
 
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
 
diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
new file mode 100644
index 0000000..ba5135f
--- /dev/null
+++ b/drivers/staging/android/ion/ion_physmem.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ *
+ * 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.
+ *
+ * Generic devicetree physmem driver for ION Memory Manager
+ *
+ */
+
+#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"
+
+#define DRVNAME "ion-physmem"
+
+static struct ion_device *idev;
+static uint32_t claimed_heap_ids;
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "ion,physmem", },
+	{ /* end of list */ }
+};
+
+struct physmem_ion_dev {
+	struct ion_platform_heap  data;
+	struct ion_heap          *heap;
+	int                       need_free_coherent;
+	void                     *freepage_ptr;
+	struct clk               *clk;
+	uint32_t                  heap_id;
+};
+
+static int ion_physmem_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
+	ion_phys_addr_t addr;
+	size_t size = 0;
+	const char *ion_heap_name = NULL;
+	struct resource *res;
+	struct physmem_ion_dev *ipdev;
+
+	/*
+	   Looks like we can only have one ION device in our system.
+	   Therefore we call ion_device_create on first probe and only
+	   add heaps to it on subsequent probe calls.
+	   FixMe:
+	   1. Do we need to hold a spinlock here?
+	   2. Can several probes race here on SMP?
+	*/
+
+	if (!idev) {
+		idev = ion_device_create(NULL);
+		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
+		if (!idev)
+			return -ENOMEM;
+	}
+
+	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	/* Read out name first for the sake of sane error-reporting */
+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+				       &ion_heap_name);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+				    &ion_heap_id);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	/* Check id to be sane first */
+	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
+		dev_err(&pdev->dev, "bad heap id specified: %d\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	if ((1 << ion_heap_id) & claimed_heap_ids) {
+		dev_err(&pdev->dev, "heap id %d is already claimed\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+				    &ion_heap_type);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+				    &ion_heap_align);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	/* Not always needed, throw no error if missing */
+	if (res) {
+		/* Fill in some defaults */
+		addr = res->start;
+		size = resource_size(res);
+	}
+
+	switch (ion_heap_type) {
+	case ION_HEAP_TYPE_DMA:
+		if (res) {
+			ret = dma_declare_coherent_memory(&pdev->dev,
+							  res->start,
+							  res->start,
+							  resource_size(res),
+							  DMA_MEMORY_MAP |
+							  DMA_MEMORY_EXCLUSIVE);
+			if (ret == 0) {
+				ret = -ENODEV;
+				goto errfreeipdev;
+			}
+		}
+		/*
+		 *  If no memory region declared in dt we assume that
+		 *  the user is okay with plain dma_alloc_coherent.
+		 */
+		break;
+	case ION_HEAP_TYPE_CARVEOUT:
+	case ION_HEAP_TYPE_CHUNK:
+		if (size == 0) {
+			ret = -EIO;
+			goto errfreeipdev;
+		}
+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
+		if (ipdev->freepage_ptr) {
+			addr = virt_to_phys(ipdev->freepage_ptr);
+		} else {
+			ret = -ENOMEM;
+			goto errfreeipdev;
+		}
+		break;
+	}
+
+	ipdev->data.id    = ion_heap_id;
+	ipdev->data.type  = ion_heap_type;
+	ipdev->data.name  = ion_heap_name;
+	ipdev->data.align = ion_heap_align;
+	ipdev->data.base  = addr;
+	ipdev->data.size  = size;
+
+	/* This one make dma_declare_coherent_memory actually work */
+	ipdev->data.priv  = &pdev->dev;
+
+	ipdev->heap = ion_heap_create(&ipdev->data);
+	if (!ipdev->heap)
+		goto errfreeipdev;
+
+	/* If it's needed - take care enable clocks */
+	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ipdev->clk))
+		ipdev->clk = NULL;
+	else
+		clk_prepare_enable(ipdev->clk);
+
+	ion_device_add_heap(idev, ipdev->heap);
+	claimed_heap_ids |= (1 << ion_heap_id);
+	ipdev->heap_id = ion_heap_id;
+
+	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
+		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
+		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
+	return 0;
+
+errfreeipdev:
+	kfree(ipdev);
+	dev_err(&pdev->dev, "Failed to register heap: %s\n",
+		ion_heap_name);
+	return -ENOMEM;
+}
+
+static int ion_physmem_remove(struct platform_device *pdev)
+{
+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
+
+	ion_heap_destroy(ipdev->heap);
+	claimed_heap_ids &= ~(1 << ipdev->heap_id);
+	if (ipdev->need_free_coherent)
+		dma_release_declared_memory(&pdev->dev);
+	if (ipdev->freepage_ptr)
+		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
+	kfree(ipdev->heap);
+	if (ipdev->clk)
+		clk_disable_unprepare(ipdev->clk);
+	kfree(ipdev);
+
+	/* We only remove heaps and disable clocks here.
+	 * There's no point in nuking the device itself, since:
+	 * a). ION driver modules can't be unloaded (yet?)
+	 * b). ion_device_destroy() looks like a stub and doesn't
+	 * take care to free heaps/clients.
+	 * c). I can't think of a scenario where it will be required
+	 */
+
+	return 0;
+}
+
+static struct platform_driver ion_physmem_driver = {
+	.probe		= ion_physmem_probe,
+	.remove		= ion_physmem_remove,
+	.driver		= {
+		.name	= "ion-physmem",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+
+static int __init ion_physmem_init(void)
+{
+	return platform_driver_register(&ion_physmem_driver);
+}
+device_initcall(ion_physmem_init);
diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
new file mode 100644
index 0000000..6b24362
--- /dev/null
+++ b/include/dt-bindings/ion,physmem.h
@@ -0,0 +1,16 @@
+/*
+ * This header provides constants for ION physmem driver.
+ *
+ */
+
+#ifndef __DT_BINDINGS_ION_PHYSMEM_H
+#define __DT_BINDINGS_ION_PHYSMEM_H
+
+#define ION_HEAP_TYPE_SYSTEM		0
+#define ION_HEAP_TYPE_SYSTEM_CONTIG	1
+#define	ION_HEAP_TYPE_CARVEOUT		2
+#define	ION_HEAP_TYPE_CHUNK		3
+#define	ION_HEAP_TYPE_DMA		4
+#define	ION_HEAP_TYPE_CUSTOM		5
+
+#endif /* __DT_BINDINGS_ION_PHYSMEM_H */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 2/2] staging: ion: Add ion-physmem documentation

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 Documentation/devicetree/bindings/ion,physmem.txt | 98 +++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
new file mode 100644
index 0000000..e8c64dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,98 @@
+ION PhysMem Driver
+#include <dt-bindings/ion,physmem.h>
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows you to
+define ION Memory Manager heaps using device tree. This is mostly useful if
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks,
+etc) that are present in the physical memory map and you want to add them to
+ION as heaps of memory.
+
+
+Examples:
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
+   address range
+
+	ion_im0: ion@0x00100000 {
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "IM0";
+	};
+
+2. The same, but using system DMA memory.
+
+	ion_dma: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "SYSDMA";
+	};
+
+3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+		ion_crv: ion@deadbeef {
+		     compatible = "ion,physmem";
+		     reg = <0x00000000 0x100000>;
+		     reg-names = "memory";
+		     ion-heap-id   = <3>;
+		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
+		     ion-heap-align = <0x10>;
+		     ion-heap-name = "carveout";
+		};
+
+4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "chunky";
+	};
+
+
+5. vmalloc();
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "sys";
+	};
+
+6. kmalloc();
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "syscont";
+	};
+
+If the underlying heap relies on some physical device that needs clock
+gating, you may need to fill the clocks field in. E.g.:
+
+
+	ion_im0: ion@0x00100000 {
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "IM0";
+	     clocks = <&oscillator_27m>;
+	     clock-names = "clk_27m";
+	};
+
+ion-physmem will do everything required to enable and disable the clock.
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5.1 0/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

Just a friendly ping. Since I got no feedback for a while I just rebased
the driver against the newly released 4.1 and checked that it builds, loads
and works. 

Andrew Andrianov (2):
  staging: ion: Add generic ion-physmem driver
  staging: ion: Add ion-physmem documentation

 Documentation/devicetree/bindings/ion,physmem.txt |  98 +++++++++
 drivers/staging/android/ion/Kconfig               |   7 +
 drivers/staging/android/ion/Makefile              |   5 +-
 drivers/staging/android/ion/ion_physmem.c         | 229 ++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h                 |  16 ++
 5 files changed, 353 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

-- 
2.1.4

[PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver

[Andrew Andrianov]

This patch adds a generic ion driver that allows
ion heaps to be added via devicetree. It provides
a simple and generic way to feed physical memory regions
to ion without writing a custom driver, e.g.

	ion_sram: ion@0x00100000 {
	     compatible = "ion,physmem";
	     reg = <0x00100000 0x40000>;
	     reg-names = "memory";
	     ion-heap-id   = <1>;
	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
	     ion-heap-align = <0x10>;
	     ion-heap-name = "SRAM";
	};

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 drivers/staging/android/ion/Kconfig       |   7 +
 drivers/staging/android/ion/Makefile      |   5 +-
 drivers/staging/android/ion/ion_physmem.c | 229 ++++++++++++++++++++++++++++++
 include/dt-bindings/ion,physmem.h         |  16 +++
 4 files changed, 255 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion_physmem.c
 create mode 100644 include/dt-bindings/ion,physmem.h

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..102c924 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,10 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_PHYSMEM
+	bool "Generic PhysMem ION driver"
+	depends on ION
+	help
+	  Provides a generic ION driver that allows defining ION heaps
+	  via devicetree entries.
+
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..ac9856d 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
 obj-$(CONFIG_ION) += compat_ion.o
 endif
 
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
 
diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
new file mode 100644
index 0000000..ba5135f
--- /dev/null
+++ b/drivers/staging/android/ion/ion_physmem.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ *
+ * 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.
+ *
+ * Generic devicetree physmem driver for ION Memory Manager
+ *
+ */
+
+#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"
+
+#define DRVNAME "ion-physmem"
+
+static struct ion_device *idev;
+static uint32_t claimed_heap_ids;
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "ion,physmem", },
+	{ /* end of list */ }
+};
+
+struct physmem_ion_dev {
+	struct ion_platform_heap  data;
+	struct ion_heap          *heap;
+	int                       need_free_coherent;
+	void                     *freepage_ptr;
+	struct clk               *clk;
+	uint32_t                  heap_id;
+};
+
+static int ion_physmem_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
+	ion_phys_addr_t addr;
+	size_t size = 0;
+	const char *ion_heap_name = NULL;
+	struct resource *res;
+	struct physmem_ion_dev *ipdev;
+
+	/*
+	   Looks like we can only have one ION device in our system.
+	   Therefore we call ion_device_create on first probe and only
+	   add heaps to it on subsequent probe calls.
+	   FixMe:
+	   1. Do we need to hold a spinlock here?
+	   2. Can several probes race here on SMP?
+	*/
+
+	if (!idev) {
+		idev = ion_device_create(NULL);
+		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
+		if (!idev)
+			return -ENOMEM;
+	}
+
+	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	/* Read out name first for the sake of sane error-reporting */
+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+				       &ion_heap_name);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+				    &ion_heap_id);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	/* Check id to be sane first */
+	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
+		dev_err(&pdev->dev, "bad heap id specified: %d\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	if ((1 << ion_heap_id) & claimed_heap_ids) {
+		dev_err(&pdev->dev, "heap id %d is already claimed\n",
+			ion_heap_id);
+		goto errfreeipdev;
+	}
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+				    &ion_heap_type);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+				    &ion_heap_align);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	/* Not always needed, throw no error if missing */
+	if (res) {
+		/* Fill in some defaults */
+		addr = res->start;
+		size = resource_size(res);
+	}
+
+	switch (ion_heap_type) {
+	case ION_HEAP_TYPE_DMA:
+		if (res) {
+			ret = dma_declare_coherent_memory(&pdev->dev,
+							  res->start,
+							  res->start,
+							  resource_size(res),
+							  DMA_MEMORY_MAP |
+							  DMA_MEMORY_EXCLUSIVE);
+			if (ret == 0) {
+				ret = -ENODEV;
+				goto errfreeipdev;
+			}
+		}
+		/*
+		 *  If no memory region declared in dt we assume that
+		 *  the user is okay with plain dma_alloc_coherent.
+		 */
+		break;
+	case ION_HEAP_TYPE_CARVEOUT:
+	case ION_HEAP_TYPE_CHUNK:
+		if (size == 0) {
+			ret = -EIO;
+			goto errfreeipdev;
+		}
+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
+		if (ipdev->freepage_ptr) {
+			addr = virt_to_phys(ipdev->freepage_ptr);
+		} else {
+			ret = -ENOMEM;
+			goto errfreeipdev;
+		}
+		break;
+	}
+
+	ipdev->data.id    = ion_heap_id;
+	ipdev->data.type  = ion_heap_type;
+	ipdev->data.name  = ion_heap_name;
+	ipdev->data.align = ion_heap_align;
+	ipdev->data.base  = addr;
+	ipdev->data.size  = size;
+
+	/* This one make dma_declare_coherent_memory actually work */
+	ipdev->data.priv  = &pdev->dev;
+
+	ipdev->heap = ion_heap_create(&ipdev->data);
+	if (!ipdev->heap)
+		goto errfreeipdev;
+
+	/* If it's needed - take care enable clocks */
+	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ipdev->clk))
+		ipdev->clk = NULL;
+	else
+		clk_prepare_enable(ipdev->clk);
+
+	ion_device_add_heap(idev, ipdev->heap);
+	claimed_heap_ids |= (1 << ion_heap_id);
+	ipdev->heap_id = ion_heap_id;
+
+	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
+		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
+		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
+	return 0;
+
+errfreeipdev:
+	kfree(ipdev);
+	dev_err(&pdev->dev, "Failed to register heap: %s\n",
+		ion_heap_name);
+	return -ENOMEM;
+}
+
+static int ion_physmem_remove(struct platform_device *pdev)
+{
+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
+
+	ion_heap_destroy(ipdev->heap);
+	claimed_heap_ids &= ~(1 << ipdev->heap_id);
+	if (ipdev->need_free_coherent)
+		dma_release_declared_memory(&pdev->dev);
+	if (ipdev->freepage_ptr)
+		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
+	kfree(ipdev->heap);
+	if (ipdev->clk)
+		clk_disable_unprepare(ipdev->clk);
+	kfree(ipdev);
+
+	/* We only remove heaps and disable clocks here.
+	 * There's no point in nuking the device itself, since:
+	 * a). ION driver modules can't be unloaded (yet?)
+	 * b). ion_device_destroy() looks like a stub and doesn't
+	 * take care to free heaps/clients.
+	 * c). I can't think of a scenario where it will be required
+	 */
+
+	return 0;
+}
+
+static struct platform_driver ion_physmem_driver = {
+	.probe		= ion_physmem_probe,
+	.remove		= ion_physmem_remove,
+	.driver		= {
+		.name	= "ion-physmem",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+
+static int __init ion_physmem_init(void)
+{
+	return platform_driver_register(&ion_physmem_driver);
+}
+device_initcall(ion_physmem_init);
diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
new file mode 100644
index 0000000..6b24362
--- /dev/null
+++ b/include/dt-bindings/ion,physmem.h
@@ -0,0 +1,16 @@
+/*
+ * This header provides constants for ION physmem driver.
+ *
+ */
+
+#ifndef __DT_BINDINGS_ION_PHYSMEM_H
+#define __DT_BINDINGS_ION_PHYSMEM_H
+
+#define ION_HEAP_TYPE_SYSTEM		0
+#define ION_HEAP_TYPE_SYSTEM_CONTIG	1
+#define	ION_HEAP_TYPE_CARVEOUT		2
+#define	ION_HEAP_TYPE_CHUNK		3
+#define	ION_HEAP_TYPE_DMA		4
+#define	ION_HEAP_TYPE_CUSTOM		5
+
+#endif /* __DT_BINDINGS_ION_PHYSMEM_H */
-- 
2.1.4

[PATCH v5.1 2/2] staging: ion: Add ion-physmem documentation

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 Documentation/devicetree/bindings/ion,physmem.txt | 98 +++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
new file mode 100644
index 0000000..e8c64dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,98 @@
+ION PhysMem Driver
+#include <dt-bindings/ion,physmem.h>
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows you to
+define ION Memory Manager heaps using device tree. This is mostly useful if
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks,
+etc) that are present in the physical memory map and you want to add them to
+ION as heaps of memory.
+
+
+Examples:
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
+   address range
+
+	ion_im0: ion@0x00100000 {
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "IM0";
+	};
+
+2. The same, but using system DMA memory.
+
+	ion_dma: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "SYSDMA";
+	};
+
+3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+		ion_crv: ion@deadbeef {
+		     compatible = "ion,physmem";
+		     reg = <0x00000000 0x100000>;
+		     reg-names = "memory";
+		     ion-heap-id   = <3>;
+		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
+		     ion-heap-align = <0x10>;
+		     ion-heap-name = "carveout";
+		};
+
+4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "chunky";
+	};
+
+
+5. vmalloc();
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "sys";
+	};
+
+6. kmalloc();
+
+	ion_chunk: ion@0xdeadbeef {
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "syscont";
+	};
+
+If the underlying heap relies on some physical device that needs clock
+gating, you may need to fill the clocks field in. E.g.:
+
+
+	ion_im0: ion@0x00100000 {
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;
+	     ion-heap-name = "IM0";
+	     clocks = <&oscillator_27m>;
+	     clock-names = "clk_27m";
+	};
+
+ion-physmem will do everything required to enable and disable the clock.
-- 
2.1.4

Re: [PATCH v5.1 2/2] staging: ion: Add ion-physmem documentation

[Laura Abbott]

(adding devicetree mailing list since I didn't see it cc-ed)

Please also remember to cc the staging list since Ion is
still a staging framework.

On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
> ---
>   Documentation/devicetree/bindings/ion,physmem.txt | 98 +++++++++++++++++++++++

The proper place is bindings/staging/ since Ion is still a staging driver

>   1 file changed, 98 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
>
> diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
> new file mode 100644
> index 0000000..e8c64dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ion,physmem.txt
> @@ -0,0 +1,98 @@
> +ION PhysMem Driver
> +#include <dt-bindings/ion,physmem.h>
> +
> +
> +ION PhysMem is a generic driver for ION Memory Manager that allows you to
> +define ION Memory Manager heaps using device tree. This is mostly useful if
> +your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks,
> +etc) that are present in the physical memory map and you want to add them to
> +ION as heaps of memory.
> +

A good start of a description. This could use a bit more detail about what the
Ion memory framework actually does (i.e. tied really strongly to Android)

You are also missing a generic description of what all goes into the binding.
Based on what you have below you would get

(name) : ion@(base-address) {
	compatible = "ion,physmem";
	reg = <(baseaddr) (size)>;
	reg-names = "memory";
	ion-heap-id = <(int-value)>;
	ion-heap-type = <(int-value)>;
	ion-heap-align = <(int-value)>;
	ion-heap-name = "(string value");
};

and then you need to describe what each of those properties actually do.
Having all the examples is still really useful, especially for heaps such as the
system heaps which are independent of any memory map.

> +
> +Examples:
> +
> +1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
> +   address range
> +
> +	ion_im0: ion@0x00100000 {
> +	     compatible = "ion,physmem";
> +	     reg = <0x00100000 0x40000>;
> +	     reg-names = "memory";
> +	     ion-heap-id   = <2>;
> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
> +	     ion-heap-align = <0x10>;
> +	     ion-heap-name = "IM0";
> +	};
> +
> +2. The same, but using system DMA memory.
> +
> +	ion_dma: ion@0xdeadbeef {
> +	     compatible = "ion,physmem";
> +	     ion-heap-id   = <2>;
> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
> +	     ion-heap-align = <0x10>;
> +	     ion-heap-name = "SYSDMA";
> +	};

CMA now has bindings upstream. I'd rather see Ion be a CMA client instead
of creating any other bindings.

> +
> +3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using
> +   alloc_pages_exact(). reg range is used for specifying size only.
> +
> +		ion_crv: ion@deadbeef {
> +		     compatible = "ion,physmem";
> +		     reg = <0x00000000 0x100000>;
> +		     reg-names = "memory";
> +		     ion-heap-id   = <3>;
> +		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
> +		     ion-heap-align = <0x10>;
> +		     ion-heap-name = "carveout";
> +		};
> +

My understanding of DT binding rules was that for foo@X, your reg must
equal X. Maintainers can correct me if I'm wrong or if that restriction
is relaxed these days.

> +4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
> +   alloc_pages_exact(). reg range is used for specifying size only.
> +
> +	ion_chunk: ion@0xdeadbeef {
> +	     compatible = "ion,physmem";
> +	     ion-heap-id   = <2>;
> +	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
> +	     ion-heap-align = <0x10>;
> +	     ion-heap-name = "chunky";
> +	};
> +
> +
> +5. vmalloc();
> +
> +	ion_chunk: ion@0xdeadbeef {
> +	     compatible = "ion,physmem";
> +	     ion-heap-id   = <2>;
> +	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
> +	     ion-heap-align = <0x10>;
> +	     ion-heap-name = "sys";
> +	};
> +
> +6. kmalloc();
> +
> +	ion_chunk: ion@0xdeadbeef {
> +	     compatible = "ion,physmem";
> +	     ion-heap-id   = <2>;
> +	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
> +	     ion-heap-align = <0x10>;
> +	     ion-heap-name = "syscont";
> +	};
> +
> +If the underlying heap relies on some physical device that needs clock
> +gating, you may need to fill the clocks field in. E.g.:
> +
> +
> +	ion_im0: ion@0x00100000 {
> +	     compatible = "ion,physmem";
> +	     reg = <0x00100000 0x40000>;
> +	     reg-names = "memory";
> +	     ion-heap-id   = <2>;
> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
> +	     ion-heap-align = <0x10>;
> +	     ion-heap-name = "IM0";
> +	     clocks = <&oscillator_27m>;
> +	     clock-names = "clk_27m";
> +	};
> +
> +ion-physmem will do everything required to enable and disable the clock.
>

I'm glad to see an attempt to get bindings submitted for Ion. There
exists other bindings out of tree already[1]. My one concern here is that
Ion is so 'experimental/staging' that trying to
shoot for an ABI is going to be difficult because of how far this has to
go. On the other hand, it's been out there long enough and it's in use
so establishing something for what there is at the present would be
beneficial. I also don't know the rules on DT bindings for staging drivers
so I'll let the maintainers comment.

Thanks,
Laura


[1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/Documentation/devicetree/bindings/arm/msm/msm_ion.txt?h=msm-3.10

Re: [PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver

[Laura Abbott]

On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
> This patch adds a generic ion driver that allows
> ion heaps to be added via devicetree. It provides
> a simple and generic way to feed physical memory regions
> to ion without writing a custom driver, e.g.
>
> 	ion_sram: ion@0x00100000 {
> 	     compatible = "ion,physmem";
> 	     reg = <0x00100000 0x40000>;
> 	     reg-names = "memory";
> 	     ion-heap-id   = <1>;
> 	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
> 	     ion-heap-align = <0x10>;
> 	     ion-heap-name = "SRAM";
> 	};
>
> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
> ---
>   drivers/staging/android/ion/Kconfig       |   7 +
>   drivers/staging/android/ion/Makefile      |   5 +-
>   drivers/staging/android/ion/ion_physmem.c | 229 ++++++++++++++++++++++++++++++
>   include/dt-bindings/ion,physmem.h         |  16 +++
>   4 files changed, 255 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/staging/android/ion/ion_physmem.c
>   create mode 100644 include/dt-bindings/ion,physmem.h
>
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index 3452346..102c924 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -33,3 +33,10 @@ config ION_TEGRA
>   	help
>   	  Choose this option if you wish to use ion on an nVidia Tegra.
>
> +config ION_PHYSMEM
> +	bool "Generic PhysMem ION driver"
> +	depends on ION
> +	help
> +	  Provides a generic ION driver that allows defining ION heaps
> +	  via devicetree entries.
> +
> diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
> index b56fd2b..ac9856d 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
>   obj-$(CONFIG_ION) += compat_ion.o
>   endif
>
> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
> -obj-$(CONFIG_ION_TEGRA) += tegra/
> +obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
> +obj-$(CONFIG_ION_TEGRA)   += tegra/
>
> diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
> new file mode 100644
> index 0000000..ba5135f
> --- /dev/null
> +++ b/drivers/staging/android/ion/ion_physmem.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2015 RC Module
> + * Andrew Andrianov <andrew@ncrmnt.org>
> + *
> + * 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.
> + *
> + * Generic devicetree physmem driver for ION Memory Manager
> + *
> + */
> +
> +#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"
> +
> +#define DRVNAME "ion-physmem"
> +
> +static struct ion_device *idev;
> +static uint32_t claimed_heap_ids;
> +
> +static const struct of_device_id of_match_table[] = {
> +	{ .compatible = "ion,physmem", },
> +	{ /* end of list */ }
> +};
> +
> +struct physmem_ion_dev {
> +	struct ion_platform_heap  data;
> +	struct ion_heap          *heap;
> +	int                       need_free_coherent;
> +	void                     *freepage_ptr;
> +	struct clk               *clk;
> +	uint32_t                  heap_id;
> +};
> +
> +static int ion_physmem_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	u32 ion_heap_id, ion_heap_align, ion_heap_type;
> +	ion_phys_addr_t addr;
> +	size_t size = 0;
> +	const char *ion_heap_name = NULL;
> +	struct resource *res;
> +	struct physmem_ion_dev *ipdev;
> +
> +	/*
> +	   Looks like we can only have one ION device in our system.
> +	   Therefore we call ion_device_create on first probe and only
> +	   add heaps to it on subsequent probe calls.
> +	   FixMe:
> +	   1. Do we need to hold a spinlock here?
> +	   2. Can several probes race here on SMP?
> +	*/
> +
> +	if (!idev) {
> +		idev = ion_device_create(NULL);
> +		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
> +		if (!idev)
> +			return -ENOMEM;
> +	}

Yeah this is a bit messy as your comments note. Since there can only be one Ion
device in the system, it seems like it would make more sense to have a top level
DT node and then have the heaps be subnodes to avoid this 'guess when to create
the device' bit.

> +
> +	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
> +	if (!ipdev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ipdev);
> +
> +	/* Read out name first for the sake of sane error-reporting */
> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
> +				       &ion_heap_name);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
> +				    &ion_heap_id);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	/* Check id to be sane first */
> +	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
> +		dev_err(&pdev->dev, "bad heap id specified: %d\n",
> +			ion_heap_id);
> +		goto errfreeipdev;
> +	}
> +
> +	if ((1 << ion_heap_id) & claimed_heap_ids) {
> +		dev_err(&pdev->dev, "heap id %d is already claimed\n",
> +			ion_heap_id);
> +		goto errfreeipdev;
> +	}

I thought the core Ion framework was already checking this but
apparently not. This is so fundamental it should really be moved into
the core framework and not at the client level.

> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
> +				    &ion_heap_type);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
> +				    &ion_heap_align);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
> +	/* Not always needed, throw no error if missing */
> +	if (res) {
> +		/* Fill in some defaults */
> +		addr = res->start;
> +		size = resource_size(res);
> +	}
> +
> +	switch (ion_heap_type) {
> +	case ION_HEAP_TYPE_DMA:
> +		if (res) {
> +			ret = dma_declare_coherent_memory(&pdev->dev,
> +							  res->start,
> +							  res->start,
> +							  resource_size(res),
> +							  DMA_MEMORY_MAP |
> +							  DMA_MEMORY_EXCLUSIVE);
> +			if (ret == 0) {
> +				ret = -ENODEV;
> +				goto errfreeipdev;
> +			}
> +		}

The name is ION_HEAP_TYPE_DMA but the real point of the heap was to
be able to use CMA memory. Calling dma_declare_coherent_memory defeats
the point since we won't use CMA memory. The coherent memory pool is closer
to a carveout anyway so I'd argue if you want the effects of a coherent
memory pool you should be using carveout memory anyway.

> +		/*
> +		 *  If no memory region declared in dt we assume that
> +		 *  the user is okay with plain dma_alloc_coherent.
> +		 */
> +		break;
> +	case ION_HEAP_TYPE_CARVEOUT:
> +	case ION_HEAP_TYPE_CHUNK:
> +		if (size == 0) {
> +			ret = -EIO;
> +			goto errfreeipdev;
> +		}
> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
> +		if (ipdev->freepage_ptr) {
> +			addr = virt_to_phys(ipdev->freepage_ptr);
> +		} else {
> +			ret = -ENOMEM;
> +			goto errfreeipdev;
> +		}
> +		break;
> +	}
> +

This won't work if the carveout region is larger than the buddy allocator
allows. That's why ion_reserve used the memblock APIs, to avoid being
limited by buddy allocator sizes.

> +	ipdev->data.id    = ion_heap_id;
> +	ipdev->data.type  = ion_heap_type;
> +	ipdev->data.name  = ion_heap_name;
> +	ipdev->data.align = ion_heap_align;
> +	ipdev->data.base  = addr;
> +	ipdev->data.size  = size;
> +
> +	/* This one make dma_declare_coherent_memory actually work */
> +	ipdev->data.priv  = &pdev->dev;
> +
> +	ipdev->heap = ion_heap_create(&ipdev->data);
> +	if (!ipdev->heap)
> +		goto errfreeipdev;
> +
> +	/* If it's needed - take care enable clocks */
> +	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ipdev->clk))
> +		ipdev->clk = NULL;
> +	else
> +		clk_prepare_enable(ipdev->clk);
> +

Probe deferal for the clocks here?

> +	ion_device_add_heap(idev, ipdev->heap);
> +	claimed_heap_ids |= (1 << ion_heap_id);
> +	ipdev->heap_id = ion_heap_id;
> +
> +	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
> +		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
> +		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
> +	return 0;
> +
> +errfreeipdev:
> +	kfree(ipdev);
> +	dev_err(&pdev->dev, "Failed to register heap: %s\n",
> +		ion_heap_name);
> +	return -ENOMEM;
> +}
> +
> +static int ion_physmem_remove(struct platform_device *pdev)
> +{
> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
> +
> +	ion_heap_destroy(ipdev->heap);
> +	claimed_heap_ids &= ~(1 << ipdev->heap_id);
> +	if (ipdev->need_free_coherent)
> +		dma_release_declared_memory(&pdev->dev);
> +	if (ipdev->freepage_ptr)
> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
> +	kfree(ipdev->heap);
> +	if (ipdev->clk)
> +		clk_disable_unprepare(ipdev->clk);
> +	kfree(ipdev);
> +
> +	/* We only remove heaps and disable clocks here.
> +	 * There's no point in nuking the device itself, since:
> +	 * a). ION driver modules can't be unloaded (yet?)
> +	 * b). ion_device_destroy() looks like a stub and doesn't
> +	 * take care to free heaps/clients.
> +	 * c). I can't think of a scenario where it will be required
> +	 */
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ion_physmem_driver = {
> +	.probe		= ion_physmem_probe,
> +	.remove		= ion_physmem_remove,
> +	.driver		= {
> +		.name	= "ion-physmem",
> +		.of_match_table = of_match_ptr(of_match_table),
> +	},
> +};
> +
> +static int __init ion_physmem_init(void)
> +{
> +	return platform_driver_register(&ion_physmem_driver);
> +}
> +device_initcall(ion_physmem_init);
> diff --git a/include/dt-bindings/ion,physmem.h b/include/dt-bindings/ion,physmem.h
> new file mode 100644
> index 0000000..6b24362
> --- /dev/null
> +++ b/include/dt-bindings/ion,physmem.h
> @@ -0,0 +1,16 @@
> +/*
> + * This header provides constants for ION physmem driver.
> + *
> + */
> +
> +#ifndef __DT_BINDINGS_ION_PHYSMEM_H
> +#define __DT_BINDINGS_ION_PHYSMEM_H
> +
> +#define ION_HEAP_TYPE_SYSTEM		0
> +#define ION_HEAP_TYPE_SYSTEM_CONTIG	1
> +#define	ION_HEAP_TYPE_CARVEOUT		2
> +#define	ION_HEAP_TYPE_CHUNK		3
> +#define	ION_HEAP_TYPE_DMA		4
> +#define	ION_HEAP_TYPE_CUSTOM		5
> +
> +#endif /* __DT_BINDINGS_ION_PHYSMEM_H */
>

These are the same as the heap types in ion.h. If they actually need to be
the same, they should be sharing definitions. If they don't need to be the same,
please changes the name to avoid name space collisions.

Thanks,
Laura

Re: [PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver

[Andrew]

Thanks for the detailed review

Laura Abbott писал 30.06.2015 20:56:
> On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
	if (!idev)
>> +			return -ENOMEM;
>> +	}
> 
> Yeah this is a bit messy as your comments note. Since there can only be 
> one Ion
> device in the system, it seems like it would make more sense to have a 
> top level
> DT node and then have the heaps be subnodes to avoid this 'guess when 
> to create
> the device' bit.
> 

I'm afraid this is not a very good idea, if the heaps represent some 
_physical_
address ranges, e.g. a SRAM memory (read below for our weird use case).
In this case, the way I understand DT philosophy, it should be a subnode 
of the
respective axi/apb/whatever node where it's connected. Correct me if I'm 
wrong.

>> +
>> +	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
>> +	if (!ipdev)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, ipdev);
>> +
>> +	/* Read out name first for the sake of sane error-reporting */
>> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
>> +				       &ion_heap_name);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
>> +				    &ion_heap_id);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	/* Check id to be sane first */
>> +	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
>> +		dev_err(&pdev->dev, "bad heap id specified: %d\n",
>> +			ion_heap_id);
>> +		goto errfreeipdev;
>> +	}
>> +
>> +	if ((1 << ion_heap_id) & claimed_heap_ids) {
>> +		dev_err(&pdev->dev, "heap id %d is already claimed\n",
>> +			ion_heap_id);
>> +		goto errfreeipdev;
>> +	}
> 
> I thought the core Ion framework was already checking this but
> apparently not. This is so fundamental it should really be moved into
> the core framework and not at the client level.

I tried to mess as little as possible (e.g. not mess at all) with the 
guts of
ION, so ended up with an extra check. If you want, I can hack into the 
ion itself,
and send the patch for the next respin.

> 
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
>> +				    &ion_heap_type);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
>> +				    &ion_heap_align);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
>> +	/* Not always needed, throw no error if missing */
>> +	if (res) {
>> +		/* Fill in some defaults */
>> +		addr = res->start;
>> +		size = resource_size(res);
>> +	}
>> +
>> +	switch (ion_heap_type) {
>> +	case ION_HEAP_TYPE_DMA:
>> +		if (res) {
>> +			ret = dma_declare_coherent_memory(&pdev->dev,
>> +							  res->start,
>> +							  res->start,
>> +							  resource_size(res),
>> +							  DMA_MEMORY_MAP |
>> +							  DMA_MEMORY_EXCLUSIVE);
>> +			if (ret == 0) {
>> +				ret = -ENODEV;
>> +				goto errfreeipdev;
>> +			}
>> +		}
> 
> The name is ION_HEAP_TYPE_DMA but the real point of the heap was to
> be able to use CMA memory. Calling dma_declare_coherent_memory defeats
> the point since we won't use CMA memory. The coherent memory pool is 
> closer
> to a carveout anyway so I'd argue if you want the effects of a coherent
> memory pool you should be using carveout memory anyway.

In our weird use case we use ION to share buffers between NeuroMatrix 
DSP
cores, video decoder, video output device and a userspace application 
that
orchestrates the whole process. The system has several dedicated SRAM 
banks,
that should represented as dedicated ION heaps. Those are differently 
wired in
the system (e.g. ARM core can't even execute code from some of them) and 
to
achieve maximum performance certain buffers should be only allocated 
from
certain banks for the thing to work fast.
(Yes, it's just as scary as it sounds)

In other words - we need several coherent pools, and 
dma_declare_coherent
looked like the only existing way to tell ION:
"hey, we want a heap out of this very physical region!"

In reality that's mostly our only use case, all the rest heap types look 
like mostly
useful for testing rather than in production, as a smarter replacement 
of ion-dummy
driver which I used as a reference while writing this one.

> 
>> +		/*
>> +		 *  If no memory region declared in dt we assume that
>> +		 *  the user is okay with plain dma_alloc_coherent.
>> +		 */
>> +		break;
>> +	case ION_HEAP_TYPE_CARVEOUT:
>> +	case ION_HEAP_TYPE_CHUNK:
>> +		if (size == 0) {
>> +			ret = -EIO;
>> +			goto errfreeipdev;
>> +		}
>> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
>> +		if (ipdev->freepage_ptr) {
>> +			addr = virt_to_phys(ipdev->freepage_ptr);
>> +		} else {
>> +			ret = -ENOMEM;
>> +			goto errfreeipdev;
>> +		}
>> +		break;
>> +	}
>> +
> 
> This won't work if the carveout region is larger than the buddy 
> allocator
> allows. That's why ion_reserve used the memblock APIs, to avoid being
> limited by buddy allocator sizes.

I guess it's okay for testing purposes. Unless I'm missing by the end of
the workday a simple way to do it properly.

> 
>> +	ipdev->data.id    = ion_heap_id;
>> +	ipdev->data.type  = ion_heap_type;
>> +	ipdev->data.name  = ion_heap_name;
>> +	ipdev->data.align = ion_heap_align;
>> +	ipdev->data.base  = addr;
>> +	ipdev->data.size  = size;
>> +
>> +	/* This one make dma_declare_coherent_memory actually work */
>> +	ipdev->data.priv  = &pdev->dev;
>> +
>> +	ipdev->heap = ion_heap_create(&ipdev->data);
>> +	if (!ipdev->heap)
>> +		goto errfreeipdev;
>> +
>> +	/* If it's needed - take care enable clocks */
>> +	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(ipdev->clk))
>> +		ipdev->clk = NULL;
>> +	else
>> +		clk_prepare_enable(ipdev->clk);
>> +
> 
> Probe deferal for the clocks here?

Oops, missed that one. Since I couldn't test clock gating (sram clock's 
not gated on my hw),
I just settled for the same way they do it in drivers/misc/sram.c (And 
they don't handle
deferral at all there!)

> 
>> +	ion_device_add_heap(idev, ipdev->heap);
>> +	claimed_heap_ids |= (1 << ion_heap_id);
>> +	ipdev->heap_id = ion_heap_id;
>> +
>> +	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx 
>> len %lu KiB\n",
>> +		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
>> +		(unsigned long int)addr, ((unsigned long int)(size / 1024)));
>> +	return 0;
>> +
>> +errfreeipdev:
>> +	kfree(ipdev);
>> +	dev_err(&pdev->dev, "Failed to register heap: %s\n",
>> +		ion_heap_name);
>> +	return -ENOMEM;
>> +}
>> +
>> +static int ion_physmem_remove(struct platform_device *pdev)
>> +{
>> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
>> +
>> +	ion_heap_destroy(ipdev->heap);
>> +	claimed_heap_ids &= ~(1 << ipdev->heap_id);
>> +	if (ipdev->need_free_coherent)
>> +		dma_release_declared_memory(&pdev->dev);
>> +	if (ipdev->freepage_ptr)
>> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
>> +	kfree(ipdev->heap);
>> +	if (ipdev->clk)
>> +		clk_disable_unprepare(ipdev->clk);
>> +	kfree(ipdev);
>> +
>> +	/* We only remove heaps and disable clocks here.
>> +	 * There's no point in nuking the device itself, since:
>> +	 * a). ION driver modules can't be unloaded (yet?)
>> +	 * b). ion_device_destroy() looks like a stub and doesn't
>> +	 * take care to free heaps/clients.
>> +	 * c). I can't think of a scenario where it will be required
>> +	 */
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver ion_physmem_driver = {
>> +	.probe		= ion_physmem_probe,
>> +	.remove		= ion_physmem_remove,
>> +	.driver		= {
>> +		.name	= "ion-physmem",
>> +		.of_match_table = of_match_ptr(of_match_table),
>> +	},
>> +};
>> +
>> +static int __init ion_physmem_init(void)
>> +{
>> +	return platform_driver_register(&ion_physmem_driver);
>> +}
>> +device_initcall(ion_physmem_init);
>> diff --git a/include/dt-bindings/ion,physmem.h 
>> b/include/dt-bindings/ion,physmem.h
>> new file mode 100644
>> index 0000000..6b24362
>> --- /dev/null
>> +++ b/include/dt-bindings/ion,physmem.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * This header provides constants for ION physmem driver.
>> + *
>> + */
>> +
>> +#ifndef __DT_BINDINGS_ION_PHYSMEM_H
>> +#define __DT_BINDINGS_ION_PHYSMEM_H
>> +
>> +#define ION_HEAP_TYPE_SYSTEM		0
>> +#define ION_HEAP_TYPE_SYSTEM_CONTIG	1
>> +#define	ION_HEAP_TYPE_CARVEOUT		2
>> +#define	ION_HEAP_TYPE_CHUNK		3
>> +#define	ION_HEAP_TYPE_DMA		4
>> +#define	ION_HEAP_TYPE_CUSTOM		5
>> +
>> +#endif /* __DT_BINDINGS_ION_PHYSMEM_H */
>> 
> 
> These are the same as the heap types in ion.h. If they actually need to 
> be
> the same, they should be sharing definitions. If they don't need to be 
> the same,
> please changes the name to avoid name space collisions.

Got it, I'll make ion.h #include <dt-bindings/ion,physmem.h> then.

> 
> Thanks,
> Laura

-- 
Regards,
Andrew
RC Module :: http://module.ru

Re: [PATCH v5.1 2/2] staging: ion: Add ion-physmem documentation

[Andrew]

Laura Abbott писал 30.06.2015 20:54:
> (adding devicetree mailing list since I didn't see it cc-ed)
> 
> Please also remember to cc the staging list since Ion is
> still a staging framework.
> 
> On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
>> Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
>> ---
>>   Documentation/devicetree/bindings/ion,physmem.txt | 98 
>> +++++++++++++++++++++++
> 
> The proper place is bindings/staging/ since Ion is still a staging 
> driver

Got it, will fix and send for the next respin

> 
>>   1 file changed, 98 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/ion,physmem.txt 
>> b/Documentation/devicetree/bindings/ion,physmem.txt
>> new file mode 100644
>> index 0000000..e8c64dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ion,physmem.txt
>> @@ -0,0 +1,98 @@
>> +ION PhysMem Driver
>> +#include <dt-bindings/ion,physmem.h>
>> +
>> +
>> +ION PhysMem is a generic driver for ION Memory Manager that allows 
>> you to
>> +define ION Memory Manager heaps using device tree. This is mostly 
>> useful if
>> +your SoC has several 'special' regions (e.g. SRAM, dedicated memory 
>> banks,
>> +etc) that are present in the physical memory map and you want to add 
>> them to
>> +ION as heaps of memory.
>> +
> 
> A good start of a description. This could use a bit more detail about 
> what the
> Ion memory framework actually does (i.e. tied really strongly to 
> Android)

Ironically we use ION without android. We even started of a liblinuxion
for use in traditional linux userspace (should be up at RC Module's 
github pretty soon)
I'll add a bit more words here, that's not a problem.

> 
> You are also missing a generic description of what all goes into the 
> binding.
> Based on what you have below you would get
> 
> (name) : ion@(base-address) {
> compatible = "ion,physmem";
> reg = <(baseaddr) (size)>;
> reg-names = "memory";
> ion-heap-id = <(int-value)>;
> ion-heap-type = <(int-value)>;
> ion-heap-align = <(int-value)>;
> ion-heap-name = "(string value");
> };
> 
> and then you need to describe what each of those properties actually 
> do.
> Having all the examples is still really useful, especially for heaps 
> such as the
> system heaps which are independent of any memory map.

Got it, will fix.

>> +
>> +Examples:
>> +
>> +1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as 
>> a physical
>> +   address range
>> +
>> +	ion_im0: ion@0x00100000 {
>> +	     compatible = "ion,physmem";
>> +	     reg = <0x00100000 0x40000>;
>> +	     reg-names = "memory";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "IM0";
>> +	};
>> +
>> +2. The same, but using system DMA memory.
>> +
>> +	ion_dma: ion@0xdeadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "SYSDMA";
>> +	};
> 
> CMA now has bindings upstream. I'd rather see Ion be a CMA client 
> instead
> of creating any other bindings.

Unfortunately, this breaks the most useful case for us, when ion uses
several dedicated physical memory areas. Maybe wrap CMA and use it as
another ion-heap-type then?

> 
>> +
>> +3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it 
>> using
>> +   alloc_pages_exact(). reg range is used for specifying size only.
>> +
>> +		ion_crv: ion@deadbeef {
>> +		     compatible = "ion,physmem";
>> +		     reg = <0x00000000 0x100000>;
>> +		     reg-names = "memory";
>> +		     ion-heap-id   = <3>;
>> +		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
>> +		     ion-heap-align = <0x10>;
>> +		     ion-heap-name = "carveout";
>> +		};
>> +
> 
> My understanding of DT binding rules was that for foo@X, your reg must
> equal X. Maintainers can correct me if I'm wrong or if that restriction
> is relaxed these days.

In case reg doesn't represent a physical memory region, but only size 
here
(for convenient resource_size calls) we may end with several ion@0 this 
way.
Is it really required to be so?

> 
>> +4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
>> +   alloc_pages_exact(). reg range is used for specifying size only.
>> +
>> +	ion_chunk: ion@0xdeadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "chunky";
>> +	};
>> +
>> +
>> +5. vmalloc();
>> +
>> +	ion_chunk: ion@0xdeadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "sys";
>> +	};
>> +
>> +6. kmalloc();
>> +
>> +	ion_chunk: ion@0xdeadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "syscont";
>> +	};
>> +
>> +If the underlying heap relies on some physical device that needs 
>> clock
>> +gating, you may need to fill the clocks field in. E.g.:
>> +
>> +
>> +	ion_im0: ion@0x00100000 {
>> +	     compatible = "ion,physmem";
>> +	     reg = <0x00100000 0x40000>;
>> +	     reg-names = "memory";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "IM0";
>> +	     clocks = <&oscillator_27m>;
>> +	     clock-names = "clk_27m";
>> +	};
>> +
>> +ion-physmem will do everything required to enable and disable the 
>> clock.
>> 
> 
> I'm glad to see an attempt to get bindings submitted for Ion. There
> exists other bindings out of tree already[1]. My one concern here is 
> that
> Ion is so 'experimental/staging' that trying to
> shoot for an ABI is going to be difficult because of how far this has 
> to
> go. On the other hand, it's been out there long enough and it's in use
> so establishing something for what there is at the present would be
> beneficial. I also don't know the rules on DT bindings for staging 
> drivers
> so I'll let the maintainers comment.

So far ION looks like the only proper way for our weird use case and I'm 
strictly
against reinventing the wheel and yet another allocator for all our DSP 
needs as long
as ION gets the job done.

> 
> Thanks,
> Laura
> 
> 
> [1]
> https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/Documentation/devicetree/bindings/arm/msm/msm_ion.txt?h=msm-3.10


-- 
Regards,
Andrew
RC Module :: http://module.ru

Re: [PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver

[Dan Carpenter]

I started reviewing this patch but then part way through I relized I
must be missing quite a bit of it...

On Tue, Jun 30, 2015 at 10:56:27AM -0700, Laura Abbott wrote:
> On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
> >+static int ion_physmem_probe(struct platform_device *pdev)
> >+{
> >+	int ret;
> >+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
> >+	ion_phys_addr_t addr;
> >+	size_t size = 0;
> >+	const char *ion_heap_name = NULL;
> >+	struct resource *res;
> >+	struct physmem_ion_dev *ipdev;
> >+
> >+	/*
> >+	   Looks like we can only have one ION device in our system.
> >+	   Therefore we call ion_device_create on first probe and only
> >+	   add heaps to it on subsequent probe calls.
> >+	   FixMe:
> >+	   1. Do we need to hold a spinlock here?
> >+	   2. Can several probes race here on SMP?
> >+	*/

Comment style.

> >+
> >+	if (!idev) {
> >+		idev = ion_device_create(NULL);
> >+		dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
> >+		if (!idev)
> >+			return -ENOMEM;
> >+	}
> 
> Yeah this is a bit messy as your comments note. Since there can only be one Ion
> device in the system, it seems like it would make more sense to have a top level
> DT node and then have the heaps be subnodes to avoid this 'guess when to create
> the device' bit.
> 
> >+
> >+	ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
> >+	if (!ipdev)
> >+		return -ENOMEM;
> >+
> >+	platform_set_drvdata(pdev, ipdev);
> >+
> >+	/* Read out name first for the sake of sane error-reporting */
> >+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
> >+				       &ion_heap_name);

Extra space after =.

> >+	if (ret != 0)
> >+		goto errfreeipdev;

Remove the double negative.  "if (ret) ".

> >+
> >+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
> >+				    &ion_heap_id);
> >+	if (ret != 0)
> >+		goto errfreeipdev;
> >+
> >+	/* Check id to be sane first */
> >+	if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {

Too many parens.  ion_heap_id is unsigned.

	if (ion_heap_id >= ION_NUM_HEAP_IDS) {


> >+		dev_err(&pdev->dev, "bad heap id specified: %d\n",
> >+			ion_heap_id);
> >+		goto errfreeipdev;

Set an error before the return.

> >+	}
> >+
> >+	if ((1 << ion_heap_id) & claimed_heap_ids) {
> >+		dev_err(&pdev->dev, "heap id %d is already claimed\n",
> >+			ion_heap_id);
> >+		goto errfreeipdev;

Missing error code.

> >+	}
> 
> I thought the core Ion framework was already checking this but
> apparently not. This is so fundamental it should really be moved into
> the core framework and not at the client level.
> 
> >+
> >+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
> >+				    &ion_heap_type);

Space.

> >+	if (ret != 0)
> >+		goto errfreeipdev;

Double negative.

> >+
> >+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
> >+				    &ion_heap_align);

Space.

> >+	if (ret != 0)
> >+		goto errfreeipdev;

Double negative.

> >+
> >+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
> >+	/* Not always needed, throw no error if missing */
> >+	if (res) {
> >+		/* Fill in some defaults */
> >+		addr = res->start;
> >+		size = resource_size(res);
> >+	}
> >+
> >+	switch (ion_heap_type) {
> >+	case ION_HEAP_TYPE_DMA:
> >+		if (res) {
> >+			ret = dma_declare_coherent_memory(&pdev->dev,
> >+							  res->start,
> >+							  res->start,
> >+							  resource_size(res),
> >+							  DMA_MEMORY_MAP |
> >+							  DMA_MEMORY_EXCLUSIVE);
> >+			if (ret == 0) {
> >+				ret = -ENODEV;
> >+				goto errfreeipdev;
> >+			}
> >+		}
> 
> The name is ION_HEAP_TYPE_DMA but the real point of the heap was to
> be able to use CMA memory. Calling dma_declare_coherent_memory defeats
> the point since we won't use CMA memory. The coherent memory pool is closer
> to a carveout anyway so I'd argue if you want the effects of a coherent
> memory pool you should be using carveout memory anyway.
> 
> >+		/*
> >+		 *  If no memory region declared in dt we assume that
> >+		 *  the user is okay with plain dma_alloc_coherent.
> >+		 */
> >+		break;
> >+	case ION_HEAP_TYPE_CARVEOUT:
> >+	case ION_HEAP_TYPE_CHUNK:
> >+		if (size == 0) {
> >+			ret = -EIO;
> >+			goto errfreeipdev;
> >+		}
> >+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
> >+		if (ipdev->freepage_ptr) {
> >+			addr = virt_to_phys(ipdev->freepage_ptr);
> >+		} else {
> >+			ret = -ENOMEM;
> >+			goto errfreeipdev;
> >+		}


Could you flip this around so it's error handling instead of success
handling?

		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
		if (!ipdev->freepage_ptr) {
			ret = -ENOMEM;
			goto errfreeipdev;
		}

		addr = virt_to_phys(ipdev->freepage_ptr);
		break;


> >+		break;
> >+	}
> >+
> 
> This won't work if the carveout region is larger than the buddy allocator
> allows. That's why ion_reserve used the memblock APIs, to avoid being
> limited by buddy allocator sizes.
> 
> >+	ipdev->data.id    = ion_heap_id;
> >+	ipdev->data.type  = ion_heap_type;
> >+	ipdev->data.name  = ion_heap_name;
> >+	ipdev->data.align = ion_heap_align;
> >+	ipdev->data.base  = addr;
> >+	ipdev->data.size  = size;
> >+
> >+	/* This one make dma_declare_coherent_memory actually work */
> >+	ipdev->data.priv  = &pdev->dev;
> >+
> >+	ipdev->heap = ion_heap_create(&ipdev->data);
> >+	if (!ipdev->heap)
> >+		goto errfreeipdev;

Set an error code.

> >+
> >+	/* If it's needed - take care enable clocks */
> >+	ipdev->clk = devm_clk_get(&pdev->dev, NULL);
> >+	if (IS_ERR(ipdev->clk))
> >+		ipdev->clk = NULL;
> >+	else
> >+		clk_prepare_enable(ipdev->clk);
> >+
> 
> Probe deferal for the clocks here?
> 
> >+	ion_device_add_heap(idev, ipdev->heap);
> >+	claimed_heap_ids |= (1 << ion_heap_id);
> >+	ipdev->heap_id = ion_heap_id;
> >+
> >+	dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
> >+		ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
> >+		(unsigned long int)addr, ((unsigned long int)(size / 1024)));


To be honest, I don't understand ion_phys_addr_t.  This code works but
I kind of feel that instead of "unsigned long int" we should be casting
to u64 the same as we would for a phys_addr_t.  We should use %zx for
(size / 1024).

> >+	return 0;
> >+
> >+errfreeipdev:
> >+	kfree(ipdev);
> >+	dev_err(&pdev->dev, "Failed to register heap: %s\n",
> >+		ion_heap_name);
> >+	return -ENOMEM;

We set "ret" on most paths.  I sort of assumed we were going to return
it.  :P  Ignore what I said earlier about missing return codes, I
suppose.

> >+}
> >+
> >+static int ion_physmem_remove(struct platform_device *pdev)
> >+{
> >+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
> >+
> >+	ion_heap_destroy(ipdev->heap);
> >+	claimed_heap_ids &= ~(1 << ipdev->heap_id);
> >+	if (ipdev->need_free_coherent)

Am I missing parts of this patch?  Where do we set this?  Never mind...
I guess I'm just going to send the review so far.

regards,
dan carpenter