Grant,
On 09/17/2011 06:53 PM, Grant Likely wrote:
> On Wed, Sep 14, 2011 at 11:31:38AM -0500, Rob Herring wrote:
>> From: Rob Herring <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> ---
>> drivers/of/irq.c | 96
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of_irq.h | 1 +
>> 2 files changed, 97 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 9f689f1..a0cd7e8 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -19,10 +19,13 @@
>> */
>>
>> #include <linux/errno.h>
>> +#include <linux/list.h>
>> +#include <linux/list_sort.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 +389,96 @@ 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 *parent;
>> +};
>> +
>> +typedef void (*irq_init_cb_t)(struct device_node *, struct device_node *);
>> +
>> +static int __init irq_cmp_intc_desc(void *unused, struct list_head *a,
>> + struct list_head *b)
>> +{
>> + const struct intc_desc *da = list_entry(a, typeof(*da), list);
>> + const struct intc_desc *db = list_entry(b, typeof(*db), list);
>> +
>> + /* same parent, so order doesn't matter */
>> + if (da->parent == db->parent)
>> + return 0;
>> +
>> + /* NULL parent comes first */
>> + if (!da->parent && db->parent)
>> + return -1;
>> + if (!db->parent && da->parent)
>> + return 1;
>> +
>> + /* parent node must be before child node */
>> + if (da->dev == db->parent)
>> + return -1;
>> + if (db->dev == da->parent)
>> + return 1;
>
> Does sort_list work for relationships 4 or more levels deep? ie. if
> there was a relationship of A <- B <- C <- D, then B compared with D
> would return 0 from this function which could potentially result in an
> incorrectly ordered list.
>
Doh! Um, 3 levels is enough for everyone!? ;)
> The other option for implementing this would be to take the probe
> deferral approach and not try to sort the list, but instead allow
> probe functions to fail & request retry if the parent hasn't yet been
> probed. I haven't thought enough about it though to say which would
> be the best approach.
>
Considering the list will typically be only a few entries, it is
probably not so important how efficiently we sort or walk the list.
The only way I see controller code knowing if it needs to defer init is
if of_irq_create_mapping fails. The core code could simply do this
itself. However, I would imagine sorting it would be faster than that path.
How about something like this (untested):
int find_order(struct intc_desc *node)
{
struct intc_desc *d;
list_for_each_entry(d, &intc_desc_list, list) {
if (node->parent != d->dev)
continue;
if (d->order < 0)
find_order(d);
node->order = d->order + 1;
break;
}
}
Then rather than sorting, do this:
list_for_each_entry(desc, &intc_desc_list, list)
find_order(desc);
for (order = 0; !list_empty(&intc_desc_list); order++) {
list_for_each_entry_safe(desc, temp_desc, &intc_desc_list,
list) {
if (desc->order != order)
continue;
match = of_match_node(matches, desc->dev);
if (match && match->data) {
irq_init_cb_t irq_init_cb = match->data;
pr_debug("of_irq_init: init %s @ %p, parent
%p\n",
match->compatible, desc->dev,
desc->parent);
irq_init_cb(desc->dev, desc->parent);
}
list_del(&desc->list);
kfree(desc);
}
}
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * 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;
>> + const struct of_device_id *match;
>> + struct intc_desc *desc;
>> + struct intc_desc *temp_desc;
>> + struct list_head intc_desc_list;
>> +
>> + INIT_LIST_HEAD(&intc_desc_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->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
>> + */
>
> I don't believe that this actually turns out to be true (and yes I
> know it is how I originally described it). :-) When the
> interrupt-parent property is at the root of the tree, then the root
> interrupt controller may very well inherit itself as it's interrupt
> parent, and of_irq_find_parent() will still return a value. This
> should probably be considered a bug in of_irq_find_parent(), and it
> should return NULL if the parent is itself.
I did hit this exact issue. There is an easy, but not obvious fix to the
device tree. Simply adding "interupt-parent;" to the root interrupt
controller node will do the trick and override the value in the tree root.
>
> of_irq_find_parent should probably be implemented thusly (completely
> untested); although the only functional change is the line:
> return (p == child) ? NULL : p;
>
> /**
> * of_irq_find_parent - Given a device node, find its interrupt parent node
> * @child: pointer to device node
> *
> * Returns a pointer to the interrupt parent node, or NULL if the
> * interrupt parent could not be determined.
> */
> struct device_node *of_irq_find_parent(struct device_node *child)
> {
> struct device_node *p, *c = child;
> const __be32 *parp;
>
> if (!of_node_get(c))
> return NULL;
>
> do {
> p = of_parse_phandle(c, "interrupt-parent", 0);
>
> if (!p && (of_irq_workarounds & OF_IMAP_NO_PHANDLE) &&
> of_find_property(c, "interrupt-parent", NULL))
> p = of_node_get(of_irq_dflt_pic);
>
> if (!p)
> p = of_get_parent(c);
>
> of_node_put(c);
> c = p;
> } while (p && !of_find_property(p, "#interrupt-cells", NULL));
>
> return (p == child) ? NULL : p;
> }
>
This change should probably be implemented as well as this is likely a
common occurrence that will be stumbled over or existing device trees
won't have this. I'll test and add to the next series.
Rob
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
