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

[PATCH v5 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>;
			alloc-order = <12>;
 		};

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 v4:
 - Added Matt Porter's patch to configure alloc order, so that the driver
   can be used for TI Davinci. 
 - Removed device tree patches as those should go through the ARM SoC tree.

regards
Philipp

---
 Documentation/devicetree/bindings/misc/sram.txt |   27 +++++
 drivers/misc/Kconfig                            |    9 ++
 drivers/misc/Makefile                           |    1 +
 drivers/misc/sram.c                             |  133 +++++++++++++++++++++++
 include/linux/genalloc.h                        |   14 +++
 include/linux/platform_data/sram.h              |   25 +++++
 lib/genalloc.c                                  |   67 ++++++++++++
 7 files changed, 276 insertions(+)

[PATCH v5 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 ca208a9..d77d240 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

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

[Greg Kroah-Hartman]

On Thu, Oct 18, 2012 at 04:27:30PM +0200, Philipp Zabel 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.
> 
> 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(+)

I need some acks from the owners of this file (Paul and Andrew?) before
I can take this series.

thanks,

greg k-h

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

[Paul Gortmaker]

On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 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.

So, I'm not seeing any added users of the of_get_named_gen_pool,
or the other exported "reverse-lookup" function.  Without that, the
anticipated use case is not clear to me.

Is there an example of some pending driver or similar, that has
a phys addr from an unknown source and needs to know what
pool it may or may not be in?  With the use case, someone might
be able to suggest alternative ways to get what you want done.

It might also be worth cross compiling this for powerpc, since the
header files you implicitly get included varies from one arch to
the next, and there might be some compile fails lurking there.

Thanks,
Paul.
--

>
> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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 ca208a9..d77d240 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Philipp Zabel]

Hi Paul,

Am Freitag, den 26.10.2012, 15:46 -0400 schrieb Paul Gortmaker: 
> On Thu, Oct 18, 2012 at 10:27 AM, 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.
> 
> So, I'm not seeing any added users of the of_get_named_gen_pool,
> or the other exported "reverse-lookup" function.  Without that, the
> anticipated use case is not clear to me.

My use case is the coda video codec driver, for a video codec IP core
that is integrated in various SoCs. It can use on-SoC SRAM as temporary
memory.

Other possible use cases are the TI Davinci sound driver.
Or the PXA frame buffer driver could allocate a frame buffer in SRAM for
low-resolution devices.

> Is there an example of some pending driver or similar, that has
> a phys addr from an unknown source and needs to know what
> pool it may or may not be in?  With the use case, someone might
> be able to suggest alternative ways to get what you want done.

drivers/media/platform/coda.c right now uses imx specific
iram_alloc/free wrappers around gen_pool_alloc/free.
I'd like to use of_get_named_gen_pool to obtain the struct gen_pool
pointer and use gen_pool_alloc/free directly, instead.

sound/soc/davinci/davinci-pcm.c right now uses davinci specific
sram_alloc/free wrappers around gen_pool_alloc/free.

drivers/dma/mmp_tdma.c and sound/soc/pxa/mmp-pcm.c already use
gen_pool_alloc/free directly, but they use a arch-mmp specific
sram_get_gpool function to obtain the struct gen_pool pointer.

> It might also be worth cross compiling this for powerpc, since the
> header files you implicitly get included varies from one arch to
> the next, and there might be some compile fails lurking there.

Thanks, I'll do that.

regards
Philipp

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

[Paul Gortmaker]

[-- Attachment #1.1: Type: text/plain, Size: 2739 bytes --]

On Thu, Nov 15, 2012 at 8:25 AM, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>wrote:

> Hi Paul,
>
> Am Freitag, den 26.10.2012, 15:46 -0400 schrieb Paul Gortmaker:
> > On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 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.
> >
> > So, I'm not seeing any added users of the of_get_named_gen_pool,
> > or the other exported "reverse-lookup" function.  Without that, the
> > anticipated use case is not clear to me.
>
> My use case is the coda video codec driver, for a video codec IP core
> that is integrated in various SoCs. It can use on-SoC SRAM as temporary

memory.
>

Thanks Philipp,

So perhaps it makes sense to somehow tie this patch to the user
patch, since from what I recall reading in the past, the infrastructure
additions that are immediately used are far more favourably rec'd
than those that are weakly bound to a hand-wavey possibly vapour
ware like use case that may or may not materialize going forward.

Paul.
--



>
> Other possible use cases are the TI Davinci sound driver.
> Or the PXA frame buffer driver could allocate a frame buffer in SRAM for
> low-resolution devices.
>
> > Is there an example of some pending driver or similar, that has
> > a phys addr from an unknown source and needs to know what
> > pool it may or may not be in?  With the use case, someone might
> > be able to suggest alternative ways to get what you want done.
>
> drivers/media/platform/coda.c right now uses imx specific
> iram_alloc/free wrappers around gen_pool_alloc/free.
> I'd like to use of_get_named_gen_pool to obtain the struct gen_pool
> pointer and use gen_pool_alloc/free directly, instead.
>
> sound/soc/davinci/davinci-pcm.c right now uses davinci specific
> sram_alloc/free wrappers around gen_pool_alloc/free.
>
> drivers/dma/mmp_tdma.c and sound/soc/pxa/mmp-pcm.c already use
> gen_pool_alloc/free directly, but they use a arch-mmp specific
> sram_get_gpool function to obtain the struct gen_pool pointer.
>
> > It might also be worth cross compiling this for powerpc, since the
> > header files you implicitly get included varies from one arch to
> > the next, and there might be some compile fails lurking there.
>
> Thanks, I'll do that.
>
> regards
> Philipp
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[-- Attachment #1.2: Type: text/html, Size: 3983 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH v5 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.

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                             |  111 +++++++++++++++++++++++
 4 files changed, 138 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..c5bbd00 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 ARM 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..7a363f2
--- /dev/null
+++ b/drivers/misc/sram.c
@@ -0,0 +1,111 @@
+/*
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/genalloc.h>
+
+struct sram_dev {
+	struct gen_pool *pool;
+};
+
+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->pool = gen_pool_create(PAGE_SHIFT, -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);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id sram_dt_ids[] = {
+	{ .compatible = "sram" },
+	{ /* sentinel */ }
+};
+#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);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Philipp Zabel, Pengutronix");
+MODULE_DESCRIPTION("Generic SRAM allocator driver");
-- 
1.7.10.4

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

[Paul Gortmaker]

On Thu, Oct 18, 2012 at 10:27 AM, 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.
>
> 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                             |  111 +++++++++++++++++++++++
>  4 files changed, 138 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..c5bbd00 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"

If it is bool, then why bother with module.h and all the
MODULE_AUTHOR and similar stuff?

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

Probably no need to call out ARM specifically here.  I'm pretty sure
quite a few of the powerpc parts have SRAM too.

> +
>  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..7a363f2
> --- /dev/null
> +++ b/drivers/misc/sram.c
> @@ -0,0 +1,111 @@
> +/*
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/genalloc.h>
> +
> +struct sram_dev {
> +       struct gen_pool *pool;
> +};

Assuming you don't ever intend adding more to the struct, then:

static struct gen_pool *sram_pool;

instead?  And you'll lose the needless indirections in the
remaining code below, and the kzalloc too (which you
currently leak, btw).  I'd also delete the "/* sentinel */"
comment too, since it is self evident. (The disease has
spread by copying from other drivers I see).

P.
--

> +
> +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->pool = gen_pool_create(PAGE_SHIFT, -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);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id sram_dt_ids[] = {
> +       { .compatible = "sram" },
> +       { /* sentinel */ }
> +};
> +#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);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> +MODULE_DESCRIPTION("Generic SRAM allocator driver");
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Philipp Zabel]

Hi Paul,

thank you for your comments.

Am Freitag, den 26.10.2012, 12:07 -0400 schrieb Paul Gortmaker:
> On Thu, Oct 18, 2012 at 10:27 AM, 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.
> >
> > 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                             |  111 +++++++++++++++++++++++
> >  4 files changed, 138 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..c5bbd00 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"
> 
> If it is bool, then why bother with module.h and all the
> MODULE_AUTHOR and similar stuff?

Yes. This driver was a module before I noticed that I then would have to
keep track in genalloc to avoid module unloading while some SRAM is
allocated. It seemed a bit too invasive.

> > +       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 ARM SoCs.
> 
> Probably no need to call out ARM specifically here.  I'm pretty sure
> quite a few of the powerpc parts have SRAM too.

Sure, I'll just s/ARM // then.

> > +
> >  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..7a363f2
> > --- /dev/null
> > +++ b/drivers/misc/sram.c
> > @@ -0,0 +1,111 @@
> > +/*
> > + * 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/module.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/genalloc.h>
> > +
> > +struct sram_dev {
> > +       struct gen_pool *pool;
> > +};
> 
> Assuming you don't ever intend adding more to the struct, then:

See the clock patch. I'm not sure if further functionality will be
needed on any architecture.

> static struct gen_pool *sram_pool;
> 
> instead?

No, a global variable would mean that only one instance of the SRAM
driver can be used. What about chips that have separate SRAM regions?

> And you'll lose the needless indirections in the
> remaining code below, and the kzalloc too (which you
> currently leak, btw).

Memory allocated by devm_kzalloc should be freed by the devres
framework. If I am missing something, could you please elaborate?

> I'd also delete the "/* sentinel */"
> comment too, since it is self evident. (The disease has
> spread by copying from other drivers I see).

I can do that.

> P.
> --
> 
> > +
> > +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->pool = gen_pool_create(PAGE_SHIFT, -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);
> > +
> > +       return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static struct of_device_id sram_dt_ids[] = {
> > +       { .compatible = "sram" },
> > +       { /* sentinel */ }
> > +};
> > +#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);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> > +MODULE_DESCRIPTION("Generic SRAM allocator driver");
> > --
> > 1.7.10.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

regards
Philipp

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

[Paul Gortmaker]

On 12-10-29 08:20 AM, Philipp Zabel wrote:
> Hi Paul,
> 
> thank you for your comments.
> 
> Am Freitag, den 26.10.2012, 12:07 -0400 schrieb Paul Gortmaker:
>> On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 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.
>>>
>>> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> Reviewed-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>  Documentation/devicetree/bindings/misc/sram.txt |   17 ++++
>>>  drivers/misc/Kconfig                            |    9 ++
>>>  drivers/misc/Makefile                           |    1 +
>>>  drivers/misc/sram.c                             |  111 +++++++++++++++++++++++
>>>  4 files changed, 138 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..c5bbd00 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"
>>
>> If it is bool, then why bother with module.h and all the
>> MODULE_AUTHOR and similar stuff?
> 
> Yes. This driver was a module before I noticed that I then would have to
> keep track in genalloc to avoid module unloading while some SRAM is
> allocated. It seemed a bit too invasive.
> 
>>> +       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 ARM SoCs.
>>
>> Probably no need to call out ARM specifically here.  I'm pretty sure
>> quite a few of the powerpc parts have SRAM too.
> 
> Sure, I'll just s/ARM // then.
> 
>>> +
>>>  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..7a363f2
>>> --- /dev/null
>>> +++ b/drivers/misc/sram.c
>>> @@ -0,0 +1,111 @@
>>> +/*
>>> + * 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/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/spinlock.h>
>>> +#include <linux/genalloc.h>
>>> +
>>> +struct sram_dev {
>>> +       struct gen_pool *pool;
>>> +};
>>
>> Assuming you don't ever intend adding more to the struct, then:
> 
> See the clock patch. I'm not sure if further functionality will be
> needed on any architecture.

Yes, the clock patch will make the above struct less lonely, once
it gets folded back into this patch.

> 
>> static struct gen_pool *sram_pool;
>>
>> instead?
> 
> No, a global variable would mean that only one instance of the SRAM
> driver can be used. What about chips that have separate SRAM regions?

True; moot point now that the clock patch gets folded in and
adds to the struct anyways...

> 
>> And you'll lose the needless indirections in the
>> remaining code below, and the kzalloc too (which you
>> currently leak, btw).
> 
> Memory allocated by devm_kzalloc should be freed by the devres
> framework. If I am missing something, could you please elaborate?

Right; my mind must have missed the devm_ prefix on the
kzalloc in my quick scan of it.

Paul.
--

> 
>> I'd also delete the "/* sentinel */"
>> comment too, since it is self evident. (The disease has
>> spread by copying from other drivers I see).
> 
> I can do that.
> 
>> P.
>> --
>>
>>> +
>>> +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->pool = gen_pool_create(PAGE_SHIFT, -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);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_OF
>>> +static struct of_device_id sram_dt_ids[] = {
>>> +       { .compatible = "sram" },
>>> +       { /* sentinel */ }
>>> +};
>>> +#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);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
>>> +MODULE_DESCRIPTION("Generic SRAM allocator driver");
>>> --
>>> 1.7.10.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
> 
> regards
> Philipp
> 

[PATCH v5 3/4] misc: sram: Add optional clock

[Philipp Zabel]

On some platforms the SRAM needs a clock to be enabled explicitly.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/misc/sram.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 7a363f2..0cc2e75 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -21,6 +21,8 @@
 #include <linux/kernel.h>
 #include <linux/module.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>
@@ -29,6 +31,7 @@
 
 struct sram_dev {
 	struct gen_pool *pool;
+	struct clk *clk;
 };
 
 static int __devinit sram_probe(struct platform_device *pdev)
@@ -53,6 +56,10 @@ static int __devinit sram_probe(struct platform_device *pdev)
 	if (!sram)
 		return -ENOMEM;
 
+	sram->clk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(sram->clk))
+		clk_prepare_enable(sram->clk);
+
 	sram->pool = gen_pool_create(PAGE_SHIFT, -1);
 	if (!sram->pool)
 		return -ENOMEM;
@@ -80,6 +87,9 @@ static int __devexit sram_remove(struct platform_device *pdev)
 
 	gen_pool_destroy(sram->pool);
 
+	if (!IS_ERR(sram->clk))
+		clk_disable_unprepare(sram->clk);
+
 	return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH v5 3/4] misc: sram: Add optional clock

[Paul Gortmaker]

On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On some platforms the SRAM needs a clock to be enabled explicitly.

Since this is a file that you've just created in the previous commit,
I don't see why this needs to exist as a standalone commit, vs just
being folded back into the previous commit?

P.
--

>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/misc/sram.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index 7a363f2..0cc2e75 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -21,6 +21,8 @@
>  #include <linux/kernel.h>
>  #include <linux/module.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>
> @@ -29,6 +31,7 @@
>
>  struct sram_dev {
>         struct gen_pool *pool;
> +       struct clk *clk;
>  };
>
>  static int __devinit sram_probe(struct platform_device *pdev)
> @@ -53,6 +56,10 @@ static int __devinit sram_probe(struct platform_device *pdev)
>         if (!sram)
>                 return -ENOMEM;
>
> +       sram->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (!IS_ERR(sram->clk))
> +               clk_prepare_enable(sram->clk);
> +
>         sram->pool = gen_pool_create(PAGE_SHIFT, -1);
>         if (!sram->pool)
>                 return -ENOMEM;
> @@ -80,6 +87,9 @@ static int __devexit sram_remove(struct platform_device *pdev)
>
>         gen_pool_destroy(sram->pool);
>
> +       if (!IS_ERR(sram->clk))
> +               clk_disable_unprepare(sram->clk);
> +
>         return 0;
>  }
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/4] misc: sram: Add optional clock

[Philipp Zabel]

Am Freitag, den 26.10.2012, 11:18 -0400 schrieb Paul Gortmaker:
> On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On some platforms the SRAM needs a clock to be enabled explicitly.
> 
> Since this is a file that you've just created in the previous commit,
> I don't see why this needs to exist as a standalone commit, vs just
> being folded back into the previous commit?

you are right, I'll do that.

thanks
Philipp

Re: [PATCH v5 3/4] misc: sram: Add optional clock

[Paul Gortmaker]

On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On some platforms the SRAM needs a clock to be enabled explicitly.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/misc/sram.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index 7a363f2..0cc2e75 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -21,6 +21,8 @@
>  #include <linux/kernel.h>
>  #include <linux/module.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>
> @@ -29,6 +31,7 @@
>
>  struct sram_dev {
>         struct gen_pool *pool;
> +       struct clk *clk;
>  };

I see another field gets added to the struct here.  (yet another
reason to have it folded into the original)   But you still
really don't need to create a sram_dev for this, because...

>
>  static int __devinit sram_probe(struct platform_device *pdev)
> @@ -53,6 +56,10 @@ static int __devinit sram_probe(struct platform_device *pdev)
>         if (!sram)
>                 return -ENOMEM;
>
> +       sram->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (!IS_ERR(sram->clk))
> +               clk_prepare_enable(sram->clk);
> +
>         sram->pool = gen_pool_create(PAGE_SHIFT, -1);
>         if (!sram->pool)
>                 return -ENOMEM;
> @@ -80,6 +87,9 @@ static int __devexit sram_remove(struct platform_device *pdev)
>
>         gen_pool_destroy(sram->pool);
>
> +       if (!IS_ERR(sram->clk))
> +               clk_disable_unprepare(sram->clk);
> +

...here, this looks confusing with the use of IS_ERR on
an entity that was not recently assigned to.  Instead, just
put a "struct clk *clk;" on the stack and do the

   clk = devm_clk_get(&pdev->dev, NULL);

in both the init and the teardown.  Then the code will be
more readable.

P.
--

>         return 0;
>  }
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/4] misc: sram: Add optional clock

[Philipp Zabel]

Am Freitag, den 26.10.2012, 12:17 -0400 schrieb Paul Gortmaker:
> On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On some platforms the SRAM needs a clock to be enabled explicitly.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/misc/sram.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > index 7a363f2..0cc2e75 100644
> > --- a/drivers/misc/sram.c
> > +++ b/drivers/misc/sram.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.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>
> > @@ -29,6 +31,7 @@
> >
> >  struct sram_dev {
> >         struct gen_pool *pool;
> > +       struct clk *clk;
> >  };
> 
> I see another field gets added to the struct here.  (yet another
> reason to have it folded into the original)   But you still
> really don't need to create a sram_dev for this, because...
> 
> >
> >  static int __devinit sram_probe(struct platform_device *pdev)
> > @@ -53,6 +56,10 @@ static int __devinit sram_probe(struct platform_device *pdev)
> >         if (!sram)
> >                 return -ENOMEM;
> >
> > +       sram->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (!IS_ERR(sram->clk))
> > +               clk_prepare_enable(sram->clk);
> > +
> >         sram->pool = gen_pool_create(PAGE_SHIFT, -1);
> >         if (!sram->pool)
> >                 return -ENOMEM;
> > @@ -80,6 +87,9 @@ static int __devexit sram_remove(struct platform_device *pdev)
> >
> >         gen_pool_destroy(sram->pool);
> >
> > +       if (!IS_ERR(sram->clk))
> > +               clk_disable_unprepare(sram->clk);
> > +
> 
> ...here, this looks confusing with the use of IS_ERR on
> an entity that was not recently assigned to.

Right.
How about I set sram->clk = NULL in sram_probe if devm_clk_get returns
an error value?

> Instead, just
> put a "struct clk *clk;" on the stack and do the
> 
>    clk = devm_clk_get(&pdev->dev, NULL);
> 
> in both the init and the teardown.  Then the code will be
> more readable.

Calling devm_clk_get on the same clock twice seems a bit weird.
I expect that eventually someone will want to disable clocks during
suspend, so I'd prefer to keep the clk pointer around.

regards
Philipp

Re: [PATCH v5 3/4] misc: sram: Add optional clock

[Paul Gortmaker]

On 12-10-29 08:20 AM, Philipp Zabel wrote:
> Am Freitag, den 26.10.2012, 12:17 -0400 schrieb Paul Gortmaker:
>> On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>>> On some platforms the SRAM needs a clock to be enabled explicitly.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> ---
>>>  drivers/misc/sram.c |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>>> index 7a363f2..0cc2e75 100644
>>> --- a/drivers/misc/sram.c
>>> +++ b/drivers/misc/sram.c
>>> @@ -21,6 +21,8 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/module.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>
>>> @@ -29,6 +31,7 @@
>>>
>>>  struct sram_dev {
>>>         struct gen_pool *pool;
>>> +       struct clk *clk;
>>>  };
>>
>> I see another field gets added to the struct here.  (yet another
>> reason to have it folded into the original)   But you still
>> really don't need to create a sram_dev for this, because...
>>
>>>
>>>  static int __devinit sram_probe(struct platform_device *pdev)
>>> @@ -53,6 +56,10 @@ static int __devinit sram_probe(struct platform_device *pdev)
>>>         if (!sram)
>>>                 return -ENOMEM;
>>>
>>> +       sram->clk = devm_clk_get(&pdev->dev, NULL);
>>> +       if (!IS_ERR(sram->clk))
>>> +               clk_prepare_enable(sram->clk);
>>> +
>>>         sram->pool = gen_pool_create(PAGE_SHIFT, -1);
>>>         if (!sram->pool)
>>>                 return -ENOMEM;
>>> @@ -80,6 +87,9 @@ static int __devexit sram_remove(struct platform_device *pdev)
>>>
>>>         gen_pool_destroy(sram->pool);
>>>
>>> +       if (!IS_ERR(sram->clk))
>>> +               clk_disable_unprepare(sram->clk);
>>> +
>>
>> ...here, this looks confusing with the use of IS_ERR on
>> an entity that was not recently assigned to.
> 
> Right.
> How about I set sram->clk = NULL in sram_probe if devm_clk_get returns
> an error value?

Sorry for delayed reply ; was out last week.
And yes, I like that better than the double call as well.

Thanks,
Paul.
--

> 
>> Instead, just
>> put a "struct clk *clk;" on the stack and do the
>>
>>    clk = devm_clk_get(&pdev->dev, NULL);
>>
>> in both the init and the teardown.  Then the code will be
>> more readable.
> 
> Calling devm_clk_get on the same clock twice seems a bit weird.
> I expect that eventually someone will want to disable clocks during
> suspend, so I'd prefer to keep the clk pointer around.
> 
> regards
> Philipp
> 

[PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Philipp Zabel]

From: Matt Porter <mporter@ti.com>

Adds support for setting the genalloc pool's minimum allocation
order via DT or platform data. The allocation order is optional
for both the DT property and platform data case. If it is not
present then the order defaults to PAGE_SHIFT to preserve the
current behavior.

Signed-off-by: Matt Porter <mporter@ti.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 Documentation/devicetree/bindings/misc/sram.txt |   12 ++++++++++-
 drivers/misc/sram.c                             |   14 ++++++++++++-
 include/linux/platform_data/sram.h              |   25 +++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/platform_data/sram.h

diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
index b64136c..b1705ec 100644
--- a/Documentation/devicetree/bindings/misc/sram.txt
+++ b/Documentation/devicetree/bindings/misc/sram.txt
@@ -8,10 +8,20 @@ Required properties:
 
 - reg : SRAM iomem address range
 
-Example:
+Optional properties:
+
+- alloc-order : Minimum allocation order for the SRAM pool
+
+Examples:
+
+sram: sram@5c000000 {
+	compatible = "sram";
+	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
+};
 
 sram: sram@5c000000 {
 	compatible = "sram";
 	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
+	alloc-order = <9>; /* Minimum 512 byte allocation */
 };
 
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 0cc2e75..3a04b77 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -28,6 +28,7 @@
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/genalloc.h>
+#include <linux/platform_data/sram.h>
 
 struct sram_dev {
 	struct gen_pool *pool;
@@ -40,6 +41,7 @@ static int __devinit sram_probe(struct platform_device *pdev)
 	struct sram_dev *sram;
 	struct resource *res;
 	unsigned long size;
+	u32 alloc_order = PAGE_SHIFT;
 	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -60,7 +62,17 @@ static int __devinit sram_probe(struct platform_device *pdev)
 	if (!IS_ERR(sram->clk))
 		clk_prepare_enable(sram->clk);
 
-	sram->pool = gen_pool_create(PAGE_SHIFT, -1);
+	if (pdev->dev.of_node)
+		of_property_read_u32(pdev->dev.of_node,
+				     "alloc-order", &alloc_order);
+	else
+		if (pdev->dev.platform_data) {
+			struct sram_pdata *pdata = pdev->dev.platform_data;
+			if (pdata->alloc_order)
+				alloc_order = pdata->alloc_order;
+		}
+
+	sram->pool = gen_pool_create(alloc_order, -1);
 	if (!sram->pool)
 		return -ENOMEM;
 
diff --git a/include/linux/platform_data/sram.h b/include/linux/platform_data/sram.h
new file mode 100644
index 0000000..e17bdaa
--- /dev/null
+++ b/include/linux/platform_data/sram.h
@@ -0,0 +1,25 @@
+/*
+ * include/linux/platform_data/sram.h
+ *
+ * Platform data for generic sram driver
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _SRAM_H_
+#define _SRAM_H_
+
+struct sram_pdata {
+	unsigned alloc_order;	/* Optional: driver defaults to PAGE_SHIFT */
+};
+
+#endif /* _SRAM_H_ */
-- 
1.7.10.4

Re: [PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Grant Likely]

On Thu, 18 Oct 2012 16:27:33 +0200, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> From: Matt Porter <mporter@ti.com>
> 
> Adds support for setting the genalloc pool's minimum allocation
> order via DT or platform data. The allocation order is optional
> for both the DT property and platform data case. If it is not
> present then the order defaults to PAGE_SHIFT to preserve the
> current behavior.
> 
> Signed-off-by: Matt Porter <mporter@ti.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/misc/sram.txt |   12 ++++++++++-
>  drivers/misc/sram.c                             |   14 ++++++++++++-
>  include/linux/platform_data/sram.h              |   25 +++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 2 deletions(-)
>  create mode 100644 include/linux/platform_data/sram.h
> 
> diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
> index b64136c..b1705ec 100644
> --- a/Documentation/devicetree/bindings/misc/sram.txt
> +++ b/Documentation/devicetree/bindings/misc/sram.txt
> @@ -8,10 +8,20 @@ Required properties:
>  
>  - reg : SRAM iomem address range
>  
> -Example:
> +Optional properties:
> +
> +- alloc-order : Minimum allocation order for the SRAM pool

Looks okay, but I think the property name is confusing. I for one had
no idea what 'order' would be and why it was important. I had to read
the code to figure it out.

It does raise the question though of what is this binding actually
for? Does it reflect a limitation of the SRAM? or of the hardware using
the SRAM? Or is it an optimization? How do you expect to use it?

Assuming it is appropriate to put into the device tree, I'd suggest a
different name. Instead of 'order', how about 'sram-alloc-align' (in
address bits) or 'sram-alloc-min-size' (in bytes).

> @@ -60,7 +62,17 @@ static int __devinit sram_probe(struct platform_device *pdev)
>  	if (!IS_ERR(sram->clk))
>  		clk_prepare_enable(sram->clk);
>  
> -	sram->pool = gen_pool_create(PAGE_SHIFT, -1);
> +	if (pdev->dev.of_node)
> +		of_property_read_u32(pdev->dev.of_node,
> +				     "alloc-order", &alloc_order);
> +	else
> +		if (pdev->dev.platform_data) {
> +			struct sram_pdata *pdata = pdev->dev.platform_data;
> +			if (pdata->alloc_order)
> +				alloc_order = pdata->alloc_order;
> +		}
> +
> +	sram->pool = gen_pool_create(alloc_order, -1);

Do you already have a user for the platform_data use case? If not then
I'd drop it entirely and skip adding the new header file.

g.

Re: [PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Philipp Zabel]

Am Mittwoch, den 14.11.2012, 19:15 +0000 schrieb Grant Likely:
> On Thu, 18 Oct 2012 16:27:33 +0200, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > From: Matt Porter <mporter@ti.com>
> > 
> > Adds support for setting the genalloc pool's minimum allocation
> > order via DT or platform data. The allocation order is optional
> > for both the DT property and platform data case. If it is not
> > present then the order defaults to PAGE_SHIFT to preserve the
> > current behavior.
> > 
> > Signed-off-by: Matt Porter <mporter@ti.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/misc/sram.txt |   12 ++++++++++-
> >  drivers/misc/sram.c                             |   14 ++++++++++++-
> >  include/linux/platform_data/sram.h              |   25 +++++++++++++++++++++++
> >  3 files changed, 49 insertions(+), 2 deletions(-)
> >  create mode 100644 include/linux/platform_data/sram.h
> > 
> > diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
> > index b64136c..b1705ec 100644
> > --- a/Documentation/devicetree/bindings/misc/sram.txt
> > +++ b/Documentation/devicetree/bindings/misc/sram.txt
> > @@ -8,10 +8,20 @@ Required properties:
> >  
> >  - reg : SRAM iomem address range
> >  
> > -Example:
> > +Optional properties:
> > +
> > +- alloc-order : Minimum allocation order for the SRAM pool
> 
> Looks okay, but I think the property name is confusing. I for one had
> no idea what 'order' would be and why it was important. I had to read
> the code to figure it out.
> 
> It does raise the question though of what is this binding actually
> for? Does it reflect a limitation of the SRAM? or of the hardware using
> the SRAM? Or is it an optimization? How do you expect to use it?

If I am not mistaken, it is about the expected use case. A driver
allocating many small buffers would quickly fill small SRAMs if the
allocations were of PAGE_SIZE granularity.

I wonder if a common allocation size (say, 512 bytes instead of
PAGE_SIZE) can be found that every prospective user could be reasonably
happy with?

> Assuming it is appropriate to put into the device tree, I'd suggest a
> different name. Instead of 'order', how about 'sram-alloc-align' (in
> address bits) or 'sram-alloc-min-size' (in bytes).

A size in bytes would be the most obvious to me, although that allows to
enter values that are not a power of two.

regards
Philipp

Re: [PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Grant Likely]

On Thu, 15 Nov 2012 14:11:35 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Mittwoch, den 14.11.2012, 19:15 +0000 schrieb Grant Likely:
> > On Thu, 18 Oct 2012 16:27:33 +0200, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > From: Matt Porter <mporter@ti.com>
> > > 
> > > Adds support for setting the genalloc pool's minimum allocation
> > > order via DT or platform data. The allocation order is optional
> > > for both the DT property and platform data case. If it is not
> > > present then the order defaults to PAGE_SHIFT to preserve the
> > > current behavior.
> > > 
> > > Signed-off-by: Matt Porter <mporter@ti.com>
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/misc/sram.txt |   12 ++++++++++-
> > >  drivers/misc/sram.c                             |   14 ++++++++++++-
> > >  include/linux/platform_data/sram.h              |   25 +++++++++++++++++++++++
> > >  3 files changed, 49 insertions(+), 2 deletions(-)
> > >  create mode 100644 include/linux/platform_data/sram.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
> > > index b64136c..b1705ec 100644
> > > --- a/Documentation/devicetree/bindings/misc/sram.txt
> > > +++ b/Documentation/devicetree/bindings/misc/sram.txt
> > > @@ -8,10 +8,20 @@ Required properties:
> > >  
> > >  - reg : SRAM iomem address range
> > >  
> > > -Example:
> > > +Optional properties:
> > > +
> > > +- alloc-order : Minimum allocation order for the SRAM pool
> > 
> > Looks okay, but I think the property name is confusing. I for one had
> > no idea what 'order' would be and why it was important. I had to read
> > the code to figure it out.
> > 
> > It does raise the question though of what is this binding actually
> > for? Does it reflect a limitation of the SRAM? or of the hardware using
> > the SRAM? Or is it an optimization? How do you expect to use it?
> 
> If I am not mistaken, it is about the expected use case. A driver
> allocating many small buffers would quickly fill small SRAMs if the
> allocations were of PAGE_SIZE granularity.
> 
> I wonder if a common allocation size (say, 512 bytes instead of
> PAGE_SIZE) can be found that every prospective user could be reasonably
> happy with?
> 
> > Assuming it is appropriate to put into the device tree, I'd suggest a
> > different name. Instead of 'order', how about 'sram-alloc-align' (in
> > address bits) or 'sram-alloc-min-size' (in bytes).
> 
> A size in bytes would be the most obvious to me, although that allows to
> enter values that are not a power of two.

That may be just fine. If the driver can only do powers of 2, then it
will just have to round up.

g.

Re: [PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Matt Porter]

On Thu, Nov 15, 2012 at 02:11:35PM +0100, Philipp Zabel wrote:
> Am Mittwoch, den 14.11.2012, 19:15 +0000 schrieb Grant Likely:
> > On Thu, 18 Oct 2012 16:27:33 +0200, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > From: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>
> > > 
> > > Adds support for setting the genalloc pool's minimum allocation
> > > order via DT or platform data. The allocation order is optional
> > > for both the DT property and platform data case. If it is not
> > > present then the order defaults to PAGE_SHIFT to preserve the
> > > current behavior.
> > > 
> > > Signed-off-by: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>
> > > Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > ---
> > >  Documentation/devicetree/bindings/misc/sram.txt |   12 ++++++++++-
> > >  drivers/misc/sram.c                             |   14 ++++++++++++-
> > >  include/linux/platform_data/sram.h              |   25 +++++++++++++++++++++++
> > >  3 files changed, 49 insertions(+), 2 deletions(-)
> > >  create mode 100644 include/linux/platform_data/sram.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
> > > index b64136c..b1705ec 100644
> > > --- a/Documentation/devicetree/bindings/misc/sram.txt
> > > +++ b/Documentation/devicetree/bindings/misc/sram.txt
> > > @@ -8,10 +8,20 @@ Required properties:
> > >  
> > >  - reg : SRAM iomem address range
> > >  
> > > -Example:
> > > +Optional properties:
> > > +
> > > +- alloc-order : Minimum allocation order for the SRAM pool
> > 
> > Looks okay, but I think the property name is confusing. I for one had
> > no idea what 'order' would be and why it was important. I had to read
> > the code to figure it out.
> > 
> > It does raise the question though of what is this binding actually
> > for? Does it reflect a limitation of the SRAM? or of the hardware using
> > the SRAM? Or is it an optimization? How do you expect to use it?
> 
> If I am not mistaken, it is about the expected use case. A driver
> allocating many small buffers would quickly fill small SRAMs if the
> allocations were of PAGE_SIZE granularity.
> 
> I wonder if a common allocation size (say, 512 bytes instead of
> PAGE_SIZE) can be found that every prospective user could be reasonably
> happy with?

Unfortunately, no, 512 bytes doesn't work either. Although it'll meet
the needs of converting davinci to this driver, I have a driver under
development (the 6502 remoteproc) which needs finer grained allocation
of SRAM yet. I suspect people will object if the default is 32 bytes as
that bloats the genalloc bitmap for all users...however that's a size
that would work for me. Another possibility is to not do the
gen_pool_create() at probe time but rather provide an interface for client
drivers where the pool is created with the appropriate client-specific
parameters. This may be a bit messy though when the point of the pool is
to share it amongst several client drivers.

> > Assuming it is appropriate to put into the device tree, I'd suggest a
> > different name. Instead of 'order', how about 'sram-alloc-align' (in
> > address bits) or 'sram-alloc-min-size' (in bytes).
> 
> A size in bytes would be the most obvious to me, although that allows to
> enter values that are not a power of two.

I think the implication is that this isn't even a h/w characteristic of
SRAM and, as such, does not belong in a DT binding (for that reason I
don't mind seeing that it's been dropped in v6). It's unfortunate since
it's otherwise a very clean solution. I sure wish I had a "Software
Tree" I could pass in too. ;)

-Matt

Re: [PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Grant Likely]

On Fri, Nov 16, 2012 at 2:09 PM, Matt Porter <mporter@ti.com> wrote:
> On Thu, Nov 15, 2012 at 02:11:35PM +0100, Philipp Zabel wrote:
>> Am Mittwoch, den 14.11.2012, 19:15 +0000 schrieb Grant Likely:
>> > Assuming it is appropriate to put into the device tree, I'd suggest a
>> > different name. Instead of 'order', how about 'sram-alloc-align' (in
>> > address bits) or 'sram-alloc-min-size' (in bytes).
>>
>> A size in bytes would be the most obvious to me, although that allows to
>> enter values that are not a power of two.
>
> I think the implication is that this isn't even a h/w characteristic of
> SRAM and, as such, does not belong in a DT binding (for that reason I
> don't mind seeing that it's been dropped in v6). It's unfortunate since
> it's otherwise a very clean solution. I sure wish I had a "Software
> Tree" I could pass in too. ;)

It is however in that grey area where which it isn't really a
characteristic of the hardware it has a very strong implied usage. I
do push back on things like this not because they shouldn't be done,
but rather to make sure it is properly thought through before going
ahead.

g.

Re: [PATCH v5 4/4] misc: sram: add support for configurable allocation order

[Philipp Zabel]

Am Freitag, den 16.11.2012, 09:09 -0500 schrieb Matt Porter:
> On Thu, Nov 15, 2012 at 02:11:35PM +0100, Philipp Zabel wrote:
> > Am Mittwoch, den 14.11.2012, 19:15 +0000 schrieb Grant Likely:
> > > On Thu, 18 Oct 2012 16:27:33 +0200, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > From: Matt Porter <mporter@ti.com>
> > > > 
> > > > Adds support for setting the genalloc pool's minimum allocation
> > > > order via DT or platform data. The allocation order is optional
> > > > for both the DT property and platform data case. If it is not
> > > > present then the order defaults to PAGE_SHIFT to preserve the
> > > > current behavior.
> > > > 
> > > > Signed-off-by: Matt Porter <mporter@ti.com>
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >  Documentation/devicetree/bindings/misc/sram.txt |   12 ++++++++++-
> > > >  drivers/misc/sram.c                             |   14 ++++++++++++-
> > > >  include/linux/platform_data/sram.h              |   25 +++++++++++++++++++++++
> > > >  3 files changed, 49 insertions(+), 2 deletions(-)
> > > >  create mode 100644 include/linux/platform_data/sram.h
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
> > > > index b64136c..b1705ec 100644
> > > > --- a/Documentation/devicetree/bindings/misc/sram.txt
> > > > +++ b/Documentation/devicetree/bindings/misc/sram.txt
> > > > @@ -8,10 +8,20 @@ Required properties:
> > > >  
> > > >  - reg : SRAM iomem address range
> > > >  
> > > > -Example:
> > > > +Optional properties:
> > > > +
> > > > +- alloc-order : Minimum allocation order for the SRAM pool
> > > 
> > > Looks okay, but I think the property name is confusing. I for one had
> > > no idea what 'order' would be and why it was important. I had to read
> > > the code to figure it out.
> > > 
> > > It does raise the question though of what is this binding actually
> > > for? Does it reflect a limitation of the SRAM? or of the hardware using
> > > the SRAM? Or is it an optimization? How do you expect to use it?
> > 
> > If I am not mistaken, it is about the expected use case. A driver
> > allocating many small buffers would quickly fill small SRAMs if the
> > allocations were of PAGE_SIZE granularity.
> > 
> > I wonder if a common allocation size (say, 512 bytes instead of
> > PAGE_SIZE) can be found that every prospective user could be reasonably
> > happy with?
> 
> Unfortunately, no, 512 bytes doesn't work either. Although it'll meet
> the needs of converting davinci to this driver, I have a driver under
> development (the 6502 remoteproc) which needs finer grained allocation
> of SRAM yet.

Of course, what was I thinking..

> I suspect people will object if the default is 32 bytes as
> that bloats the genalloc bitmap for all users...however that's a size
> that would work for me. Another possibility is to not do the
> gen_pool_create() at probe time but rather provide an interface for client
> drivers where the pool is created with the appropriate client-specific
> parameters. This may be a bit messy though when the point of the pool is
> to share it amongst several client drivers.

For a 256 KiB SRAM, 32 byte granularity would result in a 1 KiB bitmap.
While not optimal, that doesn't really hurt on i.MX, especially as
currently the only user is the coda driver.

Shall we hardcode the granularity to 32 bytes to give us some time to
ponder the device tree property?

> > > Assuming it is appropriate to put into the device tree, I'd suggest a
> > > different name. Instead of 'order', how about 'sram-alloc-align' (in
> > > address bits) or 'sram-alloc-min-size' (in bytes).
> > 
> > A size in bytes would be the most obvious to me, although that allows to
> > enter values that are not a power of two.
> 
> I think the implication is that this isn't even a h/w characteristic of
> SRAM and, as such, does not belong in a DT binding (for that reason I
> don't mind seeing that it's been dropped in v6). It's unfortunate since
> it's otherwise a very clean solution. I sure wish I had a "Software
> Tree" I could pass in too. ;)

The implication is that we shouldn't be too quick to add this to the
device tree without discussion. Therefore I'd like to split this issue
from the basic SRAM driver.

regards
Philipp