[PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Pantelis Antoniou]

A nice side-effect of the changes in DTC for supporting overlays
is that it is now possible to do dependency tracking of platform
devices automatically.

This patch implements dependency aware probe order for users
of of_platform_populate.

There are no changes in the syntax of the DTS bindings, the
dependency is generated automatically by the use of phandle
references.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/platform.c | 569 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 564 insertions(+), 5 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index cd87a36..0683d48 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -464,6 +464,549 @@ int of_platform_bus_probe(struct device_node *root,
 }
 EXPORT_SYMBOL(of_platform_bus_probe);
 
+struct of_pop_ref_entry {
+	struct list_head node;
+	struct device_node *np;
+};
+
+struct of_pop_dep_entry {
+	struct list_head node;
+	struct of_pop_entry *pe;
+};
+
+struct of_pop_entry {
+	struct of_pop_entry *parent;
+	struct list_head children;
+	struct list_head node;
+
+	struct device_node *np;
+	unsigned int bus : 1;
+	unsigned int amba : 1;
+	unsigned int children_loop : 1;
+	struct list_head refs;
+	struct list_head deps;
+
+	unsigned int loop : 1;
+	unsigned int temp_mark : 1;
+	unsigned int perm_mark : 1;
+	struct list_head sort_children;
+	struct list_head sort_node;
+	int refcnt;
+
+	int id;
+};
+
+static phandle __phandle_ref(struct device_node *lfnp, const char *prop,
+		uint32_t off)
+{
+	struct device_node *np;
+	const void *value;
+	const char *name;
+	int len;
+
+	name = of_node_full_name(lfnp);
+	name += strlen("/__local_fixups__");
+	/* pr_info("%s: %s\n", __func__, name); */
+	np = of_find_node_by_path(name);
+	if (!np)
+		return 0;
+	value = of_get_property(np, prop, &len);
+	if (off + sizeof(uint32_t) > len)
+		return 0;
+	return be32_to_cpup(value + off);
+}
+
+/* returns true is np_ref is pointing to an external tree */
+static bool __external_ref(struct device_node *np, struct device_node *np_ref)
+{
+	while (np_ref != NULL) {
+		if (np_ref == np)
+			return false;
+		np_ref = np_ref->parent;
+	}
+	return true;
+}
+
+/* returns true is np_ref is pointing to an internal tree */
+static bool __internal_ref(struct device_node *np, struct device_node *np_ref)
+{
+	do {
+		if (np_ref == np)
+			return true;
+		np_ref = np_ref->parent;
+	} while (np_ref != NULL);
+	return false;
+}
+
+static void __local_fixup_ref(struct of_pop_entry *pe, struct device_node *lfnp)
+{
+	struct device_node *child;
+	struct property *prop;
+	int i, len;
+	uint32_t off;
+	phandle ph;
+	struct device_node *phnp;
+	struct of_pop_ref_entry *re;
+	bool found;
+
+	if (!lfnp)
+		return;
+
+	for_each_property_of_node(lfnp, prop) {
+
+		/* skip the auto-generated properties */
+		if (!of_prop_cmp(prop->name, "name") ||
+		    !of_prop_cmp(prop->name, "phandle") ||
+		    !of_prop_cmp(prop->name, "linux,phandle") ||
+		    !of_prop_cmp(prop->name, "ibm,phandle"))
+			continue;
+
+		len = prop->length / 4;
+
+		/* pr_info("%s/%s (%d)\n", of_node_full_name(lfnp),
+				prop->name, len); */
+
+		for (i = 0; i < len; i++) {
+			off = be32_to_cpup((void *)prop->value + i * 4);
+			ph = __phandle_ref(lfnp, prop->name, off);
+			if (ph == 0)
+				continue;
+			phnp = of_find_node_by_phandle(ph);
+			if (!phnp)
+				continue;
+			if (!__external_ref(pe->np, phnp))
+				continue;
+
+			/* check whether the ref is already there */
+			found = false;
+			list_for_each_entry(re, &pe->refs, node) {
+				if (re->np == phnp) {
+					found = true;
+					break;
+				}
+			}
+
+			if (!found) {
+				re = kzalloc(sizeof(*re), GFP_KERNEL);
+				BUG_ON(re == NULL);
+				re->np = phnp;
+				list_add_tail(&re->node, &pe->refs);
+
+			}
+
+			/* pr_info("  %08x - ph %08x - @%s\n", off, ph,
+					of_node_full_name(phnp)); */
+
+			of_node_put(phnp);
+		}
+	}
+
+	for_each_child_of_node(lfnp, child)
+		__local_fixup_ref(pe, child);
+}
+
+static void __of_platform_populate_get_refs_internal(struct of_pop_entry *pe)
+{
+	struct device_node *np;
+	struct of_pop_ref_entry *re;
+	char *base;
+	bool found;
+
+	/* first find the local fixups */
+	base = kasprintf(GFP_KERNEL, "/__local_fixups__%s",
+			of_node_full_name(pe->np));
+	if (base) {
+		np = of_find_node_by_path(base);
+		if (np) {
+			__local_fixup_ref(pe, np);
+			of_node_put(np);
+		}
+		kfree(base);
+	}
+
+	/* now try the old style interrupt */
+	if (of_get_property(pe->np, "interrupts", NULL) &&
+			(np = of_irq_find_parent(pe->np)) != NULL) {
+
+		/* check whether the ref is already there */
+		found = false;
+		list_for_each_entry(re, &pe->refs, node) {
+			if (re->np == np) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			re = kzalloc(sizeof(*re), GFP_KERNEL);
+			BUG_ON(re == NULL);
+			re->np = np;
+			list_add_tail(&re->node, &pe->refs);
+
+		}
+
+		/* pr_info("  %08x - ph %08x - @%s\n", off, ph,
+				of_node_full_name(phnp)); */
+		of_node_put(np);
+	}
+}
+
+static void __of_platform_populate_scan_internal(struct device_node *root,
+			const struct of_device_id *matches,
+			struct of_pop_entry *pep, int level)
+{
+	struct device_node *child;
+	struct of_pop_entry *pe;
+
+	BUG_ON(root == NULL);
+
+	for_each_child_of_node(root, child) {
+
+		/* of_platform_bus_create (strict) */
+		if (!of_get_property(child, "compatible", NULL) ||
+			!of_device_is_available(child) ||
+			of_node_check_flag(child, OF_POPULATED))
+			continue;
+
+		pe = kzalloc(sizeof(*pe), GFP_KERNEL);
+		BUG_ON(pe == NULL);
+		pe->parent = pep;
+		INIT_LIST_HEAD(&pe->node);
+		INIT_LIST_HEAD(&pe->children);
+		INIT_LIST_HEAD(&pe->refs);
+		INIT_LIST_HEAD(&pe->deps);
+		INIT_LIST_HEAD(&pe->sort_children);
+		INIT_LIST_HEAD(&pe->sort_node);
+		pe->np = child;
+
+		list_add_tail(&pe->node, &pep->children);
+
+		if (of_device_is_compatible(pe->np, "arm,primecell"))
+			pe->amba = 1;
+		else if (of_match_node(matches, pe->np)) {
+			pe->bus = 1;
+			__of_platform_populate_scan_internal(child, matches, pe, level + 1);
+		}
+	}
+}
+
+static void __of_platform_get_refs(struct of_pop_entry *pep, int level)
+{
+	struct of_pop_entry *pe;
+
+	__of_platform_populate_get_refs_internal(pep);
+
+	list_for_each_entry(pe, &pep->children, node) {
+		if (pe->bus)
+			__of_platform_get_refs(pe, level + 1);
+		__of_platform_populate_get_refs_internal(pe);
+	}
+}
+
+static void __of_platform_make_deps_internal(struct of_pop_entry *pe)
+{
+	struct of_pop_ref_entry *re;
+	struct of_pop_entry *ppe, *tpe;
+	struct of_pop_dep_entry *de;
+	bool found;
+
+	/* for each ref, find sibling that contains it */
+	list_for_each_entry(re, &pe->refs, node) {
+		if (!pe->parent)
+			continue;
+		ppe = pe->parent;
+		list_for_each_entry(tpe, &ppe->children, node) {
+			if (tpe == pe)
+				continue;
+			if (!__internal_ref(tpe->np, re->np))
+				continue;
+
+			/* check whether the ref is already there */
+			found = false;
+			list_for_each_entry(de, &pe->deps, node) {
+				if (de->pe == tpe) {
+					found = true;
+					break;
+				}
+			}
+
+			if (!found) {
+				de = kzalloc(sizeof(*de), GFP_KERNEL);
+				BUG_ON(de == NULL);
+				de->pe = tpe;
+				tpe->refcnt++;
+				list_add_tail(&de->node, &pe->deps);
+
+				/* pr_info("* %s depends on %s\n",
+						of_node_full_name(pe->np),
+						of_node_full_name(tpe->np)); */
+			}
+		}
+	}
+}
+
+static void __of_platform_make_deps(struct of_pop_entry *pep, int level)
+{
+	struct of_pop_entry *pe;
+
+	__of_platform_make_deps_internal(pep);
+	list_for_each_entry(pe, &pep->children, node)
+		__of_platform_make_deps(pe, level + 1);
+}
+
+static int __of_platform_visit(struct of_pop_entry *pe)
+{
+	struct of_pop_dep_entry *de;
+	bool circle;
+	int rc;
+
+	/* don't do anything with root or a visited node */
+	if (!pe->parent || pe->perm_mark)
+		return 0;
+
+	/* cycle */
+	circle = false;
+	if (pe->temp_mark) {
+		pr_info("%s: circle at @%s\n", __func__,
+				of_node_full_name(pe->np));
+		circle = true;
+	} else {
+		pe->temp_mark = 1;
+		list_for_each_entry(de, &pe->deps, node) {
+			rc = __of_platform_visit(de->pe);
+			if (rc != 0)
+				circle = true;
+		}
+		pe->temp_mark = 0;
+		list_add_tail(&pe->sort_node, &pe->parent->sort_children);
+	}
+
+	pe->perm_mark = 1;
+
+	if (circle)
+		pe->loop = 1;
+
+	return circle ? -1 : 0;
+}
+
+static int __of_platform_reorder_internal(struct of_pop_entry *pep)
+{
+	struct of_pop_entry *pe;
+	int ret;
+	bool circle;
+
+	circle = false;
+	list_for_each_entry(pe, &pep->children, node) {
+		ret = __of_platform_visit(pe);
+		if (ret != 0)
+			circle = true;
+	}
+	return circle ? -1 : 0;
+}
+
+static void __of_platform_reorder(struct of_pop_entry *pep, int level)
+{
+	struct of_pop_entry *pe;
+	int ret;
+
+	ret = __of_platform_reorder_internal(pep);
+	if (ret != 0) {
+		pr_info("%s: circle at @%s\n", __func__,
+			of_node_full_name(pep->np));
+		pep->children_loop = 1;
+	}
+
+	list_for_each_entry(pe, &pep->children, node)
+		__of_platform_reorder(pe, level + 1);
+}
+
+static int __of_platform_assign_order(struct of_pop_entry *pep, int level, int id)
+{
+	struct of_pop_entry *pe;
+
+	pep->id = id++;
+	list_for_each_entry(pe, &pep->sort_children, sort_node)
+		id = __of_platform_assign_order(pe, level + 1, id);
+	return id;
+}
+
+static int __count_children(struct of_pop_entry *pep)
+{
+	struct of_pop_entry *pe;
+	int cnt;
+
+	cnt = 0;
+	list_for_each_entry(pe, &pep->children, node)
+		cnt++;
+	return cnt;
+}
+
+static int __count_sort_children(struct of_pop_entry *pep)
+{
+	struct of_pop_entry *pe;
+	int cnt;
+
+	cnt = 0;
+	list_for_each_entry(pe, &pep->sort_children, sort_node)
+		cnt++;
+	return cnt;
+}
+
+static void __of_platform_populate_scan_dump(struct of_pop_entry *pep, int level)
+{
+	struct of_pop_entry *pe;
+	struct of_pop_ref_entry *re;
+	struct of_pop_dep_entry *de;
+
+	pr_debug("| %s %*s @%s (%d) - count=%d\n",
+		pep->bus ? "BUS" : (pep->amba ? "AMB" : "PLT"),
+		level * 4, "", of_node_full_name(pep->np), pep->refcnt,
+		__count_children(pep));
+	list_for_each_entry(re, &pep->refs, node)
+		pr_debug("+     %*s @%s\n", level * 4, "", of_node_full_name(re->np));
+	list_for_each_entry(de, &pep->deps, node)
+		pr_debug(">     %*s @%s\n", level * 4, "", of_node_full_name(de->pe->np));
+
+	list_for_each_entry(pe, &pep->children, node)
+		__of_platform_populate_scan_dump(pe, level + 1);
+}
+
+static void __of_platform_populate_scan_sort_dump(struct of_pop_entry *pep, int level)
+{
+	struct of_pop_entry *pe;
+	struct of_pop_dep_entry *de;
+
+	pr_debug("* %s %*s @%s (%d) - sort-count=%d - id=%d\n",
+		pep->bus ? "BUS" : (pep->amba ? "AMB" : "PLT"),
+		level * 4, "", of_node_full_name(pep->np), pep->refcnt,
+		__count_sort_children(pep), pep->id);
+	list_for_each_entry(de, &pep->deps, node)
+		pr_debug("%%     %*s @%s - id=%d\n", level * 4, "",
+				of_node_full_name(de->pe->np), de->pe->id);
+
+	list_for_each_entry(pe, &pep->sort_children, sort_node)
+		__of_platform_populate_scan_sort_dump(pe, level + 1);
+}
+
+static void __of_platform_check_dep_order(struct of_pop_entry *pep, int level)
+{
+	struct of_pop_entry *pe;
+	struct of_pop_dep_entry *de;
+
+	list_for_each_entry(de, &pep->deps, node) {
+		if (de->pe->id >= pep->id) {
+			pr_info("%s: backwards reference @%s(%d) to @%s(%d)\n", __func__,
+					of_node_full_name(pep->np), pep->id,
+					of_node_full_name(de->pe->np), de->pe->id);
+		}
+	}
+
+	list_for_each_entry(pe, &pep->sort_children, sort_node)
+		__of_platform_check_dep_order(pe, level + 1);
+}
+
+
+static int __of_platform_populate_probe(struct of_pop_entry *pep,
+			const struct of_device_id *matches,
+			const struct of_dev_auxdata *lookup,
+			struct device *parent,
+			int level)
+{
+	struct of_pop_entry *pe;
+	const char *bus_id = NULL;
+	void *platform_data = NULL;
+	const struct of_dev_auxdata *auxdata;
+	struct platform_device *dev;
+	int rc = 0;
+
+	if (level > 0) {
+		auxdata = of_dev_lookup(lookup, pep->np);
+		if (auxdata) {
+			bus_id = auxdata->name;
+			platform_data = auxdata->platform_data;
+		}
+
+		if (pep->amba) {
+			/* pr_info("of_amba_device_create(%s, %s, ..)\n",
+					of_node_full_name(pep->np), bus_id ? bus_id : "<NULL>"); */
+			of_amba_device_create(pep->np, bus_id, platform_data, parent);
+			return 0;
+		}
+
+		/* pr_info("of_platform_device_create_pdata(%s, %s, ..)\n",
+				of_node_full_name(pep->np), bus_id ? bus_id : "<NULL>"); */
+		dev = of_platform_device_create_pdata(pep->np, bus_id, platform_data, parent);
+		if (!dev || !of_match_node(matches, pep->np))
+			return 0;
+	}
+
+	list_for_each_entry(pe, &pep->children, node) {
+		rc = __of_platform_populate_probe(pe, matches, lookup, parent,
+				level + 1);
+		if (rc)
+			break;
+	}
+
+	of_node_set_flag(pep->np, OF_POPULATED_BUS);
+
+	return rc;
+}
+
+
+static void __of_platform_populate_free(struct of_pop_entry *pep)
+{
+	struct of_pop_entry *pe, *pen;
+	struct of_pop_ref_entry *re, *ren;
+	struct of_pop_dep_entry *de, *den;
+
+	list_for_each_entry_safe(pe, pen, &pep->children, node) {
+		list_del(&pe->node);
+		__of_platform_populate_free(pe);
+	}
+
+	list_for_each_entry_safe(re, ren, &pep->refs, node) {
+		list_del(&re->node);
+		kfree(re);
+	}
+	list_for_each_entry_safe(de, den, &pep->deps, node) {
+		list_del(&de->node);
+		kfree(de);
+	}
+
+	kfree(pep);
+}
+
+
+static struct of_pop_entry *__of_platform_populate_scan(struct device_node *root,
+			const struct of_device_id *matches)
+{
+	struct of_pop_entry *pe;
+	struct device_node *np;
+
+	/* if no local fixups are present do not proceed */
+	np = of_find_node_by_path("/__local_fixups__");
+	if (!np)
+		return NULL;
+
+	of_node_put(np);
+
+	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
+	if (pe == NULL)
+		return NULL;
+	pe->parent = NULL;
+	INIT_LIST_HEAD(&pe->node);
+	INIT_LIST_HEAD(&pe->children);
+	INIT_LIST_HEAD(&pe->refs);
+	INIT_LIST_HEAD(&pe->deps);
+	INIT_LIST_HEAD(&pe->sort_children);
+	INIT_LIST_HEAD(&pe->sort_node);
+	pe->np = root;
+
+	__of_platform_populate_scan_internal(root, matches, pe, 0);
+	return pe;
+}
+
+
 /**
  * of_platform_populate() - Populate platform_devices from device tree data
  * @root: parent of the first level to probe or NULL for the root of the tree
@@ -488,18 +1031,34 @@ int of_platform_populate(struct device_node *root,
 			const struct of_dev_auxdata *lookup,
 			struct device *parent)
 {
-	struct device_node *child;
+	struct of_pop_entry *pe;
 	int rc = 0;
 
 	root = root ? of_node_get(root) : of_find_node_by_path("/");
 	if (!root)
 		return -EINVAL;
 
-	for_each_child_of_node(root, child) {
-		rc = of_platform_bus_create(child, matches, lookup, parent, true);
-		if (rc)
-			break;
+	pe = __of_platform_populate_scan(root, matches);
+	if (pe) {
+		__of_platform_get_refs(pe, 0);
+		__of_platform_make_deps(pe, 0);
+		__of_platform_populate_scan_dump(pe, 0);
+		__of_platform_reorder(pe, 0);
+		__of_platform_assign_order(pe, 0, 0);
+		__of_platform_populate_scan_sort_dump(pe, 0);
+		__of_platform_check_dep_order(pe, 0);
+		rc = __of_platform_populate_probe(pe, matches, lookup, parent, 0);
+		__of_platform_populate_free(pe);
+	} else {
+		struct device_node *child;
+
+		for_each_child_of_node(root, child) {
+			rc = of_platform_bus_create(child, matches, lookup, parent, true);
+			if (rc)
+				break;
+		}
 	}
+
 	of_node_set_flag(root, OF_POPULATED_BUS);
 
 	of_node_put(root);
-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Grant Likely]

On Tue, 16 Dec 2014 14:11:31 +0200
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> A nice side-effect of the changes in DTC for supporting overlays
> is that it is now possible to do dependency tracking of platform
> devices automatically.
> 
> This patch implements dependency aware probe order for users
> of of_platform_populate.
> 
> There are no changes in the syntax of the DTS bindings, the
> dependency is generated automatically by the use of phandle
> references.

Do you have measurements showing improvement? Conceptually, I don't have
a problem with having a small scale solution like this, but I want proof
that it actively makes things better, and is worth the extra complexity.
It's not an easy block of code to understand.

However, this only papers over a fundamental limitation of the device
model, and only works within the context of a single
platform_device_populate() call. It doesn't help for subsequent calls,
and it does nothing for dependecies with i2c/spi/whatever. It also
doesn't help much when drivers are in modules that won't get loaded
until after userspace starts. (although changing device registration
order may mostly get userspace to load drivers in the right order).

g.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/of/platform.c | 569 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 564 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index cd87a36..0683d48 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -464,6 +464,549 @@ int of_platform_bus_probe(struct device_node *root,
>  }
>  EXPORT_SYMBOL(of_platform_bus_probe);
>  
> +struct of_pop_ref_entry {
> +	struct list_head node;
> +	struct device_node *np;
> +};
> +
> +struct of_pop_dep_entry {
> +	struct list_head node;
> +	struct of_pop_entry *pe;
> +};
> +
> +struct of_pop_entry {
> +	struct of_pop_entry *parent;
> +	struct list_head children;
> +	struct list_head node;
> +
> +	struct device_node *np;
> +	unsigned int bus : 1;
> +	unsigned int amba : 1;
> +	unsigned int children_loop : 1;
> +	struct list_head refs;
> +	struct list_head deps;
> +
> +	unsigned int loop : 1;
> +	unsigned int temp_mark : 1;
> +	unsigned int perm_mark : 1;
> +	struct list_head sort_children;
> +	struct list_head sort_node;
> +	int refcnt;
> +
> +	int id;
> +};
> +
> +static phandle __phandle_ref(struct device_node *lfnp, const char *prop,
> +		uint32_t off)
> +{
> +	struct device_node *np;
> +	const void *value;
> +	const char *name;
> +	int len;
> +
> +	name = of_node_full_name(lfnp);
> +	name += strlen("/__local_fixups__");
> +	/* pr_info("%s: %s\n", __func__, name); */
> +	np = of_find_node_by_path(name);
> +	if (!np)
> +		return 0;
> +	value = of_get_property(np, prop, &len);
> +	if (off + sizeof(uint32_t) > len)
> +		return 0;
> +	return be32_to_cpup(value + off);
> +}
> +
> +/* returns true is np_ref is pointing to an external tree */
> +static bool __external_ref(struct device_node *np, struct device_node *np_ref)
> +{
> +	while (np_ref != NULL) {
> +		if (np_ref == np)
> +			return false;
> +		np_ref = np_ref->parent;
> +	}
> +	return true;
> +}
> +
> +/* returns true is np_ref is pointing to an internal tree */
> +static bool __internal_ref(struct device_node *np, struct device_node *np_ref)
> +{
> +	do {
> +		if (np_ref == np)
> +			return true;
> +		np_ref = np_ref->parent;
> +	} while (np_ref != NULL);
> +	return false;
> +}
> +
> +static void __local_fixup_ref(struct of_pop_entry *pe, struct device_node *lfnp)
> +{
> +	struct device_node *child;
> +	struct property *prop;
> +	int i, len;
> +	uint32_t off;
> +	phandle ph;
> +	struct device_node *phnp;
> +	struct of_pop_ref_entry *re;
> +	bool found;
> +
> +	if (!lfnp)
> +		return;
> +
> +	for_each_property_of_node(lfnp, prop) {
> +
> +		/* skip the auto-generated properties */
> +		if (!of_prop_cmp(prop->name, "name") ||
> +		    !of_prop_cmp(prop->name, "phandle") ||
> +		    !of_prop_cmp(prop->name, "linux,phandle") ||
> +		    !of_prop_cmp(prop->name, "ibm,phandle"))
> +			continue;
> +
> +		len = prop->length / 4;
> +
> +		/* pr_info("%s/%s (%d)\n", of_node_full_name(lfnp),
> +				prop->name, len); */
> +
> +		for (i = 0; i < len; i++) {
> +			off = be32_to_cpup((void *)prop->value + i * 4);
> +			ph = __phandle_ref(lfnp, prop->name, off);
> +			if (ph == 0)
> +				continue;
> +			phnp = of_find_node_by_phandle(ph);
> +			if (!phnp)
> +				continue;
> +			if (!__external_ref(pe->np, phnp))
> +				continue;
> +
> +			/* check whether the ref is already there */
> +			found = false;
> +			list_for_each_entry(re, &pe->refs, node) {
> +				if (re->np == phnp) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			if (!found) {
> +				re = kzalloc(sizeof(*re), GFP_KERNEL);
> +				BUG_ON(re == NULL);
> +				re->np = phnp;
> +				list_add_tail(&re->node, &pe->refs);
> +
> +			}
> +
> +			/* pr_info("  %08x - ph %08x - @%s\n", off, ph,
> +					of_node_full_name(phnp)); */
> +
> +			of_node_put(phnp);
> +		}
> +	}
> +
> +	for_each_child_of_node(lfnp, child)
> +		__local_fixup_ref(pe, child);
> +}
> +
> +static void __of_platform_populate_get_refs_internal(struct of_pop_entry *pe)
> +{
> +	struct device_node *np;
> +	struct of_pop_ref_entry *re;
> +	char *base;
> +	bool found;
> +
> +	/* first find the local fixups */
> +	base = kasprintf(GFP_KERNEL, "/__local_fixups__%s",
> +			of_node_full_name(pe->np));
> +	if (base) {
> +		np = of_find_node_by_path(base);
> +		if (np) {
> +			__local_fixup_ref(pe, np);
> +			of_node_put(np);
> +		}
> +		kfree(base);
> +	}
> +
> +	/* now try the old style interrupt */
> +	if (of_get_property(pe->np, "interrupts", NULL) &&
> +			(np = of_irq_find_parent(pe->np)) != NULL) {
> +
> +		/* check whether the ref is already there */
> +		found = false;
> +		list_for_each_entry(re, &pe->refs, node) {
> +			if (re->np == np) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found) {
> +			re = kzalloc(sizeof(*re), GFP_KERNEL);
> +			BUG_ON(re == NULL);
> +			re->np = np;
> +			list_add_tail(&re->node, &pe->refs);
> +
> +		}
> +
> +		/* pr_info("  %08x - ph %08x - @%s\n", off, ph,
> +				of_node_full_name(phnp)); */
> +		of_node_put(np);
> +	}
> +}
> +
> +static void __of_platform_populate_scan_internal(struct device_node *root,
> +			const struct of_device_id *matches,
> +			struct of_pop_entry *pep, int level)
> +{
> +	struct device_node *child;
> +	struct of_pop_entry *pe;
> +
> +	BUG_ON(root == NULL);
> +
> +	for_each_child_of_node(root, child) {
> +
> +		/* of_platform_bus_create (strict) */
> +		if (!of_get_property(child, "compatible", NULL) ||
> +			!of_device_is_available(child) ||
> +			of_node_check_flag(child, OF_POPULATED))
> +			continue;
> +
> +		pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +		BUG_ON(pe == NULL);
> +		pe->parent = pep;
> +		INIT_LIST_HEAD(&pe->node);
> +		INIT_LIST_HEAD(&pe->children);
> +		INIT_LIST_HEAD(&pe->refs);
> +		INIT_LIST_HEAD(&pe->deps);
> +		INIT_LIST_HEAD(&pe->sort_children);
> +		INIT_LIST_HEAD(&pe->sort_node);
> +		pe->np = child;
> +
> +		list_add_tail(&pe->node, &pep->children);
> +
> +		if (of_device_is_compatible(pe->np, "arm,primecell"))
> +			pe->amba = 1;
> +		else if (of_match_node(matches, pe->np)) {
> +			pe->bus = 1;
> +			__of_platform_populate_scan_internal(child, matches, pe, level + 1);
> +		}
> +	}
> +}
> +
> +static void __of_platform_get_refs(struct of_pop_entry *pep, int level)
> +{
> +	struct of_pop_entry *pe;
> +
> +	__of_platform_populate_get_refs_internal(pep);
> +
> +	list_for_each_entry(pe, &pep->children, node) {
> +		if (pe->bus)
> +			__of_platform_get_refs(pe, level + 1);
> +		__of_platform_populate_get_refs_internal(pe);
> +	}
> +}
> +
> +static void __of_platform_make_deps_internal(struct of_pop_entry *pe)
> +{
> +	struct of_pop_ref_entry *re;
> +	struct of_pop_entry *ppe, *tpe;
> +	struct of_pop_dep_entry *de;
> +	bool found;
> +
> +	/* for each ref, find sibling that contains it */
> +	list_for_each_entry(re, &pe->refs, node) {
> +		if (!pe->parent)
> +			continue;
> +		ppe = pe->parent;
> +		list_for_each_entry(tpe, &ppe->children, node) {
> +			if (tpe == pe)
> +				continue;
> +			if (!__internal_ref(tpe->np, re->np))
> +				continue;
> +
> +			/* check whether the ref is already there */
> +			found = false;
> +			list_for_each_entry(de, &pe->deps, node) {
> +				if (de->pe == tpe) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			if (!found) {
> +				de = kzalloc(sizeof(*de), GFP_KERNEL);
> +				BUG_ON(de == NULL);
> +				de->pe = tpe;
> +				tpe->refcnt++;
> +				list_add_tail(&de->node, &pe->deps);
> +
> +				/* pr_info("* %s depends on %s\n",
> +						of_node_full_name(pe->np),
> +						of_node_full_name(tpe->np)); */
> +			}
> +		}
> +	}
> +}
> +
> +static void __of_platform_make_deps(struct of_pop_entry *pep, int level)
> +{
> +	struct of_pop_entry *pe;
> +
> +	__of_platform_make_deps_internal(pep);
> +	list_for_each_entry(pe, &pep->children, node)
> +		__of_platform_make_deps(pe, level + 1);
> +}
> +
> +static int __of_platform_visit(struct of_pop_entry *pe)
> +{
> +	struct of_pop_dep_entry *de;
> +	bool circle;
> +	int rc;
> +
> +	/* don't do anything with root or a visited node */
> +	if (!pe->parent || pe->perm_mark)
> +		return 0;
> +
> +	/* cycle */
> +	circle = false;
> +	if (pe->temp_mark) {
> +		pr_info("%s: circle at @%s\n", __func__,
> +				of_node_full_name(pe->np));
> +		circle = true;
> +	} else {
> +		pe->temp_mark = 1;
> +		list_for_each_entry(de, &pe->deps, node) {
> +			rc = __of_platform_visit(de->pe);
> +			if (rc != 0)
> +				circle = true;
> +		}
> +		pe->temp_mark = 0;
> +		list_add_tail(&pe->sort_node, &pe->parent->sort_children);
> +	}
> +
> +	pe->perm_mark = 1;
> +
> +	if (circle)
> +		pe->loop = 1;
> +
> +	return circle ? -1 : 0;
> +}
> +
> +static int __of_platform_reorder_internal(struct of_pop_entry *pep)
> +{
> +	struct of_pop_entry *pe;
> +	int ret;
> +	bool circle;
> +
> +	circle = false;
> +	list_for_each_entry(pe, &pep->children, node) {
> +		ret = __of_platform_visit(pe);
> +		if (ret != 0)
> +			circle = true;
> +	}
> +	return circle ? -1 : 0;
> +}
> +
> +static void __of_platform_reorder(struct of_pop_entry *pep, int level)
> +{
> +	struct of_pop_entry *pe;
> +	int ret;
> +
> +	ret = __of_platform_reorder_internal(pep);
> +	if (ret != 0) {
> +		pr_info("%s: circle at @%s\n", __func__,
> +			of_node_full_name(pep->np));
> +		pep->children_loop = 1;
> +	}
> +
> +	list_for_each_entry(pe, &pep->children, node)
> +		__of_platform_reorder(pe, level + 1);
> +}
> +
> +static int __of_platform_assign_order(struct of_pop_entry *pep, int level, int id)
> +{
> +	struct of_pop_entry *pe;
> +
> +	pep->id = id++;
> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
> +		id = __of_platform_assign_order(pe, level + 1, id);
> +	return id;
> +}
> +
> +static int __count_children(struct of_pop_entry *pep)
> +{
> +	struct of_pop_entry *pe;
> +	int cnt;
> +
> +	cnt = 0;
> +	list_for_each_entry(pe, &pep->children, node)
> +		cnt++;
> +	return cnt;
> +}
> +
> +static int __count_sort_children(struct of_pop_entry *pep)
> +{
> +	struct of_pop_entry *pe;
> +	int cnt;
> +
> +	cnt = 0;
> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
> +		cnt++;
> +	return cnt;
> +}
> +
> +static void __of_platform_populate_scan_dump(struct of_pop_entry *pep, int level)
> +{
> +	struct of_pop_entry *pe;
> +	struct of_pop_ref_entry *re;
> +	struct of_pop_dep_entry *de;
> +
> +	pr_debug("| %s %*s @%s (%d) - count=%d\n",
> +		pep->bus ? "BUS" : (pep->amba ? "AMB" : "PLT"),
> +		level * 4, "", of_node_full_name(pep->np), pep->refcnt,
> +		__count_children(pep));
> +	list_for_each_entry(re, &pep->refs, node)
> +		pr_debug("+     %*s @%s\n", level * 4, "", of_node_full_name(re->np));
> +	list_for_each_entry(de, &pep->deps, node)
> +		pr_debug(">     %*s @%s\n", level * 4, "", of_node_full_name(de->pe->np));
> +
> +	list_for_each_entry(pe, &pep->children, node)
> +		__of_platform_populate_scan_dump(pe, level + 1);
> +}
> +
> +static void __of_platform_populate_scan_sort_dump(struct of_pop_entry *pep, int level)
> +{
> +	struct of_pop_entry *pe;
> +	struct of_pop_dep_entry *de;
> +
> +	pr_debug("* %s %*s @%s (%d) - sort-count=%d - id=%d\n",
> +		pep->bus ? "BUS" : (pep->amba ? "AMB" : "PLT"),
> +		level * 4, "", of_node_full_name(pep->np), pep->refcnt,
> +		__count_sort_children(pep), pep->id);
> +	list_for_each_entry(de, &pep->deps, node)
> +		pr_debug("%%     %*s @%s - id=%d\n", level * 4, "",
> +				of_node_full_name(de->pe->np), de->pe->id);
> +
> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
> +		__of_platform_populate_scan_sort_dump(pe, level + 1);
> +}
> +
> +static void __of_platform_check_dep_order(struct of_pop_entry *pep, int level)
> +{
> +	struct of_pop_entry *pe;
> +	struct of_pop_dep_entry *de;
> +
> +	list_for_each_entry(de, &pep->deps, node) {
> +		if (de->pe->id >= pep->id) {
> +			pr_info("%s: backwards reference @%s(%d) to @%s(%d)\n", __func__,
> +					of_node_full_name(pep->np), pep->id,
> +					of_node_full_name(de->pe->np), de->pe->id);
> +		}
> +	}
> +
> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
> +		__of_platform_check_dep_order(pe, level + 1);
> +}
> +
> +
> +static int __of_platform_populate_probe(struct of_pop_entry *pep,
> +			const struct of_device_id *matches,
> +			const struct of_dev_auxdata *lookup,
> +			struct device *parent,
> +			int level)
> +{
> +	struct of_pop_entry *pe;
> +	const char *bus_id = NULL;
> +	void *platform_data = NULL;
> +	const struct of_dev_auxdata *auxdata;
> +	struct platform_device *dev;
> +	int rc = 0;
> +
> +	if (level > 0) {
> +		auxdata = of_dev_lookup(lookup, pep->np);
> +		if (auxdata) {
> +			bus_id = auxdata->name;
> +			platform_data = auxdata->platform_data;
> +		}
> +
> +		if (pep->amba) {
> +			/* pr_info("of_amba_device_create(%s, %s, ..)\n",
> +					of_node_full_name(pep->np), bus_id ? bus_id : "<NULL>"); */
> +			of_amba_device_create(pep->np, bus_id, platform_data, parent);
> +			return 0;
> +		}
> +
> +		/* pr_info("of_platform_device_create_pdata(%s, %s, ..)\n",
> +				of_node_full_name(pep->np), bus_id ? bus_id : "<NULL>"); */
> +		dev = of_platform_device_create_pdata(pep->np, bus_id, platform_data, parent);
> +		if (!dev || !of_match_node(matches, pep->np))
> +			return 0;
> +	}
> +
> +	list_for_each_entry(pe, &pep->children, node) {
> +		rc = __of_platform_populate_probe(pe, matches, lookup, parent,
> +				level + 1);
> +		if (rc)
> +			break;
> +	}
> +
> +	of_node_set_flag(pep->np, OF_POPULATED_BUS);
> +
> +	return rc;
> +}
> +
> +
> +static void __of_platform_populate_free(struct of_pop_entry *pep)
> +{
> +	struct of_pop_entry *pe, *pen;
> +	struct of_pop_ref_entry *re, *ren;
> +	struct of_pop_dep_entry *de, *den;
> +
> +	list_for_each_entry_safe(pe, pen, &pep->children, node) {
> +		list_del(&pe->node);
> +		__of_platform_populate_free(pe);
> +	}
> +
> +	list_for_each_entry_safe(re, ren, &pep->refs, node) {
> +		list_del(&re->node);
> +		kfree(re);
> +	}
> +	list_for_each_entry_safe(de, den, &pep->deps, node) {
> +		list_del(&de->node);
> +		kfree(de);
> +	}
> +
> +	kfree(pep);
> +}
> +
> +
> +static struct of_pop_entry *__of_platform_populate_scan(struct device_node *root,
> +			const struct of_device_id *matches)
> +{
> +	struct of_pop_entry *pe;
> +	struct device_node *np;
> +
> +	/* if no local fixups are present do not proceed */
> +	np = of_find_node_by_path("/__local_fixups__");
> +	if (!np)
> +		return NULL;
> +
> +	of_node_put(np);
> +
> +	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +	if (pe == NULL)
> +		return NULL;
> +	pe->parent = NULL;
> +	INIT_LIST_HEAD(&pe->node);
> +	INIT_LIST_HEAD(&pe->children);
> +	INIT_LIST_HEAD(&pe->refs);
> +	INIT_LIST_HEAD(&pe->deps);
> +	INIT_LIST_HEAD(&pe->sort_children);
> +	INIT_LIST_HEAD(&pe->sort_node);
> +	pe->np = root;
> +
> +	__of_platform_populate_scan_internal(root, matches, pe, 0);
> +	return pe;
> +}
> +
> +
>  /**
>   * of_platform_populate() - Populate platform_devices from device tree data
>   * @root: parent of the first level to probe or NULL for the root of the tree
> @@ -488,18 +1031,34 @@ int of_platform_populate(struct device_node *root,
>  			const struct of_dev_auxdata *lookup,
>  			struct device *parent)
>  {
> -	struct device_node *child;
> +	struct of_pop_entry *pe;
>  	int rc = 0;
>  
>  	root = root ? of_node_get(root) : of_find_node_by_path("/");
>  	if (!root)
>  		return -EINVAL;
>  
> -	for_each_child_of_node(root, child) {
> -		rc = of_platform_bus_create(child, matches, lookup, parent, true);
> -		if (rc)
> -			break;
> +	pe = __of_platform_populate_scan(root, matches);
> +	if (pe) {
> +		__of_platform_get_refs(pe, 0);
> +		__of_platform_make_deps(pe, 0);
> +		__of_platform_populate_scan_dump(pe, 0);
> +		__of_platform_reorder(pe, 0);
> +		__of_platform_assign_order(pe, 0, 0);
> +		__of_platform_populate_scan_sort_dump(pe, 0);
> +		__of_platform_check_dep_order(pe, 0);
> +		rc = __of_platform_populate_probe(pe, matches, lookup, parent, 0);
> +		__of_platform_populate_free(pe);
> +	} else {
> +		struct device_node *child;
> +
> +		for_each_child_of_node(root, child) {
> +			rc = of_platform_bus_create(child, matches, lookup, parent, true);
> +			if (rc)
> +				break;
> +		}
>  	}
> +
>  	of_node_set_flag(root, OF_POPULATED_BUS);
>  
>  	of_node_put(root);
> -- 
> 1.7.12
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Pantelis Antoniou]

Hi Grant,

> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> On Tue, 16 Dec 2014 14:11:31 +0200
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> wrote:
>> A nice side-effect of the changes in DTC for supporting overlays
>> is that it is now possible to do dependency tracking of platform
>> devices automatically.
>> 
>> This patch implements dependency aware probe order for users
>> of of_platform_populate.
>> 
>> There are no changes in the syntax of the DTS bindings, the
>> dependency is generated automatically by the use of phandle
>> references.
> 
> Do you have measurements showing improvement? Conceptually, I don't have
> a problem with having a small scale solution like this, but I want proof
> that it actively makes things better, and is worth the extra complexity.
> It's not an easy block of code to understand.
> 

I will be the first to admit that the code it’s a bit hard to follow, but
that’s the nature of trees and recursion.

FWIW I’ve been booting with this applied for a month with no adverse effects,
besides the fact that there dependency cycles which I would file as a bug.


> However, this only papers over a fundamental limitation of the device
> model, and only works within the context of a single
> platform_device_populate() call. It doesn't help for subsequent calls,
> and it does nothing for dependecies with i2c/spi/whatever. It also
> doesn't help much when drivers are in modules that won't get loaded
> until after userspace starts. (although changing device registration
> order may mostly get userspace to load drivers in the right order).
> 

True, it only works within that single context. The logic however does
recurse into the children nodes and does rearrange the order of even
i2c/spi phandle references. So while it does nothing for the subsequent
populate() calls it does the ‘right thing’ for the probe order at that level.

The block of code that does the ‘re-arranging’ can trivially be factored out
into a general purpose of-helper so that would work for all other users besides
platform devices.

> g.
> 

Admittedly this is all a bit rough and a proof of concept, but that’s why it’s an
RFC, no? 

I would be happy to discuss the details of this at ELC. Think you can arrange for another
DT BoF?

Regards

— Pantelis

>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>> drivers/of/platform.c | 569 +++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 564 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index cd87a36..0683d48 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -464,6 +464,549 @@ int of_platform_bus_probe(struct device_node *root,
>> }
>> EXPORT_SYMBOL(of_platform_bus_probe);
>> 
>> +struct of_pop_ref_entry {
>> +	struct list_head node;
>> +	struct device_node *np;
>> +};
>> +
>> +struct of_pop_dep_entry {
>> +	struct list_head node;
>> +	struct of_pop_entry *pe;
>> +};
>> +
>> +struct of_pop_entry {
>> +	struct of_pop_entry *parent;
>> +	struct list_head children;
>> +	struct list_head node;
>> +
>> +	struct device_node *np;
>> +	unsigned int bus : 1;
>> +	unsigned int amba : 1;
>> +	unsigned int children_loop : 1;
>> +	struct list_head refs;
>> +	struct list_head deps;
>> +
>> +	unsigned int loop : 1;
>> +	unsigned int temp_mark : 1;
>> +	unsigned int perm_mark : 1;
>> +	struct list_head sort_children;
>> +	struct list_head sort_node;
>> +	int refcnt;
>> +
>> +	int id;
>> +};
>> +
>> +static phandle __phandle_ref(struct device_node *lfnp, const char *prop,
>> +		uint32_t off)
>> +{
>> +	struct device_node *np;
>> +	const void *value;
>> +	const char *name;
>> +	int len;
>> +
>> +	name = of_node_full_name(lfnp);
>> +	name += strlen("/__local_fixups__");
>> +	/* pr_info("%s: %s\n", __func__, name); */
>> +	np = of_find_node_by_path(name);
>> +	if (!np)
>> +		return 0;
>> +	value = of_get_property(np, prop, &len);
>> +	if (off + sizeof(uint32_t) > len)
>> +		return 0;
>> +	return be32_to_cpup(value + off);
>> +}
>> +
>> +/* returns true is np_ref is pointing to an external tree */
>> +static bool __external_ref(struct device_node *np, struct device_node *np_ref)
>> +{
>> +	while (np_ref != NULL) {
>> +		if (np_ref == np)
>> +			return false;
>> +		np_ref = np_ref->parent;
>> +	}
>> +	return true;
>> +}
>> +
>> +/* returns true is np_ref is pointing to an internal tree */
>> +static bool __internal_ref(struct device_node *np, struct device_node *np_ref)
>> +{
>> +	do {
>> +		if (np_ref == np)
>> +			return true;
>> +		np_ref = np_ref->parent;
>> +	} while (np_ref != NULL);
>> +	return false;
>> +}
>> +
>> +static void __local_fixup_ref(struct of_pop_entry *pe, struct device_node *lfnp)
>> +{
>> +	struct device_node *child;
>> +	struct property *prop;
>> +	int i, len;
>> +	uint32_t off;
>> +	phandle ph;
>> +	struct device_node *phnp;
>> +	struct of_pop_ref_entry *re;
>> +	bool found;
>> +
>> +	if (!lfnp)
>> +		return;
>> +
>> +	for_each_property_of_node(lfnp, prop) {
>> +
>> +		/* skip the auto-generated properties */
>> +		if (!of_prop_cmp(prop->name, "name") ||
>> +		    !of_prop_cmp(prop->name, "phandle") ||
>> +		    !of_prop_cmp(prop->name, "linux,phandle") ||
>> +		    !of_prop_cmp(prop->name, "ibm,phandle"))
>> +			continue;
>> +
>> +		len = prop->length / 4;
>> +
>> +		/* pr_info("%s/%s (%d)\n", of_node_full_name(lfnp),
>> +				prop->name, len); */
>> +
>> +		for (i = 0; i < len; i++) {
>> +			off = be32_to_cpup((void *)prop->value + i * 4);
>> +			ph = __phandle_ref(lfnp, prop->name, off);
>> +			if (ph == 0)
>> +				continue;
>> +			phnp = of_find_node_by_phandle(ph);
>> +			if (!phnp)
>> +				continue;
>> +			if (!__external_ref(pe->np, phnp))
>> +				continue;
>> +
>> +			/* check whether the ref is already there */
>> +			found = false;
>> +			list_for_each_entry(re, &pe->refs, node) {
>> +				if (re->np == phnp) {
>> +					found = true;
>> +					break;
>> +				}
>> +			}
>> +
>> +			if (!found) {
>> +				re = kzalloc(sizeof(*re), GFP_KERNEL);
>> +				BUG_ON(re == NULL);
>> +				re->np = phnp;
>> +				list_add_tail(&re->node, &pe->refs);
>> +
>> +			}
>> +
>> +			/* pr_info("  %08x - ph %08x - @%s\n", off, ph,
>> +					of_node_full_name(phnp)); */
>> +
>> +			of_node_put(phnp);
>> +		}
>> +	}
>> +
>> +	for_each_child_of_node(lfnp, child)
>> +		__local_fixup_ref(pe, child);
>> +}
>> +
>> +static void __of_platform_populate_get_refs_internal(struct of_pop_entry *pe)
>> +{
>> +	struct device_node *np;
>> +	struct of_pop_ref_entry *re;
>> +	char *base;
>> +	bool found;
>> +
>> +	/* first find the local fixups */
>> +	base = kasprintf(GFP_KERNEL, "/__local_fixups__%s",
>> +			of_node_full_name(pe->np));
>> +	if (base) {
>> +		np = of_find_node_by_path(base);
>> +		if (np) {
>> +			__local_fixup_ref(pe, np);
>> +			of_node_put(np);
>> +		}
>> +		kfree(base);
>> +	}
>> +
>> +	/* now try the old style interrupt */
>> +	if (of_get_property(pe->np, "interrupts", NULL) &&
>> +			(np = of_irq_find_parent(pe->np)) != NULL) {
>> +
>> +		/* check whether the ref is already there */
>> +		found = false;
>> +		list_for_each_entry(re, &pe->refs, node) {
>> +			if (re->np == np) {
>> +				found = true;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!found) {
>> +			re = kzalloc(sizeof(*re), GFP_KERNEL);
>> +			BUG_ON(re == NULL);
>> +			re->np = np;
>> +			list_add_tail(&re->node, &pe->refs);
>> +
>> +		}
>> +
>> +		/* pr_info("  %08x - ph %08x - @%s\n", off, ph,
>> +				of_node_full_name(phnp)); */
>> +		of_node_put(np);
>> +	}
>> +}
>> +
>> +static void __of_platform_populate_scan_internal(struct device_node *root,
>> +			const struct of_device_id *matches,
>> +			struct of_pop_entry *pep, int level)
>> +{
>> +	struct device_node *child;
>> +	struct of_pop_entry *pe;
>> +
>> +	BUG_ON(root == NULL);
>> +
>> +	for_each_child_of_node(root, child) {
>> +
>> +		/* of_platform_bus_create (strict) */
>> +		if (!of_get_property(child, "compatible", NULL) ||
>> +			!of_device_is_available(child) ||
>> +			of_node_check_flag(child, OF_POPULATED))
>> +			continue;
>> +
>> +		pe = kzalloc(sizeof(*pe), GFP_KERNEL);
>> +		BUG_ON(pe == NULL);
>> +		pe->parent = pep;
>> +		INIT_LIST_HEAD(&pe->node);
>> +		INIT_LIST_HEAD(&pe->children);
>> +		INIT_LIST_HEAD(&pe->refs);
>> +		INIT_LIST_HEAD(&pe->deps);
>> +		INIT_LIST_HEAD(&pe->sort_children);
>> +		INIT_LIST_HEAD(&pe->sort_node);
>> +		pe->np = child;
>> +
>> +		list_add_tail(&pe->node, &pep->children);
>> +
>> +		if (of_device_is_compatible(pe->np, "arm,primecell"))
>> +			pe->amba = 1;
>> +		else if (of_match_node(matches, pe->np)) {
>> +			pe->bus = 1;
>> +			__of_platform_populate_scan_internal(child, matches, pe, level + 1);
>> +		}
>> +	}
>> +}
>> +
>> +static void __of_platform_get_refs(struct of_pop_entry *pep, int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +
>> +	__of_platform_populate_get_refs_internal(pep);
>> +
>> +	list_for_each_entry(pe, &pep->children, node) {
>> +		if (pe->bus)
>> +			__of_platform_get_refs(pe, level + 1);
>> +		__of_platform_populate_get_refs_internal(pe);
>> +	}
>> +}
>> +
>> +static void __of_platform_make_deps_internal(struct of_pop_entry *pe)
>> +{
>> +	struct of_pop_ref_entry *re;
>> +	struct of_pop_entry *ppe, *tpe;
>> +	struct of_pop_dep_entry *de;
>> +	bool found;
>> +
>> +	/* for each ref, find sibling that contains it */
>> +	list_for_each_entry(re, &pe->refs, node) {
>> +		if (!pe->parent)
>> +			continue;
>> +		ppe = pe->parent;
>> +		list_for_each_entry(tpe, &ppe->children, node) {
>> +			if (tpe == pe)
>> +				continue;
>> +			if (!__internal_ref(tpe->np, re->np))
>> +				continue;
>> +
>> +			/* check whether the ref is already there */
>> +			found = false;
>> +			list_for_each_entry(de, &pe->deps, node) {
>> +				if (de->pe == tpe) {
>> +					found = true;
>> +					break;
>> +				}
>> +			}
>> +
>> +			if (!found) {
>> +				de = kzalloc(sizeof(*de), GFP_KERNEL);
>> +				BUG_ON(de == NULL);
>> +				de->pe = tpe;
>> +				tpe->refcnt++;
>> +				list_add_tail(&de->node, &pe->deps);
>> +
>> +				/* pr_info("* %s depends on %s\n",
>> +						of_node_full_name(pe->np),
>> +						of_node_full_name(tpe->np)); */
>> +			}
>> +		}
>> +	}
>> +}
>> +
>> +static void __of_platform_make_deps(struct of_pop_entry *pep, int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +
>> +	__of_platform_make_deps_internal(pep);
>> +	list_for_each_entry(pe, &pep->children, node)
>> +		__of_platform_make_deps(pe, level + 1);
>> +}
>> +
>> +static int __of_platform_visit(struct of_pop_entry *pe)
>> +{
>> +	struct of_pop_dep_entry *de;
>> +	bool circle;
>> +	int rc;
>> +
>> +	/* don't do anything with root or a visited node */
>> +	if (!pe->parent || pe->perm_mark)
>> +		return 0;
>> +
>> +	/* cycle */
>> +	circle = false;
>> +	if (pe->temp_mark) {
>> +		pr_info("%s: circle at @%s\n", __func__,
>> +				of_node_full_name(pe->np));
>> +		circle = true;
>> +	} else {
>> +		pe->temp_mark = 1;
>> +		list_for_each_entry(de, &pe->deps, node) {
>> +			rc = __of_platform_visit(de->pe);
>> +			if (rc != 0)
>> +				circle = true;
>> +		}
>> +		pe->temp_mark = 0;
>> +		list_add_tail(&pe->sort_node, &pe->parent->sort_children);
>> +	}
>> +
>> +	pe->perm_mark = 1;
>> +
>> +	if (circle)
>> +		pe->loop = 1;
>> +
>> +	return circle ? -1 : 0;
>> +}
>> +
>> +static int __of_platform_reorder_internal(struct of_pop_entry *pep)
>> +{
>> +	struct of_pop_entry *pe;
>> +	int ret;
>> +	bool circle;
>> +
>> +	circle = false;
>> +	list_for_each_entry(pe, &pep->children, node) {
>> +		ret = __of_platform_visit(pe);
>> +		if (ret != 0)
>> +			circle = true;
>> +	}
>> +	return circle ? -1 : 0;
>> +}
>> +
>> +static void __of_platform_reorder(struct of_pop_entry *pep, int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +	int ret;
>> +
>> +	ret = __of_platform_reorder_internal(pep);
>> +	if (ret != 0) {
>> +		pr_info("%s: circle at @%s\n", __func__,
>> +			of_node_full_name(pep->np));
>> +		pep->children_loop = 1;
>> +	}
>> +
>> +	list_for_each_entry(pe, &pep->children, node)
>> +		__of_platform_reorder(pe, level + 1);
>> +}
>> +
>> +static int __of_platform_assign_order(struct of_pop_entry *pep, int level, int id)
>> +{
>> +	struct of_pop_entry *pe;
>> +
>> +	pep->id = id++;
>> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
>> +		id = __of_platform_assign_order(pe, level + 1, id);
>> +	return id;
>> +}
>> +
>> +static int __count_children(struct of_pop_entry *pep)
>> +{
>> +	struct of_pop_entry *pe;
>> +	int cnt;
>> +
>> +	cnt = 0;
>> +	list_for_each_entry(pe, &pep->children, node)
>> +		cnt++;
>> +	return cnt;
>> +}
>> +
>> +static int __count_sort_children(struct of_pop_entry *pep)
>> +{
>> +	struct of_pop_entry *pe;
>> +	int cnt;
>> +
>> +	cnt = 0;
>> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
>> +		cnt++;
>> +	return cnt;
>> +}
>> +
>> +static void __of_platform_populate_scan_dump(struct of_pop_entry *pep, int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +	struct of_pop_ref_entry *re;
>> +	struct of_pop_dep_entry *de;
>> +
>> +	pr_debug("| %s %*s @%s (%d) - count=%d\n",
>> +		pep->bus ? "BUS" : (pep->amba ? "AMB" : "PLT"),
>> +		level * 4, "", of_node_full_name(pep->np), pep->refcnt,
>> +		__count_children(pep));
>> +	list_for_each_entry(re, &pep->refs, node)
>> +		pr_debug("+     %*s @%s\n", level * 4, "", of_node_full_name(re->np));
>> +	list_for_each_entry(de, &pep->deps, node)
>> +		pr_debug(">     %*s @%s\n", level * 4, "", of_node_full_name(de->pe->np));
>> +
>> +	list_for_each_entry(pe, &pep->children, node)
>> +		__of_platform_populate_scan_dump(pe, level + 1);
>> +}
>> +
>> +static void __of_platform_populate_scan_sort_dump(struct of_pop_entry *pep, int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +	struct of_pop_dep_entry *de;
>> +
>> +	pr_debug("* %s %*s @%s (%d) - sort-count=%d - id=%d\n",
>> +		pep->bus ? "BUS" : (pep->amba ? "AMB" : "PLT"),
>> +		level * 4, "", of_node_full_name(pep->np), pep->refcnt,
>> +		__count_sort_children(pep), pep->id);
>> +	list_for_each_entry(de, &pep->deps, node)
>> +		pr_debug("%%     %*s @%s - id=%d\n", level * 4, "",
>> +				of_node_full_name(de->pe->np), de->pe->id);
>> +
>> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
>> +		__of_platform_populate_scan_sort_dump(pe, level + 1);
>> +}
>> +
>> +static void __of_platform_check_dep_order(struct of_pop_entry *pep, int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +	struct of_pop_dep_entry *de;
>> +
>> +	list_for_each_entry(de, &pep->deps, node) {
>> +		if (de->pe->id >= pep->id) {
>> +			pr_info("%s: backwards reference @%s(%d) to @%s(%d)\n", __func__,
>> +					of_node_full_name(pep->np), pep->id,
>> +					of_node_full_name(de->pe->np), de->pe->id);
>> +		}
>> +	}
>> +
>> +	list_for_each_entry(pe, &pep->sort_children, sort_node)
>> +		__of_platform_check_dep_order(pe, level + 1);
>> +}
>> +
>> +
>> +static int __of_platform_populate_probe(struct of_pop_entry *pep,
>> +			const struct of_device_id *matches,
>> +			const struct of_dev_auxdata *lookup,
>> +			struct device *parent,
>> +			int level)
>> +{
>> +	struct of_pop_entry *pe;
>> +	const char *bus_id = NULL;
>> +	void *platform_data = NULL;
>> +	const struct of_dev_auxdata *auxdata;
>> +	struct platform_device *dev;
>> +	int rc = 0;
>> +
>> +	if (level > 0) {
>> +		auxdata = of_dev_lookup(lookup, pep->np);
>> +		if (auxdata) {
>> +			bus_id = auxdata->name;
>> +			platform_data = auxdata->platform_data;
>> +		}
>> +
>> +		if (pep->amba) {
>> +			/* pr_info("of_amba_device_create(%s, %s, ..)\n",
>> +					of_node_full_name(pep->np), bus_id ? bus_id : "<NULL>"); */
>> +			of_amba_device_create(pep->np, bus_id, platform_data, parent);
>> +			return 0;
>> +		}
>> +
>> +		/* pr_info("of_platform_device_create_pdata(%s, %s, ..)\n",
>> +				of_node_full_name(pep->np), bus_id ? bus_id : "<NULL>"); */
>> +		dev = of_platform_device_create_pdata(pep->np, bus_id, platform_data, parent);
>> +		if (!dev || !of_match_node(matches, pep->np))
>> +			return 0;
>> +	}
>> +
>> +	list_for_each_entry(pe, &pep->children, node) {
>> +		rc = __of_platform_populate_probe(pe, matches, lookup, parent,
>> +				level + 1);
>> +		if (rc)
>> +			break;
>> +	}
>> +
>> +	of_node_set_flag(pep->np, OF_POPULATED_BUS);
>> +
>> +	return rc;
>> +}
>> +
>> +
>> +static void __of_platform_populate_free(struct of_pop_entry *pep)
>> +{
>> +	struct of_pop_entry *pe, *pen;
>> +	struct of_pop_ref_entry *re, *ren;
>> +	struct of_pop_dep_entry *de, *den;
>> +
>> +	list_for_each_entry_safe(pe, pen, &pep->children, node) {
>> +		list_del(&pe->node);
>> +		__of_platform_populate_free(pe);
>> +	}
>> +
>> +	list_for_each_entry_safe(re, ren, &pep->refs, node) {
>> +		list_del(&re->node);
>> +		kfree(re);
>> +	}
>> +	list_for_each_entry_safe(de, den, &pep->deps, node) {
>> +		list_del(&de->node);
>> +		kfree(de);
>> +	}
>> +
>> +	kfree(pep);
>> +}
>> +
>> +
>> +static struct of_pop_entry *__of_platform_populate_scan(struct device_node *root,
>> +			const struct of_device_id *matches)
>> +{
>> +	struct of_pop_entry *pe;
>> +	struct device_node *np;
>> +
>> +	/* if no local fixups are present do not proceed */
>> +	np = of_find_node_by_path("/__local_fixups__");
>> +	if (!np)
>> +		return NULL;
>> +
>> +	of_node_put(np);
>> +
>> +	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
>> +	if (pe == NULL)
>> +		return NULL;
>> +	pe->parent = NULL;
>> +	INIT_LIST_HEAD(&pe->node);
>> +	INIT_LIST_HEAD(&pe->children);
>> +	INIT_LIST_HEAD(&pe->refs);
>> +	INIT_LIST_HEAD(&pe->deps);
>> +	INIT_LIST_HEAD(&pe->sort_children);
>> +	INIT_LIST_HEAD(&pe->sort_node);
>> +	pe->np = root;
>> +
>> +	__of_platform_populate_scan_internal(root, matches, pe, 0);
>> +	return pe;
>> +}
>> +
>> +
>> /**
>>  * of_platform_populate() - Populate platform_devices from device tree data
>>  * @root: parent of the first level to probe or NULL for the root of the tree
>> @@ -488,18 +1031,34 @@ int of_platform_populate(struct device_node *root,
>> 			const struct of_dev_auxdata *lookup,
>> 			struct device *parent)
>> {
>> -	struct device_node *child;
>> +	struct of_pop_entry *pe;
>> 	int rc = 0;
>> 
>> 	root = root ? of_node_get(root) : of_find_node_by_path("/");
>> 	if (!root)
>> 		return -EINVAL;
>> 
>> -	for_each_child_of_node(root, child) {
>> -		rc = of_platform_bus_create(child, matches, lookup, parent, true);
>> -		if (rc)
>> -			break;
>> +	pe = __of_platform_populate_scan(root, matches);
>> +	if (pe) {
>> +		__of_platform_get_refs(pe, 0);
>> +		__of_platform_make_deps(pe, 0);
>> +		__of_platform_populate_scan_dump(pe, 0);
>> +		__of_platform_reorder(pe, 0);
>> +		__of_platform_assign_order(pe, 0, 0);
>> +		__of_platform_populate_scan_sort_dump(pe, 0);
>> +		__of_platform_check_dep_order(pe, 0);
>> +		rc = __of_platform_populate_probe(pe, matches, lookup, parent, 0);
>> +		__of_platform_populate_free(pe);
>> +	} else {
>> +		struct device_node *child;
>> +
>> +		for_each_child_of_node(root, child) {
>> +			rc = of_platform_bus_create(child, matches, lookup, parent, true);
>> +			if (rc)
>> +				break;
>> +		}
>> 	}
>> +
>> 	of_node_set_flag(root, OF_POPULATED_BUS);
>> 
>> 	of_node_put(root);
>> -- 
>> 1.7.12
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Grant Likely]

On Fri, Mar 20, 2015 at 11:39 AM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Hi Grant,
>
>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>
>> On Tue, 16 Dec 2014 14:11:31 +0200
>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> wrote:
>>> A nice side-effect of the changes in DTC for supporting overlays
>>> is that it is now possible to do dependency tracking of platform
>>> devices automatically.
>>>
>>> This patch implements dependency aware probe order for users
>>> of of_platform_populate.
>>>
>>> There are no changes in the syntax of the DTS bindings, the
>>> dependency is generated automatically by the use of phandle
>>> references.
>>
>> Do you have measurements showing improvement? Conceptually, I don't have
>> a problem with having a small scale solution like this, but I want proof
>> that it actively makes things better, and is worth the extra complexity.
>> It's not an easy block of code to understand.
>>
>
> I will be the first to admit that the code it's a bit hard to follow, but
> that's the nature of trees and recursion.
>
> FWIW I've been booting with this applied for a month with no adverse effects,
> besides the fact that there dependency cycles which I would file as a bug.
>
>
>> However, this only papers over a fundamental limitation of the device
>> model, and only works within the context of a single
>> platform_device_populate() call. It doesn't help for subsequent calls,
>> and it does nothing for dependecies with i2c/spi/whatever. It also
>> doesn't help much when drivers are in modules that won't get loaded
>> until after userspace starts. (although changing device registration
>> order may mostly get userspace to load drivers in the right order).
>>
>
> True, it only works within that single context. The logic however does
> recurse into the children nodes and does rearrange the order of even
> i2c/spi phandle references. So while it does nothing for the subsequent
> populate() calls it does the 'right thing' for the probe order at that level.
>
> The block of code that does the 're-arranging' can trivially be factored out
> into a general purpose of-helper so that would work for all other users besides
> platform devices.
>
>> g.
>>
>
> Admittedly this is all a bit rough and a proof of concept, but that's why it's an
> RFC, no?
>
> I would be happy to discuss the details of this at ELC. Think you can arrange for another
> DT BoF?

I'd be happy to chat at ELC. Unfortunately, I will only be there on
Wednesday, so my time is limited.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Geert Uytterhoeven]

Hi Pantelis, Grant,

On Fri, Mar 20, 2015 at 12:39 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>
>> On Tue, 16 Dec 2014 14:11:31 +0200
>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> wrote:
>>> A nice side-effect of the changes in DTC for supporting overlays
>>> is that it is now possible to do dependency tracking of platform
>>> devices automatically.
>>>
>>> This patch implements dependency aware probe order for users
>>> of of_platform_populate.
>>>
>>> There are no changes in the syntax of the DTS bindings, the
>>> dependency is generated automatically by the use of phandle
>>> references.
>>
>> Do you have measurements showing improvement? Conceptually, I don't have
>> a problem with having a small scale solution like this, but I want proof
>> that it actively makes things better, and is worth the extra complexity.
>> It's not an easy block of code to understand.
>>
>
> I will be the first to admit that the code it’s a bit hard to follow, but
> that’s the nature of trees and recursion.
>
> FWIW I’ve been booting with this applied for a month with no adverse effects,
> besides the fact that there dependency cycles which I would file as a bug.

IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
Move pfc node to work around probe ordering bug"?
https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91

I'll give it a try when I'm back from ELC...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Pantelis Antoniou]

Hi Geert,

> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> 
> Hi Pantelis, Grant,
> 
> On Fri, Mar 20, 2015 at 12:39 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>> 
>>> On Tue, 16 Dec 2014 14:11:31 +0200
>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>> wrote:
>>>> A nice side-effect of the changes in DTC for supporting overlays
>>>> is that it is now possible to do dependency tracking of platform
>>>> devices automatically.
>>>> 
>>>> This patch implements dependency aware probe order for users
>>>> of of_platform_populate.
>>>> 
>>>> There are no changes in the syntax of the DTS bindings, the
>>>> dependency is generated automatically by the use of phandle
>>>> references.
>>> 
>>> Do you have measurements showing improvement? Conceptually, I don't have
>>> a problem with having a small scale solution like this, but I want proof
>>> that it actively makes things better, and is worth the extra complexity.
>>> It's not an easy block of code to understand.
>>> 
>> 
>> I will be the first to admit that the code it’s a bit hard to follow, but
>> that’s the nature of trees and recursion.
>> 
>> FWIW I’ve been booting with this applied for a month with no adverse effects,
>> besides the fact that there dependency cycles which I would file as a bug.
> 
> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
> Move pfc node to work around probe ordering bug"?
> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
> 

A victim^Wtester! Yeah, it’s supposed to do that, but no guarantees, this is an RFC.

I would be happy to know if it solves your problem or not, please keep me in the loop.

> I'll give it a try when I'm back from ELC...
> 
> Gr{oetje,eeting}s,
> 
>                        Geert
> 

Regards

— Pantelis

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Geert Uytterhoeven]

Hi Pantelis,

On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> On Fri, Mar 20, 2015 at 12:39 PM, Pantelis Antoniou
>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> On Tue, 16 Dec 2014 14:11:31 +0200
>>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>> wrote:
>>>>> A nice side-effect of the changes in DTC for supporting overlays
>>>>> is that it is now possible to do dependency tracking of platform
>>>>> devices automatically.
>>>>>
>>>>> This patch implements dependency aware probe order for users
>>>>> of of_platform_populate.
>>>>>
>>>>> There are no changes in the syntax of the DTS bindings, the
>>>>> dependency is generated automatically by the use of phandle
>>>>> references.
>>>>
>>>> Do you have measurements showing improvement? Conceptually, I don't have
>>>> a problem with having a small scale solution like this, but I want proof
>>>> that it actively makes things better, and is worth the extra complexity.
>>>> It's not an easy block of code to understand.
>>>
>>> I will be the first to admit that the code it’s a bit hard to follow, but
>>> that’s the nature of trees and recursion.
>>>
>>> FWIW I’ve been booting with this applied for a month with no adverse effects,
>>> besides the fact that there dependency cycles which I would file as a bug.
>>
>> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>> Move pfc node to work around probe ordering bug"?
>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
>
> A victim^Wtester! Yeah, it’s supposed to do that, but no guarantees, this is an RFC.
>
> I would be happy to know if it solves your problem or not, please keep me in the loop.
>
>> I'll give it a try when I'm back from ELC...

And so I did. I
  1. Moved the pfc node in arch/arm/boot/dts/r8a73a4.dtsi back between dmac
     and i2c5.
  2. Applied your patch,
  3. Disabled the check for "/__local_fixups__" in
     __of_platform_populate_scan().
  4. Added support for "interrupts-extended" (gmail-whitespace-damaged patch
     at the end of this email),
  5. Enabled DEBUG,
  6. Uncommented all your debug pr_info() calls (except the one for
     "interrupts", as it references non-existing variables!).

But it didn't make any difference. The debug log below shows that it detects
the dependency of pfc@e6050000 on irqc0 (interrupt-controller@e61c0000)
irqc1 (interrupt-controller@e61c0200), but it doesn't reorder the nodes,
so pfc is still initialized first:

/
/ptm
/timer
    interrupt has parent /interrupt-controller@f1001000
/cache-controller@0
/cache-controller@1
/memory-controller@e6790000
/memory-controller@e67a0000
/dma-multiplexer
/pfc@e6050000
    interrupts-extended[0] has parent /interrupt-controller@e61c0000
    interrupts-extended[1] has parent /interrupt-controller@e61c0000
    interrupts-extended[2] has parent /interrupt-controller@e61c0000
    interrupts-extended[3] has parent /interrupt-controller@e61c0000
    interrupts-extended[4] has parent /interrupt-controller@e61c0000
    interrupts-extended[5] has parent /interrupt-controller@e61c0000
    interrupts-extended[6] has parent /interrupt-controller@e61c0000
    interrupts-extended[7] has parent /interrupt-controller@e61c0000
    interrupts-extended[8] has parent /interrupt-controller@e61c0000
    interrupts-extended[9] has parent /interrupt-controller@e61c0000
    interrupts-extended[10] has parent /interrupt-controller@e61c0000
    interrupts-extended[11] has parent /interrupt-controller@e61c0000
    interrupts-extended[12] has parent /interrupt-controller@e61c0000
    interrupts-extended[13] has parent /interrupt-controller@e61c0000
    interrupts-extended[14] has parent /interrupt-controller@e61c0000
    interrupts-extended[15] has parent /interrupt-controller@e61c0000
    interrupts-extended[16] has parent /interrupt-controller@e61c0000
    interrupts-extended[17] has parent /interrupt-controller@e61c0000
    interrupts-extended[18] has parent /interrupt-controller@e61c0000
    interrupts-extended[19] has parent /interrupt-controller@e61c0000
    interrupts-extended[20] has parent /interrupt-controller@e61c0000
    interrupts-extended[21] has parent /interrupt-controller@e61c0000
    interrupts-extended[22] has parent /interrupt-controller@e61c0000
    interrupts-extended[23] has parent /interrupt-controller@e61c0000
    interrupts-extended[24] has parent /interrupt-controller@e61c0000
    interrupts-extended[25] has parent /interrupt-controller@e61c0000
    interrupts-extended[26] has parent /interrupt-controller@e61c0000
    interrupts-extended[27] has parent /interrupt-controller@e61c0000
    interrupts-extended[28] has parent /interrupt-controller@e61c0000
    interrupts-extended[29] has parent /interrupt-controller@e61c0000
    interrupts-extended[30] has parent /interrupt-controller@e61c0000
    interrupts-extended[31] has parent /interrupt-controller@e61c0000
    interrupts-extended[32] has parent /interrupt-controller@e61c0200
    interrupts-extended[33] has parent /interrupt-controller@e61c0200
    interrupts-extended[34] has parent /interrupt-controller@e61c0200
    interrupts-extended[35] has parent /interrupt-controller@e61c0200
    interrupts-extended[36] has parent /interrupt-controller@e61c0200
    interrupts-extended[37] has parent /interrupt-controller@e61c0200
    interrupts-extended[38] has parent /interrupt-controller@e61c0200
    interrupts-extended[39] has parent /interrupt-controller@e61c0200
    interrupts-extended[40] has parent /interrupt-controller@e61c0200
    interrupts-extended[41] has parent /interrupt-controller@e61c0200
    interrupts-extended[42] has parent /interrupt-controller@e61c0200
    interrupts-extended[43] has parent /interrupt-controller@e61c0200
    interrupts-extended[44] has parent /interrupt-controller@e61c0200
    interrupts-extended[45] has parent /interrupt-controller@e61c0200
    interrupts-extended[46] has parent /interrupt-controller@e61c0200
    interrupts-extended[47] has parent /interrupt-controller@e61c0200
    interrupts-extended[48] has parent /interrupt-controller@e61c0200
    interrupts-extended[49] has parent /interrupt-controller@e61c0200
    interrupts-extended[50] has parent /interrupt-controller@e61c0200
    interrupts-extended[51] has parent /interrupt-controller@e61c0200
    interrupts-extended[52] has parent /interrupt-controller@e61c0200
    interrupts-extended[53] has parent /interrupt-controller@e61c0200
    interrupts-extended[54] has parent /interrupt-controller@e61c0200
    interrupts-extended[55] has parent /interrupt-controller@e61c0200
    interrupts-extended[56] has parent /interrupt-controller@e61c0200
    interrupts-extended[57] has parent /interrupt-controller@e61c0200
/i2c@e60b0000
    interrupt has parent /interrupt-controller@f1001000
/timer@e6130000
    interrupt has parent /interrupt-controller@f1001000
/interrupt-controller@e61c0000
    interrupt has parent /interrupt-controller@f1001000
/interrupt-controller@e61c0200
    interrupt has parent /interrupt-controller@f1001000
/thermal@e61f0000
    interrupt has parent /interrupt-controller@f1001000
/serial@e6c40000
    interrupt has parent /interrupt-controller@f1001000
/sd@ee100000
    interrupt has parent /interrupt-controller@f1001000
/sd@ee120000
    interrupt has parent /interrupt-controller@f1001000
/mmc@ee200000
    interrupt has parent /interrupt-controller@f1001000
/interrupt-controller@f1001000
    interrupt has parent /interrupt-controller@f1001000
/bus@fec10000
/system-controller@e6180000
/regulator@0
/regulator@1
/regulator@2
/regulator@3
/leds
/keyboard
* /timer depends on /interrupt-controller@f1001000
* /pfc@e6050000 depends on /interrupt-controller@e61c0000
* /pfc@e6050000 depends on /interrupt-controller@e61c0200
* /i2c@e60b0000 depends on /interrupt-controller@f1001000
* /timer@e6130000 depends on /interrupt-controller@f1001000
* /interrupt-controller@e61c0000 depends on /interrupt-controller@f1001000
* /interrupt-controller@e61c0200 depends on /interrupt-controller@f1001000
* /thermal@e61f0000 depends on /interrupt-controller@f1001000
* /serial@e6c40000 depends on /interrupt-controller@f1001000
* /sd@ee100000 depends on /interrupt-controller@f1001000
* /sd@ee120000 depends on /interrupt-controller@f1001000
* /mmc@ee200000 depends on /interrupt-controller@f1001000
| PLT  @/ (0) - count=26
| PLT      @/ptm (0) - count=0
| PLT      @/timer (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/cache-controller@0 (0) - count=0
| PLT      @/cache-controller@1 (0) - count=0
| PLT      @/memory-controller@e6790000 (0) - count=0
| PLT      @/memory-controller@e67a0000 (0) - count=0
| PLT      @/dma-multiplexer (0) - count=0
| PLT      @/pfc@e6050000 (0) - count=0
+          @/interrupt-controller@e61c0000
+          @/interrupt-controller@e61c0200
>          @/interrupt-controller@e61c0000
>          @/interrupt-controller@e61c0200
| PLT      @/i2c@e60b0000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/timer@e6130000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/interrupt-controller@e61c0000 (1) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/interrupt-controller@e61c0200 (1) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/thermal@e61f0000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/serial@e6c40000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/sd@ee100000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/sd@ee120000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/mmc@ee200000 (0) - count=0
+          @/interrupt-controller@f1001000
>          @/interrupt-controller@f1001000
| PLT      @/interrupt-controller@f1001000 (10) - count=0
+          @/interrupt-controller@f1001000
| PLT      @/bus@fec10000 (0) - count=0
| PLT      @/system-controller@e6180000 (0) - count=0
| PLT      @/regulator@0 (0) - count=0
| PLT      @/regulator@1 (0) - count=0
| PLT      @/regulator@2 (0) - count=0
| PLT      @/regulator@3 (0) - count=0
| PLT      @/leds (0) - count=0
| PLT      @/keyboard (0) - count=0
* PLT  @/ (0) - sort-count=26 - id=0
* PLT      @/ptm (0) - sort-count=0 - id=1
* PLT      @/interrupt-controller@f1001000 (10) - sort-count=0 - id=2
* PLT      @/timer (0) - sort-count=0 - id=3
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/cache-controller@0 (0) - sort-count=0 - id=4
* PLT      @/cache-controller@1 (0) - sort-count=0 - id=5
* PLT      @/memory-controller@e6790000 (0) - sort-count=0 - id=6
* PLT      @/memory-controller@e67a0000 (0) - sort-count=0 - id=7
* PLT      @/dma-multiplexer (0) - sort-count=0 - id=8
* PLT      @/interrupt-controller@e61c0000 (1) - sort-count=0 - id=9
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/interrupt-controller@e61c0200 (1) - sort-count=0 - id=10
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/pfc@e6050000 (0) - sort-count=0 - id=11
%          @/interrupt-controller@e61c0000 - id=9
%          @/interrupt-controller@e61c0200 - id=10
* PLT      @/i2c@e60b0000 (0) - sort-count=0 - id=12
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/timer@e6130000 (0) - sort-count=0 - id=13
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/thermal@e61f0000 (0) - sort-count=0 - id=14
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/serial@e6c40000 (0) - sort-count=0 - id=15
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/sd@ee100000 (0) - sort-count=0 - id=16
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/sd@ee120000 (0) - sort-count=0 - id=17
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/mmc@ee200000 (0) - sort-count=0 - id=18
%          @/interrupt-controller@f1001000 - id=2
* PLT      @/bus@fec10000 (0) - sort-count=0 - id=19
* PLT      @/system-controller@e6180000 (0) - sort-count=0 - id=20
* PLT      @/regulator@0 (0) - sort-count=0 - id=21
* PLT      @/regulator@1 (0) - sort-count=0 - id=22
* PLT      @/regulator@2 (0) - sort-count=0 - id=23
* PLT      @/regulator@3 (0) - sort-count=0 - id=24
* PLT      @/leds (0) - sort-count=0 - id=25
* PLT      @/keyboard (0) - sort-count=0 - id=26
of_platform_device_create_pdata(/ptm, <NULL>, ..)
platform ptm: device is not dma coherent
platform ptm: device is not behind an iommu
of_platform_device_create_pdata(/timer, <NULL>, ..)
platform timer: device is not dma coherent
platform timer: device is not behind an iommu
of_platform_device_create_pdata(/cache-controller@0, <NULL>, ..)
platform cache-controller@0: device is not dma coherent
platform cache-controller@0: device is not behind an iommu
of_platform_device_create_pdata(/cache-controller@1, <NULL>, ..)
platform cache-controller@1: device is not dma coherent
platform cache-controller@1: device is not behind an iommu
of_platform_device_create_pdata(/memory-controller@e6790000, <NULL>, ..)
platform e6790000.memory-controller: device is not dma coherent
platform e6790000.memory-controller: device is not behind an iommu
of_platform_device_create_pdata(/memory-controller@e67a0000, <NULL>, ..)
platform e67a0000.memory-controller: device is not dma coherent
platform e67a0000.memory-controller: device is not behind an iommu
of_platform_device_create_pdata(/dma-multiplexer, <NULL>, ..)
platform dma-multiplexer: device is not dma coherent
platform dma-multiplexer: device is not behind an iommu
of_platform_device_create_pdata(/pfc@e6050000, <NULL>, ..)
irq: no irq domain found for /interrupt-controller@e61c0000 !
not all legacy IRQ resources mapped for pfc
platform e6050000.pfc: device is not dma coherent
platform e6050000.pfc: device is not behind an iommu
sh-pfc e6050000.pfc: r8a73a4_pfc handling gpio 0 -> 329
sh-pfc e6050000.pfc: r8a73a4_pfc support registered
of_platform_device_create_pdata(/i2c@e60b0000, <NULL>, ..)
platform e60b0000.i2c: device is not dma coherent
platform e60b0000.i2c: device is not behind an iommu
of_platform_device_create_pdata(/timer@e6130000, <NULL>, ..)
platform e6130000.timer: device is not dma coherent
platform e6130000.timer: device is not behind an iommu
of_platform_device_create_pdata(/interrupt-controller@e61c0000, <NULL>, ..)
platform e61c0000.interrupt-controller: device is not dma coherent
platform e61c0000.interrupt-controller: device is not behind an iommu
renesas_irqc e61c0000.interrupt-controller: driving 32 irqs
of_platform_device_create_pdata(/interrupt-controller@e61c0200, <NULL>, ..)
platform e61c0200.interrupt-controller: device is not dma coherent
platform e61c0200.interrupt-controller: device is not behind an iommu
renesas_irqc e61c0200.interrupt-controller: driving 26 irqs
of_platform_device_create_pdata(/thermal@e61f0000, <NULL>, ..)
platform e61f0000.thermal: device is not dma coherent
platform e61f0000.thermal: device is not behind an iommu
of_platform_device_create_pdata(/serial@e6c40000, <NULL>, ..)
platform e6c40000.serial: device is not dma coherent
platform e6c40000.serial: device is not behind an iommu
of_platform_device_create_pdata(/sd@ee100000, <NULL>, ..)
platform ee100000.sd: device is not dma coherent
platform ee100000.sd: device is not behind an iommu
of_platform_device_create_pdata(/sd@ee120000, <NULL>, ..)
platform ee120000.sd: device is not dma coherent
platform ee120000.sd: device is not behind an iommu
of_platform_device_create_pdata(/mmc@ee200000, <NULL>, ..)
platform ee200000.mmc: device is not dma coherent
platform ee200000.mmc: device is not behind an iommu
of_platform_device_create_pdata(/interrupt-controller@f1001000, <NULL>, ..)
platform f1001000.interrupt-controller: device is not dma coherent
platform f1001000.interrupt-controller: device is not behind an iommu
of_platform_device_create_pdata(/bus@fec10000, <NULL>, ..)
platform fec10000.bus: device is not dma coherent
platform fec10000.bus: device is not behind an iommu
of_platform_device_create_pdata(/system-controller@e6180000, <NULL>, ..)
platform e6180000.system-controller: device is not dma coherent
platform e6180000.system-controller: device is not behind an iommu
of_platform_device_create_pdata(/regulator@0, <NULL>, ..)
platform regulator@0: device is not dma coherent
platform regulator@0: device is not behind an iommu
of_platform_device_create_pdata(/regulator@1, <NULL>, ..)
platform regulator@1: device is not dma coherent
platform regulator@1: device is not behind an iommu
of_platform_device_create_pdata(/regulator@2, <NULL>, ..)
platform regulator@2: device is not dma coherent
platform regulator@2: device is not behind an iommu
of_platform_device_create_pdata(/regulator@3, <NULL>, ..)
platform regulator@3: device is not dma coherent
platform regulator@3: device is not behind an iommu
of_platform_device_create_pdata(/leds, <NULL>, ..)
platform leds: device is not dma coherent
platform leds: device is not behind an iommu
of_platform_device_create_pdata(/keyboard, <NULL>, ..)
platform keyboard: device is not dma coherent
platform keyboard: device is not behind an iommu
No ATAGs?
hw-breakpoint: found 5 (+1 reserved) breakpoint and 4 watchpoint registers.
hw-breakpoint: maximum watchpoint size is 8 bytes.
i2c-sh_mobile e60b0000.i2c: I2C adapter 0, bus speed 100000 Hz
sh_cmt e6130000.timer: ch0: used for clock events
sh_cmt e6130000.timer: ch1: used as clock source
Switched to clocksource arch_sys_counter
NET: Registered protocol family 2
TCP established hash table entries: 8192 (order: 3, 32768 bytes)
TCP bind hash table entries: 8192 (order: 6, 294912 bytes)
TCP: Hash tables configured (established 8192 bind 8192)
UDP hash table entries: 512 (order: 3, 40960 bytes)
UDP-Lite hash table entries: 512 (order: 3, 40960 bytes)
NET: Registered protocol family 1
RPC: Registered named UNIX socket transport module.
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
futex hash table entries: 2048 (order: 5, 131072 bytes)
NFS: Registering the id_resolver key type
Key type id_resolver registered
Key type id_legacy registered
nfs4filelayout_init: NFSv4 File Layout Driver Registering...
nfs4flexfilelayout_init: NFSv4 Flexfile Layout Driver Registering...
io scheduler noop registered (default)
/bus@fec10000
/bus@fec10000/ethernet@8000000
    interrupt has parent /interrupt-controller@e61c0200
| PLT  @/bus@fec10000 (0) - count=1
| PLT      @/bus@fec10000/ethernet@8000000 (0) - count=0
+          @/interrupt-controller@e61c0200
* PLT  @/bus@fec10000 (0) - sort-count=1 - id=0
* PLT      @/bus@fec10000/ethernet@8000000 (0) - sort-count=0 - id=1
of_platform_device_create_pdata(/bus@fec10000/ethernet@8000000, <NULL>, ..)
platform 8000000.ethernet: device is not dma coherent
platform 8000000.ethernet: device is not behind an iommu
/dma-multiplexer
/dma-multiplexer/dma-controller@e6700020
    interrupt has parent /interrupt-controller@f1001000
| PLT  @/dma-multiplexer (0) - count=1
| PLT      @/dma-multiplexer/dma-controller@e6700020 (0) - count=0
+          @/interrupt-controller@f1001000
* PLT  @/dma-multiplexer (0) - sort-count=1 - id=0
* PLT      @/dma-multiplexer/dma-controller@e6700020 (0) - sort-count=0 - id=1
of_platform_device_create_pdata(/dma-multiplexer/dma-controller@e6700020,
<NULL>, ..)
platform e6700020.dma-controller: device is not dma coherent
platform e6700020.dma-controller: device is not behind an iommu

Both the pfc and irqc drivers are initialized from postcore_initcall().

If there's anything else I can try, please let me know.
Thanks!

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9f71741405c2f008..886eef44fd5a3f47 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -610,7 +611,9 @@ static void __local_fixup_ref(struct of_pop_entry
*pe, struct device_node *lfnp)
 static void __of_platform_populate_get_refs_internal(struct of_pop_entry *pe)
 {
        struct device_node *np;
+       struct of_phandle_args irq;
        struct of_pop_ref_entry *re;
+       int index;
        char *base;
        bool found;

@@ -626,9 +629,39 @@ static void
__of_platform_populate_get_refs_internal(struct of_pop_entry *pe)
                kfree(base);
        }

+pr_info("%s\n", pe->np->full_name);
+       /* then try the new-style interrupts-extended */
+       for (index = 0;
+            !of_parse_phandle_with_args(pe->np, "interrupts-extended",
+                                        "#interrupt-cells", index, &irq);
+            index++) {
+               np = irq.np;
+pr_info("    interrupts-extended[%d] has parent %s\n", index, np->full_name);
+
+               /* check whether the ref is already there */
+               found = false;
+               list_for_each_entry(re, &pe->refs, node) {
+                       if (re->np == np) {
+                               found = true;
+                               break;
+                       }
+               }
+
+               if (!found) {
+                       re = kzalloc(sizeof(*re), GFP_KERNEL);
+                       BUG_ON(re == NULL);
+                       re->np = np;
+                       list_add_tail(&re->node, &pe->refs);
+
+               }
+
+               of_node_put(np);
+       }
+
        /* now try the old style interrupt */
        if (of_get_property(pe->np, "interrupts", NULL) &&
                        (np = of_irq_find_parent(pe->np)) != NULL) {
+pr_info("    interrupt has parent %s\n", np->full_name);

                /* check whether the ref is already there */
                found = false;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Pantelis Antoniou]

Hi Geert,

> On Mar 30, 2015, at 16:27 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> 
> Hi Pantelis,
> 
> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert-Td1EMuHUCqygBwfqmJ+zsg@public.gmane.orgg> wrote:
>>> On Fri, Mar 20, 2015 at 12:39 PM, Pantelis Antoniou
>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>>>> On Tue, 16 Dec 2014 14:11:31 +0200
>>>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>> wrote:
>>>>>> A nice side-effect of the changes in DTC for supporting overlays
>>>>>> is that it is now possible to do dependency tracking of platform
>>>>>> devices automatically.
>>>>>> 
>>>>>> This patch implements dependency aware probe order for users
>>>>>> of of_platform_populate.
>>>>>> 
>>>>>> There are no changes in the syntax of the DTS bindings, the
>>>>>> dependency is generated automatically by the use of phandle
>>>>>> references.
>>>>> 
>>>>> Do you have measurements showing improvement? Conceptually, I don't have
>>>>> a problem with having a small scale solution like this, but I want proof
>>>>> that it actively makes things better, and is worth the extra complexity.
>>>>> It's not an easy block of code to understand.
>>>> 
>>>> I will be the first to admit that the code it’s a bit hard to follow, but
>>>> that’s the nature of trees and recursion.
>>>> 
>>>> FWIW I’ve been booting with this applied for a month with no adverse effects,
>>>> besides the fact that there dependency cycles which I would file as a bug.
>>> 
>>> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>> Move pfc node to work around probe ordering bug"?
>>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
>> 
>> A victim^Wtester! Yeah, it’s supposed to do that, but no guarantees, this is an RFC.
>> 
>> I would be happy to know if it solves your problem or not, please keep me in the loop.
>> 
>>> I'll give it a try when I'm back from ELC...
> 
> And so I did. I
>  1. Moved the pfc node in arch/arm/boot/dts/r8a73a4.dtsi back between dmac
>     and i2c5.
>  2. Applied your patch,
>  3. Disabled the check for "/__local_fixups__" in
>     __of_platform_populate_scan().
>  4. Added support for "interrupts-extended" (gmail-whitespace-damaged patch
>     at the end of this email),
>  5. Enabled DEBUG,
>  6. Uncommented all your debug pr_info() calls (except the one for
>     "interrupts", as it references non-existing variables!).
> 
> But it didn't make any difference. The debug log below shows that it detects
> the dependency of pfc@e6050000 on irqc0 (interrupt-controller@e61c0000)
> irqc1 (interrupt-controller@e61c0200), but it doesn't reorder the nodes,
> so pfc is still initialized first:
> 

Interesting. I guess this are indeed more complicated.

It is good that it detects the dependency first, and registers
the devices in the correct order.

The problem is that this is not enough. The probe is initiated when the
drivers are registered afterwards, but not in any particular order.

So we need to do deferred matching for this to work reliably.

Thank you so much for taking the time to send this debug log.
It does help considerably.

Regards

— Pantelis

[snip]

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Grant Likely]

On Mon, 30 Mar 2015 15:27:27 +0200
, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
 wrote:
> Hi Pantelis,
> 
> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Fri, Mar 20, 2015 at 12:39 PM, Pantelis Antoniou
> >> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >>>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely@secretlab.ca> wrote:
> >>>> On Tue, 16 Dec 2014 14:11:31 +0200
> >>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >>>> wrote:
> >>>>> A nice side-effect of the changes in DTC for supporting overlays
> >>>>> is that it is now possible to do dependency tracking of platform
> >>>>> devices automatically.
> >>>>>
> >>>>> This patch implements dependency aware probe order for users
> >>>>> of of_platform_populate.
> >>>>>
> >>>>> There are no changes in the syntax of the DTS bindings, the
> >>>>> dependency is generated automatically by the use of phandle
> >>>>> references.
> >>>>
> >>>> Do you have measurements showing improvement? Conceptually, I don't have
> >>>> a problem with having a small scale solution like this, but I want proof
> >>>> that it actively makes things better, and is worth the extra complexity.
> >>>> It's not an easy block of code to understand.
> >>>
> >>> I will be the first to admit that the code it’s a bit hard to follow, but
> >>> that’s the nature of trees and recursion.
> >>>
> >>> FWIW I’ve been booting with this applied for a month with no adverse effects,
> >>> besides the fact that there dependency cycles which I would file as a bug.
> >>
> >> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
> >> Move pfc node to work around probe ordering bug"?
> >> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
> >
> > A victim^Wtester! Yeah, it’s supposed to do that, but no guarantees, this is an RFC.
> >
> > I would be happy to know if it solves your problem or not, please keep me in the loop.
> >
> >> I'll give it a try when I'm back from ELC...
> 
> And so I did. I
>   1. Moved the pfc node in arch/arm/boot/dts/r8a73a4.dtsi back between dmac
>      and i2c5.
>   2. Applied your patch,
>   3. Disabled the check for "/__local_fixups__" in
>      __of_platform_populate_scan().
>   4. Added support for "interrupts-extended" (gmail-whitespace-damaged patch
>      at the end of this email),
>   5. Enabled DEBUG,
>   6. Uncommented all your debug pr_info() calls (except the one for
>      "interrupts", as it references non-existing variables!).
> 
> But it didn't make any difference. The debug log below shows that it detects
> the dependency of pfc@e6050000 on irqc0 (interrupt-controller@e61c0000)
> irqc1 (interrupt-controller@e61c0200), but it doesn't reorder the nodes,
> so pfc is still initialized first:

I talked to Pantelis about this while we were at ELC. Even if it did
correctly reorder the registration and solved your problem, it won't
help at all for most devices registered by the of_platform_populate()
call.

The core device model is oportunistic about when drivers get bound to
devices. Both drivers and devices can be registered at any time. When a
device is registered, it kernel tries to bind it against all available
drivers, and similarly when a driver is registered the kernel tries to
bind it with all unbound devices.

In the normal case, of_platform_populate() is called at arch_initcall
(level 3).  Most drivers are registered at device_initcall (level 6).
The only reason reordering works for you is because both the renesas pfc
irqc drivers use postcore_initcall (level 2). Therefore, both drivers
get registered before of_platform_populate() which makes device
registration order matter. If the devices were registered before the
drivers, then it would be kernel link order which determines the
behaviour.

There are two ways to fix this so that .dtb order doesn't matter. The
dirty hack is to change the pfc driver to use subsys_initcall (level 4)
or later so that it happens after the devices are registered. The second
solution is to make the pfc drivers able to return -EPROBE_DEFER, but
that also requires fixing deferred probe to start retrying devices
before late_initcall time. Right now there is a holdoff flag that
prevents any retries before late_initcall(). It was done that way
because the defer workqueue conflicted with some of the initcalls in a
bad way.

We could add a manual walk of the queue between each initcall level
without using the workqueue. That would solve the problem.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Pantelis Antoniou]

Hi Grant,

> On Apr 2, 2015, at 05:38 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> On Mon, 30 Mar 2015 15:27:27 +0200
> , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> wrote:
>> Hi Pantelis,
>> 
>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Fri, Mar 20, 2015 at 12:39 PM, Pantelis Antoniou
>>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>>>> On Mar 19, 2015, at 21:18 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>>>>> On Tue, 16 Dec 2014 14:11:31 +0200
>>>>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>>> wrote:
>>>>>>> A nice side-effect of the changes in DTC for supporting overlays
>>>>>>> is that it is now possible to do dependency tracking of platform
>>>>>>> devices automatically.
>>>>>>> 
>>>>>>> This patch implements dependency aware probe order for users
>>>>>>> of of_platform_populate.
>>>>>>> 
>>>>>>> There are no changes in the syntax of the DTS bindings, the
>>>>>>> dependency is generated automatically by the use of phandle
>>>>>>> references.
>>>>>> 
>>>>>> Do you have measurements showing improvement? Conceptually, I don't have
>>>>>> a problem with having a small scale solution like this, but I want proof
>>>>>> that it actively makes things better, and is worth the extra complexity.
>>>>>> It's not an easy block of code to understand.
>>>>> 
>>>>> I will be the first to admit that the code it’s a bit hard to follow, but
>>>>> that’s the nature of trees and recursion.
>>>>> 
>>>>> FWIW I’ve been booting with this applied for a month with no adverse effects,
>>>>> besides the fact that there dependency cycles which I would file as a bug.
>>>> 
>>>> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>>> Move pfc node to work around probe ordering bug"?
>>>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
>>> 
>>> A victim^Wtester! Yeah, it’s supposed to do that, but no guarantees, this is an RFC.
>>> 
>>> I would be happy to know if it solves your problem or not, please keep me in the loop.
>>> 
>>>> I'll give it a try when I'm back from ELC...
>> 
>> And so I did. I
>>  1. Moved the pfc node in arch/arm/boot/dts/r8a73a4.dtsi back between dmac
>>     and i2c5.
>>  2. Applied your patch,
>>  3. Disabled the check for "/__local_fixups__" in
>>     __of_platform_populate_scan().
>>  4. Added support for "interrupts-extended" (gmail-whitespace-damaged patch
>>     at the end of this email),
>>  5. Enabled DEBUG,
>>  6. Uncommented all your debug pr_info() calls (except the one for
>>     "interrupts", as it references non-existing variables!).
>> 
>> But it didn't make any difference. The debug log below shows that it detects
>> the dependency of pfc@e6050000 on irqc0 (interrupt-controller@e61c0000)
>> irqc1 (interrupt-controller@e61c0200), but it doesn't reorder the nodes,
>> so pfc is still initialized first:
> 
> I talked to Pantelis about this while we were at ELC. Even if it did
> correctly reorder the registration and solved your problem, it won't
> help at all for most devices registered by the of_platform_populate()
> call.
> 
> The core device model is oportunistic about when drivers get bound to
> devices. Both drivers and devices can be registered at any time. When a
> device is registered, it kernel tries to bind it against all available
> drivers, and similarly when a driver is registered the kernel tries to
> bind it with all unbound devices.
> 
> In the normal case, of_platform_populate() is called at arch_initcall
> (level 3).  Most drivers are registered at device_initcall (level 6).
> The only reason reordering works for you is because both the renesas pfc
> irqc drivers use postcore_initcall (level 2). Therefore, both drivers
> get registered before of_platform_populate() which makes device
> registration order matter. If the devices were registered before the
> drivers, then it would be kernel link order which determines the
> behaviour.
> 
> There are two ways to fix this so that .dtb order doesn't matter. The
> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
> or later so that it happens after the devices are registered. The second
> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
> that also requires fixing deferred probe to start retrying devices
> before late_initcall time. Right now there is a holdoff flag that
> prevents any retries before late_initcall(). It was done that way
> because the defer workqueue conflicted with some of the initcalls in a
> bad way.
> 
> We could add a manual walk of the queue between each initcall level
> without using the workqueue. That would solve the problem.
> 

I’ve been spending some time getting things to work properly taking into
account the essentially random way that drivers are registered.

My complete understanding is this:

The platform devices that are part of any directly reachable from root simple busses
are registered sometime on init_machine() at arch_initcall() time.

The drivers are registered at device_initcall() but not all of them. Some of them
are _required_ to be initialized earlier in order for the devices to be instantiated first.
For instance dma engine drivers.

I am toying with marking drivers that have been matched with devices and not instantiating
them immediately, and that might work. Perhaps an easier solution would be just to delay
device register to be performed at the the end device_initcall() since all the drivers would
be registered and the device matched immediately.
 
I don’t know if that will make things go kaboom on some $RANDOM_ARCH though.

Does anything come to mind?

IMHO getting this right might make the holdoff hacks go away completely.

> g.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Geert Uytterhoeven]

Hi Grant,

On Thu, Apr 2, 2015 at 4:38 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Mon, 30 Mar 2015 15:27:27 +0200
> , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>  wrote:
>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> >> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> >> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>> >> Move pfc node to work around probe ordering bug"?
>> >> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91

> There are two ways to fix this so that .dtb order doesn't matter. The
> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
> or later so that it happens after the devices are registered. The second

I've just tried that, and it doesn't work.

> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
> that also requires fixing deferred probe to start retrying devices
> before late_initcall time. Right now there is a holdoff flag that

All the pfc driver can detect is that some platform_device.resource[i] are
empty (resource_type zero). Returning -EPROBE_DEFER won't help,
as the resource won't change later.

The problem is not the initialization order of the device drivers, but the
creation order of the platform devices.

of_device_alloc() silently (except for the pr_debug() message) ignores
any failures to setup IRQ resources. Hence platform devices for interrupt
providers must be created before platform devices for interrupt consumers,
which is what the reordering in DT fixes.

See also the information in the commit message linked at the top.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Pantelis Antoniou]

Hi Geert,

> On Apr 8, 2015, at 11:42 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> 
> Hi Grant,
> 
> On Thu, Apr 2, 2015 at 4:38 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Mon, 30 Mar 2015 15:27:27 +0200
>> , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>> wrote:
>>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>>> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>>>> Move pfc node to work around probe ordering bug"?
>>>>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
> 
>> There are two ways to fix this so that .dtb order doesn't matter. The
>> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
>> or later so that it happens after the devices are registered. The second
> 
> I've just tried that, and it doesn't work.
> 
>> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
>> that also requires fixing deferred probe to start retrying devices
>> before late_initcall time. Right now there is a holdoff flag that
> 
> All the pfc driver can detect is that some platform_device.resource[i] are
> empty (resource_type zero). Returning -EPROBE_DEFER won't help,
> as the resource won't change later.
> 
> The problem is not the initialization order of the device drivers, but the
> creation order of the platform devices.
> 
> of_device_alloc() silently (except for the pr_debug() message) ignores
> any failures to setup IRQ resources. Hence platform devices for interrupt
> providers must be created before platform devices for interrupt consumers,
> which is what the reordering in DT fixes.
> 
> See also the information in the commit message linked at the top.
> 

FWIW I’ve been digging deeper in the rabbit hole…

We do have some ugly warts in our core code. For instance the mess with the interrupt
controllers being not created normally but by a special linker section magic.

Then we have dma controllers needing to be registered at subsys_initcall() for obvious reasons.

I suspect things are similar for all subsystem services drivers.

The good thing about dependency tracking is that all of this is solved automatically.
Since the devices will have references to the interrupt controller node it will be shoved in front
of the dependency list. Same thing with dma, iommu etc.

I will have something that works shortly, but I would prefer to start some kind of discussion
about what to do about device dependencies in the core, and not hack something for just the OF
case.

What do you guys think?

> Gr{oetje,eeting}s,
> 
>                        Geert
> 

Regards

— Pantelis

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Rob Herring]

On Wed, Apr 8, 2015 at 3:42 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> Hi Grant,
>
> On Thu, Apr 2, 2015 at 4:38 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>> On Mon, 30 Mar 2015 15:27:27 +0200
>> , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>>  wrote:
>>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>> >> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>> >> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>> >> Move pfc node to work around probe ordering bug"?
>>> >> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
>
>> There are two ways to fix this so that .dtb order doesn't matter. The
>> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
>> or later so that it happens after the devices are registered. The second
>
> I've just tried that, and it doesn't work.
>
>> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
>> that also requires fixing deferred probe to start retrying devices
>> before late_initcall time. Right now there is a holdoff flag that
>
> All the pfc driver can detect is that some platform_device.resource[i] are
> empty (resource_type zero). Returning -EPROBE_DEFER won't help,
> as the resource won't change later.
>
> The problem is not the initialization order of the device drivers, but the
> creation order of the platform devices.
>
> of_device_alloc() silently (except for the pr_debug() message) ignores
> any failures to setup IRQ resources. Hence platform devices for interrupt
> providers must be created before platform devices for interrupt consumers,
> which is what the reordering in DT fixes.

This doesn't sound right. It ignores failures because platform_get_irq
will parse the interrupts when called rather than just using the
resource struct and will return EPROBE_DEFER if the irq resource is
not ready. We left the of_device_alloc code in to be safe, but we
should be able to remove it.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Rob Herring]

On Wed, Apr 8, 2015 at 4:16 AM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Hi Geert,
>
>> On Apr 8, 2015, at 11:42 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>
>> Hi Grant,
>>
>> On Thu, Apr 2, 2015 at 4:38 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Mon, 30 Mar 2015 15:27:27 +0200
>>> , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>>> wrote:
>>>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>>>> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>>>>> Move pfc node to work around probe ordering bug"?
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
>>
>>> There are two ways to fix this so that .dtb order doesn't matter. The
>>> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
>>> or later so that it happens after the devices are registered. The second
>>
>> I've just tried that, and it doesn't work.
>>
>>> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
>>> that also requires fixing deferred probe to start retrying devices
>>> before late_initcall time. Right now there is a holdoff flag that
>>
>> All the pfc driver can detect is that some platform_device.resource[i] are
>> empty (resource_type zero). Returning -EPROBE_DEFER won't help,
>> as the resource won't change later.
>>
>> The problem is not the initialization order of the device drivers, but the
>> creation order of the platform devices.
>>
>> of_device_alloc() silently (except for the pr_debug() message) ignores
>> any failures to setup IRQ resources. Hence platform devices for interrupt
>> providers must be created before platform devices for interrupt consumers,
>> which is what the reordering in DT fixes.
>>
>> See also the information in the commit message linked at the top.
>>
>
> FWIW I’ve been digging deeper in the rabbit hole…
>
> We do have some ugly warts in our core code. For instance the mess with the interrupt
> controllers being not created normally but by a special linker section magic.

As long as we need the timer interrupts before driver core starts we
have to do it early.

> Then we have dma controllers needing to be registered at subsys_initcall() for obvious reasons.

These should be fixable with deferred probe. That requires fixing all
the client drivers where as fixing the dependencies would not need
driver changes.

> I suspect things are similar for all subsystem services drivers.
>
> The good thing about dependency tracking is that all of this is solved automatically.
> Since the devices will have references to the interrupt controller node it will be shoved in front
> of the dependency list. Same thing with dma, iommu etc.

Solving the timer and timer dependencies (clocks, irq) won't really be
possible within the driver model.

> I will have something that works shortly, but I would prefer to start some kind of discussion
> about what to do about device dependencies in the core, and not hack something for just the OF
> case.
>
> What do you guys think?

I agree the dependency tracking structures should be in the core, but
filling that in is going to be OF specific.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Geert Uytterhoeven]

On Wed, Apr 8, 2015 at 4:17 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Then we have dma controllers needing to be registered at subsys_initcall() for obvious reasons.
>
> These should be fixable with deferred probe. That requires fixing all
> the client drivers where as fixing the dependencies would not need
> driver changes.

DMA is tricky in that there are three cases, not two:
  1. DMA engine is available,
  2. DMA engine is not yet available (e.g. DMA engine driver is modular),
  3. DMA engine will never be available (DMA engine or DMA engine driver
     not available).

Case 2 is the tricky one: probably you don't want the client driver to
fail hard,
but want to fall back to PIO, and try to setup DMA again later.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Rob Herring]

On Wed, Apr 8, 2015 at 9:43 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> On Wed, Apr 8, 2015 at 4:17 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Then we have dma controllers needing to be registered at subsys_initcall() for obvious reasons.
>>
>> These should be fixable with deferred probe. That requires fixing all
>> the client drivers where as fixing the dependencies would not need
>> driver changes.
>
> DMA is tricky in that there are three cases, not two:
>   1. DMA engine is available,
>   2. DMA engine is not yet available (e.g. DMA engine driver is modular),
>   3. DMA engine will never be available (DMA engine or DMA engine driver
>      not available).
>
> Case 2 is the tricky one: probably you don't want the client driver to
> fail hard,
> but want to fall back to PIO, and try to setup DMA again later.

Yes, we had just this case on the pl011 driver recently. In that case
we fixed it by moving the DMA setup to port open/init from probe.

But generically, dependency tracking is not going to have any way to know this.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Geert Uytterhoeven]

Hi Rob,

On Wed, Apr 8, 2015 at 3:40 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Apr 8, 2015 at 3:42 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Apr 2, 2015 at 4:38 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Mon, 30 Mar 2015 15:27:27 +0200
>>> , Geert Uytterhoeven <geert@linux-m68k.org>
>>>  wrote:
>>>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> >> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> >> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>>> >> Move pfc node to work around probe ordering bug"?
>>>> >> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&idäba0a9bddff3ba52cec100414d2f178440efc91
>>
>>> There are two ways to fix this so that .dtb order doesn't matter. The
>>> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
>>> or later so that it happens after the devices are registered. The second
>>
>> I've just tried that, and it doesn't work.
>>
>>> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
>>> that also requires fixing deferred probe to start retrying devices
>>> before late_initcall time. Right now there is a holdoff flag that
>>
>> All the pfc driver can detect is that some platform_device.resource[i] are
>> empty (resource_type zero). Returning -EPROBE_DEFER won't help,
>> as the resource won't change later.
>>
>> The problem is not the initialization order of the device drivers, but the
>> creation order of the platform devices.
>>
>> of_device_alloc() silently (except for the pr_debug() message) ignores
>> any failures to setup IRQ resources. Hence platform devices for interrupt
>> providers must be created before platform devices for interrupt consumers,
>> which is what the reordering in DT fixes.
>
> This doesn't sound right. It ignores failures because platform_get_irq
> will parse the interrupts when called rather than just using the
> resource struct and will return EPROBE_DEFER if the irq resource is
> not ready. We left the of_device_alloc code in to be safe, but we
> should be able to remove it.

Thanks, using platform_get_irq() (and propagating EPROBE_DEFER) instead of
accessing the platform_device's resources directly indeed does work, but will
require some rework in the sh-pfc driver.
One more thing to consider for a "porting your driver to DT" tutorial ;-)

One side effect is that almost all drivers now end up being probe-deferred, as
the pinctrl configuration can't be done until the pfc driver is active.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Geert Uytterhoeven]

Hi Rob,

On Wed, Apr 8, 2015 at 3:40 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Apr 8, 2015 at 3:42 AM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> On Thu, Apr 2, 2015 at 4:38 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>> On Mon, 30 Mar 2015 15:27:27 +0200
>>> , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>>>  wrote:
>>>> On Tue, Mar 24, 2015 at 6:56 PM, Pantelis Antoniou
>>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>> >> On Mar 24, 2015, at 07:50 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>>> >> IIUC, this would fix the issue I worked around in "ARM: shmobile: r8a73a4:
>>>> >> Move pfc node to work around probe ordering bug"?
>>>> >> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas.git/commit/?h=r8a73a4-ccf-and-multiplatform-for-v4.1&id=e4ba0a9bddff3ba52cec100414d2f178440efc91
>>
>>> There are two ways to fix this so that .dtb order doesn't matter. The
>>> dirty hack is to change the pfc driver to use subsys_initcall (level 4)
>>> or later so that it happens after the devices are registered. The second
>>
>> I've just tried that, and it doesn't work.
>>
>>> solution is to make the pfc drivers able to return -EPROBE_DEFER, but
>>> that also requires fixing deferred probe to start retrying devices
>>> before late_initcall time. Right now there is a holdoff flag that
>>
>> All the pfc driver can detect is that some platform_device.resource[i] are
>> empty (resource_type zero). Returning -EPROBE_DEFER won't help,
>> as the resource won't change later.
>>
>> The problem is not the initialization order of the device drivers, but the
>> creation order of the platform devices.
>>
>> of_device_alloc() silently (except for the pr_debug() message) ignores
>> any failures to setup IRQ resources. Hence platform devices for interrupt
>> providers must be created before platform devices for interrupt consumers,
>> which is what the reordering in DT fixes.
>
> This doesn't sound right. It ignores failures because platform_get_irq
> will parse the interrupts when called rather than just using the
> resource struct and will return EPROBE_DEFER if the irq resource is
> not ready. We left the of_device_alloc code in to be safe, but we
> should be able to remove it.

Thanks, using platform_get_irq() (and propagating EPROBE_DEFER) instead of
accessing the platform_device's resources directly indeed does work, but will
require some rework in the sh-pfc driver.
One more thing to consider for a "porting your driver to DT" tutorial ;-)

One side effect is that almost all drivers now end up being probe-deferred, as
the pinctrl configuration can't be done until the pfc driver is active.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Peter Hurley]

On 04/08/2015 09:40 AM, Rob Herring wrote:
> This doesn't sound right. It ignores failures because platform_get_irq
> will parse the interrupts when called rather than just using the
> resource struct and will return EPROBE_DEFER if the irq resource is
> not ready. We left the of_device_alloc code in to be safe, but we
> should be able to remove it.

This brings up a couple of points which are plaguing the serial drivers:
1. Is platform_get_irq() now required to properly obtain the mapped irq
   for DT-aware drivers? IOW, is platform_get_resource(IORESOURCE_IRQ)
   broken? Will it be if the of_device_alloc() code is removed?
2. Should DT-specific drivers not be using irq_of_parse_and_map()?
   On probe failure irq_dispose_mapping() will be junking the mapping,
   thus invalidating the irq assignment in the platform resource table,
   which breaks platform drivers which might otherwise probe successfully.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Rob Herring]

On Wed, Apr 15, 2015 at 9:17 AM, Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org> wrote:
> On 04/08/2015 09:40 AM, Rob Herring wrote:
>> This doesn't sound right. It ignores failures because platform_get_irq
>> will parse the interrupts when called rather than just using the
>> resource struct and will return EPROBE_DEFER if the irq resource is
>> not ready. We left the of_device_alloc code in to be safe, but we
>> should be able to remove it.
>
> This brings up a couple of points which are plaguing the serial drivers:
> 1. Is platform_get_irq() now required to properly obtain the mapped irq
>    for DT-aware drivers? IOW, is platform_get_resource(IORESOURCE_IRQ)
>    broken? Will it be if the of_device_alloc() code is removed?

Yes, and that is why we left the of_device_alloc code now that I
remember. platform_get_irq will first have to be used everywhere to
remove the code in of_device_alloc. It also has to be used for
deferred probe to work (if it is irq's you need to wait for).

> 2. Should DT-specific drivers not be using irq_of_parse_and_map()?
>    On probe failure irq_dispose_mapping() will be junking the mapping,
>    thus invalidating the irq assignment in the platform resource table,
>    which breaks platform drivers which might otherwise probe successfully.

Generally no, they should not use irq_of_parse_and_map as we want
drivers to work with platform_data, DT, ACPI, or Bob's Firmware
Interface. I think most users are PPC drivers which don't have so much
of the probe ordering problems.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Peter Hurley]

On 04/15/2015 10:35 AM, Rob Herring wrote:
> On Wed, Apr 15, 2015 at 9:17 AM, Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org> wrote:
 
>> 2. Should DT-specific drivers not be using irq_of_parse_and_map()?
>>    On probe failure irq_dispose_mapping() will be junking the mapping,
>>    thus invalidating the irq assignment in the platform resource table,
>>    which breaks platform drivers which might otherwise probe successfully.
> 
> Generally no, they should not use irq_of_parse_and_map as we want
> drivers to work with platform_data, DT, ACPI, or Bob's Firmware
> Interface. I think most users are PPC drivers which don't have so much
> of the probe ordering problems.

Apologies for hijacking this thread for a moment.

If of_device_alloc() creates the irq mapping, and no driver probes succeed,
what is disposing the mapping?

Similarly, if a platform driver fails its probe after platform_get_irq()
what should dispose of that mapping?

Regards,
Peter Hurley


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate

[Rob Herring]

On Thu, Apr 16, 2015 at 5:40 AM, Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org> wrote:
> On 04/15/2015 10:35 AM, Rob Herring wrote:
>> On Wed, Apr 15, 2015 at 9:17 AM, Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org> wrote:
>
>>> 2. Should DT-specific drivers not be using irq_of_parse_and_map()?
>>>    On probe failure irq_dispose_mapping() will be junking the mapping,
>>>    thus invalidating the irq assignment in the platform resource table,
>>>    which breaks platform drivers which might otherwise probe successfully.
>>
>> Generally no, they should not use irq_of_parse_and_map as we want
>> drivers to work with platform_data, DT, ACPI, or Bob's Firmware
>> Interface. I think most users are PPC drivers which don't have so much
>> of the probe ordering problems.
>
> Apologies for hijacking this thread for a moment.
>
> If of_device_alloc() creates the irq mapping, and no driver probes succeed,
> what is disposing the mapping?
>
> Similarly, if a platform driver fails its probe after platform_get_irq()
> what should dispose of that mapping?

The mapping is created by the irqchip init, not of_device_alloc or
platform_get_irq. Disposing would occur when the irqchip is hot
unplugged. Perhaps we could save some memory by also creating the
irqdesc when we probe. Of course, the resources set by of_device_alloc
would be stale if unplug happened, but platform_get_irq would be okay.
However, I think we don't reference count irqchips to prevent their
hot unplug while in use. GPIOs have similar hotplug issues and that is
being worked on.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html