[PATCH v6 0/4] Add generic driver for on-chip SRAM

[PATCH v6 0/4] Add generic driver for on-chip SRAM

[Philipp Zabel]

These patches add support to configure on-chip SRAM via device-tree
node or platform data and to obtain the resulting genalloc pool from
the physical address or a phandle pointing at the device tree node.
This allows drivers to allocate SRAM with the genalloc API without
hard-coding the genalloc pool pointer.

The on-chip SRAM on i.MX53 and i.MX6q can be registered via device tree
and changed to use the simple generic SRAM driver:

                ocram: ocram@00900000 {
                        compatible = "fsl,imx-ocram", "sram";
                        reg = <0x00900000 0x3f000>;
                };

A driver that needs to allocate SRAM buffers, like the video processing
unit on i.MX53, can retrieve the genalloc pool from a phandle in the
device tree using of_get_named_gen_pool(node, "iram", 0) from patch 1:

                vpu@63ff4000 {
                        /* ... */
                        iram = <&ocram>;
                };

Changes since v5:
 - Addressed Paul Gortmaker's comments, merging the clock patch
   into the SRAM driver patch.
 - Hard coded the allocation granularity to 512 bytes and dropped
   Matt Porter's patch for now. Whether or not this should be
   configured in the device tree could use further discussion.
 - Added a coda driver patch to use the genalloc API and, again,
   the i.MX53/i.MX6 device tree patch to show the whole picture.

regards
Philipp

---
 Documentation/devicetree/bindings/misc/sram.txt |   17 ++++
 arch/arm/boot/dts/imx53.dtsi                    |    5 +
 arch/arm/boot/dts/imx6q.dtsi                    |    6 ++
 drivers/media/platform/Kconfig                  |    3 +-
 drivers/media/platform/coda.c                   |   47 ++++++---
 drivers/misc/Kconfig                            |    9 ++
 drivers/misc/Makefile                           |    1 +
 drivers/misc/sram.c                             |  121 +++++++++++++++++++++++
 include/linux/genalloc.h                        |   14 +++
 lib/genalloc.c                                  |   67 +++++++++++++
 10 files changed, 274 insertions(+), 16 deletions(-)

[PATCH v6 1/4] genalloc: add a global pool list, allow to find pools by phys address

[Philipp Zabel]

This patch keeps all created pools in a global list and adds two
functions that allow to retrieve the gen_pool pointer from a known
physical address and from a device tree node.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
---
 include/linux/genalloc.h |   14 ++++++++++
 lib/genalloc.c           |   67 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index dd7c569..91d606e 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -47,6 +47,7 @@ typedef unsigned long (*genpool_algo_t)(unsigned long *map,
  *  General purpose special memory pool descriptor.
  */
 struct gen_pool {
+	struct list_head next_pool;	/* pool in global list */
 	spinlock_t lock;
 	struct list_head chunks;	/* list of chunks in this pool */
 	int min_alloc_order;		/* minimum allocation order */
@@ -105,4 +106,17 @@ extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
 extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
 		unsigned long start, unsigned int nr, void *data);
 
+extern struct gen_pool *gen_pool_find_by_phys(phys_addr_t phys);
+
+struct device_node;
+#ifdef CONFIG_OF
+extern struct gen_pool *of_get_named_gen_pool(struct device_node *np,
+	const char *propname, int index);
+#else
+inline struct gen_pool *of_get_named_gen_pool(struct device_node *np,
+	const char *propname, int index)
+{
+	return NULL;
+}
+#endif
 #endif /* __GENALLOC_H__ */
diff --git a/lib/genalloc.c b/lib/genalloc.c
index 5492043..edf4bf3 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -34,6 +34,11 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/genalloc.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+
+static LIST_HEAD(pools);
+static DEFINE_SPINLOCK(list_lock);
 
 static int set_bits_ll(unsigned long *addr, unsigned long mask_to_set)
 {
@@ -154,6 +159,9 @@ struct gen_pool *gen_pool_create(int min_alloc_order, int nid)
 		pool->min_alloc_order = min_alloc_order;
 		pool->algo = gen_pool_first_fit;
 		pool->data = NULL;
+		spin_lock(&list_lock);
+		list_add_rcu(&pool->next_pool, &pools);
+		spin_unlock(&list_lock);
 	}
 	return pool;
 }
@@ -236,6 +244,9 @@ void gen_pool_destroy(struct gen_pool *pool)
 	int order = pool->min_alloc_order;
 	int bit, end_bit;
 
+	spin_lock(&list_lock);
+	list_del_rcu(&pool->next_pool);
+	spin_unlock(&list_lock);
 	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
 		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
 		list_del(&chunk->next_chunk);
@@ -480,3 +491,59 @@ unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
 	return start_bit;
 }
 EXPORT_SYMBOL(gen_pool_best_fit);
+
+/*
+ * gen_pool_find_by_phys - find a pool by physical start address
+ * @phys: physical address as added with gen_pool_add_virt
+ *
+ * Returns the pool that contains the chunk starting at phys,
+ * or NULL if not found.
+ */
+struct gen_pool *gen_pool_find_by_phys(phys_addr_t phys)
+{
+	struct gen_pool *pool, *found = NULL;
+	struct gen_pool_chunk *chunk;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(pool, &pools, next_pool) {
+		list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) {
+			if (phys == chunk->phys_addr) {
+				found = pool;
+				break;
+			}
+		}
+	}
+	rcu_read_unlock();
+
+	return found;
+}
+EXPORT_SYMBOL_GPL(gen_pool_find_by_phys);
+
+#ifdef CONFIG_OF
+/**
+ * of_get_named_gen_pool - find a pool by phandle property
+ * @np: device node
+ * @propname: property name containing phandle(s)
+ * @index: index into the phandle array
+ *
+ * Returns the pool that contains the chunk starting at the physical
+ * address of the device tree node pointed at by the phandle property,
+ * or NULL if not found.
+ */
+struct gen_pool *of_get_named_gen_pool(struct device_node *np,
+	const char *propname, int index)
+{
+	struct device_node *np_pool;
+	struct resource res;
+	int ret;
+
+	np_pool = of_parse_phandle(np, propname, index);
+	if (!np_pool)
+		return NULL;
+	ret = of_address_to_resource(np_pool, 0, &res);
+	if (ret < 0)
+		return NULL;
+	return gen_pool_find_by_phys((phys_addr_t) res.start);
+}
+EXPORT_SYMBOL_GPL(of_get_named_gen_pool);
+#endif /* CONFIG_OF */
-- 
1.7.10.4

[PATCH v6 2/4] misc: Generic on-chip SRAM allocation driver

[Philipp Zabel]

This driver requests and remaps a memory region as configured in the
device tree. It serves memory from this region via the genalloc API.
It optionally enables the SRAM clock.

Other drivers can retrieve the genalloc pool from a phandle pointing
to this drivers' device node in the device tree.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
---
 Documentation/devicetree/bindings/misc/sram.txt |   17 ++++
 drivers/misc/Kconfig                            |    9 ++
 drivers/misc/Makefile                           |    1 +
 drivers/misc/sram.c                             |  121 +++++++++++++++++++++++
 4 files changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/sram.txt
 create mode 100644 drivers/misc/sram.c

diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
new file mode 100644
index 0000000..b64136c
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/sram.txt
@@ -0,0 +1,17 @@
+Generic on-chip SRAM
+
+Simple IO memory regions to be managed by the genalloc API.
+
+Required properties:
+
+- compatible : sram
+
+- reg : SRAM iomem address range
+
+Example:
+
+sram: sram@5c000000 {
+	compatible = "sram";
+	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
+};
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b151b7c..211468c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -499,6 +499,15 @@ config USB_SWITCH_FSA9480
 	  stereo and mono audio, video, microphone and UART data to use
 	  a common connector port.
 
+config SRAM
+	bool "Generic on-chip SRAM driver"
+	depends on HAS_IOMEM
+	select GENERIC_ALLOCATOR
+	help
+	  This driver allows to declare a memory region to be managed
+	  by the genalloc API. It is supposed to be used for small
+	  on-chip SRAM areas found on many SoCs.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 2129377..d845690 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -49,3 +49,4 @@ obj-y				+= carma/
 obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
 obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)		+= mei/
+obj-$(CONFIG_SRAM)		+= sram.o
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
new file mode 100644
index 0000000..fec8143
--- /dev/null
+++ b/drivers/misc/sram.c
@@ -0,0 +1,121 @@
+/*
+ * Generic on-chip SRAM allocation driver
+ *
+ * Copyright (C) 2012 Philipp Zabel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/genalloc.h>
+
+#define SRAM_GRANULARITY	512
+
+struct sram_dev {
+	struct gen_pool *pool;
+	struct clk *clk;
+};
+
+static int __devinit sram_probe(struct platform_device *pdev)
+{
+	void __iomem *virt_base;
+	struct sram_dev *sram;
+	struct resource *res;
+	unsigned long size;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	size = resource_size(res);
+
+	virt_base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!virt_base)
+		return -EADDRNOTAVAIL;
+
+	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
+	if (!sram)
+		return -ENOMEM;
+
+	sram->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(sram->clk))
+		sram->clk = NULL;
+	else
+		clk_prepare_enable(sram->clk);
+
+	sram->pool = gen_pool_create(ilog2(SRAM_GRANULARITY), -1);
+	if (!sram->pool)
+		return -ENOMEM;
+
+	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
+				res->start, size, -1);
+	if (ret < 0) {
+		gen_pool_destroy(sram->pool);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, sram);
+
+	dev_dbg(&pdev->dev, "SRAM pool: %ld KiB @ 0x%p\n", size / 1024, virt_base);
+
+	return 0;
+}
+
+static int __devexit sram_remove(struct platform_device *pdev)
+{
+	struct sram_dev *sram = platform_get_drvdata(pdev);
+
+	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
+		dev_dbg(&pdev->dev, "removed while SRAM allocated\n");
+
+	gen_pool_destroy(sram->pool);
+
+	if (sram->clk)
+		clk_disable_unprepare(sram->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id sram_dt_ids[] = {
+	{ .compatible = "sram" },
+	{}
+};
+#endif
+
+static struct platform_driver sram_driver = {
+	.driver = {
+		.name = "sram",
+		.of_match_table = of_match_ptr(sram_dt_ids),
+	},
+	.probe = sram_probe,
+	.remove = __devexit_p(sram_remove),
+};
+
+int __init sram_init(void)
+{
+	return platform_driver_register(&sram_driver);
+}
+
+postcore_initcall(sram_init);
-- 
1.7.10.4

[PATCH v6 3/4] media: coda: use genalloc API

[Philipp Zabel]

This patch depends on "genalloc: add a global pool list,
allow to find pools by phys address", which provides the
of_get_named_gen_pool function.

Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/media/platform/Kconfig |    3 +--
 drivers/media/platform/coda.c  |   47 ++++++++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 181c768..09d45c6 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -130,10 +130,9 @@ if V4L_MEM2MEM_DRIVERS
 
 config VIDEO_CODA
 	tristate "Chips&Media Coda multi-standard codec IP"
-	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
+	depends on VIDEO_DEV && VIDEO_V4L2
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_MEM2MEM_DEV
-	select IRAM_ALLOC if SOC_IMX53
 	---help---
 	   Coda is a range of video codec IPs that supports
 	   H.264, MPEG-4, and other video formats.
diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
index cd04ae2..f17b659 100644
--- a/drivers/media/platform/coda.c
+++ b/drivers/media/platform/coda.c
@@ -14,6 +14,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
+#include <linux/genalloc.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -24,7 +25,6 @@
 #include <linux/videodev2.h>
 #include <linux/of.h>
 
-#include <mach/iram.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -43,6 +43,7 @@
 #define CODA7_WORK_BUF_SIZE	(512 * 1024 + CODA_FMO_BUF_SIZE * 8 * 1024)
 #define CODA_PARA_BUF_SIZE	(10 * 1024)
 #define CODA_ISRAM_SIZE	(2048 * 2)
+#define CODADX6_IRAM_SIZE	0xb000
 #define CODA7_IRAM_SIZE		0x14000 /* 81920 bytes */
 
 #define CODA_MAX_FRAMEBUFFERS	2
@@ -128,7 +129,10 @@ struct coda_dev {
 
 	struct coda_aux_buf	codebuf;
 	struct coda_aux_buf	workbuf;
+	struct gen_pool		*iram_pool;
+	long unsigned int	iram_vaddr;
 	long unsigned int	iram_paddr;
+	unsigned long		iram_size;
 
 	spinlock_t		irqlock;
 	struct mutex		dev_mutex;
@@ -1958,6 +1962,22 @@ static int __devinit coda_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
+	/* Without device tree, get SRAM paddr from second memory resource */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res != NULL)
+		dev->iram_pool = gen_pool_find_by_phys(res->start);
+#ifdef CONFIG_OF
+	if (!dev->iram_pool) {
+		struct device_node *np = pdev->dev.of_node;
+
+		dev->iram_pool = of_get_named_gen_pool(np, "iram", 0);
+	}
+#endif
+	if (!dev->iram_pool) {
+		dev_err(&pdev->dev, "iram pool not available\n");
+		return -ENOMEM;
+	}
+
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
 		return ret;
@@ -1992,18 +2012,17 @@ static int __devinit coda_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	if (dev->devtype->product == CODA_DX6) {
-		dev->iram_paddr = 0xffff4c00;
-	} else {
-		void __iomem *iram_vaddr;
-
-		iram_vaddr = iram_alloc(CODA7_IRAM_SIZE,
-					&dev->iram_paddr);
-		if (!iram_vaddr) {
-			dev_err(&pdev->dev, "unable to alloc iram\n");
-			return -ENOMEM;
-		}
+	if (dev->devtype->product == CODA_DX6)
+		dev->iram_size = CODADX6_IRAM_SIZE;
+	else
+		dev->iram_size = CODA7_IRAM_SIZE;
+	dev->iram_vaddr = gen_pool_alloc(dev->iram_pool, dev->iram_size);
+	if (!dev->iram_vaddr) {
+		dev_err(&pdev->dev, "unable to alloc iram\n");
+		return -ENOMEM;
 	}
+	dev->iram_paddr = gen_pool_virt_to_phys(dev->iram_pool,
+						dev->iram_vaddr);
 
 	platform_set_drvdata(pdev, dev);
 
@@ -2020,8 +2039,8 @@ static int coda_remove(struct platform_device *pdev)
 	if (dev->alloc_ctx)
 		vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
 	v4l2_device_unregister(&dev->v4l2_dev);
-	if (dev->iram_paddr)
-		iram_free(dev->iram_paddr, CODA7_IRAM_SIZE);
+	if (dev->iram_vaddr)
+		gen_pool_free(dev->iram_pool, dev->iram_vaddr, dev->iram_size);
 	if (dev->codebuf.vaddr)
 		dma_free_coherent(&pdev->dev, dev->codebuf.size,
 				  &dev->codebuf.vaddr, dev->codebuf.paddr);
-- 
1.7.10.4

[PATCH v6 4/4] ARM: dts: add sram for imx53 and imx6q

[Philipp Zabel]

Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Reviewed-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/imx53.dtsi |    5 +++++
 arch/arm/boot/dts/imx6q.dtsi |    6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index 76ebb1a..7677218 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -541,5 +541,10 @@
 				status = "disabled";
 			};
 		};
+
+		ocram: ocram@f8000000 {
+			compatible = "fsl,imx-ocram", "sram";
+			reg = <0xf8000000 0x20000>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index f3990b0..855ac25 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -116,6 +116,12 @@
 			status = "disabled";
 		};
 
+		ocram: ocram@00900000 {
+			compatible = "fsl,imx-ocram", "sram";
+			reg = <0x00900000 0x3f000>;
+			clocks = <&clks 142>;
+		};
+
 		timer@00a00600 {
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0x00a00600 0x20>;
-- 
1.7.10.4

Re: [PATCH v6 3/4] media: coda: use genalloc API

[Paul Gortmaker]

On 12-11-16 05:30 AM, Philipp Zabel wrote:
> This patch depends on "genalloc: add a global pool list,
> allow to find pools by phys address", which provides the
> of_get_named_gen_pool function.
> 
> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/media/platform/Kconfig |    3 +--
>  drivers/media/platform/coda.c  |   47 ++++++++++++++++++++++++++++------------
>  2 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 181c768..09d45c6 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -130,10 +130,9 @@ if V4L_MEM2MEM_DRIVERS
>  
>  config VIDEO_CODA
>  	tristate "Chips&Media Coda multi-standard codec IP"
> -	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
> +	depends on VIDEO_DEV && VIDEO_V4L2

What was the logic for reducing the dependency scope here?
Your commit log doesn't mention that at all, and when I see
things like that, I predict allyesconfig build failures,
unless there is a similar dependency elsewhere that isn't
visible in just the context of this patch alone.

P.
--

>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
> -	select IRAM_ALLOC if SOC_IMX53
>  	---help---
>  	   Coda is a range of video codec IPs that supports
>  	   H.264, MPEG-4, and other video formats.
> diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
> index cd04ae2..f17b659 100644
> --- a/drivers/media/platform/coda.c
> +++ b/drivers/media/platform/coda.c
> @@ -14,6 +14,7 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
> +#include <linux/genalloc.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
> @@ -24,7 +25,6 @@
>  #include <linux/videodev2.h>
>  #include <linux/of.h>
>  
> -#include <mach/iram.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> @@ -43,6 +43,7 @@
>  #define CODA7_WORK_BUF_SIZE	(512 * 1024 + CODA_FMO_BUF_SIZE * 8 * 1024)
>  #define CODA_PARA_BUF_SIZE	(10 * 1024)
>  #define CODA_ISRAM_SIZE	(2048 * 2)
> +#define CODADX6_IRAM_SIZE	0xb000
>  #define CODA7_IRAM_SIZE		0x14000 /* 81920 bytes */
>  
>  #define CODA_MAX_FRAMEBUFFERS	2
> @@ -128,7 +129,10 @@ struct coda_dev {
>  
>  	struct coda_aux_buf	codebuf;
>  	struct coda_aux_buf	workbuf;
> +	struct gen_pool		*iram_pool;
> +	long unsigned int	iram_vaddr;
>  	long unsigned int	iram_paddr;
> +	unsigned long		iram_size;
>  
>  	spinlock_t		irqlock;
>  	struct mutex		dev_mutex;
> @@ -1958,6 +1962,22 @@ static int __devinit coda_probe(struct platform_device *pdev)
>  		return -ENOENT;
>  	}
>  
> +	/* Without device tree, get SRAM paddr from second memory resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res != NULL)
> +		dev->iram_pool = gen_pool_find_by_phys(res->start);
> +#ifdef CONFIG_OF
> +	if (!dev->iram_pool) {
> +		struct device_node *np = pdev->dev.of_node;
> +
> +		dev->iram_pool = of_get_named_gen_pool(np, "iram", 0);
> +	}
> +#endif
> +	if (!dev->iram_pool) {
> +		dev_err(&pdev->dev, "iram pool not available\n");
> +		return -ENOMEM;
> +	}
> +
>  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>  	if (ret)
>  		return ret;
> @@ -1992,18 +2012,17 @@ static int __devinit coda_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	if (dev->devtype->product == CODA_DX6) {
> -		dev->iram_paddr = 0xffff4c00;
> -	} else {
> -		void __iomem *iram_vaddr;
> -
> -		iram_vaddr = iram_alloc(CODA7_IRAM_SIZE,
> -					&dev->iram_paddr);
> -		if (!iram_vaddr) {
> -			dev_err(&pdev->dev, "unable to alloc iram\n");
> -			return -ENOMEM;
> -		}
> +	if (dev->devtype->product == CODA_DX6)
> +		dev->iram_size = CODADX6_IRAM_SIZE;
> +	else
> +		dev->iram_size = CODA7_IRAM_SIZE;
> +	dev->iram_vaddr = gen_pool_alloc(dev->iram_pool, dev->iram_size);
> +	if (!dev->iram_vaddr) {
> +		dev_err(&pdev->dev, "unable to alloc iram\n");
> +		return -ENOMEM;
>  	}
> +	dev->iram_paddr = gen_pool_virt_to_phys(dev->iram_pool,
> +						dev->iram_vaddr);
>  
>  	platform_set_drvdata(pdev, dev);
>  
> @@ -2020,8 +2039,8 @@ static int coda_remove(struct platform_device *pdev)
>  	if (dev->alloc_ctx)
>  		vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
>  	v4l2_device_unregister(&dev->v4l2_dev);
> -	if (dev->iram_paddr)
> -		iram_free(dev->iram_paddr, CODA7_IRAM_SIZE);
> +	if (dev->iram_vaddr)
> +		gen_pool_free(dev->iram_pool, dev->iram_vaddr, dev->iram_size);
>  	if (dev->codebuf.vaddr)
>  		dma_free_coherent(&pdev->dev, dev->codebuf.size,
>  				  &dev->codebuf.vaddr, dev->codebuf.paddr);
> 

Re: [PATCH v6 3/4] media: coda: use genalloc API

[Philipp Zabel]

Am Freitag, den 16.11.2012, 10:08 -0500 schrieb Paul Gortmaker:
> On 12-11-16 05:30 AM, Philipp Zabel wrote:
> > This patch depends on "genalloc: add a global pool list,
> > allow to find pools by phys address", which provides the
> > of_get_named_gen_pool function.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/media/platform/Kconfig |    3 +--
> >  drivers/media/platform/coda.c  |   47 ++++++++++++++++++++++++++++------------
> >  2 files changed, 34 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 181c768..09d45c6 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -130,10 +130,9 @@ if V4L_MEM2MEM_DRIVERS
> >  
> >  config VIDEO_CODA
> >  	tristate "Chips&Media Coda multi-standard codec IP"
> > -	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
> > +	depends on VIDEO_DEV && VIDEO_V4L2
> 
> What was the logic for reducing the dependency scope here?
> Your commit log doesn't mention that at all, and when I see
> things like that, I predict allyesconfig build failures,
> unless there is a similar dependency elsewhere that isn't
> visible in just the context of this patch alone.
> 
> P.

iram_alloc and iram_free are i.MX specific wrappers around
gen_pool_alloc and gen_pool_free, located in <mach/iram.h>.
Those were responsible for the dependency in the first place.

> --
> 
> >  	select VIDEOBUF2_DMA_CONTIG
> >  	select V4L2_MEM2MEM_DEV
> > -	select IRAM_ALLOC if SOC_IMX53
> >  	---help---
> >  	   Coda is a range of video codec IPs that supports
> >  	   H.264, MPEG-4, and other video formats.
> > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
> > index cd04ae2..f17b659 100644
> > --- a/drivers/media/platform/coda.c
> > +++ b/drivers/media/platform/coda.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/firmware.h>
> > +#include <linux/genalloc.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/irq.h>
> > @@ -24,7 +25,6 @@
> >  #include <linux/videodev2.h>
> >  #include <linux/of.h>
> >  
> > -#include <mach/iram.h>
[...]

After dropping the #include <mach/iram.h>, there is no need
to depend on ARCH_MXC anymore.

regards
Philipp

Re: [PATCH v6 3/4] media: coda: use genalloc API

[Paul Gortmaker]

On 12-11-16 10:21 AM, Philipp Zabel wrote:
> Am Freitag, den 16.11.2012, 10:08 -0500 schrieb Paul Gortmaker:
>> On 12-11-16 05:30 AM, Philipp Zabel wrote:
>>> This patch depends on "genalloc: add a global pool list,
>>> allow to find pools by phys address", which provides the
>>> of_get_named_gen_pool function.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> ---
>>>  drivers/media/platform/Kconfig |    3 +--
>>>  drivers/media/platform/coda.c  |   47 ++++++++++++++++++++++++++++------------
>>>  2 files changed, 34 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>> index 181c768..09d45c6 100644
>>> --- a/drivers/media/platform/Kconfig
>>> +++ b/drivers/media/platform/Kconfig
>>> @@ -130,10 +130,9 @@ if V4L_MEM2MEM_DRIVERS
>>>  
>>>  config VIDEO_CODA
>>>  	tristate "Chips&Media Coda multi-standard codec IP"
>>> -	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
>>> +	depends on VIDEO_DEV && VIDEO_V4L2
>>
>> What was the logic for reducing the dependency scope here?
>> Your commit log doesn't mention that at all, and when I see
>> things like that, I predict allyesconfig build failures,
>> unless there is a similar dependency elsewhere that isn't
>> visible in just the context of this patch alone.
>>
>> P.
> 
> iram_alloc and iram_free are i.MX specific wrappers around
> gen_pool_alloc and gen_pool_free, located in <mach/iram.h>.
> Those were responsible for the dependency in the first place.

So when I do an allyesconfig for sparc, or parisc or alpha,
and VIDEO_CODA gets selected, it will build just fine then?
My point was that when you remove the ARCH_MXC dep, this
probably gets opened up as a viable option to a _lot_ more
platforms than you might want it exposed to.

Paul.
--

> 
>> --
>>
>>>  	select VIDEOBUF2_DMA_CONTIG
>>>  	select V4L2_MEM2MEM_DEV
>>> -	select IRAM_ALLOC if SOC_IMX53
>>>  	---help---
>>>  	   Coda is a range of video codec IPs that supports
>>>  	   H.264, MPEG-4, and other video formats.
>>> diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
>>> index cd04ae2..f17b659 100644
>>> --- a/drivers/media/platform/coda.c
>>> +++ b/drivers/media/platform/coda.c
>>> @@ -14,6 +14,7 @@
>>>  #include <linux/clk.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/firmware.h>
>>> +#include <linux/genalloc.h>
>>>  #include <linux/interrupt.h>
>>>  #include <linux/io.h>
>>>  #include <linux/irq.h>
>>> @@ -24,7 +25,6 @@
>>>  #include <linux/videodev2.h>
>>>  #include <linux/of.h>
>>>  
>>> -#include <mach/iram.h>
> [...]
> 
> After dropping the #include <mach/iram.h>, there is no need
> to depend on ARCH_MXC anymore.
> 
> regards
> Philipp
> 

Re: [PATCH v6 3/4] media: coda: use genalloc API

[Philipp Zabel]

Am Freitag, den 16.11.2012, 11:00 -0500 schrieb Paul Gortmaker:
> On 12-11-16 10:21 AM, Philipp Zabel wrote:
> > Am Freitag, den 16.11.2012, 10:08 -0500 schrieb Paul Gortmaker:
> >> On 12-11-16 05:30 AM, Philipp Zabel wrote:
> >>> This patch depends on "genalloc: add a global pool list,
> >>> allow to find pools by phys address", which provides the
> >>> of_get_named_gen_pool function.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>  drivers/media/platform/Kconfig |    3 +--
> >>>  drivers/media/platform/coda.c  |   47 ++++++++++++++++++++++++++++------------
> >>>  2 files changed, 34 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >>> index 181c768..09d45c6 100644
> >>> --- a/drivers/media/platform/Kconfig
> >>> +++ b/drivers/media/platform/Kconfig
> >>> @@ -130,10 +130,9 @@ if V4L_MEM2MEM_DRIVERS
> >>>  
> >>>  config VIDEO_CODA
> >>>  	tristate "Chips&Media Coda multi-standard codec IP"
> >>> -	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
> >>> +	depends on VIDEO_DEV && VIDEO_V4L2
> >>
> >> What was the logic for reducing the dependency scope here?
> >> Your commit log doesn't mention that at all, and when I see
> >> things like that, I predict allyesconfig build failures,
> >> unless there is a similar dependency elsewhere that isn't
> >> visible in just the context of this patch alone.
> >>
> >> P.
> > 
> > iram_alloc and iram_free are i.MX specific wrappers around
> > gen_pool_alloc and gen_pool_free, located in <mach/iram.h>.
> > Those were responsible for the dependency in the first place.
> 
> So when I do an allyesconfig for sparc, or parisc or alpha,
> and VIDEO_CODA gets selected, it will build just fine then?

I don't know, as I don't have compilers for those available right now.
I'd like to know if it doesn't, though. It builds fine on x86 and mips,
for example.

> My point was that when you remove the ARCH_MXC dep, this
> probably gets opened up as a viable option to a _lot_ more
> platforms than you might want it exposed to.

I don't see the problem. Isn't this a good thing?

regards
Philipp

Re: [PATCH v6 3/4] media: coda: use genalloc API

[Paul Gortmaker]

On 12-11-16 11:51 AM, Philipp Zabel wrote:
> Am Freitag, den 16.11.2012, 11:00 -0500 schrieb Paul Gortmaker:
>> On 12-11-16 10:21 AM, Philipp Zabel wrote:
>>> Am Freitag, den 16.11.2012, 10:08 -0500 schrieb Paul Gortmaker:
>>>> On 12-11-16 05:30 AM, Philipp Zabel wrote:
>>>>> This patch depends on "genalloc: add a global pool list,
>>>>> allow to find pools by phys address", which provides the
>>>>> of_get_named_gen_pool function.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>> ---
>>>>>  drivers/media/platform/Kconfig |    3 +--
>>>>>  drivers/media/platform/coda.c  |   47 ++++++++++++++++++++++++++++------------
>>>>>  2 files changed, 34 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>>> index 181c768..09d45c6 100644
>>>>> --- a/drivers/media/platform/Kconfig
>>>>> +++ b/drivers/media/platform/Kconfig
>>>>> @@ -130,10 +130,9 @@ if V4L_MEM2MEM_DRIVERS
>>>>>  
>>>>>  config VIDEO_CODA
>>>>>  	tristate "Chips&Media Coda multi-standard codec IP"
>>>>> -	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
>>>>> +	depends on VIDEO_DEV && VIDEO_V4L2
>>>>
>>>> What was the logic for reducing the dependency scope here?
>>>> Your commit log doesn't mention that at all, and when I see
>>>> things like that, I predict allyesconfig build failures,
>>>> unless there is a similar dependency elsewhere that isn't
>>>> visible in just the context of this patch alone.
>>>>
>>>> P.
>>>
>>> iram_alloc and iram_free are i.MX specific wrappers around
>>> gen_pool_alloc and gen_pool_free, located in <mach/iram.h>.
>>> Those were responsible for the dependency in the first place.
>>
>> So when I do an allyesconfig for sparc, or parisc or alpha,
>> and VIDEO_CODA gets selected, it will build just fine then?
> 
> I don't know, as I don't have compilers for those available right now.
> I'd like to know if it doesn't, though. It builds fine on x86 and mips,
> for example.

Probably worthwhile to watch the linux-next builds once you
know your commit(s) will be present there, since it has a
wide arch coverage.

> 
>> My point was that when you remove the ARCH_MXC dep, this
>> probably gets opened up as a viable option to a _lot_ more
>> platforms than you might want it exposed to.
> 
> I don't see the problem. Isn't this a good thing?

Well, it can be, if it was intentional, and if the hardware
is genuinely architecture agnostic.  On the other hand, I
don't think it makes sense to be building arm specific drivers
on sparc (or similar) just because we can.  It just adds to
the overall build coverage overhead for minimal gain.

Paul.
--

> 
> regards
> Philipp
> 

Re: [PATCH v6 1/4] genalloc: add a global pool list, allow to find pools by phys address

[Andrew Morton]

On Fri, 16 Nov 2012 11:30:14 +0100 Philipp Zabel <p.zabel@pengutronix.de> wrote:

> This patch keeps all created pools in a global list and adds two
> functions that allow to retrieve the gen_pool pointer from a known
> physical address and from a device tree node.
>
> ...
>
> +/*
> + * gen_pool_find_by_phys - find a pool by physical start address
> + * @phys: physical address as added with gen_pool_add_virt
> + *
> + * Returns the pool that contains the chunk starting at phys,
> + * or NULL if not found.
> + */
> +struct gen_pool *gen_pool_find_by_phys(phys_addr_t phys)
> +{
> +	struct gen_pool *pool, *found = NULL;
> +	struct gen_pool_chunk *chunk;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(pool, &pools, next_pool) {
> +		list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) {
> +			if (phys == chunk->phys_addr) {
> +				found = pool;
> +				break;
> +			}
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(gen_pool_find_by_phys);

It is rather pointless to use the fancy super-fast RCU locking
around a linear search!  We have various data structures which can be
used to make this search much more efficient.  radix-tree is one, if
the search keys are unique (which is the case here).

Secondly, that whole "phys" concept doesn't need to be in there.  It
would be better to implement a far more general
gen_pool_find_by_key(unsigned long key) and then do the phys->ulong
specialization elsewhere.

Finally the changelog gives no indication *why* you feel the kernel
needs this feature.  What is it for?  What are the use cases?  This is
the most important information for reviewers, hence it should be up
there front and center, in lavish detail.

Because once this is understood:

a) people might be able to suggest alternatives.  Can't do that
   without the required info and

b) people might then be interested in merging the patch into a kernel!

> +#ifdef CONFIG_OF
> +/**
> + * of_get_named_gen_pool - find a pool by phandle property
> + * @np: device node
> + * @propname: property name containing phandle(s)
> + * @index: index into the phandle array
> + *
> + * Returns the pool that contains the chunk starting at the physical
> + * address of the device tree node pointed at by the phandle property,
> + * or NULL if not found.
> + */
> +struct gen_pool *of_get_named_gen_pool(struct device_node *np,
> +	const char *propname, int index)
> +{
> +	struct device_node *np_pool;
> +	struct resource res;
> +	int ret;
> +
> +	np_pool = of_parse_phandle(np, propname, index);
> +	if (!np_pool)
> +		return NULL;
> +	ret = of_address_to_resource(np_pool, 0, &res);
> +	if (ret < 0)
> +		return NULL;
> +	return gen_pool_find_by_phys((phys_addr_t) res.start);
> +}
> +EXPORT_SYMBOL_GPL(of_get_named_gen_pool);

Seems rather inappropriate that this should be in lib/genpool.c. 
Put it somewhere such as drivers/of/base.c, perhaps.