[PATCH 0/3] GIC OF bindings

[PATCH 0/3] GIC OF bindings

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Hopefully, this is the final or near final version of GIC binding support.

Changes from the previous version:
- SPIs and PPIs are numbered starting at 0. Now the gic has it's own irq
  domain translate function instead of the simple domain one.
- interrupt cell format has changed based on Grant's proposal.
- Dropped "ARM: gic: allow irq_start to be 0". Instead, the first 16 irqs
  are skipped and the domain irq_base adjusted accordingly.
- Added a fix to of_irq_find_parent when the parent == child.
- Renamed intc_desc.parent to intc_desc.interrupt_parent.
- Implemented Grant's algorithm for walking the list of interrupt
  controllers. Added a return value to interrupt init functions, so they
  don't get added to the parent list on a init failure.

The changes are significant enough that I did not include previous
acked/reviewed/tested-by's.

Rob

Rob Herring (3):
  of/irq: of_irq_find_parent: check for parent equal to child
  of/irq: introduce of_irq_init
  ARM: gic: add OF based initialization

 Documentation/devicetree/bindings/arm/gic.txt |   55 +++++++++++
 arch/arm/common/gic.c                         |   89 ++++++++++++++++-
 arch/arm/include/asm/hardware/gic.h           |   12 +++
 drivers/of/irq.c                              |  128 +++++++++++++++++++++++--
 include/linux/of_irq.h                        |    1 +
 5 files changed, 274 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

-- 
1.7.5.4

[PATCH 1/3] of/irq: of_irq_find_parent: check for parent equal to child

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

An interrupt controller may often implicitly inherit itself from a parent
node when in fact the controller is the interrupt root controller. Guard
against the case of child == parent and return NULL in this case.

This can also be fixed by adding an explicit "interrupt-parent;" to a root
interrupt controller node.

Based on code from Grant Likely.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 drivers/of/irq.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 9f689f1..6a5b5e7 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -58,27 +58,27 @@ EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
  */
 struct device_node *of_irq_find_parent(struct device_node *child)
 {
-	struct device_node *p;
+	struct device_node *p, *c = child;
 	const __be32 *parp;
 
-	if (!of_node_get(child))
+	if (!of_node_get(c))
 		return NULL;
 
 	do {
-		parp = of_get_property(child, "interrupt-parent", NULL);
+		parp = of_get_property(c, "interrupt-parent", NULL);
 		if (parp == NULL)
-			p = of_get_parent(child);
+			p = of_get_parent(c);
 		else {
 			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
 				p = of_node_get(of_irq_dflt_pic);
 			else
 				p = of_find_node_by_phandle(be32_to_cpup(parp));
 		}
-		of_node_put(child);
-		child = p;
+		of_node_put(c);
+		c = p;
 	} while (p && of_get_property(p, "#interrupt-cells", NULL) == NULL);
 
-	return p;
+	return (p == child) ? NULL : p;
 }
 
 /**
-- 
1.7.5.4

Re: [PATCH 1/3] of/irq: of_irq_find_parent: check for parent equal to child

[Grant Likely]

On Tue, Sep 20, 2011 at 03:24:02PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> An interrupt controller may often implicitly inherit itself from a parent
> node when in fact the controller is the interrupt root controller. Guard
> against the case of child == parent and return NULL in this case.
> 
> This can also be fixed by adding an explicit "interrupt-parent;" to a root
> interrupt controller node.
> 
> Based on code from Grant Likely.
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

Looks great.

Ben, can you look at this please?  Make sure I've not missed something
about how powerpc expects of_irq_find_parent() to behave.

g.

> ---
>  drivers/of/irq.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 9f689f1..6a5b5e7 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -58,27 +58,27 @@ EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
>   */
>  struct device_node *of_irq_find_parent(struct device_node *child)
>  {
> -	struct device_node *p;
> +	struct device_node *p, *c = child;
>  	const __be32 *parp;
>  
> -	if (!of_node_get(child))
> +	if (!of_node_get(c))
>  		return NULL;
>  
>  	do {
> -		parp = of_get_property(child, "interrupt-parent", NULL);
> +		parp = of_get_property(c, "interrupt-parent", NULL);
>  		if (parp == NULL)
> -			p = of_get_parent(child);
> +			p = of_get_parent(c);
>  		else {
>  			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
>  				p = of_node_get(of_irq_dflt_pic);
>  			else
>  				p = of_find_node_by_phandle(be32_to_cpup(parp));
>  		}
> -		of_node_put(child);
> -		child = p;
> +		of_node_put(c);
> +		c = p;
>  	} while (p && of_get_property(p, "#interrupt-cells", NULL) == NULL);
>  
> -	return p;
> +	return (p == child) ? NULL : p;
>  }
>  
>  /**
> -- 
> 1.7.5.4
> 

[PATCH 2/3] of/irq: introduce of_irq_init

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

of_irq_init will scan the devicetree for matching interrupt controller
nodes. Then it calls an initialization function for each found controller
in the proper order with parent nodes initialized before child nodes.

Based on initial pseudo code from Grant Likely.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 drivers/of/irq.c       |  114 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_irq.h |    1 +
 2 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 6a5b5e7..1d51bc7 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -19,10 +19,12 @@
  */
 
 #include <linux/errno.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/string.h>
+#include <linux/slab.h>
 
 /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
 #ifndef NO_IRQ
@@ -386,3 +388,115 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
 
 	return i;
 }
+
+struct intc_desc {
+	struct list_head	list;
+	struct device_node	*dev;
+	struct device_node	*interrupt_parent;
+};
+
+typedef int (*irq_init_cb_t)(struct device_node *, struct device_node *);
+
+/**
+ * of_irq_init - Scan the device tree for matching interrupt controllers and
+ * call their initialization functions in order with parents first.
+ * @matches: 0 terminated array of nodes to match and initialization function
+ * to call on match
+ */
+void __init of_irq_init(const struct of_device_id *matches)
+{
+	struct device_node *np;
+	struct intc_desc *desc;
+	struct intc_desc *temp_desc;
+	struct intc_desc *parent_desc = NULL;
+	struct list_head intc_desc_list;
+	struct list_head intc_parent_list;
+
+	INIT_LIST_HEAD(&intc_desc_list);
+	INIT_LIST_HEAD(&intc_parent_list);
+
+	for_each_matching_node(np, matches) {
+		if (!of_find_property(np, "interrupt-controller", NULL))
+			continue;
+		/* Here, we allocate and populate an intc_desc with the node
+		* pointer, interrupt-parent device_node etc. */
+		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+		if (!desc) {
+			WARN_ON(1);
+			goto err;
+		}
+		desc->dev = np;
+		desc->interrupt_parent = of_irq_find_parent(np);
+		list_add(&desc->list, &intc_desc_list);
+	}
+	if (list_empty(&intc_desc_list))
+		return;
+
+	/*
+	 * The root irq controller is the one without an interrupt-parent.
+	 * That one goes first, followed by the controllers that reference it,
+	 * followed by the ones that reference the 2nd level controllers, etc
+	 */
+	while (!list_empty(&intc_desc_list)) {
+		/*
+		 * Process all controllers with the current 'parent'.
+		 * First pass will be looking for NULL as the parent.
+		 * The assumption is that NULL parent means a root controller.
+		 */
+		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
+			const struct of_device_id *match;
+			int ret;
+			irq_init_cb_t irq_init_cb;
+
+			if (parent_desc &&
+				(desc->interrupt_parent != parent_desc->dev))
+				continue;
+
+			list_del(&desc->list);
+			match = of_match_node(matches, desc->dev);
+			if (!match || !match->data)
+				continue;
+
+			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
+				 match->compatible,
+				 desc->dev, desc->interrupt_parent);
+			irq_init_cb = match->data;
+			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
+			if (ret)
+				continue;
+
+			/*
+			 * This one is now set up; add it to the parent list so
+			 * its children can get processed in a subsequent pass.
+			 */
+			list_add_tail(&desc->list, &intc_parent_list);
+		}
+		/*
+		 * All the direct children for the current parent are
+		 * processed, so free the parent now.
+		 */
+		if (parent_desc)
+			kfree(parent_desc);
+
+		/* Get the next pending parent that might have children */
+		parent_desc = list_first_entry(&intc_parent_list,
+					       typeof(*parent_desc), list);
+		if (list_empty(&intc_parent_list) || !parent_desc) {
+			pr_debug("of_irq_init: children remain, but no parents\n");
+			goto err;
+		}
+
+		list_del(&parent_desc->list);
+	}
+	return;
+
+err:
+	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
+		list_del(&desc->list);
+		kfree(desc);
+	}
+	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
+		list_del(&desc->list);
+		kfree(desc);
+	}
+}
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index cd2e61c..032d76c 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -73,6 +73,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
 		struct resource *res, int nr_irqs);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
 
+extern void of_irq_init(const struct of_device_id *matches);
 
 #endif /* CONFIG_OF_IRQ */
 #endif /* CONFIG_OF */
-- 
1.7.5.4

Re: [PATCH 2/3] of/irq: introduce of_irq_init

[Grant Likely]

On Tue, Sep 20, 2011 at 03:24:03PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> of_irq_init will scan the devicetree for matching interrupt controller
> nodes. Then it calls an initialization function for each found controller
> in the proper order with parent nodes initialized before child nodes.
> 
> Based on initial pseudo code from Grant Likely.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Thanks Rob.  Looks pretty good.

Comments below.

> ---
>  drivers/of/irq.c       |  114 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_irq.h |    1 +
>  2 files changed, 115 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 6a5b5e7..1d51bc7 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -19,10 +19,12 @@
>   */
>  
>  #include <linux/errno.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/string.h>
> +#include <linux/slab.h>
>  
>  /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
>  #ifndef NO_IRQ
> @@ -386,3 +388,115 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
>  
>  	return i;
>  }
> +
> +struct intc_desc {
> +	struct list_head	list;
> +	struct device_node	*dev;
> +	struct device_node	*interrupt_parent;
> +};
> +
> +typedef int (*irq_init_cb_t)(struct device_node *, struct device_node *);

This typedef should be in a header file.  I'd like to try and find a
way to typecheck the .data parameter in the matches table (maybe with
a macro), but I've not dug into the problem.

> +
> +/**
> + * of_irq_init - Scan the device tree for matching interrupt controllers and

kerneldoc format is:
 * of_irq_init() - ...

And the short description should fit on the one line.  Further
description can appear after the argument list documentation.

> + * call their initialization functions in order with parents first.
> + * @matches: 0 terminated array of nodes to match and initialization function
> + * to call on match
> + */
> +void __init of_irq_init(const struct of_device_id *matches)
> +{
> +	struct device_node *np;
> +	struct intc_desc *desc;
> +	struct intc_desc *temp_desc;
> +	struct intc_desc *parent_desc = NULL;
> +	struct list_head intc_desc_list;
> +	struct list_head intc_parent_list;

Nit: these lines can be collapsed a bit.
	struct intc_desc *desc, *temp_desc, *parent_desc = NULL;
	struct list_head intc_desc_list, intc_parent_list;

> +	INIT_LIST_HEAD(&intc_desc_list);
> +	INIT_LIST_HEAD(&intc_parent_list);
> +
> +	for_each_matching_node(np, matches) {
> +		if (!of_find_property(np, "interrupt-controller", NULL))
> +			continue;
> +		/* Here, we allocate and populate an intc_desc with the node
> +		* pointer, interrupt-parent device_node etc. */
> +		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +		if (!desc) {
> +			WARN_ON(1);
> +			goto err;
> +		}
> +		desc->dev = np;
> +		desc->interrupt_parent = of_irq_find_parent(np);
> +		list_add(&desc->list, &intc_desc_list);

list_add_tail() would keep things ordered.

> +	}
> +	if (list_empty(&intc_desc_list))
> +		return;

This test can be dropped because the while loop performs the exact
same test.

> +
> +	/*
> +	 * The root irq controller is the one without an interrupt-parent.
> +	 * That one goes first, followed by the controllers that reference it,
> +	 * followed by the ones that reference the 2nd level controllers, etc
> +	 */
> +	while (!list_empty(&intc_desc_list)) {
> +		/*
> +		 * Process all controllers with the current 'parent'.
> +		 * First pass will be looking for NULL as the parent.
> +		 * The assumption is that NULL parent means a root controller.
> +		 */
> +		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +			const struct of_device_id *match;
> +			int ret;
> +			irq_init_cb_t irq_init_cb;
> +
> +			if (parent_desc &&
> +				(desc->interrupt_parent != parent_desc->dev))
> +				continue;

If you changed
	struct intc_desc *parent_desc = NULL;
to
	struct device_node *parent_np = NULL;

Then this test could be simplified to:
	if (desc->interupt_parent != parent_np)
		continue;

And the kfree could be done immediately after the next parent is
pulled off the intc_desc_list since the only thing actually needed
from the parent_desc is the parent node pointer.

> +
> +			list_del(&desc->list);
> +			match = of_match_node(matches, desc->dev);
> +			if (!match || !match->data)
> +				continue;
> +
> +			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
> +				 match->compatible,
> +				 desc->dev, desc->interrupt_parent);
> +			irq_init_cb = match->data;
> +			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
> +			if (ret)
> +				continue;
> +
> +			/*
> +			 * This one is now set up; add it to the parent list so
> +			 * its children can get processed in a subsequent pass.
> +			 */
> +			list_add_tail(&desc->list, &intc_parent_list);
> +		}
> +		/*
> +		 * All the direct children for the current parent are
> +		 * processed, so free the parent now.
> +		 */
> +		if (parent_desc)
> +			kfree(parent_desc);
> +
> +		/* Get the next pending parent that might have children */
> +		parent_desc = list_first_entry(&intc_parent_list,
> +					       typeof(*parent_desc), list);
> +		if (list_empty(&intc_parent_list) || !parent_desc) {
> +			pr_debug("of_irq_init: children remain, but no parents\n");
> +			goto err;
> +		}
> +
> +		list_del(&parent_desc->list);
> +	}

Need a loop here to clear and free any remaining entries in the
intc_parent_list.  Actually, everything would work fine by simply
removing the below 'return' statement and letting the code fall
through to the free loops unconditionally.  And the "goto err" above
can be simplified to "break".

> +	return;
> +
> +err:
> +	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +}
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index cd2e61c..032d76c 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -73,6 +73,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
>  		struct resource *res, int nr_irqs);
>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>  
> +extern void of_irq_init(const struct of_device_id *matches);
>  
>  #endif /* CONFIG_OF_IRQ */
>  #endif /* CONFIG_OF */
> -- 
> 1.7.5.4
> 

Re: [PATCH 2/3] of/irq: introduce of_irq_init

[Jamie Iles]

Hi Rob,

On Tue, Sep 20, 2011 at 03:24:03PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> of_irq_init will scan the devicetree for matching interrupt controller
> nodes. Then it calls an initialization function for each found controller
> in the proper order with parent nodes initialized before child nodes.
> 
> Based on initial pseudo code from Grant Likely.
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
>  drivers/of/irq.c       |  114 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_irq.h |    1 +
>  2 files changed, 115 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 6a5b5e7..1d51bc7 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -19,10 +19,12 @@
>   */
>  
>  #include <linux/errno.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/string.h>
> +#include <linux/slab.h>
>  
>  /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
>  #ifndef NO_IRQ
> @@ -386,3 +388,115 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
>  
>  	return i;
>  }
> +
> +struct intc_desc {
> +	struct list_head	list;
> +	struct device_node	*dev;
> +	struct device_node	*interrupt_parent;
> +};
> +
> +typedef int (*irq_init_cb_t)(struct device_node *, struct device_node *);
> +
> +/**
> + * of_irq_init - Scan the device tree for matching interrupt controllers and
> + * call their initialization functions in order with parents first.
> + * @matches: 0 terminated array of nodes to match and initialization function
> + * to call on match
> + */
> +void __init of_irq_init(const struct of_device_id *matches)
> +{
> +	struct device_node *np;
> +	struct intc_desc *desc;
> +	struct intc_desc *temp_desc;
> +	struct intc_desc *parent_desc = NULL;
> +	struct list_head intc_desc_list;
> +	struct list_head intc_parent_list;
> +
> +	INIT_LIST_HEAD(&intc_desc_list);
> +	INIT_LIST_HEAD(&intc_parent_list);
> +
> +	for_each_matching_node(np, matches) {
> +		if (!of_find_property(np, "interrupt-controller", NULL))
> +			continue;
> +		/* Here, we allocate and populate an intc_desc with the node
> +		* pointer, interrupt-parent device_node etc. */
> +		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +		if (!desc) {
> +			WARN_ON(1);
> +			goto err;
> +		}
> +		desc->dev = np;
> +		desc->interrupt_parent = of_irq_find_parent(np);
> +		list_add(&desc->list, &intc_desc_list);
> +	}
> +	if (list_empty(&intc_desc_list))
> +		return;
> +
> +	/*
> +	 * The root irq controller is the one without an interrupt-parent.
> +	 * That one goes first, followed by the controllers that reference it,
> +	 * followed by the ones that reference the 2nd level controllers, etc
> +	 */
> +	while (!list_empty(&intc_desc_list)) {
> +		/*
> +		 * Process all controllers with the current 'parent'.
> +		 * First pass will be looking for NULL as the parent.
> +		 * The assumption is that NULL parent means a root controller.
> +		 */
> +		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +			const struct of_device_id *match;
> +			int ret;
> +			irq_init_cb_t irq_init_cb;
> +
> +			if (parent_desc &&
> +				(desc->interrupt_parent != parent_desc->dev))
> +				continue;
> +
> +			list_del(&desc->list);
> +			match = of_match_node(matches, desc->dev);
> +			if (!match || !match->data)
> +				continue;

Doesn't this case need to kfree(desc)?

> +
> +			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
> +				 match->compatible,
> +				 desc->dev, desc->interrupt_parent);
> +			irq_init_cb = match->data;
> +			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
> +			if (ret)
> +				continue;

and here?

> +
> +			/*
> +			 * This one is now set up; add it to the parent list so
> +			 * its children can get processed in a subsequent pass.
> +			 */
> +			list_add_tail(&desc->list, &intc_parent_list);
> +		}
> +		/*
> +		 * All the direct children for the current parent are
> +		 * processed, so free the parent now.
> +		 */
> +		if (parent_desc)
> +			kfree(parent_desc);
> +
> +		/* Get the next pending parent that might have children */
> +		parent_desc = list_first_entry(&intc_parent_list,
> +					       typeof(*parent_desc), list);
> +		if (list_empty(&intc_parent_list) || !parent_desc) {
> +			pr_debug("of_irq_init: children remain, but no parents\n");
> +			goto err;
> +		}
> +
> +		list_del(&parent_desc->list);
> +	}
> +	return;
> +
> +err:
> +	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +}
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index cd2e61c..032d76c 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -73,6 +73,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
>  		struct resource *res, int nr_irqs);
>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>  
> +extern void of_irq_init(const struct of_device_id *matches);
>  
>  #endif /* CONFIG_OF_IRQ */
>  #endif /* CONFIG_OF */
> -- 
> 1.7.5.4
> 

[PATCH v3] of/irq: introduce of_irq_init

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

of_irq_init will scan the devicetree for matching interrupt controller
nodes. Then it calls an initialization function for each found controller
in the proper order with parent nodes initialized before child nodes.

Based on initial pseudo code from Grant Likely.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
Changes from v2:
- add missing kfree's found by Jamie
- Implement Grant's comments to simplify the init loop
- fix function comments

 drivers/of/irq.c       |  106 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_irq.h |    3 +
 2 files changed, 109 insertions(+), 0 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 6a5b5e7..aedd39b 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -19,10 +19,12 @@
  */
 
 #include <linux/errno.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/string.h>
+#include <linux/slab.h>
 
 /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
 #ifndef NO_IRQ
@@ -386,3 +388,107 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
 
 	return i;
 }
+
+struct intc_desc {
+	struct list_head	list;
+	struct device_node	*dev;
+	struct device_node	*interrupt_parent;
+};
+
+/**
+ * of_irq_init - Scan and init matching interrupt controllers in DT
+ * @matches: 0 terminated array of nodes to match and init function to call
+ *
+ * This function scans the device tree for matching interrupt controller nodes,
+ * and calls their initialization functions in order with parents first.
+ */
+void __init of_irq_init(const struct of_device_id *matches)
+{
+	struct device_node *np, *parent = NULL;
+	struct intc_desc *desc, *temp_desc;
+	struct list_head intc_desc_list, intc_parent_list;
+
+	INIT_LIST_HEAD(&intc_desc_list);
+	INIT_LIST_HEAD(&intc_parent_list);
+
+	for_each_matching_node(np, matches) {
+		if (!of_find_property(np, "interrupt-controller", NULL))
+			continue;
+		/* Here, we allocate and populate an intc_desc with the node
+		* pointer, interrupt-parent device_node etc. */
+		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+		if (!desc) {
+			WARN_ON(1);
+			goto err;
+		}
+		desc->dev = np;
+		desc->interrupt_parent = of_irq_find_parent(np);
+		list_add_tail(&desc->list, &intc_desc_list);
+	}
+	if (list_empty(&intc_desc_list))
+		return;
+
+	/*
+	 * The root irq controller is the one without an interrupt-parent.
+	 * That one goes first, followed by the controllers that reference it,
+	 * followed by the ones that reference the 2nd level controllers, etc
+	 */
+	while (!list_empty(&intc_desc_list)) {
+		/*
+		 * Process all controllers with the current 'parent'.
+		 * First pass will be looking for NULL as the parent.
+		 * The assumption is that NULL parent means a root controller.
+		 */
+		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
+			const struct of_device_id *match;
+			int ret;
+			of_irq_init_cb_t irq_init_cb;
+
+			if (desc->interrupt_parent != parent)
+				continue;
+
+			list_del(&desc->list);
+			match = of_match_node(matches, desc->dev);
+			if (!match || !match->data) {
+				kfree(desc);
+				continue;
+			}
+
+			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
+				 match->compatible,
+				 desc->dev, desc->interrupt_parent);
+			irq_init_cb = match->data;
+			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
+			if (ret) {
+				kfree(desc);
+				continue;
+			}
+
+			/*
+			 * This one is now set up; add it to the parent list so
+			 * its children can get processed in a subsequent pass.
+			 */
+			list_add_tail(&desc->list, &intc_parent_list);
+		}
+
+		/* Get the next pending parent that might have children */
+		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
+		if (list_empty(&intc_parent_list) || !desc) {
+			pr_debug("of_irq_init: children remain, but no parents\n");
+			break;
+		}
+		list_del(&desc->list);
+		parent = desc->dev;
+		kfree(desc);
+	}
+
+	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
+		list_del(&desc->list);
+		kfree(desc);
+	}
+err:
+	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
+		list_del(&desc->list);
+		kfree(desc);
+	}
+}
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index cd2e61c..d0307ee 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -33,6 +33,8 @@ struct of_irq {
 	u32 specifier[OF_MAX_IRQ_SPEC]; /* Specifier copy */
 };
 
+typedef int (*of_irq_init_cb_t)(struct device_node *, struct device_node *);
+
 /*
  * Workarounds only applied to 32bit powermac machines
  */
@@ -73,6 +75,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
 		struct resource *res, int nr_irqs);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
 
+extern void of_irq_init(const struct of_device_id *matches);
 
 #endif /* CONFIG_OF_IRQ */
 #endif /* CONFIG_OF */
-- 
1.7.5.4

Re: [PATCH v3] of/irq: introduce of_irq_init

[Grant Likely]

On Thu, Sep 22, 2011 at 09:21:13PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> of_irq_init will scan the devicetree for matching interrupt controller
> nodes. Then it calls an initialization function for each found controller
> in the proper order with parent nodes initialized before child nodes.
> 
> Based on initial pseudo code from Grant Likely.
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

Looking really good, comments below...

> ---
> Changes from v2:
> - add missing kfree's found by Jamie
> - Implement Grant's comments to simplify the init loop
> - fix function comments

Nit: put changelog above the s-o-b lines so that it appears in the git
commit text.

> 
>  drivers/of/irq.c       |  106 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_irq.h |    3 +
>  2 files changed, 109 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 6a5b5e7..aedd39b 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -19,10 +19,12 @@
>   */
>  
>  #include <linux/errno.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/string.h>
> +#include <linux/slab.h>
>  
>  /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
>  #ifndef NO_IRQ
> @@ -386,3 +388,107 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
>  
>  	return i;
>  }
> +
> +struct intc_desc {
> +	struct list_head	list;
> +	struct device_node	*dev;
> +	struct device_node	*interrupt_parent;
> +};
> +
> +/**
> + * of_irq_init - Scan and init matching interrupt controllers in DT
> + * @matches: 0 terminated array of nodes to match and init function to call
> + *
> + * This function scans the device tree for matching interrupt controller nodes,
> + * and calls their initialization functions in order with parents first.
> + */
> +void __init of_irq_init(const struct of_device_id *matches)
> +{
> +	struct device_node *np, *parent = NULL;
> +	struct intc_desc *desc, *temp_desc;
> +	struct list_head intc_desc_list, intc_parent_list;
> +
> +	INIT_LIST_HEAD(&intc_desc_list);
> +	INIT_LIST_HEAD(&intc_parent_list);
> +
> +	for_each_matching_node(np, matches) {
> +		if (!of_find_property(np, "interrupt-controller", NULL))
> +			continue;
> +		/* Here, we allocate and populate an intc_desc with the node
> +		* pointer, interrupt-parent device_node etc. */

nit: weird indenting

> +		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +		if (!desc) {
> +			WARN_ON(1);
> +			goto err;
> +		}
> +		desc->dev = np;
> +		desc->interrupt_parent = of_irq_find_parent(np);
> +		list_add_tail(&desc->list, &intc_desc_list);
> +	}
> +	if (list_empty(&intc_desc_list))
> +		return;

This test in unnecessary

> +
> +	/*
> +	 * The root irq controller is the one without an interrupt-parent.
> +	 * That one goes first, followed by the controllers that reference it,
> +	 * followed by the ones that reference the 2nd level controllers, etc
> +	 */
> +	while (!list_empty(&intc_desc_list)) {
> +		/*
> +		 * Process all controllers with the current 'parent'.
> +		 * First pass will be looking for NULL as the parent.
> +		 * The assumption is that NULL parent means a root controller.
> +		 */
> +		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +			const struct of_device_id *match;
> +			int ret;
> +			of_irq_init_cb_t irq_init_cb;
> +
> +			if (desc->interrupt_parent != parent)
> +				continue;
> +
> +			list_del(&desc->list);
> +			match = of_match_node(matches, desc->dev);
> +			if (!match || !match->data) {

match will always be !NULL.  If match_data is NULL; something is
seriously wrong and the code should WARN().

> +				kfree(desc);
> +				continue;
> +			}
> +
> +			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
> +				 match->compatible,
> +				 desc->dev, desc->interrupt_parent);
> +			irq_init_cb = match->data;
> +			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
> +			if (ret) {
> +				kfree(desc);
> +				continue;
> +			}
> +
> +			/*
> +			 * This one is now set up; add it to the parent list so
> +			 * its children can get processed in a subsequent pass.
> +			 */
> +			list_add_tail(&desc->list, &intc_parent_list);
> +		}
> +
> +		/* Get the next pending parent that might have children */
> +		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
> +		if (list_empty(&intc_parent_list) || !desc) {
> +			pr_debug("of_irq_init: children remain, but no parents\n");

This is an error condition.  Use pr_err()

> +			break;
> +		}
> +		list_del(&desc->list);
> +		parent = desc->dev;
> +		kfree(desc);
> +	}
> +
> +	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +err:
> +	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +}
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index cd2e61c..d0307ee 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -33,6 +33,8 @@ struct of_irq {
>  	u32 specifier[OF_MAX_IRQ_SPEC]; /* Specifier copy */
>  };
>  
> +typedef int (*of_irq_init_cb_t)(struct device_node *, struct device_node *);
> +
>  /*
>   * Workarounds only applied to 32bit powermac machines
>   */
> @@ -73,6 +75,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
>  		struct resource *res, int nr_irqs);
>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>  
> +extern void of_irq_init(const struct of_device_id *matches);
>  
>  #endif /* CONFIG_OF_IRQ */
>  #endif /* CONFIG_OF */
> -- 
> 1.7.5.4
> 

[PATCH v4] of/irq: introduce of_irq_init

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

of_irq_init will scan the devicetree for matching interrupt controller
nodes. Then it calls an initialization function for each found controller
in the proper order with parent nodes initialized before child nodes.

Based on initial pseudo code from Grant Likely.

Changes in v4:
- Drop unnecessary empty list check
- Be more verbose on errors
- Simplify "if (!desc) WARN_ON(1)" to "if (WARN_ON(!desc))"

Changes in v3:
- add missing kfree's found by Jamie
- Implement Grant's comments to simplify the init loop
- fix function comments

Changes in v2:
- Complete re-write of list searching code from Grant Likely

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 drivers/of/irq.c       |  107 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_irq.h |    3 +
 2 files changed, 110 insertions(+), 0 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 6a5b5e7..6d3dd39 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -19,10 +19,12 @@
  */
 
 #include <linux/errno.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/string.h>
+#include <linux/slab.h>
 
 /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
 #ifndef NO_IRQ
@@ -386,3 +388,108 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
 
 	return i;
 }
+
+struct intc_desc {
+	struct list_head	list;
+	struct device_node	*dev;
+	struct device_node	*interrupt_parent;
+};
+
+/**
+ * of_irq_init - Scan and init matching interrupt controllers in DT
+ * @matches: 0 terminated array of nodes to match and init function to call
+ *
+ * This function scans the device tree for matching interrupt controller nodes,
+ * and calls their initialization functions in order with parents first.
+ */
+void __init of_irq_init(const struct of_device_id *matches)
+{
+	struct device_node *np, *parent = NULL;
+	struct intc_desc *desc, *temp_desc;
+	struct list_head intc_desc_list, intc_parent_list;
+
+	INIT_LIST_HEAD(&intc_desc_list);
+	INIT_LIST_HEAD(&intc_parent_list);
+
+	for_each_matching_node(np, matches) {
+		if (!of_find_property(np, "interrupt-controller", NULL))
+			continue;
+		/*
+		 * Here, we allocate and populate an intc_desc with the node
+		 * pointer, interrupt-parent device_node etc.
+		 */
+		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+		if (WARN_ON(!desc))
+			goto err;
+
+		desc->dev = np;
+		desc->interrupt_parent = of_irq_find_parent(np);
+		list_add_tail(&desc->list, &intc_desc_list);
+	}
+
+	/*
+	 * The root irq controller is the one without an interrupt-parent.
+	 * That one goes first, followed by the controllers that reference it,
+	 * followed by the ones that reference the 2nd level controllers, etc.
+	 */
+	while (!list_empty(&intc_desc_list)) {
+		/*
+		 * Process all controllers with the current 'parent'.
+		 * First pass will be looking for NULL as the parent.
+		 * The assumption is that NULL parent means a root controller.
+		 */
+		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
+			const struct of_device_id *match;
+			int ret;
+			of_irq_init_cb_t irq_init_cb;
+
+			if (desc->interrupt_parent != parent)
+				continue;
+
+			list_del(&desc->list);
+			match = of_match_node(matches, desc->dev);
+			if (WARN(!match->data,
+			    "of_irq_init: no init function for %s\n",
+			    match->compatible)) {
+				kfree(desc);
+				continue;
+			}
+
+			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
+				 match->compatible,
+				 desc->dev, desc->interrupt_parent);
+			irq_init_cb = match->data;
+			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
+			if (ret) {
+				kfree(desc);
+				continue;
+			}
+
+			/*
+			 * This one is now set up; add it to the parent list so
+			 * its children can get processed in a subsequent pass.
+			 */
+			list_add_tail(&desc->list, &intc_parent_list);
+		}
+
+		/* Get the next pending parent that might have children */
+		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
+		if (list_empty(&intc_parent_list) || !desc) {
+			pr_err("of_irq_init: children remain, but no parents\n");
+			break;
+		}
+		list_del(&desc->list);
+		parent = desc->dev;
+		kfree(desc);
+	}
+
+	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
+		list_del(&desc->list);
+		kfree(desc);
+	}
+err:
+	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
+		list_del(&desc->list);
+		kfree(desc);
+	}
+}
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index cd2e61c..d0307ee 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -33,6 +33,8 @@ struct of_irq {
 	u32 specifier[OF_MAX_IRQ_SPEC]; /* Specifier copy */
 };
 
+typedef int (*of_irq_init_cb_t)(struct device_node *, struct device_node *);
+
 /*
  * Workarounds only applied to 32bit powermac machines
  */
@@ -73,6 +75,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
 		struct resource *res, int nr_irqs);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
 
+extern void of_irq_init(const struct of_device_id *matches);
 
 #endif /* CONFIG_OF_IRQ */
 #endif /* CONFIG_OF */
-- 
1.7.5.4

Re: [PATCH v4] of/irq: introduce of_irq_init

[Grant Likely]

On Mon, Sep 26, 2011 at 02:24:43PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> of_irq_init will scan the devicetree for matching interrupt controller
> nodes. Then it calls an initialization function for each found controller
> in the proper order with parent nodes initialized before child nodes.
> 
> Based on initial pseudo code from Grant Likely.
> 
> Changes in v4:
> - Drop unnecessary empty list check
> - Be more verbose on errors
> - Simplify "if (!desc) WARN_ON(1)" to "if (WARN_ON(!desc))"
> 
> Changes in v3:
> - add missing kfree's found by Jamie
> - Implement Grant's comments to simplify the init loop
> - fix function comments
> 
> Changes in v2:
> - Complete re-write of list searching code from Grant Likely
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

Looks good to me.  Merged.

g.

> ---
>  drivers/of/irq.c       |  107 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_irq.h |    3 +
>  2 files changed, 110 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 6a5b5e7..6d3dd39 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -19,10 +19,12 @@
>   */
>  
>  #include <linux/errno.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/string.h>
> +#include <linux/slab.h>
>  
>  /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
>  #ifndef NO_IRQ
> @@ -386,3 +388,108 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
>  
>  	return i;
>  }
> +
> +struct intc_desc {
> +	struct list_head	list;
> +	struct device_node	*dev;
> +	struct device_node	*interrupt_parent;
> +};
> +
> +/**
> + * of_irq_init - Scan and init matching interrupt controllers in DT
> + * @matches: 0 terminated array of nodes to match and init function to call
> + *
> + * This function scans the device tree for matching interrupt controller nodes,
> + * and calls their initialization functions in order with parents first.
> + */
> +void __init of_irq_init(const struct of_device_id *matches)
> +{
> +	struct device_node *np, *parent = NULL;
> +	struct intc_desc *desc, *temp_desc;
> +	struct list_head intc_desc_list, intc_parent_list;
> +
> +	INIT_LIST_HEAD(&intc_desc_list);
> +	INIT_LIST_HEAD(&intc_parent_list);
> +
> +	for_each_matching_node(np, matches) {
> +		if (!of_find_property(np, "interrupt-controller", NULL))
> +			continue;
> +		/*
> +		 * Here, we allocate and populate an intc_desc with the node
> +		 * pointer, interrupt-parent device_node etc.
> +		 */
> +		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +		if (WARN_ON(!desc))
> +			goto err;
> +
> +		desc->dev = np;
> +		desc->interrupt_parent = of_irq_find_parent(np);
> +		list_add_tail(&desc->list, &intc_desc_list);
> +	}
> +
> +	/*
> +	 * The root irq controller is the one without an interrupt-parent.
> +	 * That one goes first, followed by the controllers that reference it,
> +	 * followed by the ones that reference the 2nd level controllers, etc.
> +	 */
> +	while (!list_empty(&intc_desc_list)) {
> +		/*
> +		 * Process all controllers with the current 'parent'.
> +		 * First pass will be looking for NULL as the parent.
> +		 * The assumption is that NULL parent means a root controller.
> +		 */
> +		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +			const struct of_device_id *match;
> +			int ret;
> +			of_irq_init_cb_t irq_init_cb;
> +
> +			if (desc->interrupt_parent != parent)
> +				continue;
> +
> +			list_del(&desc->list);
> +			match = of_match_node(matches, desc->dev);
> +			if (WARN(!match->data,
> +			    "of_irq_init: no init function for %s\n",
> +			    match->compatible)) {
> +				kfree(desc);
> +				continue;
> +			}
> +
> +			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
> +				 match->compatible,
> +				 desc->dev, desc->interrupt_parent);
> +			irq_init_cb = match->data;
> +			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
> +			if (ret) {
> +				kfree(desc);
> +				continue;
> +			}
> +
> +			/*
> +			 * This one is now set up; add it to the parent list so
> +			 * its children can get processed in a subsequent pass.
> +			 */
> +			list_add_tail(&desc->list, &intc_parent_list);
> +		}
> +
> +		/* Get the next pending parent that might have children */
> +		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
> +		if (list_empty(&intc_parent_list) || !desc) {
> +			pr_err("of_irq_init: children remain, but no parents\n");
> +			break;
> +		}
> +		list_del(&desc->list);
> +		parent = desc->dev;
> +		kfree(desc);
> +	}
> +
> +	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +err:
> +	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
> +		list_del(&desc->list);
> +		kfree(desc);
> +	}
> +}
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index cd2e61c..d0307ee 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -33,6 +33,8 @@ struct of_irq {
>  	u32 specifier[OF_MAX_IRQ_SPEC]; /* Specifier copy */
>  };
>  
> +typedef int (*of_irq_init_cb_t)(struct device_node *, struct device_node *);
> +
>  /*
>   * Workarounds only applied to 32bit powermac machines
>   */
> @@ -73,6 +75,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
>  		struct resource *res, int nr_irqs);
>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>  
> +extern void of_irq_init(const struct of_device_id *matches);
>  
>  #endif /* CONFIG_OF_IRQ */
>  #endif /* CONFIG_OF */
> -- 
> 1.7.5.4
> 

Re: [PATCH v4] of/irq: introduce of_irq_init

[Rob Herring]

Grant,

On 09/26/2011 08:53 PM, Grant Likely wrote:
> On Mon, Sep 26, 2011 at 02:24:43PM -0500, Rob Herring wrote:
>> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>
>> of_irq_init will scan the devicetree for matching interrupt controller
>> nodes. Then it calls an initialization function for each found controller
>> in the proper order with parent nodes initialized before child nodes.
>>
>> Based on initial pseudo code from Grant Likely.
>>
>> Changes in v4:
>> - Drop unnecessary empty list check
>> - Be more verbose on errors
>> - Simplify "if (!desc) WARN_ON(1)" to "if (WARN_ON(!desc))"
>>
>> Changes in v3:
>> - add missing kfree's found by Jamie
>> - Implement Grant's comments to simplify the init loop
>> - fix function comments
>>
>> Changes in v2:
>> - Complete re-write of list searching code from Grant Likely
>>
>> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> 
> Looks good to me.  Merged.
> 

I'm dependent on this and things in rmk's tree for initial highbank
support, so should this series go in thru arm-soc tree? Several others
are dependent on this as well.

Rob

> g.
> 
>> ---
>>  drivers/of/irq.c       |  107 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_irq.h |    3 +
>>  2 files changed, 110 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 6a5b5e7..6d3dd39 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -19,10 +19,12 @@
>>   */
>>  
>>  #include <linux/errno.h>
>> +#include <linux/list.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/string.h>
>> +#include <linux/slab.h>
>>  
>>  /* For archs that don't support NO_IRQ (such as x86), provide a dummy value */
>>  #ifndef NO_IRQ
>> @@ -386,3 +388,108 @@ int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
>>  
>>  	return i;
>>  }
>> +
>> +struct intc_desc {
>> +	struct list_head	list;
>> +	struct device_node	*dev;
>> +	struct device_node	*interrupt_parent;
>> +};
>> +
>> +/**
>> + * of_irq_init - Scan and init matching interrupt controllers in DT
>> + * @matches: 0 terminated array of nodes to match and init function to call
>> + *
>> + * This function scans the device tree for matching interrupt controller nodes,
>> + * and calls their initialization functions in order with parents first.
>> + */
>> +void __init of_irq_init(const struct of_device_id *matches)
>> +{
>> +	struct device_node *np, *parent = NULL;
>> +	struct intc_desc *desc, *temp_desc;
>> +	struct list_head intc_desc_list, intc_parent_list;
>> +
>> +	INIT_LIST_HEAD(&intc_desc_list);
>> +	INIT_LIST_HEAD(&intc_parent_list);
>> +
>> +	for_each_matching_node(np, matches) {
>> +		if (!of_find_property(np, "interrupt-controller", NULL))
>> +			continue;
>> +		/*
>> +		 * Here, we allocate and populate an intc_desc with the node
>> +		 * pointer, interrupt-parent device_node etc.
>> +		 */
>> +		desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> +		if (WARN_ON(!desc))
>> +			goto err;
>> +
>> +		desc->dev = np;
>> +		desc->interrupt_parent = of_irq_find_parent(np);
>> +		list_add_tail(&desc->list, &intc_desc_list);
>> +	}
>> +
>> +	/*
>> +	 * The root irq controller is the one without an interrupt-parent.
>> +	 * That one goes first, followed by the controllers that reference it,
>> +	 * followed by the ones that reference the 2nd level controllers, etc.
>> +	 */
>> +	while (!list_empty(&intc_desc_list)) {
>> +		/*
>> +		 * Process all controllers with the current 'parent'.
>> +		 * First pass will be looking for NULL as the parent.
>> +		 * The assumption is that NULL parent means a root controller.
>> +		 */
>> +		list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
>> +			const struct of_device_id *match;
>> +			int ret;
>> +			of_irq_init_cb_t irq_init_cb;
>> +
>> +			if (desc->interrupt_parent != parent)
>> +				continue;
>> +
>> +			list_del(&desc->list);
>> +			match = of_match_node(matches, desc->dev);
>> +			if (WARN(!match->data,
>> +			    "of_irq_init: no init function for %s\n",
>> +			    match->compatible)) {
>> +				kfree(desc);
>> +				continue;
>> +			}
>> +
>> +			pr_debug("of_irq_init: init %s @ %p, parent %p\n",
>> +				 match->compatible,
>> +				 desc->dev, desc->interrupt_parent);
>> +			irq_init_cb = match->data;
>> +			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
>> +			if (ret) {
>> +				kfree(desc);
>> +				continue;
>> +			}
>> +
>> +			/*
>> +			 * This one is now set up; add it to the parent list so
>> +			 * its children can get processed in a subsequent pass.
>> +			 */
>> +			list_add_tail(&desc->list, &intc_parent_list);
>> +		}
>> +
>> +		/* Get the next pending parent that might have children */
>> +		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
>> +		if (list_empty(&intc_parent_list) || !desc) {
>> +			pr_err("of_irq_init: children remain, but no parents\n");
>> +			break;
>> +		}
>> +		list_del(&desc->list);
>> +		parent = desc->dev;
>> +		kfree(desc);
>> +	}
>> +
>> +	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
>> +		list_del(&desc->list);
>> +		kfree(desc);
>> +	}
>> +err:
>> +	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
>> +		list_del(&desc->list);
>> +		kfree(desc);
>> +	}
>> +}
>> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
>> index cd2e61c..d0307ee 100644
>> --- a/include/linux/of_irq.h
>> +++ b/include/linux/of_irq.h
>> @@ -33,6 +33,8 @@ struct of_irq {
>>  	u32 specifier[OF_MAX_IRQ_SPEC]; /* Specifier copy */
>>  };
>>  
>> +typedef int (*of_irq_init_cb_t)(struct device_node *, struct device_node *);
>> +
>>  /*
>>   * Workarounds only applied to 32bit powermac machines
>>   */
>> @@ -73,6 +75,7 @@ extern int of_irq_to_resource_table(struct device_node *dev,
>>  		struct resource *res, int nr_irqs);
>>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>>  
>> +extern void of_irq_init(const struct of_device_id *matches);
>>  
>>  #endif /* CONFIG_OF_IRQ */
>>  #endif /* CONFIG_OF */
>> -- 
>> 1.7.5.4
>>

Re: [PATCH v4] of/irq: introduce of_irq_init

[Grant Likely]

On Tue, Sep 27, 2011 at 7:03 AM, Rob Herring <robherring2@gmail.com> wrote:
> Grant,
>
> On 09/26/2011 08:53 PM, Grant Likely wrote:
>> On Mon, Sep 26, 2011 at 02:24:43PM -0500, Rob Herring wrote:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> of_irq_init will scan the devicetree for matching interrupt controller
>>> nodes. Then it calls an initialization function for each found controller
>>> in the proper order with parent nodes initialized before child nodes.
>>>
>>> Based on initial pseudo code from Grant Likely.
>>>
>>> Changes in v4:
>>> - Drop unnecessary empty list check
>>> - Be more verbose on errors
>>> - Simplify "if (!desc) WARN_ON(1)" to "if (WARN_ON(!desc))"
>>>
>>> Changes in v3:
>>> - add missing kfree's found by Jamie
>>> - Implement Grant's comments to simplify the init loop
>>> - fix function comments
>>>
>>> Changes in v2:
>>> - Complete re-write of list searching code from Grant Likely
>>>
>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>> Cc: Grant Likely <grant.likely@secretlab.ca>
>>
>> Looks good to me.  Merged.
>>
>
> I'm dependent on this and things in rmk's tree for initial highbank
> support, so should this series go in thru arm-soc tree? Several others
> are dependent on this as well.

Sure, it can go in via arm-soc.  I don't have anything depending on it
in mine.  Add my acked-by.

g.

[PATCH 3/3] ARM: gic: add OF based initialization

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

This adds ARM gic interrupt controller initialization using device tree
data.

The initialization function is intended to be called by of_irq_init
function like this:

const static struct of_device_id irq_match[] = {
        { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
        {}
};

static void __init init_irqs(void)
{
        of_irq_init(irq_match);
}

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/gic.txt |   55 +++++++++++++++
 arch/arm/common/gic.c                         |   89 +++++++++++++++++++++++-
 arch/arm/include/asm/hardware/gic.h           |   12 ++++
 3 files changed, 152 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
new file mode 100644
index 0000000..52916b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -0,0 +1,55 @@
+* ARM Generic Interrupt Controller
+
+ARM SMP cores are often associated with a GIC, providing per processor
+interrupts (PPI), shared processor interrupts (SPI) and software
+generated interrupts (SGI).
+
+Primary GIC is attached directly to the CPU and typically has PPIs and SGIs.
+Secondary GICs are cascaded into the upward interrupt controller and do not
+have PPIs or SGIs.
+
+Main node required properties:
+
+- compatible : should be one of:
+	"arm,cortex-a9-gic"
+	"arm,arm11mp-gic"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source.  The type shall be a <u32> and the value shall be 3.
+
+  The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI
+  interrupts.
+
+  The 2nd cell contains the interrupt number for the interrupt type.
+  SPI interrupts are in the range [0-987].  PPI interrupts are in the
+  range [0-15].
+
+  The 3rd cell is the flags, encoded as follows:
+	bits[3:0] trigger type and level flags.
+		1 = low-to-high edge triggered
+		2 = high-to-low edge triggered
+		4 = active high level-sensitive
+		8 = active low level-sensitive
+	bits[15:8] PPI interrupt cpu mask.  Each bit corresponds to each of
+	the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
+	the interrupt is wired to that CPU.  Only valid for PPI interrupts.
+
+- reg : Specifies base physical address(s) and size of the GIC registers. The
+  first region is the GIC distributor register base and size. The 2nd region is
+  the GIC cpu interface register base and size.
+
+Optional
+- interrupts	: Interrupt source of the parent interrupt controller. Only
+  present on secondary GICs.
+
+Example:
+
+	intc: interrupt-controller@fff11000 {
+		compatible = "arm,cortex-a9-gic";
+		#interrupt-cells = <3>;
+		#address-cells = <1>;
+		interrupt-controller;
+		reg = <0xfff11000 0x1000>,
+		      <0xfff10100 0x100>;
+	};
+
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 666b278..84e69a4 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -28,6 +28,10 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
@@ -255,6 +259,15 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
+static int gic_irq_count(void __iomem *dist_base)
+{
+	int gic_irqs = readl_relaxed(dist_base + GIC_DIST_CTR) & 0x1f;
+	gic_irqs = (gic_irqs + 1) * 32;
+	if (gic_irqs > 1020)
+		gic_irqs = 1020;
+	return gic_irqs;
+}
+
 static void __init gic_dist_init(struct gic_chip_data *gic,
 	unsigned int irq_start)
 {
@@ -277,10 +290,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	 * Find out how many interrupts are supported.
 	 * The GIC only supports up to 1020 interrupt sources.
 	 */
-	gic_irqs = readl_relaxed(base + GIC_DIST_CTR) & 0x1f;
-	gic_irqs = (gic_irqs + 1) * 32;
-	if (gic_irqs > 1020)
-		gic_irqs = 1020;
+	gic_irqs = gic_irq_count(base);
 
 	/*
 	 * Set all global interrupts to be level triggered, active low.
@@ -405,3 +415,74 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	writel_relaxed(map << 16 | irq, gic_data[0].dist_base + GIC_DIST_SOFTINT);
 }
 #endif
+
+#ifdef CONFIG_OF
+static int gic_cnt __initdata = 0;
+
+int gic_irq_domain_dt_translate(struct irq_domain *d,
+			    struct device_node *controller,
+			    const u32 *intspec, unsigned int intsize,
+			    unsigned long *out_hwirq, unsigned int *out_type)
+{
+	struct gic_chip_data *gic_data = d->priv;
+
+	if (d->of_node != controller)
+		return -EINVAL;
+	if (intsize < 3)
+		return -EINVAL;
+
+	*out_hwirq = intspec[1];
+	/*
+	 * We've already skipped over SGIs, so PPIs need no translation.
+	 * For SPIs, we need to skip over 16 PPIs on primary GICs.
+	 */
+	if (!intspec[0] && !gic_data->irq_offset)
+		*out_hwirq += 16;
+
+	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+
+struct irq_domain_ops gic_irq_domain_ops = {
+	.dt_translate = gic_irq_domain_dt_translate,
+};
+
+int __init gic_of_init(struct device_node *node, struct device_node *parent)
+{
+	void __iomem *cpu_base;
+	void __iomem *dist_base;
+	int irq;
+	struct irq_domain *domain = &gic_data[gic_cnt].domain;
+
+	if (WARN_ON(!node))
+		return -ENODEV;
+
+	dist_base = of_iomap(node, 0);
+	WARN(!dist_base, "unable to map gic dist registers\n");
+
+	cpu_base = of_iomap(node, 1);
+	WARN(!cpu_base, "unable to map gic cpu registers\n");
+
+	domain->nr_irq = gic_irq_count(dist_base);
+	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
+	if (parent)
+		domain->nr_irq -= 32;
+	else
+		domain->nr_irq -= 16;
+
+	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());
+	domain->of_node = of_node_get(node);
+	domain->ops = &gic_irq_domain_ops;
+	domain->priv = &gic_data[gic_cnt];
+	irq_domain_add(domain);
+
+	gic_init(gic_cnt, domain->irq_base, dist_base, cpu_base);
+
+	if (parent) {
+		irq = irq_of_parse_and_map(node, 0);
+		gic_cascade_irq(gic_cnt, irq);
+	}
+	gic_cnt++;
+	return 0;
+}
+#endif
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 435d3f8..2862d0e 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -33,10 +33,21 @@
 #define GIC_DIST_SOFTINT		0xf00
 
 #ifndef __ASSEMBLY__
+#include <linux/irqdomain.h>
+
 extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
+#ifdef CONFIG_OF
+int gic_of_init(struct device_node *node, struct device_node *parent);
+#else
+static inline void gic_of_init(struct device_node *node,
+			       struct device_node *parent)
+{
+	return -ENODEV;
+}
+#endif
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
@@ -46,6 +57,7 @@ struct gic_chip_data {
 	unsigned int irq_offset;
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
+	struct irq_domain domain;
 };
 #endif
 
-- 
1.7.5.4

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Grant Likely]

On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> This adds ARM gic interrupt controller initialization using device tree
> data.
> 
> The initialization function is intended to be called by of_irq_init
> function like this:
> 
> const static struct of_device_id irq_match[] = {
>         { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
>         {}
> };
> 
> static void __init init_irqs(void)
> {
>         of_irq_init(irq_match);
> }
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   55 +++++++++++++++
>  arch/arm/common/gic.c                         |   89 +++++++++++++++++++++++-
>  arch/arm/include/asm/hardware/gic.h           |   12 ++++
>  3 files changed, 152 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> new file mode 100644
> index 0000000..52916b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -0,0 +1,55 @@
> +* ARM Generic Interrupt Controller
> +
> +ARM SMP cores are often associated with a GIC, providing per processor
> +interrupts (PPI), shared processor interrupts (SPI) and software
> +generated interrupts (SGI).
> +
> +Primary GIC is attached directly to the CPU and typically has PPIs and SGIs.
> +Secondary GICs are cascaded into the upward interrupt controller and do not
> +have PPIs or SGIs.
> +
> +Main node required properties:
> +
> +- compatible : should be one of:
> +	"arm,cortex-a9-gic"
> +	"arm,arm11mp-gic"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source.  The type shall be a <u32> and the value shall be 3.
> +
> +  The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI
> +  interrupts.
> +
> +  The 2nd cell contains the interrupt number for the interrupt type.
> +  SPI interrupts are in the range [0-987].  PPI interrupts are in the
> +  range [0-15].
> +
> +  The 3rd cell is the flags, encoded as follows:
> +	bits[3:0] trigger type and level flags.
> +		1 = low-to-high edge triggered
> +		2 = high-to-low edge triggered
> +		4 = active high level-sensitive
> +		8 = active low level-sensitive
> +	bits[15:8] PPI interrupt cpu mask.  Each bit corresponds to each of
> +	the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
> +	the interrupt is wired to that CPU.  Only valid for PPI interrupts.
> +
> +- reg : Specifies base physical address(s) and size of the GIC registers. The
> +  first region is the GIC distributor register base and size. The 2nd region is
> +  the GIC cpu interface register base and size.
> +
> +Optional
> +- interrupts	: Interrupt source of the parent interrupt controller. Only
> +  present on secondary GICs.
> +
> +Example:
> +
> +	intc: interrupt-controller@fff11000 {
> +		compatible = "arm,cortex-a9-gic";
> +		#interrupt-cells = <3>;
> +		#address-cells = <1>;
> +		interrupt-controller;
> +		reg = <0xfff11000 0x1000>,
> +		      <0xfff10100 0x100>;
> +	};
> +

Ack on the binding.

> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 666b278..84e69a4 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -28,6 +28,10 @@
>  #include <linux/smp.h>
>  #include <linux/cpumask.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqdomain.h>
>  
>  #include <asm/irq.h>
>  #include <asm/mach/irq.h>
> @@ -255,6 +259,15 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>  	irq_set_chained_handler(irq, gic_handle_cascade_irq);
>  }
>  
> +static int gic_irq_count(void __iomem *dist_base)
> +{
> +	int gic_irqs = readl_relaxed(dist_base + GIC_DIST_CTR) & 0x1f;
> +	gic_irqs = (gic_irqs + 1) * 32;
> +	if (gic_irqs > 1020)
> +		gic_irqs = 1020;
> +	return gic_irqs;
> +}
> +
>  static void __init gic_dist_init(struct gic_chip_data *gic,
>  	unsigned int irq_start)
>  {
> @@ -277,10 +290,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>  	 * Find out how many interrupts are supported.
>  	 * The GIC only supports up to 1020 interrupt sources.
>  	 */
> -	gic_irqs = readl_relaxed(base + GIC_DIST_CTR) & 0x1f;
> -	gic_irqs = (gic_irqs + 1) * 32;
> -	if (gic_irqs > 1020)
> -		gic_irqs = 1020;
> +	gic_irqs = gic_irq_count(base);
>  
>  	/*
>  	 * Set all global interrupts to be level triggered, active low.
> @@ -405,3 +415,74 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>  	writel_relaxed(map << 16 | irq, gic_data[0].dist_base + GIC_DIST_SOFTINT);
>  }
>  #endif
> +
> +#ifdef CONFIG_OF
> +static int gic_cnt __initdata = 0;
> +
> +int gic_irq_domain_dt_translate(struct irq_domain *d,
> +			    struct device_node *controller,
> +			    const u32 *intspec, unsigned int intsize,
> +			    unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	struct gic_chip_data *gic_data = d->priv;
> +
> +	if (d->of_node != controller)
> +		return -EINVAL;
> +	if (intsize < 3)
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[1];
> +	/*
> +	 * We've already skipped over SGIs, so PPIs need no translation.
> +	 * For SPIs, we need to skip over 16 PPIs on primary GICs.
> +	 */
> +	if (!intspec[0] && !gic_data->irq_offset)
> +		*out_hwirq += 16;

I though that the offset was 32 to get from SPI number to Interrrupt
ID?  And that PPI interrupts start at Interrupt ID 16?  Or am I
mistaken about the hwirq number that this driver uses internally?

Otherwise, the change looks about right to me.

g.

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Rob Herring]

On 09/20/2011 06:08 PM, Grant Likely wrote:
> On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
>> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>
>> This adds ARM gic interrupt controller initialization using device tree
>> data.
>>
>> The initialization function is intended to be called by of_irq_init
>> function like this:
>>
>> const static struct of_device_id irq_match[] = {
>>         { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
>>         {}
>> };
>>
>> static void __init init_irqs(void)
>> {
>>         of_irq_init(irq_match);
>> }
>>
>> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/arm/gic.txt |   55 +++++++++++++++
>>  arch/arm/common/gic.c                         |   89 +++++++++++++++++++++++-
>>  arch/arm/include/asm/hardware/gic.h           |   12 ++++
>>  3 files changed, 152 insertions(+), 4 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> new file mode 100644
>> index 0000000..52916b4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -0,0 +1,55 @@
>> +* ARM Generic Interrupt Controller
>> +
>> +ARM SMP cores are often associated with a GIC, providing per processor
>> +interrupts (PPI), shared processor interrupts (SPI) and software
>> +generated interrupts (SGI).
>> +
>> +Primary GIC is attached directly to the CPU and typically has PPIs and SGIs.
>> +Secondary GICs are cascaded into the upward interrupt controller and do not
>> +have PPIs or SGIs.
>> +
>> +Main node required properties:
>> +
>> +- compatible : should be one of:
>> +	"arm,cortex-a9-gic"
>> +	"arm,arm11mp-gic"
>> +- interrupt-controller : Identifies the node as an interrupt controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> +  interrupt source.  The type shall be a <u32> and the value shall be 3.
>> +
>> +  The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI
>> +  interrupts.
>> +
>> +  The 2nd cell contains the interrupt number for the interrupt type.
>> +  SPI interrupts are in the range [0-987].  PPI interrupts are in the
>> +  range [0-15].
>> +
>> +  The 3rd cell is the flags, encoded as follows:
>> +	bits[3:0] trigger type and level flags.
>> +		1 = low-to-high edge triggered
>> +		2 = high-to-low edge triggered
>> +		4 = active high level-sensitive
>> +		8 = active low level-sensitive
>> +	bits[15:8] PPI interrupt cpu mask.  Each bit corresponds to each of
>> +	the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
>> +	the interrupt is wired to that CPU.  Only valid for PPI interrupts.
>> +
>> +- reg : Specifies base physical address(s) and size of the GIC registers. The
>> +  first region is the GIC distributor register base and size. The 2nd region is
>> +  the GIC cpu interface register base and size.
>> +
>> +Optional
>> +- interrupts	: Interrupt source of the parent interrupt controller. Only
>> +  present on secondary GICs.
>> +
>> +Example:
>> +
>> +	intc: interrupt-controller@fff11000 {
>> +		compatible = "arm,cortex-a9-gic";
>> +		#interrupt-cells = <3>;
>> +		#address-cells = <1>;
>> +		interrupt-controller;
>> +		reg = <0xfff11000 0x1000>,
>> +		      <0xfff10100 0x100>;
>> +	};
>> +
> 
> Ack on the binding.
> 
>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index 666b278..84e69a4 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -28,6 +28,10 @@
>>  #include <linux/smp.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irqdomain.h>
>>  
>>  #include <asm/irq.h>
>>  #include <asm/mach/irq.h>
>> @@ -255,6 +259,15 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>>  	irq_set_chained_handler(irq, gic_handle_cascade_irq);
>>  }
>>  
>> +static int gic_irq_count(void __iomem *dist_base)
>> +{
>> +	int gic_irqs = readl_relaxed(dist_base + GIC_DIST_CTR) & 0x1f;
>> +	gic_irqs = (gic_irqs + 1) * 32;
>> +	if (gic_irqs > 1020)
>> +		gic_irqs = 1020;
>> +	return gic_irqs;
>> +}
>> +
>>  static void __init gic_dist_init(struct gic_chip_data *gic,
>>  	unsigned int irq_start)
>>  {
>> @@ -277,10 +290,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>>  	 * Find out how many interrupts are supported.
>>  	 * The GIC only supports up to 1020 interrupt sources.
>>  	 */
>> -	gic_irqs = readl_relaxed(base + GIC_DIST_CTR) & 0x1f;
>> -	gic_irqs = (gic_irqs + 1) * 32;
>> -	if (gic_irqs > 1020)
>> -		gic_irqs = 1020;
>> +	gic_irqs = gic_irq_count(base);
>>  
>>  	/*
>>  	 * Set all global interrupts to be level triggered, active low.
>> @@ -405,3 +415,74 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>  	writel_relaxed(map << 16 | irq, gic_data[0].dist_base + GIC_DIST_SOFTINT);
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_OF
>> +static int gic_cnt __initdata = 0;
>> +
>> +int gic_irq_domain_dt_translate(struct irq_domain *d,
>> +			    struct device_node *controller,
>> +			    const u32 *intspec, unsigned int intsize,
>> +			    unsigned long *out_hwirq, unsigned int *out_type)
>> +{
>> +	struct gic_chip_data *gic_data = d->priv;
>> +
>> +	if (d->of_node != controller)
>> +		return -EINVAL;
>> +	if (intsize < 3)
>> +		return -EINVAL;
>> +
>> +	*out_hwirq = intspec[1];
>> +	/*
>> +	 * We've already skipped over SGIs, so PPIs need no translation.
>> +	 * For SPIs, we need to skip over 16 PPIs on primary GICs.
>> +	 */
>> +	if (!intspec[0] && !gic_data->irq_offset)
>> +		*out_hwirq += 16;
> 
> I though that the offset was 32 to get from SPI number to Interrrupt
> ID?  And that PPI interrupts start at Interrupt ID 16?  Or am I
> mistaken about the hwirq number that this driver uses internally?
> 

There's basically 2 cases to handle: primary and secondary controllers.

For primary ctrlrs, the domain irq_base is already 16 to skip over SGIs
and legacy ISA interrupts. So PPIs don't need any conversion and SPIs
only need 16 more added. I misspoke previously as it's not quite
independent of virq numbering because entry-macro-gic.S is still fixed
GIC ID = Linux virq (or offset of 32 in Exynos case).

For secondary ctrlrs, the gic code already accounts for 32 offset in ID
to Linux virq with the irq_offset field which is set to Linux virq base
- 32. So in this case, no conversion is needed.

I don't really see a better way without breaking non-DT use of the gic.

Rob

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Cousson, Benoit]

Hi Rob,

I'm testing that series with OMAP4 but have some issues for the moment :-(

[    0.000000] WARNING: at kernel/irq/irqdomain.c:34 gic_of_init+0x10c/0x180()
[    0.000000] error: irq_desc already assigned to a domain
[    0.000000] Modules linked in:
[    0.000000] [<c001b284>] (unwind_backtrace+0x0/0xf0) from [<c0051c34>] (warn_slowpath_common+0x4c/0x64)
[    0.000000] [<c0051c34>] (warn_slowpath_common+0x4c/0x64) from [<c0051ce0>] (warn_slowpath_fmt+0x30/0x40)
[    0.000000] [<c0051ce0>] (warn_slowpath_fmt+0x30/0x40) from [<c05f6874>] (gic_of_init+0x10c/0x180)
[    0.000000] [<c05f6874>] (gic_of_init+0x10c/0x180) from [<c05fa2e0>] (omap_gic_of_init+0x8/0x28)
[    0.000000] [<c05fa2e0>] (omap_gic_of_init+0x8/0x28) from [<c0616b44>] (of_irq_init+0x148/0x28c)
[    0.000000] [<c0616b44>] (of_irq_init+0x148/0x28c) from [<c05f3074>] (init_IRQ+0x14/0x1c)
[    0.000000] [<c05f3074>] (init_IRQ+0x14/0x1c) from [<c05f0650>] (start_kernel+0x184/0x2fc)
[    0.000000] [<c05f0650>] (start_kernel+0x184/0x2fc) from [<80008040>] (0x80008040)
 
I'm not super familiar with all the irq stuff but I'm wondering if there is not something wrong with the test that print that message:

void irq_domain_add(struct irq_domain *domain)
{
	struct irq_data *d;
	int hwirq;

	/*
	 * This assumes that the irq_domain owner has already allocated
	 * the irq_descs.  This block will be removed when support for dynamic
	 * allocation of irq_descs is added to irq_domain.
	 */
	for (hwirq = 0; hwirq < domain->nr_irq; hwirq++) {
		d = irq_get_irq_data(irq_domain_to_irq(domain, hwirq));
		if (d || d->domain) {
			/* things are broken; just report, don't clean up */
			WARN(1, "error: irq_desc already assigned to a domain");
			return;
		}
[...]

Is the (d || d->domain) correct? Shouldn't it be (d && d->domain)?

But since that used to work properly, I have some doubt. Moreover the driver will not even get the proper interrupt later...

Do you have any clue?

Thanks,
Benoit


On 9/20/2011 10:24 PM, Rob Herring wrote:
> From: Rob Herring<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> This adds ARM gic interrupt controller initialization using device tree
> data.
> 
> The initialization function is intended to be called by of_irq_init
> function like this:
> 
> const static struct of_device_id irq_match[] = {
>          { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
>          {}
> };
> 
> static void __init init_irqs(void)
> {
>          of_irq_init(irq_match);
> }
> 
> Signed-off-by: Rob Herring<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
>   Documentation/devicetree/bindings/arm/gic.txt |   55 +++++++++++++++
>   arch/arm/common/gic.c                         |   89 +++++++++++++++++++++++-
>   arch/arm/include/asm/hardware/gic.h           |   12 ++++
>   3 files changed, 152 insertions(+), 4 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> new file mode 100644
> index 0000000..52916b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -0,0 +1,55 @@
> +* ARM Generic Interrupt Controller
> +
> +ARM SMP cores are often associated with a GIC, providing per processor
> +interrupts (PPI), shared processor interrupts (SPI) and software
> +generated interrupts (SGI).
> +
> +Primary GIC is attached directly to the CPU and typically has PPIs and SGIs.
> +Secondary GICs are cascaded into the upward interrupt controller and do not
> +have PPIs or SGIs.
> +
> +Main node required properties:
> +
> +- compatible : should be one of:
> +	"arm,cortex-a9-gic"
> +	"arm,arm11mp-gic"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source.  The type shall be a<u32>  and the value shall be 3.
> +
> +  The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI
> +  interrupts.
> +
> +  The 2nd cell contains the interrupt number for the interrupt type.
> +  SPI interrupts are in the range [0-987].  PPI interrupts are in the
> +  range [0-15].
> +
> +  The 3rd cell is the flags, encoded as follows:
> +	bits[3:0] trigger type and level flags.
> +		1 = low-to-high edge triggered
> +		2 = high-to-low edge triggered
> +		4 = active high level-sensitive
> +		8 = active low level-sensitive
> +	bits[15:8] PPI interrupt cpu mask.  Each bit corresponds to each of
> +	the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
> +	the interrupt is wired to that CPU.  Only valid for PPI interrupts.
> +
> +- reg : Specifies base physical address(s) and size of the GIC registers. The
> +  first region is the GIC distributor register base and size. The 2nd region is
> +  the GIC cpu interface register base and size.
> +
> +Optional
> +- interrupts	: Interrupt source of the parent interrupt controller. Only
> +  present on secondary GICs.
> +
> +Example:
> +
> +	intc: interrupt-controller@fff11000 {
> +		compatible = "arm,cortex-a9-gic";
> +		#interrupt-cells =<3>;
> +		#address-cells =<1>;
> +		interrupt-controller;
> +		reg =<0xfff11000 0x1000>,
> +		<0xfff10100 0x100>;
> +	};
> +
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 666b278..84e69a4 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -28,6 +28,10 @@
>   #include<linux/smp.h>
>   #include<linux/cpumask.h>
>   #include<linux/io.h>
> +#include<linux/of.h>
> +#include<linux/of_address.h>
> +#include<linux/of_irq.h>
> +#include<linux/irqdomain.h>
> 
>   #include<asm/irq.h>
>   #include<asm/mach/irq.h>
> @@ -255,6 +259,15 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>   	irq_set_chained_handler(irq, gic_handle_cascade_irq);
>   }
> 
> +static int gic_irq_count(void __iomem *dist_base)
> +{
> +	int gic_irqs = readl_relaxed(dist_base + GIC_DIST_CTR)&  0x1f;
> +	gic_irqs = (gic_irqs + 1) * 32;
> +	if (gic_irqs>  1020)
> +		gic_irqs = 1020;
> +	return gic_irqs;
> +}
> +
>   static void __init gic_dist_init(struct gic_chip_data *gic,
>   	unsigned int irq_start)
>   {
> @@ -277,10 +290,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>   	 * Find out how many interrupts are supported.
>   	 * The GIC only supports up to 1020 interrupt sources.
>   	 */
> -	gic_irqs = readl_relaxed(base + GIC_DIST_CTR)&  0x1f;
> -	gic_irqs = (gic_irqs + 1) * 32;
> -	if (gic_irqs>  1020)
> -		gic_irqs = 1020;
> +	gic_irqs = gic_irq_count(base);
> 
>   	/*
>   	 * Set all global interrupts to be level triggered, active low.
> @@ -405,3 +415,74 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>   	writel_relaxed(map<<  16 | irq, gic_data[0].dist_base + GIC_DIST_SOFTINT);
>   }
>   #endif
> +
> +#ifdef CONFIG_OF
> +static int gic_cnt __initdata = 0;
> +
> +int gic_irq_domain_dt_translate(struct irq_domain *d,
> +			    struct device_node *controller,
> +			    const u32 *intspec, unsigned int intsize,
> +			    unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	struct gic_chip_data *gic_data = d->priv;
> +
> +	if (d->of_node != controller)
> +		return -EINVAL;
> +	if (intsize<  3)
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[1];
> +	/*
> +	 * We've already skipped over SGIs, so PPIs need no translation.
> +	 * For SPIs, we need to skip over 16 PPIs on primary GICs.
> +	 */
> +	if (!intspec[0]&&  !gic_data->irq_offset)
> +		*out_hwirq += 16;
> +
> +	*out_type = intspec[2]&  IRQ_TYPE_SENSE_MASK;
> +	return 0;
> +}
> +
> +struct irq_domain_ops gic_irq_domain_ops = {
> +	.dt_translate = gic_irq_domain_dt_translate,
> +};
> +
> +int __init gic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	void __iomem *cpu_base;
> +	void __iomem *dist_base;
> +	int irq;
> +	struct irq_domain *domain =&gic_data[gic_cnt].domain;
> +
> +	if (WARN_ON(!node))
> +		return -ENODEV;
> +
> +	dist_base = of_iomap(node, 0);
> +	WARN(!dist_base, "unable to map gic dist registers\n");
> +
> +	cpu_base = of_iomap(node, 1);
> +	WARN(!cpu_base, "unable to map gic cpu registers\n");
> +
> +	domain->nr_irq = gic_irq_count(dist_base);
> +	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
> +	if (parent)
> +		domain->nr_irq -= 32;
> +	else
> +		domain->nr_irq -= 16;
> +
> +	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());
> +	domain->of_node = of_node_get(node);
> +	domain->ops =&gic_irq_domain_ops;
> +	domain->priv =&gic_data[gic_cnt];
> +	irq_domain_add(domain);
> +
> +	gic_init(gic_cnt, domain->irq_base, dist_base, cpu_base);
> +
> +	if (parent) {
> +		irq = irq_of_parse_and_map(node, 0);
> +		gic_cascade_irq(gic_cnt, irq);
> +	}
> +	gic_cnt++;
> +	return 0;
> +}
> +#endif
> diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
> index 435d3f8..2862d0e 100644
> --- a/arch/arm/include/asm/hardware/gic.h
> +++ b/arch/arm/include/asm/hardware/gic.h
> @@ -33,10 +33,21 @@
>   #define GIC_DIST_SOFTINT		0xf00
> 
>   #ifndef __ASSEMBLY__
> +#include<linux/irqdomain.h>
> +
>   extern void __iomem *gic_cpu_base_addr;
>   extern struct irq_chip gic_arch_extn;
> 
>   void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
> +#ifdef CONFIG_OF
> +int gic_of_init(struct device_node *node, struct device_node *parent);
> +#else
> +static inline void gic_of_init(struct device_node *node,
> +			       struct device_node *parent)
> +{
> +	return -ENODEV;
> +}
> +#endif
>   void gic_secondary_init(unsigned int);
>   void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
>   void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
> @@ -46,6 +57,7 @@ struct gic_chip_data {
>   	unsigned int irq_offset;
>   	void __iomem *dist_base;
>   	void __iomem *cpu_base;
> +	struct irq_domain domain;
>   };
>   #endif
> 

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Rob Herring]

Benoit,

On 09/21/2011 12:15 PM, Cousson, Benoit wrote:
> Hi Rob,
> 
> I'm testing that series with OMAP4 but have some issues for the moment :-(
> 
> [    0.000000] WARNING: at kernel/irq/irqdomain.c:34 gic_of_init+0x10c/0x180()
> [    0.000000] error: irq_desc already assigned to a domain
> [    0.000000] Modules linked in:
> [    0.000000] [<c001b284>] (unwind_backtrace+0x0/0xf0) from [<c0051c34>] (warn_slowpath_common+0x4c/0x64)
> [    0.000000] [<c0051c34>] (warn_slowpath_common+0x4c/0x64) from [<c0051ce0>] (warn_slowpath_fmt+0x30/0x40)
> [    0.000000] [<c0051ce0>] (warn_slowpath_fmt+0x30/0x40) from [<c05f6874>] (gic_of_init+0x10c/0x180)
> [    0.000000] [<c05f6874>] (gic_of_init+0x10c/0x180) from [<c05fa2e0>] (omap_gic_of_init+0x8/0x28)
> [    0.000000] [<c05fa2e0>] (omap_gic_of_init+0x8/0x28) from [<c0616b44>] (of_irq_init+0x148/0x28c)
> [    0.000000] [<c0616b44>] (of_irq_init+0x148/0x28c) from [<c05f3074>] (init_IRQ+0x14/0x1c)
> [    0.000000] [<c05f3074>] (init_IRQ+0x14/0x1c) from [<c05f0650>] (start_kernel+0x184/0x2fc)
> [    0.000000] [<c05f0650>] (start_kernel+0x184/0x2fc) from [<80008040>] (0x80008040)
>  
> I'm not super familiar with all the irq stuff but I'm wondering if there is not something wrong with the test that print that message:
> 
> void irq_domain_add(struct irq_domain *domain)
> {
> 	struct irq_data *d;
> 	int hwirq;
> 
> 	/*
> 	 * This assumes that the irq_domain owner has already allocated
> 	 * the irq_descs.  This block will be removed when support for dynamic
> 	 * allocation of irq_descs is added to irq_domain.
> 	 */
> 	for (hwirq = 0; hwirq < domain->nr_irq; hwirq++) {
> 		d = irq_get_irq_data(irq_domain_to_irq(domain, hwirq));
> 		if (d || d->domain) {
> 			/* things are broken; just report, don't clean up */
> 			WARN(1, "error: irq_desc already assigned to a domain");
> 			return;
> 		}
> [...]
> 
> Is the (d || d->domain) correct? Shouldn't it be (d && d->domain)?
> 
> But since that used to work properly, I have some doubt. Moreover the driver will not even get the proper interrupt later...
> 
> Do you have any clue?

I fixed that in the prior series and tglx picked it up, so I did not
repost. It should hit mainline for 3.1, but I haven't verified if it is
in yet. Sorry for the confusion, I should have mentioned that.

Rob

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Cousson, Benoit]

On 9/21/2011 7:55 PM, Rob Herring wrote:

[...]

> I fixed that in the prior series and tglx picked it up, so I did not
> repost. It should hit mainline for 3.1, but I haven't verified if it is
> in yet. Sorry for the confusion, I should have mentioned that.

OK, I remember now as well that Thomas took patched 2 patches from your 
previous one. And I found it:
https://lkml.org/lkml/2011/9/14/190

Now, I have to find why the twl interrupt-controller is not working 
anymore even with that fix.

Almost-Tested-by: Benoit Cousson <b-cousson-l0cyMroinI0@public.gmane.org>

Thanks,
Benoit

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Cousson, Benoit]

On 9/21/2011 9:28 PM, Cousson, Benoit wrote:
> On 9/21/2011 7:55 PM, Rob Herring wrote:
>
> [...]
>
>> I fixed that in the prior series and tglx picked it up, so I did not
>> repost. It should hit mainline for 3.1, but I haven't verified if it is
>> in yet. Sorry for the confusion, I should have mentioned that.
>
> OK, I remember now as well that Thomas took patched 2 patches from your
> previous one. And I found it:
> https://lkml.org/lkml/2011/9/14/190
>
> Now, I have to find why the twl interrupt-controller is not working
> anymore even with that fix.

This is much better with the proper #interrupt-cells size:-)

> Almost-Tested-by: Benoit Cousson<b-cousson-l0cyMroinI0@public.gmane.org>

Tested-by: Benoit Cousson<b-cousson-l0cyMroinI0@public.gmane.org>

With a still very limited number of OMAP4 peripherals for the moment.

Regards,
Benoit

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Jamie Iles]

Hi Rob,

On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
[...]
> +int __init gic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	void __iomem *cpu_base;
> +	void __iomem *dist_base;
> +	int irq;
> +	struct irq_domain *domain = &gic_data[gic_cnt].domain;
> +
> +	if (WARN_ON(!node))
> +		return -ENODEV;
> +
> +	dist_base = of_iomap(node, 0);
> +	WARN(!dist_base, "unable to map gic dist registers\n");
> +
> +	cpu_base = of_iomap(node, 1);
> +	WARN(!cpu_base, "unable to map gic cpu registers\n");
> +
> +	domain->nr_irq = gic_irq_count(dist_base);
> +	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
> +	if (parent)
> +		domain->nr_irq -= 32;
> +	else
> +		domain->nr_irq -= 16;
> +
> +	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());

The way I understand irq_alloc_descs() (probably not very well) is that 
having the irq parameter < 0 and the from parameter 16 means that it 
needs to find domain->nr_irq descs starting from at least 16.  But if 
the base is greater than 16, does this still work with the gic entry 
macros as they are?

Jamie

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Rob Herring]

On 09/26/2011 02:57 PM, Jamie Iles wrote:
> Hi Rob,
> 
> On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
> [...]
>> +int __init gic_of_init(struct device_node *node, struct device_node *parent)
>> +{
>> +	void __iomem *cpu_base;
>> +	void __iomem *dist_base;
>> +	int irq;
>> +	struct irq_domain *domain = &gic_data[gic_cnt].domain;
>> +
>> +	if (WARN_ON(!node))
>> +		return -ENODEV;
>> +
>> +	dist_base = of_iomap(node, 0);
>> +	WARN(!dist_base, "unable to map gic dist registers\n");
>> +
>> +	cpu_base = of_iomap(node, 1);
>> +	WARN(!cpu_base, "unable to map gic cpu registers\n");
>> +
>> +	domain->nr_irq = gic_irq_count(dist_base);
>> +	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
>> +	if (parent)
>> +		domain->nr_irq -= 32;
>> +	else
>> +		domain->nr_irq -= 16;
>> +
>> +	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());
> 
> The way I understand irq_alloc_descs() (probably not very well) is that 
> having the irq parameter < 0 and the from parameter 16 means that it 
> needs to find domain->nr_irq descs starting from at least 16.  But if 
> the base is greater than 16, does this still work with the gic entry 
> macros as they are?

No, but that would only happen if a platform calls irq_alloc_descs prior
to this code. The root controller must be initialized first (for other
reasons as well). There are no calls to irq_alloc_descs in arch/arm.

With the MULTI_IRQ GIC support Marc Z is working on, we could make the
GIC irq mapping be completely dynamic. Although, there's probably not
much reason to do so for the root controller.

Rob

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Jamie Iles]

On Mon, Sep 26, 2011 at 03:49:11PM -0500, Rob Herring wrote:
> On 09/26/2011 02:57 PM, Jamie Iles wrote:
> > Hi Rob,
> > 
> > On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
> > [...]
> >> +int __init gic_of_init(struct device_node *node, struct device_node *parent)
> >> +{
> >> +	void __iomem *cpu_base;
> >> +	void __iomem *dist_base;
> >> +	int irq;
> >> +	struct irq_domain *domain = &gic_data[gic_cnt].domain;
> >> +
> >> +	if (WARN_ON(!node))
> >> +		return -ENODEV;
> >> +
> >> +	dist_base = of_iomap(node, 0);
> >> +	WARN(!dist_base, "unable to map gic dist registers\n");
> >> +
> >> +	cpu_base = of_iomap(node, 1);
> >> +	WARN(!cpu_base, "unable to map gic cpu registers\n");
> >> +
> >> +	domain->nr_irq = gic_irq_count(dist_base);
> >> +	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
> >> +	if (parent)
> >> +		domain->nr_irq -= 32;
> >> +	else
> >> +		domain->nr_irq -= 16;
> >> +
> >> +	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());
> > 
> > The way I understand irq_alloc_descs() (probably not very well) is that 
> > having the irq parameter < 0 and the from parameter 16 means that it 
> > needs to find domain->nr_irq descs starting from at least 16.  But if 
> > the base is greater than 16, does this still work with the gic entry 
> > macros as they are?
> 
> No, but that would only happen if a platform calls irq_alloc_descs prior
> to this code. The root controller must be initialized first (for other
> reasons as well). There are no calls to irq_alloc_descs in arch/arm.
> 
> With the MULTI_IRQ GIC support Marc Z is working on, we could make the
> GIC irq mapping be completely dynamic. Although, there's probably not
> much reason to do so for the root controller.

OK, that makes sense.  I think that if you were to do 
irq_alloc_descs(16, 16, domain->nr_irqs, num_node_id()) then that will 
guarantee the descriptors start from 16 (if they are available) which is 
probably nicer.

Jamie

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Rob Herring]

On 09/26/2011 04:11 PM, Jamie Iles wrote:
> On Mon, Sep 26, 2011 at 03:49:11PM -0500, Rob Herring wrote:
>> On 09/26/2011 02:57 PM, Jamie Iles wrote:
>>> Hi Rob,
>>>
>>> On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
>>> [...]
>>>> +int __init gic_of_init(struct device_node *node, struct device_node *parent)
>>>> +{
>>>> +	void __iomem *cpu_base;
>>>> +	void __iomem *dist_base;
>>>> +	int irq;
>>>> +	struct irq_domain *domain = &gic_data[gic_cnt].domain;
>>>> +
>>>> +	if (WARN_ON(!node))
>>>> +		return -ENODEV;
>>>> +
>>>> +	dist_base = of_iomap(node, 0);
>>>> +	WARN(!dist_base, "unable to map gic dist registers\n");
>>>> +
>>>> +	cpu_base = of_iomap(node, 1);
>>>> +	WARN(!cpu_base, "unable to map gic cpu registers\n");
>>>> +
>>>> +	domain->nr_irq = gic_irq_count(dist_base);
>>>> +	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
>>>> +	if (parent)
>>>> +		domain->nr_irq -= 32;
>>>> +	else
>>>> +		domain->nr_irq -= 16;
>>>> +
>>>> +	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());
>>>
>>> The way I understand irq_alloc_descs() (probably not very well) is that 
>>> having the irq parameter < 0 and the from parameter 16 means that it 
>>> needs to find domain->nr_irq descs starting from at least 16.  But if 
>>> the base is greater than 16, does this still work with the gic entry 
>>> macros as they are?
>>
>> No, but that would only happen if a platform calls irq_alloc_descs prior
>> to this code. The root controller must be initialized first (for other
>> reasons as well). There are no calls to irq_alloc_descs in arch/arm.
>>
>> With the MULTI_IRQ GIC support Marc Z is working on, we could make the
>> GIC irq mapping be completely dynamic. Although, there's probably not
>> much reason to do so for the root controller.
> 
> OK, that makes sense.  I think that if you were to do 
> irq_alloc_descs(16, 16, domain->nr_irqs, num_node_id()) then that will 
> guarantee the descriptors start from 16 (if they are available) which is 
> probably nicer.
> 
That would break secondary GICs though as it would always fail. For
secondary GIC, we skip SGIs and PPIs and allocate the first available
block of irq_desc's.

Rob

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Jamie Iles]

On Mon, Sep 26, 2011 at 04:32:17PM -0500, Rob Herring wrote:
> On 09/26/2011 04:11 PM, Jamie Iles wrote:
> > On Mon, Sep 26, 2011 at 03:49:11PM -0500, Rob Herring wrote:
> >> On 09/26/2011 02:57 PM, Jamie Iles wrote:
> >>> Hi Rob,
> >>>
> >>> On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
> >>> [...]
> >>>> +int __init gic_of_init(struct device_node *node, struct device_node *parent)
> >>>> +{
> >>>> +	void __iomem *cpu_base;
> >>>> +	void __iomem *dist_base;
> >>>> +	int irq;
> >>>> +	struct irq_domain *domain = &gic_data[gic_cnt].domain;
> >>>> +
> >>>> +	if (WARN_ON(!node))
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	dist_base = of_iomap(node, 0);
> >>>> +	WARN(!dist_base, "unable to map gic dist registers\n");
> >>>> +
> >>>> +	cpu_base = of_iomap(node, 1);
> >>>> +	WARN(!cpu_base, "unable to map gic cpu registers\n");
> >>>> +
> >>>> +	domain->nr_irq = gic_irq_count(dist_base);
> >>>> +	/* subtract off SGIs. Also subtract off PPIs for secondary GICs */
> >>>> +	if (parent)
> >>>> +		domain->nr_irq -= 32;
> >>>> +	else
> >>>> +		domain->nr_irq -= 16;
> >>>> +
> >>>> +	domain->irq_base = irq_alloc_descs(-1, 16, domain->nr_irq, numa_node_id());
> >>>
> >>> The way I understand irq_alloc_descs() (probably not very well) is that 
> >>> having the irq parameter < 0 and the from parameter 16 means that it 
> >>> needs to find domain->nr_irq descs starting from at least 16.  But if 
> >>> the base is greater than 16, does this still work with the gic entry 
> >>> macros as they are?
> >>
> >> No, but that would only happen if a platform calls irq_alloc_descs prior
> >> to this code. The root controller must be initialized first (for other
> >> reasons as well). There are no calls to irq_alloc_descs in arch/arm.
> >>
> >> With the MULTI_IRQ GIC support Marc Z is working on, we could make the
> >> GIC irq mapping be completely dynamic. Although, there's probably not
> >> much reason to do so for the root controller.
> > 
> > OK, that makes sense.  I think that if you were to do 
> > irq_alloc_descs(16, 16, domain->nr_irqs, num_node_id()) then that will 
> > guarantee the descriptors start from 16 (if they are available) which is 
> > probably nicer.
> > 
> That would break secondary GICs though as it would always fail. For
> secondary GIC, we skip SGIs and PPIs and allocate the first available
> block of irq_desc's.

Ahh, OK that makes sense.  Thanks for the explanation Rob!  That'll 
hopefully help me with a common VIC binding.

Jamie

Re: [PATCH 3/3] ARM: gic: add OF based initialization

[Jamie Iles]

Hi Rob,

Apologies for the noise!  One minor comment below.

Jamie

On Tue, Sep 20, 2011 at 03:24:04PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> This adds ARM gic interrupt controller initialization using device tree
> data.
> 
> The initialization function is intended to be called by of_irq_init
> function like this:
> 
> const static struct of_device_id irq_match[] = {
>         { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
>         {}
> };
> 
> static void __init init_irqs(void)
> {
>         of_irq_init(irq_match);
> }
> 
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
[...]
> diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
> index 435d3f8..2862d0e 100644
> --- a/arch/arm/include/asm/hardware/gic.h
> +++ b/arch/arm/include/asm/hardware/gic.h
> @@ -33,10 +33,21 @@
>  #define GIC_DIST_SOFTINT		0xf00
>  
>  #ifndef __ASSEMBLY__
> +#include <linux/irqdomain.h>
> +
>  extern void __iomem *gic_cpu_base_addr;
>  extern struct irq_chip gic_arch_extn;
>  
>  void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
> +#ifdef CONFIG_OF
> +int gic_of_init(struct device_node *node, struct device_node *parent);
> +#else
> +static inline void gic_of_init(struct device_node *node,
> +			       struct device_node *parent)

This should return int?

> +{
> +	return -ENODEV;
> +}
> +#endif

Re: [PATCH 0/3] GIC OF bindings

[Shawn Guo]

On Tue, Sep 20, 2011 at 03:24:01PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> Hopefully, this is the final or near final version of GIC binding support.
> 
> Changes from the previous version:
> - SPIs and PPIs are numbered starting at 0. Now the gic has it's own irq
>   domain translate function instead of the simple domain one.
> - interrupt cell format has changed based on Grant's proposal.
> - Dropped "ARM: gic: allow irq_start to be 0". Instead, the first 16 irqs
>   are skipped and the domain irq_base adjusted accordingly.
> - Added a fix to of_irq_find_parent when the parent == child.
> - Renamed intc_desc.parent to intc_desc.interrupt_parent.
> - Implemented Grant's algorithm for walking the list of interrupt
>   controllers. Added a return value to interrupt init functions, so they
>   don't get added to the parent list on a init failure.
> 
> The changes are significant enough that I did not include previous
> acked/reviewed/tested-by's.
> 
You can add mine back.  On imx6q:

Tested-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Hopefully, this is the last round of binding scheme change.  There are
63 nodes in imx6q dts with 'interrupts' property.  I have changed them
3 times :)

-- 
Regards,
Shawn

Re: [PATCH 0/3] GIC OF bindings

[David Miller]

From: Rob Herring <robherring2@gmail.com>
Date: Tue, 20 Sep 2011 15:24:01 -0500

> Hopefully, this is the final or near final version of GIC binding support.
> 
> Changes from the previous version:
> - SPIs and PPIs are numbered starting at 0. Now the gic has it's own irq
>   domain translate function instead of the simple domain one.
> - interrupt cell format has changed based on Grant's proposal.
> - Dropped "ARM: gic: allow irq_start to be 0". Instead, the first 16 irqs
>   are skipped and the domain irq_base adjusted accordingly.
> - Added a fix to of_irq_find_parent when the parent == child.
> - Renamed intc_desc.parent to intc_desc.interrupt_parent.
> - Implemented Grant's algorithm for walking the list of interrupt
>   controllers. Added a return value to interrupt init functions, so they
>   don't get added to the parent list on a init failure.
> 
> The changes are significant enough that I did not include previous
> acked/reviewed/tested-by's.

Just out of curiosity where does this "interrupt-parent" property
come from?

On platforms I am familiar with, the parent path is walked to the root
and we stop at device nodes that have "interrupt-map" and
"interrupt-map-mask" properties.

The map and mask are applied to the "reg" property of the device in
question to see which map entry matches, if a match is found the map
entry contains the translated interrupt.

And this process continues over and over all the way to the root to get
the system interrupt that processor actually deals with.

The mechanism shown here seems overly simplistic and not able to handle
the cases handled by existing OF property schemes in use for several
years on real systems.

Re: [PATCH 0/3] GIC OF bindings

[Grant Likely]

On Tue, Sep 20, 2011 at 8:49 PM, David Miller <davem@davemloft.net> wrote:
> From: Rob Herring <robherring2@gmail.com>
> Date: Tue, 20 Sep 2011 15:24:01 -0500
>
>> Hopefully, this is the final or near final version of GIC binding support.
>>
>> Changes from the previous version:
>> - SPIs and PPIs are numbered starting at 0. Now the gic has it's own irq
>>   domain translate function instead of the simple domain one.
>> - interrupt cell format has changed based on Grant's proposal.
>> - Dropped "ARM: gic: allow irq_start to be 0". Instead, the first 16 irqs
>>   are skipped and the domain irq_base adjusted accordingly.
>> - Added a fix to of_irq_find_parent when the parent == child.
>> - Renamed intc_desc.parent to intc_desc.interrupt_parent.
>> - Implemented Grant's algorithm for walking the list of interrupt
>>   controllers. Added a return value to interrupt init functions, so they
>>   don't get added to the parent list on a init failure.
>>
>> The changes are significant enough that I did not include previous
>> acked/reviewed/tested-by's.
>
> Just out of curiosity where does this "interrupt-parent" property
> come from?
>
> On platforms I am familiar with, the parent path is walked to the root
> and we stop at device nodes that have "interrupt-map" and
> "interrupt-map-mask" properties.
>
> The map and mask are applied to the "reg" property of the device in
> question to see which map entry matches, if a match is found the map
> entry contains the translated interrupt.
>
> And this process continues over and over all the way to the root to get
> the system interrupt that processor actually deals with.
>
> The mechanism shown here seems overly simplistic and not able to handle
> the cases handled by existing OF property schemes in use for several
> years on real systems.

interrupt-parent has been implemented for years on powerpc.  I don't
know if it was ever an Open Firmware thing, but it is in ePAPR [1],
and ARM isn't doing anything novel in that regard.

[1] section 2.4, page 30,
https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf

It is true that is cannot handle all situations, but for those
interrupt-map is still available.

g.

Re: [PATCH 0/3] GIC OF bindings

[Mitch Bradley]

On 9/20/2011 6:14 PM, Grant Likely wrote:
> On Tue, Sep 20, 2011 at 8:49 PM, David Miller<davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>  wrote:
>> From: Rob Herring<robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Date: Tue, 20 Sep 2011 15:24:01 -0500
>>
>>> Hopefully, this is the final or near final version of GIC binding support.
>>>
>>> Changes from the previous version:
>>> - SPIs and PPIs are numbered starting at 0. Now the gic has it's own irq
>>>    domain translate function instead of the simple domain one.
>>> - interrupt cell format has changed based on Grant's proposal.
>>> - Dropped "ARM: gic: allow irq_start to be 0". Instead, the first 16 irqs
>>>    are skipped and the domain irq_base adjusted accordingly.
>>> - Added a fix to of_irq_find_parent when the parent == child.
>>> - Renamed intc_desc.parent to intc_desc.interrupt_parent.
>>> - Implemented Grant's algorithm for walking the list of interrupt
>>>    controllers. Added a return value to interrupt init functions, so they
>>>    don't get added to the parent list on a init failure.
>>>
>>> The changes are significant enough that I did not include previous
>>> acked/reviewed/tested-by's.
>>
>> Just out of curiosity where does this "interrupt-parent" property
>> come from?
>>
>> On platforms I am familiar with, the parent path is walked to the root
>> and we stop at device nodes that have "interrupt-map" and
>> "interrupt-map-mask" properties.
>>
>> The map and mask are applied to the "reg" property of the device in
>> question to see which map entry matches, if a match is found the map
>> entry contains the translated interrupt.
>>
>> And this process continues over and over all the way to the root to get
>> the system interrupt that processor actually deals with.
>>
>> The mechanism shown here seems overly simplistic and not able to handle
>> the cases handled by existing OF property schemes in use for several
>> years on real systems.
>
> interrupt-parent has been implemented for years on powerpc.  I don't
> know if it was ever an Open Firmware thing, but it is in ePAPR [1],
> and ARM isn't doing anything novel in that regard.

interrupt-parent has been part of the interrupt mapping spec from the 
inception of same.

http://www.openfirmware.info/docs/rec.intmap.d09.pdf

>
> [1] section 2.4, page 30,
> https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf
>
> It is true that is cannot handle all situations, but for those
> interrupt-map is still available.
>
> g.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
>

Re: [PATCH 0/3] GIC OF bindings

[David Miller]

From: Mitch Bradley <wmb@firmworks.com>
Date: Tue, 20 Sep 2011 18:58:12 -1000

> interrupt-parent has been part of the interrupt mapping spec from the
> inception of same.
> 
> http://www.openfirmware.info/docs/rec.intmap.d09.pdf

Thanks for the link.

In my vast collection of Sun OF dumps, I cannot find a single instance
where they've made use of this property, which is why I've never seen
it before.

Re: [PATCH 0/3] GIC OF bindings

[Mitch Bradley]

On 9/20/2011 7:21 PM, David Miller wrote:
> From: Mitch Bradley<wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>
> Date: Tue, 20 Sep 2011 18:58:12 -1000
>
>> interrupt-parent has been part of the interrupt mapping spec from the
>> inception of same.
>>
>> http://www.openfirmware.info/docs/rec.intmap.d09.pdf
>
> Thanks for the link.
>
> In my vast collection of Sun OF dumps, I cannot find a single instance
> where they've made use of this property, which is why I've never seen
> it before.

I can't say I'm surprised at that.  Sun hardware tends to be rather more 
hierarchical than is the norm in, for example, the PC space.  This 
particular document came into being during the CHRP era, when IBM and 
Apple were trying to glue a Macintosh legacy I/O system onto the side of 
a PC legacy I/O system.  The result was not pretty.

Sun had at least the semblance of a coherent architectural design and 
review process.

Re: [PATCH 0/3] GIC OF bindings

[Segher Boessenkool]

> Just out of curiosity where does this "interrupt-parent" property
> come from?
>
> On platforms I am familiar with, the parent path is walked to the root
> and we stop at device nodes that have "interrupt-map" and
> "interrupt-map-mask" properties.

"interrupt-parent" is defined in the Open Firmware "interrupt mapping"
recommended practice, the same place as "interrupt-map" etc. are.

> The mechanism shown here seems overly simplistic and not able to  
> handle
> the cases handled by existing OF property schemes in use for several
> years on real systems.

"interrupt-parent" is only meant to be used for the simple cases.  It's
quite handy there.  It is also required in all "interrupt-controller"
nodes that aren't the root of the interrupt tree.


Segher