[RFC PATCH v2 0/4] Core device subsystem

[RFC PATCH v2 0/4] Core device subsystem

[Marc Zyngier]

There is a small number of devices that the core kernel needs very
early in the boot process, namely an interrupt controller and a timer,
long before the driver model is up and running.

Most architectures implement this requirement by hardcoding the
initialisation of a "well known" piece of hardware which is standard
enough to work on any platform.

This is very different on the ARM architecture, where platforms have a
variety of interrupt controllers and timers. While the same hardcoding
is possible (and is actually used), it makes it almost impossible to
support several platforms in the same kernel.

Though the device tree is helping greatly to solve this problem, some
platform won't ever be converted to DT, hence the need to have a
mechanism supporting a variety of information source. Early platform
devices having been deemed unsuitable (complexity, abuse of various
subsystems), this subsystem has been designed to provide the very
minimal level of functionality.

The "core device subsystem" offers a class based device/driver
matching model, doesn't rely on any other subsystem, is very (too?)
simple, and support getting information both from DT as well as from
static data provided by the platform. It also gives the opportunity to
define the probing order by offering a sorting hook at run-time.

As for the Linux driver model, the core device subsystem deals mainly
with device and driver objects. It also has the notion of "class" to
designate a group of devices implementing the same functionality, and
a group of drivers to be matched against the above devices
(CORE_DEV_CLASS_TIMER for example).

Tested on RealView PB-11MP (with both dt and non dt setups), and
Samsug SMDKv310 (non-dt only).

[RFC PATCH v2 1/4] dt: expose device resource allocator

[Marc Zyngier]

of_device_alloc() takes care of allocating both a platform
device and its associated resource, as part of the runtime
device-tree to platform_device conversion.

The resource allocation itself is useful on its own, and
would be used by the core driver subsystem.  Create the
of_device_alloc_resources() function, and let of_device_alloc()
use it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/of/platform.c       |   60 +++++++++++++++++++++++++++++-------------
 include/linux/of_platform.h |    2 +
 2 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e75af39..f839f79 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -122,22 +122,16 @@ void of_device_make_bus_id(struct device *dev)
 }
 
 /**
- * of_device_alloc - Allocate and initialize an of_device
- * @np: device node to assign to device
- * @bus_id: Name to assign to the device.  May be null to use default name.
- * @parent: Parent device.
+ * of_device_alloc_resources - Allocate and initialize resources for an
+ *			       of_device
+ * @np: device node to fetch resources from
+ * @num_res: number of allocated resources
  */
-struct platform_device *of_device_alloc(struct device_node *np,
-				  const char *bus_id,
-				  struct device *parent)
+struct resource *of_device_alloc_resources(struct device_node *np,
+					   int *num_res)
 {
-	struct platform_device *dev;
 	int rc, i, num_reg = 0, num_irq;
-	struct resource *res, temp_res;
-
-	dev = platform_device_alloc("", -1);
-	if (!dev)
-		return NULL;
+	struct resource *res = NULL, temp_res;
 
 	/* count the io and irq resources */
 	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
@@ -148,19 +142,47 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	if (num_irq || num_reg) {
 		res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
 		if (!res) {
-			platform_device_put(dev);
+			*num_res = -1;
 			return NULL;
 		}
 
-		dev->num_resources = num_reg + num_irq;
-		dev->resource = res;
-		for (i = 0; i < num_reg; i++, res++) {
-			rc = of_address_to_resource(np, i, res);
+		for (i = 0; i < num_reg; i++) {
+			rc = of_address_to_resource(np, i, &res[i]);
 			WARN_ON(rc);
 		}
-		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
+		WARN_ON(of_irq_to_resource_table(np, &res[i], num_irq) != num_irq);
+	}
+
+	*num_res = num_irq + num_reg;
+	return res;
+}
+
+/**
+ * of_device_alloc - Allocate and initialize an of_device
+ * @np: device node to assign to device
+ * @bus_id: Name to assign to the device.  May be null to use default name.
+ * @parent: Parent device.
+ */
+struct platform_device *of_device_alloc(struct device_node *np,
+				  const char *bus_id,
+				  struct device *parent)
+{
+	struct platform_device *dev;
+	int num_res;
+	struct resource *res;
+
+	dev = platform_device_alloc("", -1);
+	if (!dev)
+		return NULL;
+
+	res = of_device_alloc_resources(np, &num_res);
+	if (num_res == -1) {
+		platform_device_put(dev);
+		return NULL;
 	}
 
+	dev->num_resources = num_res;
+	dev->resource = res;
 	dev->dev.of_node = of_node_get(np);
 #if defined(CONFIG_PPC) || defined(CONFIG_MICROBLAZE)
 	dev->dev.dma_mask = &dev->archdata.dma_mask;
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 5a6f458..773a880 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -77,6 +77,8 @@ struct of_platform_driver
 extern const struct of_device_id of_default_bus_match_table[];
 
 /* Platform drivers register/unregister */
+extern struct resource *of_device_alloc_resources(struct device_node *np,
+						  int *num_res);
 extern struct platform_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
-- 
1.7.0.4

[RFC PATCH v2 1/4] dt: expose device resource allocator

[Grant Likely]

On Fri, Jul 08, 2011 at 09:54:07AM +0100, Marc Zyngier wrote:
> of_device_alloc() takes care of allocating both a platform
> device and its associated resource, as part of the runtime
> device-tree to platform_device conversion.
> 
> The resource allocation itself is useful on its own, and
> would be used by the core driver subsystem.  Create the
> of_device_alloc_resources() function, and let of_device_alloc()
> use it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Hi Marc,

Comments below.

> ---
>  drivers/of/platform.c       |   60 +++++++++++++++++++++++++++++-------------
>  include/linux/of_platform.h |    2 +
>  2 files changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index e75af39..f839f79 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -122,22 +122,16 @@ void of_device_make_bus_id(struct device *dev)
>  }
>  
>  /**
> - * of_device_alloc - Allocate and initialize an of_device
> - * @np: device node to assign to device
> - * @bus_id: Name to assign to the device.  May be null to use default name.
> - * @parent: Parent device.
> + * of_device_alloc_resources - Allocate and initialize resources for an
> + *			       of_device
> + * @np: device node to fetch resources from
> + * @num_res: number of allocated resources
>   */
> -struct platform_device *of_device_alloc(struct device_node *np,
> -				  const char *bus_id,
> -				  struct device *parent)
> +struct resource *of_device_alloc_resources(struct device_node *np,
> +					   int *num_res)
>  {
> -	struct platform_device *dev;
>  	int rc, i, num_reg = 0, num_irq;
> -	struct resource *res, temp_res;
> -
> -	dev = platform_device_alloc("", -1);
> -	if (!dev)
> -		return NULL;
> +	struct resource *res = NULL, temp_res;
>  
>  	/* count the io and irq resources */
>  	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> @@ -148,19 +142,47 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  	if (num_irq || num_reg) {
>  		res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
>  		if (!res) {
> -			platform_device_put(dev);
> +			*num_res = -1;

I would argue that it is not a good idea to modify num_res in the
failure case.  Though I man not be fond of the pattern, the common
convention in the kernel is to use ERR_PTR when a function needs to
differentiate between a failure case and simply "no data".

>  			return NULL;
>  		}
>  
> -		dev->num_resources = num_reg + num_irq;
> -		dev->resource = res;
> -		for (i = 0; i < num_reg; i++, res++) {
> -			rc = of_address_to_resource(np, i, res);
> +		for (i = 0; i < num_reg; i++) {
> +			rc = of_address_to_resource(np, i, &res[i]);

Nit: the patch would be less invasive if the 'res' pointer handling
was left alone.  Just keep a copy of the starting position to use for
returning.

>  			WARN_ON(rc);
>  		}
> -		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> +		WARN_ON(of_irq_to_resource_table(np, &res[i], num_irq) != num_irq);
> +	}
> +
> +	*num_res = num_irq + num_reg;
> +	return res;
> +}
> +
> +/**
> + * of_device_alloc - Allocate and initialize an of_device
> + * @np: device node to assign to device
> + * @bus_id: Name to assign to the device.  May be null to use default name.
> + * @parent: Parent device.
> + */
> +struct platform_device *of_device_alloc(struct device_node *np,
> +				  const char *bus_id,
> +				  struct device *parent)
> +{
> +	struct platform_device *dev;
> +	int num_res;
> +	struct resource *res;
> +
> +	dev = platform_device_alloc("", -1);
> +	if (!dev)
> +		return NULL;
> +
> +	res = of_device_alloc_resources(np, &num_res);
> +	if (num_res == -1) {
> +		platform_device_put(dev);
> +		return NULL;
>  	}
>  
> +	dev->num_resources = num_res;
> +	dev->resource = res;
>  	dev->dev.of_node = of_node_get(np);
>  #if defined(CONFIG_PPC) || defined(CONFIG_MICROBLAZE)
>  	dev->dev.dma_mask = &dev->archdata.dma_mask;
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 5a6f458..773a880 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -77,6 +77,8 @@ struct of_platform_driver
>  extern const struct of_device_id of_default_bus_match_table[];
>  
>  /* Platform drivers register/unregister */
> +extern struct resource *of_device_alloc_resources(struct device_node *np,
> +						  int *num_res);
>  extern struct platform_device *of_device_alloc(struct device_node *np,
>  					 const char *bus_id,
>  					 struct device *parent);
> -- 
> 1.7.0.4
> 
> 

[RFC PATCH v2 2/4] Core device subsystem implementation

[Marc Zyngier]

There is a small number of devices that the core kernel needs very
early in the boot process, namely an interrupt controller and a timer,
long before the device model is up and running.

The "core device subsystem" offers a class based device/driver
matching model, doesn't rely on any other subsystem, is very (too?)
simple, and support getting information both from DT as well as from
static data provided by the platform. It also gives the opportunity to
define the probing order by offering a sorting hook at runtime.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kernel/vmlinux.lds.S     |    1 +
 drivers/base/Makefile             |    3 +-
 drivers/base/core_device.c        |  108 +++++++++++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h |    6 ++
 include/linux/core_device.h       |   69 +++++++++++++++++++++++
 5 files changed, 186 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/core_device.c
 create mode 100644 include/linux/core_device.h

diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index e5287f2..ae33310 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -70,6 +70,7 @@ SECTIONS
 
 		INIT_SETUP(16)
 
+		INIT_CORE_DRV
 		INIT_CALLS
 		CON_INITCALL
 		SECURITY_INITCALL
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..79476f0 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -3,7 +3,8 @@
 obj-y			:= core.o sys.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
-			   attribute_container.o transport_class.o
+			   attribute_container.o transport_class.o \
+			   core_device.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
 obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
diff --git a/drivers/base/core_device.c b/drivers/base/core_device.c
new file mode 100644
index 0000000..9262145
--- /dev/null
+++ b/drivers/base/core_device.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2011 ARM Ltd
+ * Written by Marc Zyngier <marc.zyngier@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Core device init support
+ */
+#include <linux/core_device.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+static struct list_head anchors[CORE_DEV_CLASS_MAX] __initdata = {
+	[CORE_DEV_CLASS_IRQ]   = LIST_HEAD_INIT(anchors[CORE_DEV_CLASS_IRQ]),
+	[CORE_DEV_CLASS_TIMER] = LIST_HEAD_INIT(anchors[CORE_DEV_CLASS_TIMER]),
+};
+
+static int __init core_device_match(struct core_device *dev,
+				    struct core_device_id *ids)
+{
+	int i;
+#ifdef CONFIG_OF
+	if (dev->of_node)
+		for (i = 0; ids[i].name != NULL; i++)
+			if (of_device_is_compatible(dev->of_node,
+						    ids[i].name))
+				return 1;
+#endif
+	for (i = 0; ids[i].name != NULL; i++)
+		if (!strcmp(dev->name, ids[i].name))
+			return 1;
+
+	return 0;
+}
+
+void __init core_device_register(enum core_device_class class,
+			  struct core_device *dev)
+{
+	list_add_tail(&dev->entry, &anchors[class]);
+}
+
+extern const struct core_driver_setup_block __core_driver_block_start[],
+					    __core_driver_block_end[];
+
+void __init core_driver_init_class(enum core_device_class class,
+				   void (*sort)(struct list_head *))
+{
+	const struct core_driver_setup_block *b;
+	struct core_device *dev, *tmp;
+	int err;
+
+	if (sort)
+		sort(&anchors[class]);
+
+	list_for_each_entry_safe(dev, tmp, &anchors[class], entry) {
+		pr_debug("core: matching device %s(%d)\n", dev->name, class);
+		for (b = __core_driver_block_start;
+		     b < __core_driver_block_end;
+		     b++) {
+			if (b->class != class ||
+			    !core_device_match(dev, b->drv->ids))
+				continue;
+			
+			pr_debug("core: matched device %s\n", dev->name);
+			err = b->drv->init(dev);
+			if (err)
+				pr_warning("core: %s init failed\n", dev->name);
+		}
+
+		list_del(&dev->entry);
+#ifdef CONFIG_OF
+		if (dev->of_node) { /* These are dynamically allocated */
+			kfree(dev->resource);
+			kfree(dev);
+		}
+#endif
+	}
+}
+
+#ifdef CONFIG_OF
+void __init of_core_device_populate(enum core_device_class class,
+			     struct of_device_id *matches)
+{
+	struct device_node *np;
+	struct core_device *dev;
+	int num_res;
+
+	for_each_matching_node(np, matches) {
+		pr_debug("core: registering OF device %s\n", np->full_name);
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev)
+			return;
+		dev->name = np->name;
+		dev->of_node = np;
+
+		dev->resource = of_device_alloc_resources(np, &num_res);
+		if (num_res < 0) {
+			kfree(dev);
+			return;
+		}
+		dev->num_resources = num_res;
+
+		core_device_register(class, dev);
+	}
+}
+#endif
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index db22d13..2b1590a 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -616,6 +616,11 @@
 		*(.init.setup)						\
 		VMLINUX_SYMBOL(__setup_end) = .;
 
+#define INIT_CORE_DRV							\
+		VMLINUX_SYMBOL(__core_driver_block_start) = .;		\
+		*(.init.core_driver)					\
+		VMLINUX_SYMBOL(__core_driver_block_end) = .;
+
 #define INITCALLS							\
 	*(.initcallearly.init)						\
 	VMLINUX_SYMBOL(__early_initcall_end) = .;			\
@@ -797,6 +802,7 @@
 	.init.data : AT(ADDR(.init.data) - LOAD_OFFSET) {		\
 		INIT_DATA						\
 		INIT_SETUP(initsetup_align)				\
+		INIT_CORE_DRV						\
 		INIT_CALLS						\
 		CON_INITCALL						\
 		SECURITY_INITCALL					\
diff --git a/include/linux/core_device.h b/include/linux/core_device.h
new file mode 100644
index 0000000..ca67e5e
--- /dev/null
+++ b/include/linux/core_device.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2011 ARM Ltd
+ * Written by Marc Zyngier <marc.zyngier@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Core device init support
+ */
+
+#ifndef _CORE_DEVICE_H
+#define _CORE_DEVICE_H
+
+#include <linux/list.h>
+#include <linux/ioport.h>
+#include <linux/of.h>
+
+struct core_device_id {
+	const char		*name;
+};
+
+enum core_device_class {
+	CORE_DEV_CLASS_IRQ,
+	CORE_DEV_CLASS_TIMER,
+	CORE_DEV_CLASS_MAX		/* Do not use as a class */
+};
+
+struct core_device {
+	const char		*name;
+	u32			num_resources;
+	struct resource		*resource;
+#ifdef CONFIG_OF
+	struct device_node	*of_node;
+#endif
+	struct list_head	entry;
+};
+
+struct core_driver {
+	int			(*init)(struct core_device *);
+	struct core_device_id	*ids;
+};
+
+void core_device_register(enum core_device_class class,
+			  struct core_device *dev);
+void core_driver_init_class(enum core_device_class class,
+			    void (*sort)(struct list_head *));
+#ifdef CONFIG_OF
+void of_core_device_populate(enum core_device_class class,
+			     struct of_device_id *matches);
+#else
+static inline void of_core_device_populate(enum core_device_class class,
+					   struct of_device_id *matches)
+{
+}
+#endif
+
+struct core_driver_setup_block {
+	enum core_device_class	class;
+	struct core_driver	*drv;
+};
+
+#define core_driver_register(cls, drv)					\
+static struct core_driver_setup_block __core_driver_block_##cls##_##drv	\
+	__used __section(.init.core_driver)				\
+	__attribute__((aligned((sizeof(long)))))			\
+	= { cls, &drv }
+
+#endif
-- 
1.7.0.4

[RFC PATCH v2 2/4] Core device subsystem implementation

[Michał Mirosław]

2011/7/8 Marc Zyngier <marc.zyngier@arm.com>:
> There is a small number of devices that the core kernel needs very
> early in the boot process, namely an interrupt controller and a timer,
> long before the device model is up and running.
>
> The "core device subsystem" offers a class based device/driver
> matching model, doesn't rely on any other subsystem, is very (too?)
> simple, and support getting information both from DT as well as from
> static data provided by the platform. It also gives the opportunity to
> define the probing order by offering a sorting hook at runtime.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
[...]
> --- /dev/null
> +++ b/drivers/base/core_device.c
> @@ -0,0 +1,108 @@
[...]
> +static int __init core_device_match(struct core_device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct core_device_id *ids)
> +{
> + ? ? ? int i;
> +#ifdef CONFIG_OF
> + ? ? ? if (dev->of_node)
> + ? ? ? ? ? ? ? for (i = 0; ids[i].name != NULL; i++)
> + ? ? ? ? ? ? ? ? ? ? ? if (of_device_is_compatible(dev->of_node,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ids[i].name))
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;

Add an else here? I assume DT devices shouldn't be matched by name.

> +#endif
> + ? ? ? for (i = 0; ids[i].name != NULL; i++)
> + ? ? ? ? ? ? ? if (!strcmp(dev->name, ids[i].name))
> + ? ? ? ? ? ? ? ? ? ? ? return 1;
> +
> + ? ? ? return 0;
> +}
> +
[...]
> --- /dev/null
> +++ b/include/linux/core_device.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright (C) 2011 ARM Ltd
> + * Written by Marc Zyngier <marc.zyngier@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Core device init support
> + */
> +
> +#ifndef _CORE_DEVICE_H
> +#define _CORE_DEVICE_H
> +
> +#include <linux/list.h>
> +#include <linux/ioport.h>
> +#include <linux/of.h>
> +
> +struct core_device_id {
> + ? ? ? const char ? ? ? ? ? ? ?*name;
> +};
> +
> +enum core_device_class {
> + ? ? ? CORE_DEV_CLASS_IRQ,
> + ? ? ? CORE_DEV_CLASS_TIMER,
> + ? ? ? CORE_DEV_CLASS_MAX ? ? ? ? ? ? ?/* Do not use as a class */
> +};

CORE_DEV_CLASS_MAX -> CORE_DEV_CLASS_COUNT

_MAX suggests that this is the largest value, but in this case it is a count.

> +
> +struct core_device {
> + ? ? ? const char ? ? ? ? ? ? ?*name;
> + ? ? ? u32 ? ? ? ? ? ? ? ? ? ? num_resources;
> + ? ? ? struct resource ? ? ? ? *resource;
> +#ifdef CONFIG_OF
> + ? ? ? struct device_node ? ? ?*of_node;
> +#endif
> + ? ? ? struct list_head ? ? ? ?entry;
> +};
> +
> +struct core_driver {
> + ? ? ? int ? ? ? ? ? ? ? ? ? ? (*init)(struct core_device *);
> + ? ? ? struct core_device_id ? *ids;
> +};
> +
> +void core_device_register(enum core_device_class class,
> + ? ? ? ? ? ? ? ? ? ? ? ? struct core_device *dev);
> +void core_driver_init_class(enum core_device_class class,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? void (*sort)(struct list_head *));
> +#ifdef CONFIG_OF
> +void of_core_device_populate(enum core_device_class class,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct of_device_id *matches);
> +#else
> +static inline void of_core_device_populate(enum core_device_class class,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct of_device_id *matches)
> +{
> +}
> +#endif
> +
> +struct core_driver_setup_block {
> + ? ? ? enum core_device_class ?class;
> + ? ? ? struct core_driver ? ? ?*drv;
> +};
> +
> +#define core_driver_register(cls, drv) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> +static struct core_driver_setup_block __core_driver_block_##cls##_##drv ? ? ? ?\
> + ? ? ? __used __section(.init.core_driver) ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? __attribute__((aligned((sizeof(long))))) ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? = { cls, &drv }
> +
> +#endif

Since core_driver_register() is not a function it shouldn't look like
one. Something like DEFINE_CORE_DRIVER_ENTRY() would be better. I
would make cls to be only the last word of CORE_DEV_CLASS_* values so
its less typing in the board files (unless you don't like CPP string
concatenation).

+#define DEFINE_CORE_DRIVER_ENTRY(cls, drv)                               \
+static struct core_driver_setup_block __core_driver_block_##cls##_##drv  \
+       __used __section(.init.core_driver)                               \
+       = { CORE_DEV_CLASS_##cls, &drv }

Best Regards,
Micha? Miros?aw

[RFC PATCH v2 2/4] Core device subsystem implementation

[Marc Zyngier]

On 08/07/11 11:18, Micha? Miros?aw wrote:
> 2011/7/8 Marc Zyngier <marc.zyngier@arm.com>:
>> There is a small number of devices that the core kernel needs very
>> early in the boot process, namely an interrupt controller and a timer,
>> long before the device model is up and running.
>>
>> The "core device subsystem" offers a class based device/driver
>> matching model, doesn't rely on any other subsystem, is very (too?)
>> simple, and support getting information both from DT as well as from
>> static data provided by the platform. It also gives the opportunity to
>> define the probing order by offering a sorting hook at runtime.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> [...]
>> --- /dev/null
>> +++ b/drivers/base/core_device.c
>> @@ -0,0 +1,108 @@
> [...]
>> +static int __init core_device_match(struct core_device *dev,
>> +                                   struct core_device_id *ids)
>> +{
>> +       int i;
>> +#ifdef CONFIG_OF
>> +       if (dev->of_node)
>> +               for (i = 0; ids[i].name != NULL; i++)
>> +                       if (of_device_is_compatible(dev->of_node,
>> +                                                   ids[i].name))
>> +                               return 1;
> 
> Add an else here? I assume DT devices shouldn't be matched by name.

Good point. I'll update that.

>> +#endif
>> +       for (i = 0; ids[i].name != NULL; i++)
>> +               if (!strcmp(dev->name, ids[i].name))
>> +                       return 1;
>> +
>> +       return 0;
>> +}
>> +
> [...]
>> --- /dev/null
>> +++ b/include/linux/core_device.h
>> @@ -0,0 +1,69 @@
>> +/*
>> + * Copyright (C) 2011 ARM Ltd
>> + * Written by Marc Zyngier <marc.zyngier@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Core device init support
>> + */
>> +
>> +#ifndef _CORE_DEVICE_H
>> +#define _CORE_DEVICE_H
>> +
>> +#include <linux/list.h>
>> +#include <linux/ioport.h>
>> +#include <linux/of.h>
>> +
>> +struct core_device_id {
>> +       const char              *name;
>> +};
>> +
>> +enum core_device_class {
>> +       CORE_DEV_CLASS_IRQ,
>> +       CORE_DEV_CLASS_TIMER,
>> +       CORE_DEV_CLASS_MAX              /* Do not use as a class */
>> +};
> 
> CORE_DEV_CLASS_MAX -> CORE_DEV_CLASS_COUNT
> 
> _MAX suggests that this is the largest value, but in this case it is a count.

_MAX seem to be the established usage in the kernel (I just grep-ed for
"_MAX," in include/linux, and found a number of similar uses, while only
ACPI seem to be using _COUNT).

>> +
>> +struct core_device {
>> +       const char              *name;
>> +       u32                     num_resources;
>> +       struct resource         *resource;
>> +#ifdef CONFIG_OF
>> +       struct device_node      *of_node;
>> +#endif
>> +       struct list_head        entry;
>> +};
>> +
>> +struct core_driver {
>> +       int                     (*init)(struct core_device *);
>> +       struct core_device_id   *ids;
>> +};
>> +
>> +void core_device_register(enum core_device_class class,
>> +                         struct core_device *dev);
>> +void core_driver_init_class(enum core_device_class class,
>> +                           void (*sort)(struct list_head *));
>> +#ifdef CONFIG_OF
>> +void of_core_device_populate(enum core_device_class class,
>> +                            struct of_device_id *matches);
>> +#else
>> +static inline void of_core_device_populate(enum core_device_class class,
>> +                                          struct of_device_id *matches)
>> +{
>> +}
>> +#endif
>> +
>> +struct core_driver_setup_block {
>> +       enum core_device_class  class;
>> +       struct core_driver      *drv;
>> +};
>> +
>> +#define core_driver_register(cls, drv)                                 \
>> +static struct core_driver_setup_block __core_driver_block_##cls##_##drv        \
>> +       __used __section(.init.core_driver)                             \
>> +       __attribute__((aligned((sizeof(long)))))                        \
>> +       = { cls, &drv }
>> +
>> +#endif
> 
> Since core_driver_register() is not a function it shouldn't look like
> one. Something like DEFINE_CORE_DRIVER_ENTRY() would be better. I
> would make cls to be only the last word of CORE_DEV_CLASS_* values so
> its less typing in the board files (unless you don't like CPP string
> concatenation).
> 
> +#define DEFINE_CORE_DRIVER_ENTRY(cls, drv)                               \
> +static struct core_driver_setup_block __core_driver_block_##cls##_##drv  \
> +       __used __section(.init.core_driver)                               \
> +       = { CORE_DEV_CLASS_##cls, &drv }

I fully agree with the "not a function" approach. I'm more cautious with
the "CORE_DEV_CLASS_##cls" bit.  I feel like it's hiding a bit too much
of what's going on, but it's only my own feeling, and I'd be happy to
change it if that's what people prefer.

Thanks for reviewing,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH v2 2/4] Core device subsystem implementation

[Greg KH]

On Fri, Jul 08, 2011 at 09:54:08AM +0100, Marc Zyngier wrote:
> There is a small number of devices that the core kernel needs very
> early in the boot process, namely an interrupt controller and a timer,
> long before the device model is up and running.
> 
> The "core device subsystem" offers a class based device/driver
> matching model, doesn't rely on any other subsystem, is very (too?)
> simple, and support getting information both from DT as well as from
> static data provided by the platform. It also gives the opportunity to
> define the probing order by offering a sorting hook at runtime.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Any reason why you didn't use scripts/get_maintainer.pl to at least
figure out who to send something like this to for review?

Please do so in the future, otherwise it will get overlooked...

greg k-h

[RFC PATCH v2 2/4] Core device subsystem implementation

[Grant Likely]

On Fri, Jul 08, 2011 at 09:54:08AM +0100, Marc Zyngier wrote:
> There is a small number of devices that the core kernel needs very
> early in the boot process, namely an interrupt controller and a timer,
> long before the device model is up and running.
> 
> The "core device subsystem" offers a class based device/driver
> matching model, doesn't rely on any other subsystem, is very (too?)
> simple, and support getting information both from DT as well as from
> static data provided by the platform. It also gives the opportunity to
> define the probing order by offering a sorting hook at runtime.

I know you're trying to support platforms that probably won't be
converted to DT, but is that really worth the effort?  Those platforms
are already supported by the kernel, and so are not dependant on this
infrastructure.  New ARM platform support AFAIK now is required to use
DT.

For the DT case, I'm not convinced this is the right direction to go.
My (admittedly limited) understanding of this patch series is that it
purposefully stratifies different kinds of devices into an explicit
init order, while I think we need *less* stratification of device
initialization, and provide device drivers with the ability to sort
themselves.

That said, I do understand the issues your trying to solve to get a
core timer and the core irq controllers set up really early, without a
lot of code duplication.  I've been thinking about the same problem.
It is a very small set of devices that need to be set up this early.
I've been looking at it from the perspective of what can be done to
get things bootstrapped, but then hand off to real devices in the real
device model without an artificial separation between the two.

Regardless, I'll go through this patch set and give you my feedback,
and I'll try to keep my own biases in check as I do so....

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kernel/vmlinux.lds.S     |    1 +
>  drivers/base/Makefile             |    3 +-
>  drivers/base/core_device.c        |  108 +++++++++++++++++++++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h |    6 ++
>  include/linux/core_device.h       |   69 +++++++++++++++++++++++
>  5 files changed, 186 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/base/core_device.c
>  create mode 100644 include/linux/core_device.h
> 
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index e5287f2..ae33310 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -70,6 +70,7 @@ SECTIONS
>  
>  		INIT_SETUP(16)
>  
> +		INIT_CORE_DRV
>  		INIT_CALLS
>  		CON_INITCALL
>  		SECURITY_INITCALL
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 4c5701c..79476f0 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -3,7 +3,8 @@
>  obj-y			:= core.o sys.o bus.o dd.o syscore.o \
>  			   driver.o class.o platform.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
> -			   attribute_container.o transport_class.o
> +			   attribute_container.o transport_class.o \
> +			   core_device.o

Should this really be hard-enabled?  If a majority of platforms are
not using this, then it should be CONFIG'ed out.

>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-y			+= power/
>  obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
> diff --git a/drivers/base/core_device.c b/drivers/base/core_device.c
> new file mode 100644
> index 0000000..9262145
> --- /dev/null
> +++ b/drivers/base/core_device.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (C) 2011 ARM Ltd
> + * Written by Marc Zyngier <marc.zyngier@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Core device init support
> + */
> +#include <linux/core_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +static struct list_head anchors[CORE_DEV_CLASS_MAX] __initdata = {
> +	[CORE_DEV_CLASS_IRQ]   = LIST_HEAD_INIT(anchors[CORE_DEV_CLASS_IRQ]),
> +	[CORE_DEV_CLASS_TIMER] = LIST_HEAD_INIT(anchors[CORE_DEV_CLASS_TIMER]),
> +};
> +
> +static int __init core_device_match(struct core_device *dev,
> +				    struct core_device_id *ids)
> +{
> +	int i;
> +#ifdef CONFIG_OF
> +	if (dev->of_node)
> +		for (i = 0; ids[i].name != NULL; i++)
> +			if (of_device_is_compatible(dev->of_node,
> +						    ids[i].name))
> +				return 1;
> +#endif
> +	for (i = 0; ids[i].name != NULL; i++)
> +		if (!strcmp(dev->name, ids[i].name))
> +			return 1;
> +
> +	return 0;
> +}
> +
> +void __init core_device_register(enum core_device_class class,
> +			  struct core_device *dev)
> +{
> +	list_add_tail(&dev->entry, &anchors[class]);
> +}
> +
> +extern const struct core_driver_setup_block __core_driver_block_start[],
> +					    __core_driver_block_end[];
> +
> +void __init core_driver_init_class(enum core_device_class class,
> +				   void (*sort)(struct list_head *))
> +{
> +	const struct core_driver_setup_block *b;
> +	struct core_device *dev, *tmp;
> +	int err;
> +
> +	if (sort)
> +		sort(&anchors[class]);
> +
> +	list_for_each_entry_safe(dev, tmp, &anchors[class], entry) {
> +		pr_debug("core: matching device %s(%d)\n", dev->name, class);
> +		for (b = __core_driver_block_start;
> +		     b < __core_driver_block_end;
> +		     b++) {
> +			if (b->class != class ||
> +			    !core_device_match(dev, b->drv->ids))
> +				continue;
> +			
> +			pr_debug("core: matched device %s\n", dev->name);
> +			err = b->drv->init(dev);
> +			if (err)
> +				pr_warning("core: %s init failed\n", dev->name);
> +		}
> +
> +		list_del(&dev->entry);
> +#ifdef CONFIG_OF
> +		if (dev->of_node) { /* These are dynamically allocated */
> +			kfree(dev->resource);
> +			kfree(dev);
> +		}
> +#endif
> +	}
> +}

I think Greg commented on this, but I'll say the same thing.  I don't
think it is a good idea to try and classify core devices.  If it is
needed, there should be only one level of core devices, and they
should have the ability to sort themselves into the correct init
order.

> +
> +#ifdef CONFIG_OF
> +void __init of_core_device_populate(enum core_device_class class,
> +			     struct of_device_id *matches)
> +{
> +	struct device_node *np;
> +	struct core_device *dev;
> +	int num_res;
> +
> +	for_each_matching_node(np, matches) {
> +		pr_debug("core: registering OF device %s\n", np->full_name);
> +		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +		if (!dev)
> +			return;
> +		dev->name = np->name;
> +		dev->of_node = np;
> +
> +		dev->resource = of_device_alloc_resources(np, &num_res);
> +		if (num_res < 0) {
> +			kfree(dev);
> +			return;
> +		}
> +		dev->num_resources = num_res;
> +
> +		core_device_register(class, dev);
> +	}
> +}
> +#endif

I'm concerned here about how a platform decides which devices are
'core' and which are not.  It doesn't make sense to have a
"core-device" compatible value or similar because it doesn't actually
describe the hardware requirements.  It only describes how the current
Linux kernel wants to initialize the world, and that is something very
much subject to change.

Also, for any device nodes picked up by this function, the driver
model would also need to inhibit those same nodes from being picked up
by of_platform_populate(), otherwise it could end up with two drivers
bound to the same device.  How are you thinking about handling it?

g.

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index db22d13..2b1590a 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -616,6 +616,11 @@
>  		*(.init.setup)						\
>  		VMLINUX_SYMBOL(__setup_end) = .;
>  
> +#define INIT_CORE_DRV							\
> +		VMLINUX_SYMBOL(__core_driver_block_start) = .;		\
> +		*(.init.core_driver)					\
> +		VMLINUX_SYMBOL(__core_driver_block_end) = .;
> +
>  #define INITCALLS							\
>  	*(.initcallearly.init)						\
>  	VMLINUX_SYMBOL(__early_initcall_end) = .;			\
> @@ -797,6 +802,7 @@
>  	.init.data : AT(ADDR(.init.data) - LOAD_OFFSET) {		\
>  		INIT_DATA						\
>  		INIT_SETUP(initsetup_align)				\
> +		INIT_CORE_DRV						\
>  		INIT_CALLS						\
>  		CON_INITCALL						\
>  		SECURITY_INITCALL					\
> diff --git a/include/linux/core_device.h b/include/linux/core_device.h
> new file mode 100644
> index 0000000..ca67e5e
> --- /dev/null
> +++ b/include/linux/core_device.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright (C) 2011 ARM Ltd
> + * Written by Marc Zyngier <marc.zyngier@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Core device init support
> + */
> +
> +#ifndef _CORE_DEVICE_H
> +#define _CORE_DEVICE_H
> +
> +#include <linux/list.h>
> +#include <linux/ioport.h>
> +#include <linux/of.h>
> +
> +struct core_device_id {
> +	const char		*name;
> +};
> +
> +enum core_device_class {
> +	CORE_DEV_CLASS_IRQ,
> +	CORE_DEV_CLASS_TIMER,
> +	CORE_DEV_CLASS_MAX		/* Do not use as a class */
> +};
> +
> +struct core_device {
> +	const char		*name;
> +	u32			num_resources;
> +	struct resource		*resource;
> +#ifdef CONFIG_OF
> +	struct device_node	*of_node;
> +#endif
> +	struct list_head	entry;
> +};
> +
> +struct core_driver {
> +	int			(*init)(struct core_device *);
> +	struct core_device_id	*ids;
> +};
> +
> +void core_device_register(enum core_device_class class,
> +			  struct core_device *dev);
> +void core_driver_init_class(enum core_device_class class,
> +			    void (*sort)(struct list_head *));
> +#ifdef CONFIG_OF
> +void of_core_device_populate(enum core_device_class class,
> +			     struct of_device_id *matches);
> +#else
> +static inline void of_core_device_populate(enum core_device_class class,
> +					   struct of_device_id *matches)
> +{
> +}
> +#endif
> +
> +struct core_driver_setup_block {
> +	enum core_device_class	class;
> +	struct core_driver	*drv;
> +};
> +
> +#define core_driver_register(cls, drv)					\
> +static struct core_driver_setup_block __core_driver_block_##cls##_##drv	\
> +	__used __section(.init.core_driver)				\
> +	__attribute__((aligned((sizeof(long)))))			\
> +	= { cls, &drv }
> +
> +#endif
> -- 
> 1.7.0.4
> 
> 

[RFC PATCH v2 3/4] Core devices: add OF interrupt controller sorting method

[Marc Zyngier]

When several interrupt controllers are initialized, it is
necessary to probe them in the order described by their
cascading interrupts (or interrupt-parent in OF parlance).

This patch introduces a method that can be passed to
core_driver_init_class() at runtime and that will
reorder the device list according to the OF properties.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/base/core_device.c  |  109 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/core_device.h |    2 +
 2 files changed, 111 insertions(+), 0 deletions(-)

diff --git a/drivers/base/core_device.c b/drivers/base/core_device.c
index 9262145..a8df59d 100644
--- a/drivers/base/core_device.c
+++ b/drivers/base/core_device.c
@@ -10,7 +10,9 @@
  */
 #include <linux/core_device.h>
 #include <linux/of_platform.h>
+#include <linux/of_irq.h>
 #include <linux/slab.h>
+#include <linux/sort.h>
 
 static struct list_head anchors[CORE_DEV_CLASS_MAX] __initdata = {
 	[CORE_DEV_CLASS_IRQ]   = LIST_HEAD_INIT(anchors[CORE_DEV_CLASS_IRQ]),
@@ -105,4 +107,111 @@ void __init of_core_device_populate(enum core_device_class class,
 		core_device_register(class, dev);
 	}
 }
+
+struct intc_desc {
+	struct core_device	*dev;
+	struct intc_desc	*parent;
+	int			order;
+};
+
+static struct intc_desc * __init irq_find_parent(struct core_device *dev,
+						 struct intc_desc *intcs,
+						 int nr)
+{
+	struct device_node *np;
+	int i;
+
+	if (!dev->of_node)
+		return NULL;
+
+	np = of_irq_find_parent(dev->of_node);
+	if (!np || dev->of_node == np) {
+		pr_debug("%s has no interrupt-parent\n",
+			dev->of_node->full_name);
+		return NULL;
+	}
+
+	of_node_put(np);
+	for (i = 0; i < nr; i++)
+		if (intcs[i].dev->of_node == np) {
+			pr_debug("%s interrupt-parent %s found in probe list\n",
+				 dev->of_node->full_name, np->full_name);
+			return &intcs[i];
+		}
+
+	pr_warning("%s interrupt-parent %s not in probe list\n",
+		   dev->of_node->full_name, np->full_name);
+	return NULL;
+}
+
+static int __init irq_cmp_intc_desc(const void *x1, const void *x2)
+{
+	const struct intc_desc *d1 = x1, *d2 = x2;
+	return d1->order - d2->order;
+}
+
+void __init core_device_irq_sort(struct list_head *head)
+{
+	struct intc_desc *intcs;
+	struct core_device *dev, *tmp;
+	int count = 0, i = 0, inc, max_order = 0;
+
+	if (list_empty(head))
+		return;
+
+	/* Count the number of interrupt controllers */
+	list_for_each_entry(dev, head, entry)
+		count++;
+
+	if (count == 1)
+		return;
+
+	/* Allocate a big enough array */
+	intcs = kmalloc(sizeof(*intcs) * count, GFP_KERNEL);
+	if (!intcs) {
+		pr_err("irq_core_device_sort: allocation failed");
+		return;
+	}
+
+	/* Populate the array */
+	i = 0;
+	list_for_each_entry(dev, head, entry) {
+		intcs[i].dev = dev;
+		intcs[i].parent = NULL;
+		intcs[i].order = 0;
+		i++;
+	}
+
+	/* Find out the interrupt-parents */
+	for (i = 0; i < count; i++)
+		intcs[i].parent = irq_find_parent(intcs[i].dev, intcs, count);
+
+	/* Compute the orders */
+	do {
+		inc = 0;
+		for (i = 0; i < count; i++)
+			if (intcs[i].parent &&
+			    intcs[i].order <= intcs[i].parent->order) {
+				intcs[i].order =  intcs[i].parent->order + 1;
+				if (max_order < intcs[i].order)
+					max_order = intcs[i].order;
+				inc = 1;
+			}
+	} while (inc);
+
+	if (max_order) {
+		/* Sort the array according to the order, if necessary */
+		sort(intcs, count, sizeof(*intcs), irq_cmp_intc_desc, NULL);
+
+		/* Empty the list... */
+		list_for_each_entry_safe(dev, tmp, head, entry)
+			list_del(&dev->entry);
+
+		/* ... and populate the list back, preserving the ordering */
+		for (i = 0; i < count; i++)
+			list_add_tail(&intcs[i].dev->entry, head);
+	}
+	/* Free the array */
+	kfree(intcs);
+}
 #endif
diff --git a/include/linux/core_device.h b/include/linux/core_device.h
index ca67e5e..e632868 100644
--- a/include/linux/core_device.h
+++ b/include/linux/core_device.h
@@ -48,11 +48,13 @@ void core_driver_init_class(enum core_device_class class,
 #ifdef CONFIG_OF
 void of_core_device_populate(enum core_device_class class,
 			     struct of_device_id *matches);
+void core_device_irq_sort(struct list_head *head);
 #else
 static inline void of_core_device_populate(enum core_device_class class,
 					   struct of_device_id *matches)
 {
 }
+#define core_device_irq_sort	NULL
 #endif
 
 struct core_driver_setup_block {
-- 
1.7.0.4

[RFC PATCH v2 3/4] Core devices: add OF interrupt controller sorting method

[Grant Likely]

On Fri, Jul 08, 2011 at 09:54:09AM +0100, Marc Zyngier wrote:
> When several interrupt controllers are initialized, it is
> necessary to probe them in the order described by their
> cascading interrupts (or interrupt-parent in OF parlance).
> 
> This patch introduces a method that can be passed to
> core_driver_init_class() at runtime and that will
> reorder the device list according to the OF properties.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/base/core_device.c  |  109 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/core_device.h |    2 +
>  2 files changed, 111 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/core_device.c b/drivers/base/core_device.c
> index 9262145..a8df59d 100644
> --- a/drivers/base/core_device.c
> +++ b/drivers/base/core_device.c
> @@ -10,7 +10,9 @@
>   */
>  #include <linux/core_device.h>
>  #include <linux/of_platform.h>
> +#include <linux/of_irq.h>
>  #include <linux/slab.h>
> +#include <linux/sort.h>
>  
>  static struct list_head anchors[CORE_DEV_CLASS_MAX] __initdata = {
>  	[CORE_DEV_CLASS_IRQ]   = LIST_HEAD_INIT(anchors[CORE_DEV_CLASS_IRQ]),
> @@ -105,4 +107,111 @@ void __init of_core_device_populate(enum core_device_class class,
>  		core_device_register(class, dev);
>  	}
>  }
> +
> +struct intc_desc {
> +	struct core_device	*dev;
> +	struct intc_desc	*parent;
> +	int			order;
> +};
> +
> +static struct intc_desc * __init irq_find_parent(struct core_device *dev,
> +						 struct intc_desc *intcs,
> +						 int nr)
> +{
> +	struct device_node *np;
> +	int i;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	np = of_irq_find_parent(dev->of_node);
> +	if (!np || dev->of_node == np) {
> +		pr_debug("%s has no interrupt-parent\n",
> +			dev->of_node->full_name);
> +		return NULL;
> +	}
> +
> +	of_node_put(np);
> +	for (i = 0; i < nr; i++)
> +		if (intcs[i].dev->of_node == np) {
> +			pr_debug("%s interrupt-parent %s found in probe list\n",
> +				 dev->of_node->full_name, np->full_name);
> +			return &intcs[i];
> +		}
> +
> +	pr_warning("%s interrupt-parent %s not in probe list\n",
> +		   dev->of_node->full_name, np->full_name);
> +	return NULL;
> +}
> +
> +static int __init irq_cmp_intc_desc(const void *x1, const void *x2)
> +{
> +	const struct intc_desc *d1 = x1, *d2 = x2;
> +	return d1->order - d2->order;
> +}
> +
> +void __init core_device_irq_sort(struct list_head *head)
> +{
> +	struct intc_desc *intcs;
> +	struct core_device *dev, *tmp;
> +	int count = 0, i = 0, inc, max_order = 0;
> +
> +	if (list_empty(head))
> +		return;
> +
> +	/* Count the number of interrupt controllers */
> +	list_for_each_entry(dev, head, entry)
> +		count++;
> +
> +	if (count == 1)
> +		return;

If you change this to 'if (count <= 1)', then I think the list_empty() test above becomes redundant.

> +
> +	/* Allocate a big enough array */
> +	intcs = kmalloc(sizeof(*intcs) * count, GFP_KERNEL);

kzalloc()

> +	if (!intcs) {
> +		pr_err("irq_core_device_sort: allocation failed");
> +		return;
> +	}
> +
> +	/* Populate the array */
> +	i = 0;
> +	list_for_each_entry(dev, head, entry) {
> +		intcs[i].dev = dev;
> +		intcs[i].parent = NULL;
> +		intcs[i].order = 0;

Clearing parent and order become redundant when using kzalloc().

> +		i++;
> +	}
> +
> +	/* Find out the interrupt-parents */
> +	for (i = 0; i < count; i++)
> +		intcs[i].parent = irq_find_parent(intcs[i].dev, intcs, count);
> +
> +	/* Compute the orders */
> +	do {
> +		inc = 0;
> +		for (i = 0; i < count; i++)
> +			if (intcs[i].parent &&
> +			    intcs[i].order <= intcs[i].parent->order) {
> +				intcs[i].order =  intcs[i].parent->order + 1;
> +				if (max_order < intcs[i].order)
> +					max_order = intcs[i].order;
> +				inc = 1;
> +			}
> +	} while (inc);

Can't this look be rolled into the list_for_each_entry() loop?

This is a good start (and is probably pragmatically what should be
done immediately), but there is an added complexity that an irq
controller can actually have multiple parents if it is attached to an
interrupt nexus.

That said, it might just be best to punt that use-case out to a 'real'
device driver than can defer probing until its dependencies are met
(assuming deferred probe gets merged).  Not all interrupt controllers
need to be probed early.

As for the implementation, I think we can do better with fewer loops:

	for (i = 0; i < count; i++) {
		parent = irq_find_parent(intcs[i].dev, intcs, count);
		if (parent) {
			list_add(&intcs[i].list, &parent->child_list);
		} else {
			WARN_ON(root_parent);
			root_parent = &intcs[i];
		}
	}

And then simply walk each intcs' child_list starting at the root_parent.

> diff --git a/include/linux/core_device.h b/include/linux/core_device.h
> index ca67e5e..e632868 100644
> --- a/include/linux/core_device.h
> +++ b/include/linux/core_device.h
> @@ -48,11 +48,13 @@ void core_driver_init_class(enum core_device_class class,
>  #ifdef CONFIG_OF
>  void of_core_device_populate(enum core_device_class class,
>  			     struct of_device_id *matches);
> +void core_device_irq_sort(struct list_head *head);
>  #else
>  static inline void of_core_device_populate(enum core_device_class class,
>  					   struct of_device_id *matches)
>  {
>  }
> +#define core_device_irq_sort	NULL
>  #endif
>  
>  struct core_driver_setup_block {
> -- 
> 1.7.0.4
> 
> 

[RFC PATCH v2 4/4] Core devices: documentation

[Marc Zyngier]

Add the documentation file for core devices.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/core_devices.txt |  247 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 247 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/core_devices.txt

diff --git a/Documentation/core_devices.txt b/Documentation/core_devices.txt
new file mode 100644
index 0000000..5d1581f
--- /dev/null
+++ b/Documentation/core_devices.txt
@@ -0,0 +1,247 @@
+Core Device Subsystem:
+=====================
+
+There is a small number of devices that the core kernel needs very
+early in the boot process, namely an interrupt controller and a timer,
+long before the driver model is up and running.
+
+Most architectures implement this requirement by hardcoding the
+initialisation of a "well known" piece of hardware which is standard
+enough to work on any platform.
+
+This is very different on the ARM architecture, where platforms have a
+variety of interrupt controllers and timers. While the same hardcoding
+is possible (and is actually used), it makes it almost impossible to
+support several platforms in the same kernel.
+
+Though the device tree is helping greatly to solve this problem, some
+platform won't ever be converted to DT, hence the need to have a
+mechanism supporting a variety of information source. Early platform
+devices having been deemed unsuitable (complexity, abuse of various
+subsystems), this subsystem has been designed to provide the very
+minimal level of functionality.
+
+The "core device subsystem" offers a class based device/driver
+matching model, doesn't rely on any other subsystem, is very (too?)
+simple, and support getting information both from DT as well as from
+static data provided by the platform. It also gives the opportunity to
+define the probing order by offering a sorting hook at run-time.
+
+As for the Linux driver model, the core device subsystem deals mainly
+with device and driver objects. It also has the notion of "class" to
+designate a group of devices implementing the same functionality, and
+a group of drivers to be matched against the above devices
+(CORE_DEV_CLASS_TIMER for example).
+
+One of the features is that the whole subsystem is discarded once the
+kernel has booted. No structures can or should be retained after the
+device has been probed. Of course, no support for module or other
+evolved features. Another design feature is that it is *NOT* thread
+safe. If you need any kind of mutual exclusion, you're probably using
+core devices for something they are not designed for.
+
+* Core Device:
+  ===========
+
+The struct core_device is fairly similar to a platform_device.
+From "include/linux/core_device.h":
+
+struct core_device {
+	const char		*name;
+	u32			num_resources;
+	struct resource		*resource;
+	struct device_node	*of_node;
+	struct list_head	entry;
+};
+
+- name: friendly name for the device, will be used to match the driver
+- num_resources: number of resources associated with the device
+- resource: address of the resource array
+- of_node: pointer to the DT node if the device has been populated by
+  parsing the device tree. This is managed internally by the subsystem.
+- entry: internal management list (not to be initialised).
+
+The device is registered with the core device subsystem with:
+void core_device_register(enum core_device_class class,
+			  struct core_device *dev);
+
+where:
+- class is one of CORE_DEV_CLASS_IRQ or CORE_DEV_CLASS_TIMER
+- dev is the core device to be registered.
+
+A typical use is the following:
+static struct resources twd_resources[] __initdata = {
+	{
+		.start	= 0x1f000600,
+		.end	= 0x1f0006ff,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= IRQ_LOCALTIMER,
+		.end	= IRQ_LOCALTIMER,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct core_device twd_device _initdata = {
+	.name		= "arm_smp_twd",
+	.resource	= twd_resources,
+	.num_resources	= ARRAY_SIZE(twd_resources),
+};
+
+static void __init timer_init(void)
+{
+	core_device_register(CORE_DEV_CLASS_TIMER, &twd_device);
+}
+
+Note that all structures are marked as __inidata, as none of them is
+expected to be used after the kernel has booted.
+
+The devices can also be automatically allocated and registered by
+parsing the device tree (if available) with the following function:
+
+void of_core_device_populate(enum core_device_class class,
+			     struct of_device_id *matches);
+
+The allocated core_device structures will have their of_node member
+pointing to the corresponding DT node. Resources will be allocated and
+populated according to attributes found in the device tree.
+
+
+
+* Core driver:
+  ===========
+
+The struct core_driver is the pendant to the core_device.
+
+struct core_driver {
+	int			(*init)(struct core_device *);
+	struct core_device_id	*ids;
+};
+
+- init: initialisation function. Returns 0 on success, error code on
+  failure.
+- ids: a null-terminated array of struct core_device_id against which
+  the device is matched.
+
+struct core_device_id {
+	const char		*name;
+};
+
+- name: string against which the device is matched
+
+core_driver_register(class, driver);
+
+Note that core_driver_register() is *not* a function, but expands to a
+static data structure stored in a discardable section.
+
+A typical use is the following:
+
+static int __init twd_core_init(struct core_device *dev)
+{
+	[...]
+	return 0;
+}
+static struct core_device_id twd_core_ids[] __initdata = {
+	{ .name = "arm,smp-twd", },
+	{ .name	= "arm_smp_twd", },
+	{},
+};
+
+static struct core_driver twd_core_driver __initdata = {
+	.init		= twd_core_init,
+	.ids		= twd_core_ids,
+};
+
+core_driver_register(CORE_DEV_CLASS_TIMER, twd_core_driver);
+
+As for the core_device, all structures should be marked __initdata,
+and the init function should be marked __init. The driver code must
+*not* hold any reference to the core_device, as it can be freed just
+after the init function has returned.
+
+
+
+* Device/Driver matching:
+  ======================
+
+The core kernel code directly controls when devices and drivers are
+matched (no matching-at-register-time) by calling:
+
+void core_driver_init_class(enum core_device_class class,
+			    void (*sort)(struct list_head *));
+
+Where:
+- class is one of CORE_DEV_CLASS_IRQ or CORE_DEV_CLASS_TIMER,
+- sort is a pointer to a function sorting the device list before they
+  are matched (NULL if unused).
+
+When this function is called:
+
+- All devices registered in "class" are probed with the matching
+  registered drivers
+- Once the devices in the class have been tried against the compiled
+  in drivers, they are removed from the list (whether they have
+  actually been probed or not).
+- If core devices have been dynamically allocated (by
+  of_core_device_populate()), they are freed.
+
+For example:
+
+/* List of supported timers */
+static struct of_device_id timer_ids[] __initdata = {
+	{ .compatible = "arm,smp-twd", },
+	{},
+};
+
+static void __init __arm_late_time_init(void)
+{
+	if (arm_late_time_init)
+		arm_late_time_init();
+
+	/* Fetch the supported timers from the device tree */
+	of_core_device_populate(CORE_DEV_CLASS_TIMER, timer_ids);
+	/* Init the devices (both DT based and static), no preliminary sort */
+	core_driver_init_class(CORE_DEV_CLASS_TIMER, NULL);
+}
+
+
+
+* Sorting functions
+  =================
+
+This may well fall into the hack category, and is probably only useful
+when used with the device tree.
+
+Imagine you have a bunch of interrupt controllers to initialise. There
+is probably one controller directly attached to the CPUs, and all the
+others cascading (in)directly into the first one. There is a strong
+requirement that these controllers are initialised in the right order
+(closest to the CPU first).
+
+This is easy enough to achieve when static core devices are registered
+(the registration order is preserved when probing), but is very
+unlikely to occur when devices are imported from the device tree.
+
+The "sort" function that can be passed to core_driver_init_class() is
+used to solve such a problem. It is called just before the devices are
+matched against the drivers, and is allowed to reorganise the list
+completely. It must not drop elements from the list though.
+
+One such sorting function is core_device_irq_sort(), which is designed
+to solve the above problem, and is used like this:
+
+static struct of_device_id of_irq_controller_ids[] __initdata = {
+	{ .compatible   = "arm,gic-spi", },
+	{},
+};
+
+void __init init_IRQ(void)
+{
+	machine_desc->init_irq();
+	of_core_device_populate(CORE_DEV_CLASS_IRQ, of_irq_controller_ids);
+	core_driver_init_class(CORE_DEV_CLASS_IRQ, core_device_irq_sort);
+}
+
+In this snippet, all the "arm,gic-spi" devices are registered, and
+then sorted at initialisation time by core_device_irq_sort().
-- 
1.7.0.4

[RFC PATCH v2 4/4] Core devices: documentation

[Randy Dunlap]

On Fri,  8 Jul 2011 09:54:10 +0100 Marc Zyngier wrote:

> Add the documentation file for core devices.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/core_devices.txt |  247 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 247 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/core_devices.txt
> 
> diff --git a/Documentation/core_devices.txt b/Documentation/core_devices.txt
> new file mode 100644
> index 0000000..5d1581f
> --- /dev/null
> +++ b/Documentation/core_devices.txt
> @@ -0,0 +1,247 @@
> +Core Device Subsystem:
> +=====================
> +
> +There is a small number of devices that the core kernel needs very

   There are

> +early in the boot process, namely an interrupt controller and a timer,
> +long before the driver model is up and running.
> +
> +Most architectures implement this requirement by hardcoding the
> +initialisation of a "well known" piece of hardware which is standard
> +enough to work on any platform.
> +
> +This is very different on the ARM architecture, where platforms have a
> +variety of interrupt controllers and timers. While the same hardcoding
> +is possible (and is actually used), it makes it almost impossible to
> +support several platforms in the same kernel.
> +
> +Though the device tree is helping greatly to solve this problem, some
> +platform won't ever be converted to DT, hence the need to have a
> +mechanism supporting a variety of information source. Early platform

                                                 sources.

> +devices having been deemed unsuitable (complexity, abuse of various
> +subsystems), this subsystem has been designed to provide the very

s/,/;/

> +minimal level of functionality.
> +
> +The "core device subsystem" offers a class based device/driver
> +matching model, doesn't rely on any other subsystem, is very (too?)
> +simple, and support getting information both from DT as well as from

               supports

> +static data provided by the platform. It also gives the opportunity to
> +define the probing order by offering a sorting hook at run-time.
> +
> +As for the Linux driver model, the core device subsystem deals mainly
> +with device and driver objects. It also has the notion of "class" to
> +designate a group of devices implementing the same functionality, and
> +a group of drivers to be matched against the above devices
> +(CORE_DEV_CLASS_TIMER for example).
> +
> +One of the features is that the whole subsystem is discarded once the
> +kernel has booted. No structures can or should be retained after the
> +device has been probed. Of course, no support for module or other

                                      there is no support ...

> +evolved features. Another design feature is that it is *NOT* thread
> +safe. If you need any kind of mutual exclusion, you're probably using
> +core devices for something they are not designed for.
> +
> +* Core Device:
> +  ===========


[snip]


> +* Core driver:
> +  ===========


[snip]


> +* Device/Driver matching:
> +  ======================
> +
> +The core kernel code directly controls when devices and drivers are
> +matched (no matching-at-register-time) by calling:
> +
> +void core_driver_init_class(enum core_device_class class,
> +			    void (*sort)(struct list_head *));
> +
> +Where:
> +- class is one of CORE_DEV_CLASS_IRQ or CORE_DEV_CLASS_TIMER,
> +- sort is a pointer to a function sorting the device list before they
> +  are matched (NULL if unused).
> +

so the sort key ordering is not defined (or is user-defined), right?

> +When this function is called:
> +
> +- All devices registered in "class" are probed with the matching
> +  registered drivers
> +- Once the devices in the class have been tried against the compiled
> +  in drivers, they are removed from the list (whether they have
> +  actually been probed or not).
> +- If core devices have been dynamically allocated (by
> +  of_core_device_populate()), they are freed.
> +
> +For example:

[snip]



---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[RFC PATCH v2 4/4] Core devices: documentation

[Grant Likely]

On Fri, Jul 08, 2011 at 09:54:10AM +0100, Marc Zyngier wrote:
> Add the documentation file for core devices.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/core_devices.txt |  247 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 247 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/core_devices.txt
> 
> diff --git a/Documentation/core_devices.txt b/Documentation/core_devices.txt
> new file mode 100644
> index 0000000..5d1581f
> --- /dev/null
> +++ b/Documentation/core_devices.txt
> @@ -0,0 +1,247 @@
> +Core Device Subsystem:
> +=====================
> +
> +There is a small number of devices that the core kernel needs very
> +early in the boot process, namely an interrupt controller and a timer,
> +long before the driver model is up and running.
> +
> +Most architectures implement this requirement by hardcoding the
> +initialisation of a "well known" piece of hardware which is standard
> +enough to work on any platform.
> +
> +This is very different on the ARM architecture, where platforms have a
> +variety of interrupt controllers and timers. While the same hardcoding
> +is possible (and is actually used), it makes it almost impossible to
> +support several platforms in the same kernel.
> +
> +Though the device tree is helping greatly to solve this problem, some
> +platform won't ever be converted to DT, hence the need to have a
> +mechanism supporting a variety of information source. Early platform
> +devices having been deemed unsuitable (complexity, abuse of various
> +subsystems), this subsystem has been designed to provide the very
> +minimal level of functionality.
> +
> +The "core device subsystem" offers a class based device/driver
> +matching model, doesn't rely on any other subsystem, is very (too?)
> +simple, and support getting information both from DT as well as from
> +static data provided by the platform. It also gives the opportunity to
> +define the probing order by offering a sorting hook at run-time.
> +
> +As for the Linux driver model, the core device subsystem deals mainly
> +with device and driver objects. It also has the notion of "class" to
> +designate a group of devices implementing the same functionality, and
> +a group of drivers to be matched against the above devices
> +(CORE_DEV_CLASS_TIMER for example).
> +
> +One of the features is that the whole subsystem is discarded once the
> +kernel has booted. No structures can or should be retained after the
> +device has been probed. Of course, no support for module or other
> +evolved features. Another design feature is that it is *NOT* thread
> +safe. If you need any kind of mutual exclusion, you're probably using
> +core devices for something they are not designed for.
> +
> +* Core Device:
> +  ===========
> +
> +The struct core_device is fairly similar to a platform_device.
> +From "include/linux/core_device.h":
> +
> +struct core_device {
> +	const char		*name;
> +	u32			num_resources;
> +	struct resource		*resource;
> +	struct device_node	*of_node;
> +	struct list_head	entry;
> +};
> +
> +- name: friendly name for the device, will be used to match the driver
> +- num_resources: number of resources associated with the device
> +- resource: address of the resource array
> +- of_node: pointer to the DT node if the device has been populated by
> +  parsing the device tree. This is managed internally by the subsystem.
> +- entry: internal management list (not to be initialised).

Ignoring the question of "which devices are actually core devices" for
the moment, (which is a question that does need to be answered
regardless), I don't think the 'struct core_device' abstraction, or at
least the "core_device_register()" abstraction is what is needed.
When doing early setup for an SoC family, I better already have a
pretty darn good idea about what devices are "core devices".  We *do*
need a way to hook in setup code for a lot of different hardware in
abstract ways, but something like a device model I think goes to far.

It should be sufficient to have either implicitly or explicitly
registered setup functions and a helper function (lets call it an
early setup agent) that knows how to call them (which is pretty much
what you've got implemented), but instead of a separate
core_device_register() step, SoC code can pass a list of core devices
to the setup agent directly.  Or in the DT case, a match table.

g.

> +
> +The device is registered with the core device subsystem with:
> +void core_device_register(enum core_device_class class,
> +			  struct core_device *dev);
> +
> +where:
> +- class is one of CORE_DEV_CLASS_IRQ or CORE_DEV_CLASS_TIMER
> +- dev is the core device to be registered.
> +
> +A typical use is the following:
> +static struct resources twd_resources[] __initdata = {
> +	{
> +		.start	= 0x1f000600,
> +		.end	= 0x1f0006ff,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	{
> +		.start	= IRQ_LOCALTIMER,
> +		.end	= IRQ_LOCALTIMER,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct core_device twd_device _initdata = {
> +	.name		= "arm_smp_twd",
> +	.resource	= twd_resources,
> +	.num_resources	= ARRAY_SIZE(twd_resources),
> +};
> +
> +static void __init timer_init(void)
> +{
> +	core_device_register(CORE_DEV_CLASS_TIMER, &twd_device);
> +}
> +
> +Note that all structures are marked as __inidata, as none of them is
> +expected to be used after the kernel has booted.
> +
> +The devices can also be automatically allocated and registered by
> +parsing the device tree (if available) with the following function:
> +
> +void of_core_device_populate(enum core_device_class class,
> +			     struct of_device_id *matches);
> +
> +The allocated core_device structures will have their of_node member
> +pointing to the corresponding DT node. Resources will be allocated and
> +populated according to attributes found in the device tree.
> +
> +
> +
> +* Core driver:
> +  ===========
> +
> +The struct core_driver is the pendant to the core_device.
> +
> +struct core_driver {
> +	int			(*init)(struct core_device *);
> +	struct core_device_id	*ids;
> +};
> +
> +- init: initialisation function. Returns 0 on success, error code on
> +  failure.
> +- ids: a null-terminated array of struct core_device_id against which
> +  the device is matched.
> +
> +struct core_device_id {
> +	const char		*name;
> +};
> +
> +- name: string against which the device is matched
> +
> +core_driver_register(class, driver);
> +
> +Note that core_driver_register() is *not* a function, but expands to a
> +static data structure stored in a discardable section.
> +
> +A typical use is the following:
> +
> +static int __init twd_core_init(struct core_device *dev)
> +{
> +	[...]
> +	return 0;
> +}
> +static struct core_device_id twd_core_ids[] __initdata = {
> +	{ .name = "arm,smp-twd", },
> +	{ .name	= "arm_smp_twd", },
> +	{},
> +};
> +
> +static struct core_driver twd_core_driver __initdata = {
> +	.init		= twd_core_init,
> +	.ids		= twd_core_ids,
> +};
> +
> +core_driver_register(CORE_DEV_CLASS_TIMER, twd_core_driver);
> +
> +As for the core_device, all structures should be marked __initdata,
> +and the init function should be marked __init. The driver code must
> +*not* hold any reference to the core_device, as it can be freed just
> +after the init function has returned.
> +
> +
> +
> +* Device/Driver matching:
> +  ======================
> +
> +The core kernel code directly controls when devices and drivers are
> +matched (no matching-at-register-time) by calling:
> +
> +void core_driver_init_class(enum core_device_class class,
> +			    void (*sort)(struct list_head *));
> +
> +Where:
> +- class is one of CORE_DEV_CLASS_IRQ or CORE_DEV_CLASS_TIMER,
> +- sort is a pointer to a function sorting the device list before they
> +  are matched (NULL if unused).
> +
> +When this function is called:
> +
> +- All devices registered in "class" are probed with the matching
> +  registered drivers
> +- Once the devices in the class have been tried against the compiled
> +  in drivers, they are removed from the list (whether they have
> +  actually been probed or not).
> +- If core devices have been dynamically allocated (by
> +  of_core_device_populate()), they are freed.
> +
> +For example:
> +
> +/* List of supported timers */
> +static struct of_device_id timer_ids[] __initdata = {
> +	{ .compatible = "arm,smp-twd", },
> +	{},
> +};
> +
> +static void __init __arm_late_time_init(void)
> +{
> +	if (arm_late_time_init)
> +		arm_late_time_init();
> +
> +	/* Fetch the supported timers from the device tree */
> +	of_core_device_populate(CORE_DEV_CLASS_TIMER, timer_ids);
> +	/* Init the devices (both DT based and static), no preliminary sort */
> +	core_driver_init_class(CORE_DEV_CLASS_TIMER, NULL);
> +}
> +
> +
> +
> +* Sorting functions
> +  =================
> +
> +This may well fall into the hack category, and is probably only useful
> +when used with the device tree.
> +
> +Imagine you have a bunch of interrupt controllers to initialise. There
> +is probably one controller directly attached to the CPUs, and all the
> +others cascading (in)directly into the first one. There is a strong
> +requirement that these controllers are initialised in the right order
> +(closest to the CPU first).
> +
> +This is easy enough to achieve when static core devices are registered
> +(the registration order is preserved when probing), but is very
> +unlikely to occur when devices are imported from the device tree.
> +
> +The "sort" function that can be passed to core_driver_init_class() is
> +used to solve such a problem. It is called just before the devices are
> +matched against the drivers, and is allowed to reorganise the list
> +completely. It must not drop elements from the list though.
> +
> +One such sorting function is core_device_irq_sort(), which is designed
> +to solve the above problem, and is used like this:
> +
> +static struct of_device_id of_irq_controller_ids[] __initdata = {
> +	{ .compatible   = "arm,gic-spi", },
> +	{},
> +};
> +
> +void __init init_IRQ(void)
> +{
> +	machine_desc->init_irq();
> +	of_core_device_populate(CORE_DEV_CLASS_IRQ, of_irq_controller_ids);
> +	core_driver_init_class(CORE_DEV_CLASS_IRQ, core_device_irq_sort);
> +}
> +
> +In this snippet, all the "arm,gic-spi" devices are registered, and
> +then sorted at initialisation time by core_device_irq_sort().
> -- 
> 1.7.0.4
> 
> 

[RFC PATCH v2 0/4] Core device subsystem

[Michał Mirosław]

2011/7/8 Marc Zyngier <marc.zyngier@arm.com>:
> There is a small number of devices that the core kernel needs very
> early in the boot process, namely an interrupt controller and a timer,
> long before the driver model is up and running.
>
> Most architectures implement this requirement by hardcoding the
> initialisation of a "well known" piece of hardware which is standard
> enough to work on any platform.
>
> This is very different on the ARM architecture, where platforms have a
> variety of interrupt controllers and timers. While the same hardcoding
> is possible (and is actually used), it makes it almost impossible to
> support several platforms in the same kernel.
>
> Though the device tree is helping greatly to solve this problem, some
> platform won't ever be converted to DT, hence the need to have a
> mechanism supporting a variety of information source. Early platform
> devices having been deemed unsuitable (complexity, abuse of various
> subsystems), this subsystem has been designed to provide the very
> minimal level of functionality.
>
> The "core device subsystem" offers a class based device/driver
> matching model, doesn't rely on any other subsystem, is very (too?)
> simple, and support getting information both from DT as well as from
> static data provided by the platform. It also gives the opportunity to
> define the probing order by offering a sorting hook at run-time.
[...]

I gave it some thought and it looks like your idea can be modified to
make it even less demanding on machine setup code.

For the DT case, all this can work without adding a single
board-specific code (even machine_desc->init_irq() is not needed).

of_init_core_device_class(class)
{
  int ids_count = 0;
  struct of_device_id *ids, *p;

  for_each_core_device_driver(drv, class)
    for_each_core_device_id(id, drv)
      ++ids_count;

  p = ids = kzalloc(...);

  for_each_core_device_driver(drv, class)
    for_each_core_device_id(id, drv) {
      p->compatible = id->name;
      p->data = drv;
    }

  // generate matching device list from devicetree

  if (class == CORE_DEV_CLASS_IRQ)
    // sort the list (for intc class)

  core_driver_init_class(class);
  // call above could use matched ids' data field for finding
  // driver struct instead of traversing the driver list again
}

For non-DT case, the board code would need to provide the devices with
core_device_register() in init_irq() or wherever is apropriate.

For simplicity I thing mixing static and DT initialization should not
be allowed (for a single machine) - then static/DT initialization
order won't matter.

I hope this is useful for you. :)

Best Regards,
Micha? Miros?aw

[RFC PATCH v2 0/4] Core device subsystem

[Marc Zyngier]

On 08/07/11 12:37, Micha? Miros?aw wrote:
> 2011/7/8 Marc Zyngier <marc.zyngier@arm.com>:
>> There is a small number of devices that the core kernel needs very
>> early in the boot process, namely an interrupt controller and a timer,
>> long before the driver model is up and running.
>>
>> Most architectures implement this requirement by hardcoding the
>> initialisation of a "well known" piece of hardware which is standard
>> enough to work on any platform.
>>
>> This is very different on the ARM architecture, where platforms have a
>> variety of interrupt controllers and timers. While the same hardcoding
>> is possible (and is actually used), it makes it almost impossible to
>> support several platforms in the same kernel.
>>
>> Though the device tree is helping greatly to solve this problem, some
>> platform won't ever be converted to DT, hence the need to have a
>> mechanism supporting a variety of information source. Early platform
>> devices having been deemed unsuitable (complexity, abuse of various
>> subsystems), this subsystem has been designed to provide the very
>> minimal level of functionality.
>>
>> The "core device subsystem" offers a class based device/driver
>> matching model, doesn't rely on any other subsystem, is very (too?)
>> simple, and support getting information both from DT as well as from
>> static data provided by the platform. It also gives the opportunity to
>> define the probing order by offering a sorting hook at run-time.
> [...]
> 
> I gave it some thought and it looks like your idea can be modified to
> make it even less demanding on machine setup code.
> 
> For the DT case, all this can work without adding a single
> board-specific code (even machine_desc->init_irq() is not needed).

machine_desc->init_irq() is only here for legacy purpose.
And this call is already outside of the platform code.

> of_init_core_device_class(class)
> {
>   int ids_count = 0;
>   struct of_device_id *ids, *p;
> 
>   for_each_core_device_driver(drv, class)
>     for_each_core_device_id(id, drv)
>       ++ids_count;
> 
>   p = ids = kzalloc(...);
> 
>   for_each_core_device_driver(drv, class)
>     for_each_core_device_id(id, drv) {
>       p->compatible = id->name;
>       p->data = drv;
>     }
> 
>   // generate matching device list from devicetree
> 
>   if (class == CORE_DEV_CLASS_IRQ)
>     // sort the list (for intc class)
> 
>   core_driver_init_class(class);
>   // call above could use matched ids' data field for finding
>   // driver struct instead of traversing the driver list again
> }

So you're basically folding of_core_device_populate() and
core_driver_init_class() into one call, and generating the
of_device_ids on the fly. If you're going down that road,
it would be even simpler to directly use of_device_ids
instead of core_device_ids and skip the generation altogether.

That would also remove the static declaration of devices to be
probed in the architecture support code...

Let me think of it and prototype that.

> For non-DT case, the board code would need to provide the devices with
> core_device_register() in init_irq() or wherever is apropriate.

That's the way it is supposed to work already
(from my local pb11mp board file):

[...]
#ifndef CONFIG_OF
static struct resource realview_pb11mp_primary_gic_resources[] __initdata = {
[...]
};

static struct core_device realview_pb11mp_primary_gic __initdata = {
	.name		= "arm_gic",
	.resource	= realview_pb11mp_primary_gic_resources,
	.num_resources	= ARRAY_SIZE(realview_pb11mp_primary_gic_resources),
};

static struct resource realview_pb11mp_secondary_gic_resources[] __initdata = {
[...]
};

static struct core_device realview_pb11mp_secondary_gic __initdata = {
	.name		= "arm_gic",
	.resource	= realview_pb11mp_secondary_gic_resources,
	.num_resources	= ARRAY_SIZE(realview_pb11mp_secondary_gic_resources),
};
#endif

static void __init gic_init_irq(void)
{
	unsigned int pldctrl;

	/* new irq mode with no DCC */
	writel(0x0000a05f, __io_address(REALVIEW_SYS_LOCK));
	pldctrl = readl(__io_address(REALVIEW_SYS_BASE)	+ REALVIEW_PB11MP_SYS_PLD_CTRL1);
	pldctrl |= 2 << 22;
	writel(pldctrl, __io_address(REALVIEW_SYS_BASE) + REALVIEW_PB11MP_SYS_PLD_CTRL1);
	writel(0x00000000, __io_address(REALVIEW_SYS_LOCK));
#ifndef CONFIG_OF
	core_device_register(CORE_DEV_CLASS_IRQ, &realview_pb11mp_primary_gic);
	core_device_register(CORE_DEV_CLASS_IRQ, &realview_pb11mp_secondary_gic);
#endif
}


> For simplicity I thing mixing static and DT initialization should not
> be allowed (for a single machine) - then static/DT initialization
> order won't matter.

I don't think I ever thought of supporting such a setup, at least not
within a single class (different classes are definitely supported).

> I hope this is useful for you. :)

It is. Thanks for the comments!

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH v2 0/4] Core device subsystem

[Marc Zyngier]

On 08/07/11 14:08, Marc Zyngier wrote:
> So you're basically folding of_core_device_populate() and
> core_driver_init_class() into one call, and generating the
> of_device_ids on the fly. If you're going down that road,
> it would be even simpler to directly use of_device_ids
> instead of core_device_ids and skip the generation altogether.
> 
> That would also remove the static declaration of devices to be
> probed in the architecture support code...
> 
> Let me think of it and prototype that.

See the attached patch against branch dt_gic_twd from
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git

It boots fine on my PB11MP.

What do you think?

	M.
-- 
Jazz is not dead. It just smells funny...
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: code_dev.patch.txt
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110708/ff104de1/attachment.txt>

[RFC PATCH v2 0/4] Core device subsystem

[Michał Mirosław]

W dniu 8 lipca 2011 17:13 u?ytkownik Marc Zyngier
<marc.zyngier@arm.com> napisa?:
> On 08/07/11 14:08, Marc Zyngier wrote:
>> So you're basically folding of_core_device_populate() and
>> core_driver_init_class() into one call, and generating the
>> of_device_ids on the fly. If you're going down that road,
>> it would be even simpler to directly use of_device_ids
>> instead of core_device_ids and skip the generation altogether.
>>
>> That would also remove the static declaration of devices to be
>> probed in the architecture support code...
>>
>> Let me think of it and prototype that.
>
> See the attached patch against branch dt_gic_twd from
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>
> It boots fine on my PB11MP.
>
> What do you think?
>
> ? ? ? ?M.
> --
> Jazz is not dead. It just smells funny...
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 74825c2..e7d9922 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -712,9 +712,9 @@ static int __init gic_core_init(struct core_device *dev)
> ? ? ? ?return 0;
> ?}
>
> -static struct core_device_id gic_ids[] __initdata = {
> - ? ? ? { .name = "arm,gic-spi" }, /* DT */
> - ? ? ? { .name = "arm_gic" }, ? ? /* Static device */
> +static struct of_device_id gic_ids[] __initdata = {
> + ? ? ? { .compatible = "arm,gic-spi" }, /* DT */
> + ? ? ? { .name = "arm_gic" }, ? ? ? ? ? /* Static device */
> ? ? ? ?{ },
> ?};

This will also match devices with name "arm_gic" in DT. Unlikely, but
probably not what you want.

I thought about something more evil. ;) See below.

Assuming we modify of_find_matching_node() to also return matched ID entry:

---- of_core_device_populate()

const struct of_device_id *id;
struct device_node *dn;
struct core_device *dev;

for_each_matching_node_id(dn, matches, id) {
  dev = create_core_dev(...);
  dev->init = id->data;
  add_to_list(class, dev);
}

if (intc class)
  sort_list()

----

And then drivers would register like this:

int __init etc_init(struct core_device *);

DEFINE_CORE_DEVICE_TABLE_DT(class, name) = {
  { .compatible = "etc", .data = etc_init },
};

DEFINE_CORE_DEVICE_TABLE_LEGACY(class, name) = {
  { .name = "etc", .data = etc_init },
};


This removes the need for struct core_driver and instead uses linker
to directly generate match tables for INTC and TIMER classes. This
also allows to get rid of legacy IDs when kernel is built with support
for boards not using DT.

Best Regards,
Micha? Miros?aw

[RFC PATCH v2 0/4] Core device subsystem

[Michał Mirosław]

W dniu 8 lipca 2011 20:13 u?ytkownik Micha? Miros?aw <mirqus@gmail.com> napisa?:
> W dniu 8 lipca 2011 17:13 u?ytkownik Marc Zyngier
> <marc.zyngier@arm.com> napisa?:
>> On 08/07/11 14:08, Marc Zyngier wrote:
>>> So you're basically folding of_core_device_populate() and
>>> core_driver_init_class() into one call, and generating the
>>> of_device_ids on the fly. If you're going down that road,
>>> it would be even simpler to directly use of_device_ids
>>> instead of core_device_ids and skip the generation altogether.
>>>
>>> That would also remove the static declaration of devices to be
>>> probed in the architecture support code...
>>>
>>> Let me think of it and prototype that.
>>
>> See the attached patch against branch dt_gic_twd from
>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>
>> It boots fine on my PB11MP.
>>
>> What do you think?
[...]
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -712,9 +712,9 @@ static int __init gic_core_init(struct core_device *dev)
>> ? ? ? ?return 0;
>> ?}
>>
>> -static struct core_device_id gic_ids[] __initdata = {
>> - ? ? ? { .name = "arm,gic-spi" }, /* DT */
>> - ? ? ? { .name = "arm_gic" }, ? ? /* Static device */
>> +static struct of_device_id gic_ids[] __initdata = {
>> + ? ? ? { .compatible = "arm,gic-spi" }, /* DT */
>> + ? ? ? { .name = "arm_gic" }, ? ? ? ? ? /* Static device */
>> ? ? ? ?{ },
>> ?};
>
> This will also match devices with name "arm_gic" in DT. Unlikely, but
> probably not what you want.
>
> I thought about something more evil. ;) See below.
>
> Assuming we modify of_find_matching_node() to also return matched ID entry:
>
> ---- of_core_device_populate()
>
> const struct of_device_id *id;
> struct device_node *dn;
> struct core_device *dev;
>
> for_each_matching_node_id(dn, matches, id) {
> ?dev = create_core_dev(...);
> ?dev->init = id->data;
> ?add_to_list(class, dev);
> }
>
> if (intc class)
> ?sort_list()
>
> ----
>
> And then drivers would register like this:
>
> int __init etc_init(struct core_device *);
>
> DEFINE_CORE_DEVICE_TABLE_DT(class, name) = {
> ?{ .compatible = "etc", .data = etc_init },
> };
>
> DEFINE_CORE_DEVICE_TABLE_LEGACY(class, name) = {
> ?{ .name = "etc", .data = etc_init },
> };
>
>
> This removes the need for struct core_driver and instead uses linker
> to directly generate match tables for INTC and TIMER classes. This
> also allows to get rid of legacy IDs when kernel is built with support
> for boards not using DT.

DEFINE_CORE_DEVICE_TABLE_* could be implemented like this:

#define __DEFINE_CORE_DEVICE_TABLE(type,cls,name) \
  static const struct of_device_id __coredev_##type##_##name##_##cls[] \
    __used __section(.init.coredev_##type##_##cls)

#define DEFINE_CORE_DEVICE_TABLE_DT_IRQ(name) \
  __DEFINE_CORE_DEVICE_TABLE(dt, irq, name)
#define DEFINE_CORE_DEVICE_TABLE_LEGACY_IRQ(name) \
  __DEFINE_CORE_DEVICE_TABLE(legacy, irq, name)
#define DEFINE_CORE_DEVICE_TABLE_DT_TIMER(name) \
  __DEFINE_CORE_DEVICE_TABLE(dt, timer, name)
#define DEFINE_CORE_DEVICE_TABLE_LEGACY_TIMER(name) \
  __DEFINE_CORE_DEVICE_TABLE(legacy, timer, name)

(I folded 'class' parameter into the define's name but that might not
be the best idea if the class set is to be expanded - compiler error
for invalid classes can be generated also in other ways.)

Best Regards,
Micha? Miros?aw

[RFC PATCH v2 0/4] Core device subsystem

[Greg KH]

On Fri, Jul 08, 2011 at 09:54:06AM +0100, Marc Zyngier wrote:
> There is a small number of devices that the core kernel needs very
> early in the boot process, namely an interrupt controller and a timer,
> long before the driver model is up and running.

What in the "driver model" needs to be up and running early for you to
use it?

Why not just start it up sooner if it's so needed?

Are you really just needing your bus to start up sooner?  Or your
"platform" bus?

> Most architectures implement this requirement by hardcoding the
> initialisation of a "well known" piece of hardware which is standard
> enough to work on any platform.
> 
> This is very different on the ARM architecture, where platforms have a
> variety of interrupt controllers and timers. While the same hardcoding
> is possible (and is actually used), it makes it almost impossible to
> support several platforms in the same kernel.
> 
> Though the device tree is helping greatly to solve this problem, some
> platform won't ever be converted to DT, hence the need to have a
> mechanism supporting a variety of information source.

So because you are not willing to convert to DT, we must support
yet-another-one-off type of thing instead?  That's a pretty poor reason.

Why can't you just move the platform to DT instead, which would solve
this for you?  That seems much easier than adding this core code.

> Early platform
> devices having been deemed unsuitable (complexity, abuse of various
> subsystems), this subsystem has been designed to provide the very
> minimal level of functionality.
> 
> The "core device subsystem" offers a class based device/driver
> matching model, doesn't rely on any other subsystem, is very (too?)
> simple, and support getting information both from DT as well as from
> static data provided by the platform. It also gives the opportunity to
> define the probing order by offering a sorting hook at run-time.

Again, just use DT and all would be fine, right?

> As for the Linux driver model, the core device subsystem deals mainly
> with device and driver objects. It also has the notion of "class" to
> designate a group of devices implementing the same functionality, and
> a group of drivers to be matched against the above devices
> (CORE_DEV_CLASS_TIMER for example).

Classes are going away, never create a new one, so please don't create
new code that emulates it.

I'm still not sold as to why this is needed.

greg k-h