linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 0/4] Add generic driver for on-chip SRAM
@ 2013-02-04 11:32 Philipp Zabel
  2013-02-04 11:32 ` [PATCH v8 1/4] genalloc: add devres support, allow to find a managed pool by device Philipp Zabel
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Philipp Zabel @ 2013-02-04 11:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Grant Likely, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss

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 struct device pointer 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.

I changed the implementation in genalloc.c since v7. To avoid the global
pool list, a devres managed version of gen_pool_create is added, and
gen_pool_find_by_phys of the previous versions is replaced by dev_get_gen_pool,
that retrieves a gen_pool created with devm_gen_pool_create from the
corresponding device pointer. of_get_named_gen_pool is called unchanged.

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 v7:
- Removed the global pool list in genalloc. Instead, added a devres managed
  version of gen_pool_create, replacing gen_pool_find_by_phys with
  dev_get_gen_pool (and made of_get_named_gen_pool use that)
- In the coda driver, switched to dev_get_gen_pool and added a platform_data
  struct to pass the SRAM device pointer (instead of using a second IO memory
  resource). Added device tree binding documentation.

regards
Philipp

---
 Documentation/devicetree/bindings/media/coda.txt |   30 ++++++
 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                   |    1 -
 drivers/media/platform/coda.c                    |   45 +++++---
 drivers/misc/Kconfig                             |    9 ++
 drivers/misc/Makefile                            |    1 +
 drivers/misc/sram.c                              |  121 ++++++++++++++++++++++
 include/linux/genalloc.h                         |   15 +++
 include/linux/platform_data/coda.h               |   18 ++++
 lib/genalloc.c                                   |   81 +++++++++++++++
 12 files changed, 334 insertions(+), 15 deletions(-)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v8 1/4] genalloc: add devres support, allow to find a managed pool by device
  2013-02-04 11:32 [PATCH v8 0/4] Add generic driver for on-chip SRAM Philipp Zabel
@ 2013-02-04 11:32 ` Philipp Zabel
  2013-02-04 11:32 ` [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver Philipp Zabel
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Philipp Zabel @ 2013-02-04 11:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Grant Likely, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss, Philipp Zabel

This patch adds three exported functions to lib/genalloc.c:
devm_gen_pool_create, dev_get_gen_pool, and of_get_named_gen_pool.

devm_gen_pool_create is a managed version of gen_pool_create that keeps
track of the pool via devres and allows the management code to automatically
destroy it after device removal.

dev_get_gen_pool retrieves the gen_pool for a given device, if it was
created with devm_gen_pool_create, using devres_find.

of_get_named_gen_pool retrieves the gen_pool for a given device node and
property name, where the property must contain a phandle pointing to a
platform device node. The corresponding platform device is then fed into
dev_get_gen_pool and the resulting gen_pool is returned.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v7:
- Removed the global pool list. Instead, added a devres managed version of
  gen_pool_create, replacing gen_pool_find_by_phys with dev_get_gen_pool
---
 include/linux/genalloc.h |   15 +++++++++
 lib/genalloc.c           |   81 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index dd7c569..383e8f4 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -105,4 +105,19 @@ 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 *devm_gen_pool_create(struct device *dev,
+		int min_alloc_order, int nid);
+extern struct gen_pool *dev_get_gen_pool(struct device *dev);
+
+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..b35cfa9 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -34,6 +34,8 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/genalloc.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
 
 static int set_bits_ll(unsigned long *addr, unsigned long mask_to_set)
 {
@@ -480,3 +482,82 @@ unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
 	return start_bit;
 }
 EXPORT_SYMBOL(gen_pool_best_fit);
+
+static void devm_gen_pool_release(struct device *dev, void *res)
+{
+	gen_pool_destroy(*(struct gen_pool **)res);
+}
+
+/**
+ * devm_gen_pool_create - managed gen_pool_create
+ * @dev: device that provides the gen_pool
+ * @min_alloc_order: log base 2 of number of bytes each bitmap bit represents
+ * @nid: node id of the node the pool structure should be allocated on, or -1
+ *
+ * Create a new special memory pool that can be used to manage special purpose
+ * memory not managed by the regular kmalloc/kfree interface. The pool will be
+ * automatically destroyed by the device management code.
+ */
+struct gen_pool *devm_gen_pool_create(struct device *dev, int min_alloc_order,
+		int nid)
+{
+	struct gen_pool **ptr, *pool;
+
+	ptr = devres_alloc(devm_gen_pool_release, sizeof(*ptr), GFP_KERNEL);
+
+	pool = gen_pool_create(min_alloc_order, nid);
+	if (pool) {
+		*ptr = pool;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return pool;
+}
+
+/**
+ * dev_get_gen_pool - Obtain the gen_pool (if any) for a device
+ * @dev: device to retrieve the gen_pool from
+ * @name: Optional name for the gen_pool, usually NULL
+ *
+ * Returns the gen_pool for the device if one is present, or NULL.
+ */
+struct gen_pool *dev_get_gen_pool(struct device *dev)
+{
+	struct gen_pool **p = devres_find(dev, devm_gen_pool_release, NULL,
+					NULL);
+
+	if (!p)
+		return NULL;
+	return *p;
+}
+EXPORT_SYMBOL_GPL(dev_get_gen_pool);
+
+#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 platform_device *pdev;
+	struct device_node *np_pool;
+
+	np_pool = of_parse_phandle(np, propname, index);
+	if (!np_pool)
+		return NULL;
+	pdev = of_find_device_by_node(np_pool);
+	if (!pdev)
+		return NULL;
+	return dev_get_gen_pool(&pdev->dev);
+}
+EXPORT_SYMBOL_GPL(of_get_named_gen_pool);
+#endif /* CONFIG_OF */
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-04 11:32 [PATCH v8 0/4] Add generic driver for on-chip SRAM Philipp Zabel
  2013-02-04 11:32 ` [PATCH v8 1/4] genalloc: add devres support, allow to find a managed pool by device Philipp Zabel
@ 2013-02-04 11:32 ` Philipp Zabel
  2013-02-04 15:53   ` Paul Mundt
  2013-02-08 20:16   ` Grant Likely
  2013-02-04 11:32 ` [PATCH v8 3/4] media: coda: use genalloc API Philipp Zabel
  2013-02-04 11:32 ` [PATCH v8 4/4] ARM: dts: add sram for imx53 and imx6q Philipp Zabel
  3 siblings, 2 replies; 12+ messages in thread
From: Philipp Zabel @ 2013-02-04 11:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Grant Likely, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss, 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.

The allocation granularity is hard-coded to 32 bytes for now,
to make the SRAM driver useful for the 6502 remoteproc driver.
There is overhead for bigger SRAMs, where only a much coarser
allocation granularity is needed: At 32 bytes minimum allocation
size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v7:
 - Removed obsolete __devinit/__devexit/__devexit_p
---
 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 e83fdfe..4878507 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -510,6 +510,15 @@ config LATTICE_ECP3_CONFIG
 
 	  If unsure, say N.
 
+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 35a1463..08e2007 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,3 +52,4 @@ obj-$(CONFIG_INTEL_MEI)		+= mei/
 obj-$(CONFIG_MAX8997_MUIC)	+= max8997-muic.o
 obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
+obj-$(CONFIG_SRAM)		+= sram.o
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
new file mode 100644
index 0000000..94d12ea
--- /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	32
+
+struct sram_dev {
+	struct gen_pool *pool;
+	struct clk *clk;
+};
+
+static int 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 = devm_gen_pool_create(&pdev->dev, 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 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 = sram_remove,
+};
+
+int __init sram_init(void)
+{
+	return platform_driver_register(&sram_driver);
+}
+
+postcore_initcall(sram_init);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v8 3/4] media: coda: use genalloc API
  2013-02-04 11:32 [PATCH v8 0/4] Add generic driver for on-chip SRAM Philipp Zabel
  2013-02-04 11:32 ` [PATCH v8 1/4] genalloc: add devres support, allow to find a managed pool by device Philipp Zabel
  2013-02-04 11:32 ` [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver Philipp Zabel
@ 2013-02-04 11:32 ` Philipp Zabel
  2013-02-05  8:12   ` javier Martin
  2013-02-04 11:32 ` [PATCH v8 4/4] ARM: dts: add sram for imx53 and imx6q Philipp Zabel
  3 siblings, 1 reply; 12+ messages in thread
From: Philipp Zabel @ 2013-02-04 11:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Grant Likely, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss, Philipp Zabel

This patch depends on "genalloc: add devres support, allow to find
a managed pool by device", which provides the of_get_named_gen_pool
and dev_get_gen_pool functions.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v7:
 - In the platform data case, retrieve gen_pool by device instead of
   by physical address. The information about the SRAM pool can't be
   provided via memory resource, so add a platform data struct that
   contains a pointer to the SRAM device.
 - Add device tree binding documentation.
---
 Documentation/devicetree/bindings/media/coda.txt |   30 +++++++++++++++
 drivers/media/platform/Kconfig                   |    1 -
 drivers/media/platform/coda.c                    |   45 +++++++++++++++-------
 include/linux/platform_data/coda.h               |   18 +++++++++
 4 files changed, 79 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/coda.txt
 create mode 100644 include/linux/platform_data/coda.h

diff --git a/Documentation/devicetree/bindings/media/coda.txt b/Documentation/devicetree/bindings/media/coda.txt
new file mode 100644
index 0000000..2865d04
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/coda.txt
@@ -0,0 +1,30 @@
+Chips&Media Coda multi-standard codec IP
+========================================
+
+Coda codec IPs are present in i.MX SoCs in various versions,
+called VPU (Video Processing Unit).
+
+Required properties:
+- compatible : should be "fsl,<chip>-src" for i.MX SoCs:
+  (a) "fsl,imx27-vpu" for CodaDx6 present in i.MX27
+  (b) "fsl,imx53-vpu" for CODA7541 present in i.MX53
+  (c) "fsl,imx6q-vpu" for CODA960 present in i.MX6q
+- reg: should be register base and length as documented in the
+  SoC reference manual
+- interrupts : Should contain the VPU interrupt. For CODA960,
+  a second interrupt is needed for the MJPEG unit.
+- clocks : Should contain the ahb and per clocks, in the order
+  determined by the clock-names property.
+- clock-names : Should be "ahb", "per"
+- iram : phandle pointing to the SRAM device node
+
+Example:
+
+vpu: vpu@63ff4000 {
+	compatible = "fsl,imx53-vpu";
+	reg = <0x63ff4000 0x1000>;
+	interrupts = <9>;
+	clocks = <&clks 63>, <&clks 63>;
+	clock-names = "ahb", "per";
+	iram = <&ocram>;
+};
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 05d7b63..bbf83c1 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -145,7 +145,6 @@ config VIDEO_CODA
 	depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
 	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 20827ba..b931c2a 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>
@@ -23,7 +24,7 @@
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 #include <linux/of.h>
-#include <linux/platform_data/imx-iram.h>
+#include <linux/platform_data/coda.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -43,6 +44,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 +130,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;
@@ -1926,6 +1931,9 @@ static int coda_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id =
 			of_match_device(of_match_ptr(coda_dt_ids), &pdev->dev);
 	const struct platform_device_id *pdev_id;
+	struct coda_platform_data *pdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
+	struct gen_pool *pool;
 	struct coda_dev *dev;
 	struct resource *res;
 	int ret, irq;
@@ -1988,6 +1996,16 @@ static int coda_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
+	/* Get IRAM pool from device tree or platform data */
+	pool = of_get_named_gen_pool(np, "iram", 0);
+	if (!pool && pdata)
+		pool = dev_get_gen_pool(pdata->iram_dev);
+	if (!pool) {
+		dev_err(&pdev->dev, "iram pool not available\n");
+		return -ENOMEM;
+	}
+	dev->iram_pool = pool;
+
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
 		return ret;
@@ -2022,18 +2040,17 @@ static int 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);
 
@@ -2050,8 +2067,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);
diff --git a/include/linux/platform_data/coda.h b/include/linux/platform_data/coda.h
new file mode 100644
index 0000000..6ad4410
--- /dev/null
+++ b/include/linux/platform_data/coda.h
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2013 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.
+ */
+#ifndef PLATFORM_CODA_H
+#define PLATFORM_CODA_H
+
+struct device;
+
+struct coda_platform_data {
+	struct device *iram_dev;
+};
+
+#endif
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v8 4/4] ARM: dts: add sram for imx53 and imx6q
  2013-02-04 11:32 [PATCH v8 0/4] Add generic driver for on-chip SRAM Philipp Zabel
                   ` (2 preceding siblings ...)
  2013-02-04 11:32 ` [PATCH v8 3/4] media: coda: use genalloc API Philipp Zabel
@ 2013-02-04 11:32 ` Philipp Zabel
  3 siblings, 0 replies; 12+ messages in thread
From: Philipp Zabel @ 2013-02-04 11:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Grant Likely, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss, Philipp Zabel

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Shawn Guo <shawn.guo@linaro.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 edc3f1e..8faca1a 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -664,5 +664,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 d6265ca..740e714 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -124,6 +124,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-04 11:32 ` [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver Philipp Zabel
@ 2013-02-04 15:53   ` Paul Mundt
  2013-02-04 17:02     ` Matt Porter
  2013-02-05  8:57     ` Philipp Zabel
  2013-02-08 20:16   ` Grant Likely
  1 sibling, 2 replies; 12+ messages in thread
From: Paul Mundt @ 2013-02-04 15:53 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, Grant Likely,
	Rob Herring, Paul Gortmaker, Shawn Guo, Richard Zhao,
	Huang Shijie, Dong Aisheng, Matt Porter, Fabio Estevam,
	Javier Martin, kernel, devicetree-discuss

On Mon, Feb 04, 2013 at 12:32:16PM +0100, Philipp Zabel wrote:
> 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.
> 
> The allocation granularity is hard-coded to 32 bytes for now,
> to make the SRAM driver useful for the 6502 remoteproc driver.
> There is overhead for bigger SRAMs, where only a much coarser
> allocation granularity is needed: At 32 bytes minimum allocation
> size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Shawn Guo <shawn.guo@linaro.org>

How exactly is this "generic" if you have randomly hard-coded an
allocation granularity that is larger than half of the in-tree SRAM pool
users today can even support? Did you even bother to look at in-tree SRAM
pool users other than the one you are working on?

There also doesn't seem to be any real reason for the hard-coding either,
this information could easily be fetched via platform data or the device
tree, and the driver in question would simply need to be able to
determine whether the size is suitable for it or not.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-04 15:53   ` Paul Mundt
@ 2013-02-04 17:02     ` Matt Porter
  2013-02-05  8:57     ` Philipp Zabel
  1 sibling, 0 replies; 12+ messages in thread
From: Matt Porter @ 2013-02-04 17:02 UTC (permalink / raw)
  To: Paul Mundt
  Cc: Philipp Zabel, linux-kernel, Arnd Bergmann, Greg Kroah-Hartman,
	Grant Likely, Rob Herring, Paul Gortmaker, Shawn Guo,
	Richard Zhao, Huang Shijie, Dong Aisheng, Fabio Estevam,
	Javier Martin, kernel, devicetree-discuss

On Tue, Feb 05, 2013 at 12:53:45AM +0900, Paul Mundt wrote:
> On Mon, Feb 04, 2013 at 12:32:16PM +0100, Philipp Zabel wrote:
> > 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.
> > 
> > The allocation granularity is hard-coded to 32 bytes for now,
> > to make the SRAM driver useful for the 6502 remoteproc driver.
> > There is overhead for bigger SRAMs, where only a much coarser
> > allocation granularity is needed: At 32 bytes minimum allocation
> > size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
> 
> How exactly is this "generic" if you have randomly hard-coded an
> allocation granularity that is larger than half of the in-tree SRAM pool
> users today can even support? Did you even bother to look at in-tree SRAM
> pool users other than the one you are working on?
>
> There also doesn't seem to be any real reason for the hard-coding either,
> this information could easily be fetched via platform data or the device
> tree, and the driver in question would simply need to be able to
> determine whether the size is suitable for it or not.

Please see http://thread.gmane.org/gmane.linux.kernel/1377702 for the
original discussion on my patch for configurable allocation granularity.
I believe there was an implied agreement from Grant that it was ok if we
went with a more descriptive name even though it hits the grey area of
not describing hardware.

-Matt

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 3/4] media: coda: use genalloc API
  2013-02-04 11:32 ` [PATCH v8 3/4] media: coda: use genalloc API Philipp Zabel
@ 2013-02-05  8:12   ` javier Martin
  0 siblings, 0 replies; 12+ messages in thread
From: javier Martin @ 2013-02-05  8:12 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, Grant Likely,
	Rob Herring, Paul Gortmaker, Shawn Guo, Richard Zhao,
	Huang Shijie, Dong Aisheng, Matt Porter, Fabio Estevam, kernel,
	devicetree-discuss

Hi Philipp,
thank you for preserving compatibility for platform data.

For the coda sources:

Acked-By: Javier Martin <javier.martin@vista-silicon.com>

On 4 February 2013 12:32, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> This patch depends on "genalloc: add devres support, allow to find
> a managed pool by device", which provides the of_get_named_gen_pool
> and dev_get_gen_pool functions.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v7:
>  - In the platform data case, retrieve gen_pool by device instead of
>    by physical address. The information about the SRAM pool can't be
>    provided via memory resource, so add a platform data struct that
>    contains a pointer to the SRAM device.
>  - Add device tree binding documentation.
> ---
>  Documentation/devicetree/bindings/media/coda.txt |   30 +++++++++++++++
>  drivers/media/platform/Kconfig                   |    1 -
>  drivers/media/platform/coda.c                    |   45 +++++++++++++++-------
>  include/linux/platform_data/coda.h               |   18 +++++++++
>  4 files changed, 79 insertions(+), 15 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/coda.txt
>  create mode 100644 include/linux/platform_data/coda.h
>
> diff --git a/Documentation/devicetree/bindings/media/coda.txt b/Documentation/devicetree/bindings/media/coda.txt
> new file mode 100644
> index 0000000..2865d04
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/coda.txt
> @@ -0,0 +1,30 @@
> +Chips&Media Coda multi-standard codec IP
> +========================================
> +
> +Coda codec IPs are present in i.MX SoCs in various versions,
> +called VPU (Video Processing Unit).
> +
> +Required properties:
> +- compatible : should be "fsl,<chip>-src" for i.MX SoCs:
> +  (a) "fsl,imx27-vpu" for CodaDx6 present in i.MX27
> +  (b) "fsl,imx53-vpu" for CODA7541 present in i.MX53
> +  (c) "fsl,imx6q-vpu" for CODA960 present in i.MX6q
> +- reg: should be register base and length as documented in the
> +  SoC reference manual
> +- interrupts : Should contain the VPU interrupt. For CODA960,
> +  a second interrupt is needed for the MJPEG unit.
> +- clocks : Should contain the ahb and per clocks, in the order
> +  determined by the clock-names property.
> +- clock-names : Should be "ahb", "per"
> +- iram : phandle pointing to the SRAM device node
> +
> +Example:
> +
> +vpu: vpu@63ff4000 {
> +       compatible = "fsl,imx53-vpu";
> +       reg = <0x63ff4000 0x1000>;
> +       interrupts = <9>;
> +       clocks = <&clks 63>, <&clks 63>;
> +       clock-names = "ahb", "per";
> +       iram = <&ocram>;
> +};
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 05d7b63..bbf83c1 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -145,7 +145,6 @@ config VIDEO_CODA
>         depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC
>         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 20827ba..b931c2a 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>
> @@ -23,7 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
>  #include <linux/of.h>
> -#include <linux/platform_data/imx-iram.h>
> +#include <linux/platform_data/coda.h>
>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> @@ -43,6 +44,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 +130,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;
> @@ -1926,6 +1931,9 @@ static int coda_probe(struct platform_device *pdev)
>         const struct of_device_id *of_id =
>                         of_match_device(of_match_ptr(coda_dt_ids), &pdev->dev);
>         const struct platform_device_id *pdev_id;
> +       struct coda_platform_data *pdata = pdev->dev.platform_data;
> +       struct device_node *np = pdev->dev.of_node;
> +       struct gen_pool *pool;
>         struct coda_dev *dev;
>         struct resource *res;
>         int ret, irq;
> @@ -1988,6 +1996,16 @@ static int coda_probe(struct platform_device *pdev)
>                 return -ENOENT;
>         }
>
> +       /* Get IRAM pool from device tree or platform data */
> +       pool = of_get_named_gen_pool(np, "iram", 0);
> +       if (!pool && pdata)
> +               pool = dev_get_gen_pool(pdata->iram_dev);
> +       if (!pool) {
> +               dev_err(&pdev->dev, "iram pool not available\n");
> +               return -ENOMEM;
> +       }
> +       dev->iram_pool = pool;
> +
>         ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>         if (ret)
>                 return ret;
> @@ -2022,18 +2040,17 @@ static int 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);
>
> @@ -2050,8 +2067,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);
> diff --git a/include/linux/platform_data/coda.h b/include/linux/platform_data/coda.h
> new file mode 100644
> index 0000000..6ad4410
> --- /dev/null
> +++ b/include/linux/platform_data/coda.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright (C) 2013 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.
> + */
> +#ifndef PLATFORM_CODA_H
> +#define PLATFORM_CODA_H
> +
> +struct device;
> +
> +struct coda_platform_data {
> +       struct device *iram_dev;
> +};
> +
> +#endif
> --
> 1.7.10.4
>



-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-04 15:53   ` Paul Mundt
  2013-02-04 17:02     ` Matt Porter
@ 2013-02-05  8:57     ` Philipp Zabel
  1 sibling, 0 replies; 12+ messages in thread
From: Philipp Zabel @ 2013-02-05  8:57 UTC (permalink / raw)
  To: Paul Mundt
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, Grant Likely,
	Rob Herring, Paul Gortmaker, Shawn Guo, Richard Zhao,
	Huang Shijie, Dong Aisheng, Matt Porter, Fabio Estevam,
	Javier Martin, kernel, devicetree-discuss

Am Dienstag, den 05.02.2013, 00:53 +0900 schrieb Paul Mundt:
> On Mon, Feb 04, 2013 at 12:32:16PM +0100, Philipp Zabel wrote:
> > 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.
> > 
> > The allocation granularity is hard-coded to 32 bytes for now,
> > to make the SRAM driver useful for the 6502 remoteproc driver.
> > There is overhead for bigger SRAMs, where only a much coarser
> > allocation granularity is needed: At 32 bytes minimum allocation
> > size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
> 
> How exactly is this "generic" if you have randomly hard-coded an
> allocation granularity that is larger than half of the in-tree SRAM pool
> users today can even support? Did you even bother to look at in-tree SRAM
> pool users other than the one you are working on?

I agree it should be configurable. As stated in the thread pointed out
by Matt, I'd just like to separate the discussion about the device tree
bindings for the allocation granularity from this patchset.

About the "generic" labeling, I had a look around for users of
gen_pool_create() - I admit to overlooking arch/sh/mm/sram.c, and most
of the others (except arch/arm/kernel/tcm.c drivers/acpi/apei/ghes.c,
both of which I didn't consider potential users of the sram driver) seem
to use a granularity significantly larger than 32 bytes.

> There also doesn't seem to be any real reason for the hard-coding either,
> this information could easily be fetched via platform data or the device
> tree,

True. Although the device tree is supposed to describe the hardware. The
allocation granularity is an implementation detail of the gen_pool
allocator (or possibly kind of a hardware detail of the gen_pool user,
if there are alignment requirements). That is why I didn't want to press
on with the device tree granularity configuration part yet.

> and the driver in question would simply need to be able to
> determine whether the size is suitable for it or not.

Sounds sensible to me. I'd like to follow up with Matt's patch and
something like this in a second step.

regards
Philipp


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-04 11:32 ` [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver Philipp Zabel
  2013-02-04 15:53   ` Paul Mundt
@ 2013-02-08 20:16   ` Grant Likely
  2013-02-11 18:15     ` Philipp Zabel
  1 sibling, 1 reply; 12+ messages in thread
From: Grant Likely @ 2013-02-08 20:16 UTC (permalink / raw)
  To: Philipp Zabel, linux-kernel
  Cc: Arnd Bergmann, Greg Kroah-Hartman, Rob Herring, Paul Gortmaker,
	Shawn Guo, Richard Zhao, Huang Shijie, Dong Aisheng, Matt Porter,
	Fabio Estevam, Javier Martin, kernel, devicetree-discuss,
	Philipp Zabel

On Mon,  4 Feb 2013 12:32:16 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 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.
> 
> The allocation granularity is hard-coded to 32 bytes for now,
> to make the SRAM driver useful for the 6502 remoteproc driver.
> There is overhead for bigger SRAMs, where only a much coarser
> allocation granularity is needed: At 32 bytes minimum allocation
> size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes since v7:
>  - Removed obsolete __devinit/__devexit/__devexit_p
> ---
>  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

I'm a little concerned that 'sram' is just too generic for a compatible
value and we may end up needing a blacklist of systems where the sram
device should not be driven with this driver. If you can think of
a more descriptive name here then I would use it.

However, I'm not worried about it enough to nak it and the rest of the
series looks fine.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-08 20:16   ` Grant Likely
@ 2013-02-11 18:15     ` Philipp Zabel
  2013-02-12 18:09       ` Grant Likely
  0 siblings, 1 reply; 12+ messages in thread
From: Philipp Zabel @ 2013-02-11 18:15 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss

Hi Grant,

Am Freitag, den 08.02.2013, 20:16 +0000 schrieb Grant Likely:
> On Mon,  4 Feb 2013 12:32:16 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 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.
> > 
> > The allocation granularity is hard-coded to 32 bytes for now,
> > to make the SRAM driver useful for the 6502 remoteproc driver.
> > There is overhead for bigger SRAMs, where only a much coarser
> > allocation granularity is needed: At 32 bytes minimum allocation
> > size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > Changes since v7:
> >  - Removed obsolete __devinit/__devexit/__devexit_p
> > ---
> >  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
> 
> I'm a little concerned that 'sram' is just too generic for a compatible
> value and we may end up needing a blacklist of systems where the sram
> device should not be driven with this driver. If you can think of
> a more descriptive name here then I would use it.

various SoC vendors call this (variations of) "on-chip" or "internal"
SRAM/memory. "on-chip-sram" or "internal-sram" are still plenty generic,
though. How about "mmio-sram", as opposed to an SRAM that needs more
than the simple mmio region handled by this driver?

An alternative would be to use the vendor specific names and grow a
compatible list in the driver ("fsl,ocram", "ti,ocm", ...).

> However, I'm not worried about it enough to nak it and the rest of the
> series looks fine.
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Thank you.

regards
Philipp


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver
  2013-02-11 18:15     ` Philipp Zabel
@ 2013-02-12 18:09       ` Grant Likely
  0 siblings, 0 replies; 12+ messages in thread
From: Grant Likely @ 2013-02-12 18:09 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
	Paul Gortmaker, Shawn Guo, Richard Zhao, Huang Shijie,
	Dong Aisheng, Matt Porter, Fabio Estevam, Javier Martin, kernel,
	devicetree-discuss

On Mon, 11 Feb 2013 19:15:24 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Grant,
> 
> Am Freitag, den 08.02.2013, 20:16 +0000 schrieb Grant Likely:
> > On Mon,  4 Feb 2013 12:32:16 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > 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.
> > > 
> > > The allocation granularity is hard-coded to 32 bytes for now,
> > > to make the SRAM driver useful for the 6502 remoteproc driver.
> > > There is overhead for bigger SRAMs, where only a much coarser
> > > allocation granularity is needed: At 32 bytes minimum allocation
> > > size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
> > > ---
> > > Changes since v7:
> > >  - Removed obsolete __devinit/__devexit/__devexit_p
> > > ---
> > >  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
> > 
> > I'm a little concerned that 'sram' is just too generic for a compatible
> > value and we may end up needing a blacklist of systems where the sram
> > device should not be driven with this driver. If you can think of
> > a more descriptive name here then I would use it.
> 
> various SoC vendors call this (variations of) "on-chip" or "internal"
> SRAM/memory. "on-chip-sram" or "internal-sram" are still plenty generic,
> though. How about "mmio-sram", as opposed to an SRAM that needs more
> than the simple mmio region handled by this driver?
> 
> An alternative would be to use the vendor specific names and grow a
> compatible list in the driver ("fsl,ocram", "ti,ocm", ...).

I'd prefer a hybrid. Be specific with the part that it is on
("fsl,ocram"), but also include "mmio-sram":

compatible = "fsl,ocram", "mmio-sram";

That gives drivers the option of overriding the generic mmio-sram
driver.

g.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-02-12 18:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-04 11:32 [PATCH v8 0/4] Add generic driver for on-chip SRAM Philipp Zabel
2013-02-04 11:32 ` [PATCH v8 1/4] genalloc: add devres support, allow to find a managed pool by device Philipp Zabel
2013-02-04 11:32 ` [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver Philipp Zabel
2013-02-04 15:53   ` Paul Mundt
2013-02-04 17:02     ` Matt Porter
2013-02-05  8:57     ` Philipp Zabel
2013-02-08 20:16   ` Grant Likely
2013-02-11 18:15     ` Philipp Zabel
2013-02-12 18:09       ` Grant Likely
2013-02-04 11:32 ` [PATCH v8 3/4] media: coda: use genalloc API Philipp Zabel
2013-02-05  8:12   ` javier Martin
2013-02-04 11:32 ` [PATCH v8 4/4] ARM: dts: add sram for imx53 and imx6q Philipp Zabel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).