[PATCH v5 0/2] dtc: Dynamic DT support

[PATCH v5 0/2] dtc: Dynamic DT support

[Pantelis Antoniou]

Enable the generation of symbol & fixup information for
use with dynamic DT loading.

Changes since v4:
* Rebase to latest dtc version.
* Completely redesigned the generation of resolution data.
Now instead of being generated as part of blob generation
they are created in the live tree.
* Consequently the patchset is much smaller.
* Added -A auto-label alias generation option.
* Addressed maintainer comments.
* Added syntactic sugar for overlays in the form of .dtsi
* Added /dts-v1/ /plugin/ preferred plugin form and deprecate
the previous form (although still works for backward compatibility)

Changes since v3:
* Rebase to latest dtc version.

Changes since v2:
* Split single patch to a patchset.
* Updated to dtc mainline.
* Changed __local_fixups__ format
* Clean up for better legibility.


Pantelis Antoniou (2):
  dtc: Plugin and fixup support
  dtc: Document the dynamic plugin internals

 Documentation/dt-object-internal.txt | 317 +++++++++++++++++++++++++++++++++++
 Documentation/manual.txt             |  16 ++
 checks.c                             |  18 +-
 dtc-lexer.l                          |   5 +
 dtc-parser.y                         |  54 +++++-
 dtc.c                                |  21 ++-
 dtc.h                                |  13 +-
 livetree.c                           | 202 ++++++++++++++++++++++
 treesource.c                         |   3 +
 9 files changed, 638 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/dt-object-internal.txt

-- 
1.7.12

[PATCH v5 1/2] dtc: Plugin and fixup support

[Pantelis Antoniou]

This patch enable the generation of symbols & local fixup information
for trees compiled with the -@ (--symbols) option.

Using this patch labels in the tree and their users emit information
in __symbols__ and __local_fixups__ nodes.

The __fixups__ node make possible the dynamic resolution of phandle
references which are present in the plugin tree but lie in the
tree that are applying the overlay against.

panto: The original alternative syntax patch required the use of an empty node
which is no longer required.
Numbering of fragments in sequence was also implemented.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/manual.txt |  16 ++++
 checks.c                 |  18 ++++-
 dtc-lexer.l              |   5 ++
 dtc-parser.y             |  54 +++++++++++--
 dtc.c                    |  21 ++++-
 dtc.h                    |  13 ++-
 livetree.c               | 202 +++++++++++++++++++++++++++++++++++++++++++++++
 treesource.c             |   3 +
 8 files changed, 321 insertions(+), 11 deletions(-)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 398de32..29682df 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -119,6 +119,20 @@ Options:
 	Make space for <number> reserve map entries
 	Relevant for dtb and asm output only.
 
+    -@
+        Generates a __symbols__ node at the root node of the resulting blob
+	for any node labels used, and for any local references using phandles
+	it also generates a __local_fixups__ node that tracks them.
+
+	When using the /plugin/ tag all unresolved label references to
+	be tracked in the __fixups__ node, making dynamic resolution possible.
+
+    -A
+	Generate automatically aliases for all node labels. This is similar to
+	the -@ option (the __symbols__ node contain identical information) but
+	the semantics are slightly different since no phandles are automatically
+	generated for labeled nodes.
+
     -S <bytes>
 	Ensure the blob at least <bytes> long, adding additional
 	space if needed.
@@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
 
     devicetree:   '/' nodedef
 
+    plugindecl:   '/' 'plugin' '/' ';'
+
     nodedef:      '{' list_of_property list_of_subnode '}' ';'
 
     property:     label PROPNAME '=' propdata ';'
diff --git a/checks.c b/checks.c
index 0c03ac9..65bf6fd 100644
--- a/checks.c
+++ b/checks.c
@@ -466,8 +466,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
 
 		refnode = get_node_by_ref(dt, m->ref);
 		if (! refnode) {
-			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
-			     m->ref);
+			if (!source_is_plugin)
+				FAIL(c, "Reference to non-existent node or "
+						"label \"%s\"\n", m->ref);
+			else /* mark the entry as unresolved */
+				*((cell_t *)(prop->val.val + m->offset)) =
+					cpu_to_fdt32(0xffffffff);
 			continue;
 		}
 
@@ -652,6 +656,15 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
 }
 TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
 
+static void check_deprecated_plugin_syntax(struct check *c,
+					   struct node *dt)
+{
+	if (deprecated_plugin_syntax_warning)
+		FAIL(c, "Use '/dts-v1/ /plugin/'; syntax. /dts-v1/; /plugin/; "
+				"is going to be removed in next versions");
+}
+TREE_WARNING(deprecated_plugin_syntax, NULL);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,
@@ -669,6 +682,7 @@ static struct check *check_table[] = {
 
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
+	&deprecated_plugin_syntax,
 
 	&always_fail,
 };
diff --git a/dtc-lexer.l b/dtc-lexer.l
index 0ee1caf..dd44ba2 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -113,6 +113,11 @@ static void lexical_error(const char *fmt, ...);
 			return DT_V1;
 		}
 
+<*>"/plugin/"	{
+			DPRINT("Keyword: /plugin/\n");
+			return DT_PLUGIN;
+		}
+
 <*>"/memreserve/"	{
 			DPRINT("Keyword: /memreserve/\n");
 			BEGIN_DEFAULT();
diff --git a/dtc-parser.y b/dtc-parser.y
index 5a897e3..accccba 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -19,6 +19,7 @@
  */
 %{
 #include <stdio.h>
+#include <inttypes.h>
 
 #include "dtc.h"
 #include "srcpos.h"
@@ -52,9 +53,11 @@ extern bool treesource_error;
 	struct node *nodelist;
 	struct reserve_info *re;
 	uint64_t integer;
+	bool is_plugin;
 }
 
 %token DT_V1
+%token DT_PLUGIN
 %token DT_MEMRESERVE
 %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
 %token DT_BITS
@@ -71,6 +74,7 @@ extern bool treesource_error;
 
 %type <data> propdata
 %type <data> propdataprefix
+%type <is_plugin> plugindecl
 %type <re> memreserve
 %type <re> memreserves
 %type <array> arrayprefix
@@ -101,10 +105,39 @@ extern bool treesource_error;
 %%
 
 sourcefile:
-	  DT_V1 ';' memreserves devicetree
+	    basesource
+	  | pluginsource
+	  ;
+
+basesource:
+	  DT_V1 ';' plugindecl memreserves devicetree
+		{
+			source_is_plugin = $3;
+			if (source_is_plugin)
+				deprecated_plugin_syntax_warning = true;
+			the_boot_info = build_boot_info($4, $5,
+							guess_boot_cpuid($5));
+		}
+	;
+
+plugindecl:
+	/* empty */
+		{
+			$$ = false;
+		}
+	| DT_PLUGIN ';'
+		{
+			$$ = true;
+		}
+	;
+
+pluginsource:
+	DT_V1 DT_PLUGIN ';' memreserves devicetree
 		{
-			the_boot_info = build_boot_info($3, $4,
-							guess_boot_cpuid($4));
+			source_is_plugin = true;
+			deprecated_plugin_syntax_warning = false;
+			the_boot_info = build_boot_info($4, $5,
+							guess_boot_cpuid($5));
 		}
 	;
 
@@ -156,10 +189,14 @@ devicetree:
 		{
 			struct node *target = get_node_by_ref($1, $2);
 
-			if (target)
+			if (target) {
 				merge_nodes(target, $3);
-			else
-				ERROR(&@2, "Label or path %s not found", $2);
+			} else {
+				if (symbol_fixup_support)
+					add_orphan_node($1, $3, $2);
+				else
+					ERROR(&@2, "Label or path %s not found", $2);
+			}
 			$$ = $1;
 		}
 	| devicetree DT_DEL_NODE DT_REF ';'
@@ -174,6 +211,11 @@ devicetree:
 
 			$$ = $1;
 		}
+	| /* empty */
+		{
+			/* build empty node */
+			$$ = name_node(build_node(NULL, NULL), "");
+		}
 	;
 
 nodedef:
diff --git a/dtc.c b/dtc.c
index 5fa23c4..ee37be9 100644
--- a/dtc.c
+++ b/dtc.c
@@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
 int minsize;		/* Minimum blob size */
 int padsize;		/* Additional padding to blob */
 int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
+int symbol_fixup_support;
+int auto_label_aliases;
 
 static void fill_fullpaths(struct node *tree, const char *prefix)
 {
@@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
 #define FDT_VERSION(version)	_FDT_VERSION(version)
 #define _FDT_VERSION(version)	#version
 static const char usage_synopsis[] = "dtc [options] <input file>";
-static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
 static struct option const usage_long_opts[] = {
 	{"quiet",            no_argument, NULL, 'q'},
 	{"in-format",         a_argument, NULL, 'I'},
@@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
 	{"phandle",           a_argument, NULL, 'H'},
 	{"warning",           a_argument, NULL, 'W'},
 	{"error",             a_argument, NULL, 'E'},
+	{"symbols",	     no_argument, NULL, '@'},
+	{"auto-alias",       no_argument, NULL, 'A'},
 	{"help",             no_argument, NULL, 'h'},
 	{"version",          no_argument, NULL, 'v'},
 	{NULL,               no_argument, NULL, 0x0},
@@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
 	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
 	"\n\tEnable/disable warnings (prefix with \"no-\")",
 	"\n\tEnable/disable errors (prefix with \"no-\")",
+	"\n\tEnable symbols/fixup support",
+	"\n\tEnable auto-alias of labels",
 	"\n\tPrint this help and exit",
 	"\n\tPrint version and exit",
 	NULL,
@@ -233,7 +239,12 @@ int main(int argc, char *argv[])
 		case 'E':
 			parse_checks_option(false, true, optarg);
 			break;
-
+		case '@':
+			symbol_fixup_support = 1;
+			break;
+		case 'A':
+			auto_label_aliases = 1;
+			break;
 		case 'h':
 			usage(NULL);
 		default:
@@ -294,6 +305,12 @@ int main(int argc, char *argv[])
 	if (sort)
 		sort_tree(bi);
 
+	if (symbol_fixup_support || auto_label_aliases)
+		generate_label_node(bi->dt, bi->dt);
+
+	if (symbol_fixup_support)
+		generate_fixups_node(bi->dt, bi->dt);
+
 	if (streq(outname, "-")) {
 		outf = stdout;
 	} else {
diff --git a/dtc.h b/dtc.h
index 56212c8..d025111 100644
--- a/dtc.h
+++ b/dtc.h
@@ -20,7 +20,7 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
  *                                                                   USA
  */
-
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -54,6 +54,14 @@ extern int reservenum;		/* Number of memory reservation slots */
 extern int minsize;		/* Minimum blob size */
 extern int padsize;		/* Additional padding to blob */
 extern int phandle_format;	/* Use linux,phandle or phandle properties */
+extern int symbol_fixup_support;/* enable symbols & fixup support */
+extern int auto_label_aliases;	/* auto generate labels -> aliases */
+
+/*
+ * Tree source globals
+ */
+extern bool source_is_plugin;
+extern bool deprecated_plugin_syntax_warning;
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
@@ -194,6 +202,7 @@ struct node *build_node_delete(void);
 struct node *name_node(struct node *node, char *name);
 struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
+void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
 
 void add_property(struct node *node, struct property *prop);
 void delete_property_by_name(struct node *node, char *name);
@@ -244,6 +253,8 @@ struct boot_info {
 struct boot_info *build_boot_info(struct reserve_info *reservelist,
 				  struct node *tree, uint32_t boot_cpuid_phys);
 void sort_tree(struct boot_info *bi);
+void generate_label_node(struct node *node, struct node *dt);
+void generate_fixups_node(struct node *node, struct node *dt);
 
 /* Checks */
 
diff --git a/livetree.c b/livetree.c
index e229b84..1ef9fc4 100644
--- a/livetree.c
+++ b/livetree.c
@@ -216,6 +216,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 	return old_node;
 }
 
+void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
+{
+	static unsigned int next_orphan_fragment = 0;
+	struct node *ovl = xmalloc(sizeof(*ovl));
+	struct property *p;
+	struct data d = empty_data;
+	char *name;
+	int ret;
+
+	memset(ovl, 0, sizeof(*ovl));
+
+	d = data_add_marker(d, REF_PHANDLE, ref);
+	d = data_append_integer(d, 0xffffffff, 32);
+
+	p = build_property("target", d);
+	add_property(ovl, p);
+
+	ret = asprintf(&name, "fragment@%u",
+			next_orphan_fragment++);
+	if (ret == -1)
+		die("asprintf() failed\n");
+	name_node(ovl, name);
+	name_node(new_node, "__overlay__");
+
+	add_child(dt, ovl);
+	add_child(ovl, new_node);
+}
+
 struct node *chain_node(struct node *first, struct node *list)
 {
 	assert(first->next_sibling == NULL);
@@ -709,3 +737,177 @@ void sort_tree(struct boot_info *bi)
 	sort_reserve_entries(bi);
 	sort_node(bi->dt);
 }
+
+void generate_label_node(struct node *node, struct node *dt)
+{
+	struct node *c, *an;
+	struct property *p;
+	struct label *l;
+	int has_label;
+	char *gen_node_name;
+
+	if (auto_label_aliases)
+		gen_node_name = "aliases";
+	else
+		gen_node_name = "__symbols__";
+
+	/* Make sure the label isn't already there */
+	has_label = 0;
+	for_each_label(node->labels, l) {
+		has_label = 1;
+		break;
+	}
+
+	if (has_label) {
+
+		/* an is the aliases/__symbols__ node */
+		an = get_subnode(dt, gen_node_name);
+		/* if no node exists, create it */
+		if (!an) {
+			an = build_node(NULL, NULL);
+			name_node(an, gen_node_name);
+			add_child(dt, an);
+		}
+
+		/* now add the label in the node */
+		for_each_label(node->labels, l) {
+			/* check whether the label already exists */
+			p = get_property(an, l->label);
+			if (p) {
+				fprintf(stderr, "WARNING: label %s already"
+					" exists in /%s", l->label,
+					gen_node_name);
+				continue;
+			}
+
+			/* insert it */
+			p = build_property(l->label,
+				data_copy_escape_string(node->fullpath,
+						strlen(node->fullpath)));
+			add_property(an, p);
+		}
+
+		/* force allocation of a phandle for this node */
+		if (symbol_fixup_support)
+			(void)get_node_phandle(dt, node);
+	}
+
+	for_each_child(node, c)
+		generate_label_node(c, dt);
+}
+
+static void add_fixup_entry(struct node *dt, struct node *node,
+		struct property *prop, struct marker *m)
+{
+	struct node *fn;	/* local fixup node */
+	struct property *p;
+	char *fixups_name = "__fixups__";
+	struct data d;
+	char *entry;
+	int ret;
+
+	/* fn is the node we're putting entries in */
+	fn = get_subnode(dt, fixups_name);
+	/* if no node exists, create it */
+	if (!fn) {
+		fn = build_node(NULL, NULL);
+		name_node(fn, fixups_name);
+		add_child(dt, fn);
+	}
+
+	ret = asprintf(&entry, "%s:%s:%u",
+			node->fullpath, prop->name, m->offset);
+	if (ret == -1)
+		die("asprintf() failed\n");
+
+	p = get_property(fn, m->ref);
+	d = data_append_data(p ? p->val : empty_data, entry, strlen(entry) + 1);
+	if (!p)
+		add_property(fn, build_property(m->ref, d));
+	else
+		p->val = d;
+}
+
+static void add_local_fixup_entry(struct node *dt, struct node *node,
+		struct property *prop, struct marker *m,
+		struct node *refnode)
+{
+	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
+	struct property *p;
+	struct data d;
+	char *local_fixups_name = "__local_fixups__";
+	char *s, *e, *comp;
+	int len;
+
+	/* fn is the node we're putting entries in */
+	lfn = get_subnode(dt, local_fixups_name);
+	/* if no node exists, create it */
+	if (!lfn) {
+		lfn = build_node(NULL, NULL);
+		name_node(lfn, local_fixups_name);
+		add_child(dt, lfn);
+	}
+
+	/* walk the path components creating nodes if they don't exist */
+	comp = NULL;
+	/* start skipping the first / */
+	s = node->fullpath + 1;
+	wn = lfn;
+	while (*s) {
+		/* retrieve path component */
+		e = strchr(s, '/');
+		if (e == NULL)
+			e = s + strlen(s);
+		len = e - s;
+		comp = xrealloc(comp, len + 1);
+		memcpy(comp, s, len);
+		comp[len] = '\0';
+
+		/* if no node exists, create it */
+		nwn = get_subnode(wn, comp);
+		if (!nwn) {
+			nwn = build_node(NULL, NULL);
+			name_node(nwn, strdup(comp));
+			add_child(wn, nwn);
+		}
+		wn = nwn;
+
+		/* last path component */
+		if (!*e)
+			break;
+
+		/* next path component */
+		s = e + 1;
+	}
+	free(comp);
+
+	p = get_property(wn, prop->name);
+	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
+	if (!p)
+		add_property(wn, build_property(prop->name, d));
+	else
+		p->val = d;
+}
+
+void generate_fixups_node(struct node *node, struct node *dt)
+{
+	struct node *c;
+	struct property *prop;
+	struct marker *m;
+	struct node *refnode;
+
+	for_each_property(node, prop) {
+		m = prop->val.markers;
+		for_each_marker_of_type(m, REF_PHANDLE) {
+			refnode = get_node_by_ref(dt, m->ref);
+			if (!refnode)
+				add_fixup_entry(dt, node, prop, m);
+			else
+				add_local_fixup_entry(dt, node, prop, m,
+						refnode);
+		}
+	}
+
+	for_each_child(node, c)
+		generate_fixups_node(c, dt);
+}
diff --git a/treesource.c b/treesource.c
index a55d1d1..e1d6657 100644
--- a/treesource.c
+++ b/treesource.c
@@ -28,6 +28,9 @@ extern YYLTYPE yylloc;
 struct boot_info *the_boot_info;
 bool treesource_error;
 
+bool source_is_plugin;
+bool deprecated_plugin_syntax_warning;
+
 struct boot_info *dt_from_source(const char *fname)
 {
 	the_boot_info = NULL;
-- 
1.7.12

Re: [PATCH v5 1/2] dtc: Plugin and fixup support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 22591 bytes --]

On Wed, Aug 26, 2015 at 09:44:33PM +0300, Pantelis Antoniou wrote:
> This patch enable the generation of symbols & local fixup information
> for trees compiled with the -@ (--symbols) option.
> 
> Using this patch labels in the tree and their users emit information
> in __symbols__ and __local_fixups__ nodes.
> 
> The __fixups__ node make possible the dynamic resolution of phandle
> references which are present in the plugin tree but lie in the
> tree that are applying the overlay against.
> 
> panto: The original alternative syntax patch required the use of an empty node
> which is no longer required.
> Numbering of fragments in sequence was also implemented.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Sorry it's taken me so very long to look at this.  I've been a bit
swamped by critical bugs in my day job.

As I've said before (and say several times below), I don't much like
the fixups/symbols format, but it's in use so I guess we're stuck with
it.

So, the concept and behaviour of this patch seems mostly fine to me.
I've made a number of comments below.  Some are just trivial nits, but
a few are important implementation problems that could lead to nasty
behaviour in edge cases.

> ---
>  Documentation/manual.txt |  16 ++++
>  checks.c                 |  18 ++++-
>  dtc-lexer.l              |   5 ++
>  dtc-parser.y             |  54 +++++++++++--
>  dtc.c                    |  21 ++++-
>  dtc.h                    |  13 ++-
>  livetree.c               | 202 +++++++++++++++++++++++++++++++++++++++++++++++
>  treesource.c             |   3 +
>  8 files changed, 321 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 398de32..29682df 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -119,6 +119,20 @@ Options:
>  	Make space for <number> reserve map entries
>  	Relevant for dtb and asm output only.
>  
> +    -@
> +        Generates a __symbols__ node at the root node of the resulting blob

Nit: indentation error here.

> +	for any node labels used, and for any local references using phandles
> +	it also generates a __local_fixups__ node that tracks them.
> +
> +	When using the /plugin/ tag all unresolved label references to
> +	be tracked in the __fixups__ node, making dynamic resolution possible.
> +
> +    -A
> +	Generate automatically aliases for all node labels. This is similar to
> +	the -@ option (the __symbols__ node contain identical information) but
> +	the semantics are slightly different since no phandles are automatically
> +	generated for labeled nodes.
> +
>      -S <bytes>
>  	Ensure the blob at least <bytes> long, adding additional
>  	space if needed.
> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
>  
>      devicetree:   '/' nodedef
>  
> +    plugindecl:   '/' 'plugin' '/' ';'
> +
>      nodedef:      '{' list_of_property list_of_subnode '}' ';'
>  
>      property:     label PROPNAME '=' propdata ';'
> diff --git a/checks.c b/checks.c
> index 0c03ac9..65bf6fd 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -466,8 +466,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>  
>  		refnode = get_node_by_ref(dt, m->ref);
>  		if (! refnode) {
> -			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> -			     m->ref);
> +			if (!source_is_plugin)
> +				FAIL(c, "Reference to non-existent node or "
> +						"label \"%s\"\n", m->ref);
> +			else /* mark the entry as unresolved */
> +				*((cell_t *)(prop->val.val + m->offset)) =
> +					cpu_to_fdt32(0xffffffff);
>  			continue;
>  		}
>  
> @@ -652,6 +656,15 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
>  }
>  TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
>  
> +static void check_deprecated_plugin_syntax(struct check *c,
> +					   struct node *dt)
> +{
> +	if (deprecated_plugin_syntax_warning)
> +		FAIL(c, "Use '/dts-v1/ /plugin/'; syntax. /dts-v1/; /plugin/; "
> +				"is going to be removed in next versions");

Better to put this warning directly where the bad syntax is detected
in the parser (using ERROR), transferring it to here with the global
is pretty ugly.

The checks infrastructure isn't meant to handle all possible error
conditions - just those that are related to the tree's semantic
content, rather than pars

Plus.. the old syntax was never committed upstream, so I'm inclined to
just drop it now, making this irrelevant.

> +}
> +TREE_WARNING(deprecated_plugin_syntax, NULL);
> +
>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
>  	&node_name_chars, &node_name_format, &property_name_chars,
> @@ -669,6 +682,7 @@ static struct check *check_table[] = {
>  
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
> +	&deprecated_plugin_syntax,
>  
>  	&always_fail,
>  };
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 0ee1caf..dd44ba2 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -113,6 +113,11 @@ static void lexical_error(const char *fmt, ...);
>  			return DT_V1;
>  		}
>  
> +<*>"/plugin/"	{
> +			DPRINT("Keyword: /plugin/\n");
> +			return DT_PLUGIN;
> +		}
> +
>  <*>"/memreserve/"	{
>  			DPRINT("Keyword: /memreserve/\n");
>  			BEGIN_DEFAULT();
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 5a897e3..accccba 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -19,6 +19,7 @@
>   */
>  %{
>  #include <stdio.h>
> +#include <inttypes.h>
>  
>  #include "dtc.h"
>  #include "srcpos.h"
> @@ -52,9 +53,11 @@ extern bool treesource_error;
>  	struct node *nodelist;
>  	struct reserve_info *re;
>  	uint64_t integer;
> +	bool is_plugin;
>  }
>  
>  %token DT_V1
> +%token DT_PLUGIN
>  %token DT_MEMRESERVE
>  %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>  %token DT_BITS
> @@ -71,6 +74,7 @@ extern bool treesource_error;
>  
>  %type <data> propdata
>  %type <data> propdataprefix
> +%type <is_plugin> plugindecl
>  %type <re> memreserve
>  %type <re> memreserves
>  %type <array> arrayprefix
> @@ -101,10 +105,39 @@ extern bool treesource_error;
>  %%
>  
>  sourcefile:
> -	  DT_V1 ';' memreserves devicetree
> +	    basesource
> +	  | pluginsource
> +	  ;
> +
> +basesource:
> +	  DT_V1 ';' plugindecl memreserves devicetree
> +		{
> +			source_is_plugin = $3;
> +			if (source_is_plugin)
> +				deprecated_plugin_syntax_warning = true;
> +			the_boot_info = build_boot_info($4, $5,
> +							guess_boot_cpuid($5));
> +		}
> +	;
> +
> +plugindecl:
> +	/* empty */
> +		{
> +			$$ = false;
> +		}
> +	| DT_PLUGIN ';'
> +		{
> +			$$ = true;
> +		}
> +	;
> +
> +pluginsource:
> +	DT_V1 DT_PLUGIN ';' memreserves devicetree
>  		{
> -			the_boot_info = build_boot_info($3, $4,
> -							guess_boot_cpuid($4));
> +			source_is_plugin = true;
> +			deprecated_plugin_syntax_warning = false;
> +			the_boot_info = build_boot_info($4, $5,
> +							guess_boot_cpuid($5));
>  		}

I think the above will be clearer if you make a new non-terminal, say
'versioninfo', that incorporates the DT_V1 and (optionally) the
/plugin/ tag.  We can extend that with other global flags if we ever
need them.

>  	;
>  
> @@ -156,10 +189,14 @@ devicetree:
>  		{
>  			struct node *target = get_node_by_ref($1, $2);
>  
> -			if (target)
> +			if (target) {
>  				merge_nodes(target, $3);
> -			else
> -				ERROR(&@2, "Label or path %s not found", $2);
> +			} else {
> +				if (symbol_fixup_support)
> +					add_orphan_node($1, $3, $2);
> +				else
> +					ERROR(&@2, "Label or path %s not found", $2);
> +			}
>  			$$ = $1;
>  		}
>  	| devicetree DT_DEL_NODE DT_REF ';'
> @@ -174,6 +211,11 @@ devicetree:
>  
>  			$$ = $1;
>  		}
> +	| /* empty */
> +		{
> +			/* build empty node */
> +			$$ = name_node(build_node(NULL, NULL), "");
> +		}
>  	;

What's the importance of this new rule?  It's connection to plugins
isn't obvious to me.

>  
>  nodedef:
> diff --git a/dtc.c b/dtc.c
> index 5fa23c4..ee37be9 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
>  int minsize;		/* Minimum blob size */
>  int padsize;		/* Additional padding to blob */
>  int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
> +int symbol_fixup_support;
> +int auto_label_aliases;
>  
>  static void fill_fullpaths(struct node *tree, const char *prefix)
>  {
> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>  #define FDT_VERSION(version)	_FDT_VERSION(version)
>  #define _FDT_VERSION(version)	#version
>  static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
>  static struct option const usage_long_opts[] = {
>  	{"quiet",            no_argument, NULL, 'q'},
>  	{"in-format",         a_argument, NULL, 'I'},
> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
>  	{"phandle",           a_argument, NULL, 'H'},
>  	{"warning",           a_argument, NULL, 'W'},
>  	{"error",             a_argument, NULL, 'E'},
> +	{"symbols",	     no_argument, NULL, '@'},
> +	{"auto-alias",       no_argument, NULL, 'A'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
>  	{NULL,               no_argument, NULL, 0x0},
> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
>  	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
>  	"\n\tEnable/disable warnings (prefix with \"no-\")",
>  	"\n\tEnable/disable errors (prefix with \"no-\")",
> +	"\n\tEnable symbols/fixup support",
> +	"\n\tEnable auto-alias of labels",
>  	"\n\tPrint this help and exit",
>  	"\n\tPrint version and exit",
>  	NULL,
> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
>  		case 'E':
>  			parse_checks_option(false, true, optarg);
>  			break;
> -
> +		case '@':
> +			symbol_fixup_support = 1;
> +			break;
> +		case 'A':
> +			auto_label_aliases = 1;
> +			break;
>  		case 'h':
>  			usage(NULL);
>  		default:
> @@ -294,6 +305,12 @@ int main(int argc, char *argv[])
>  	if (sort)
>  		sort_tree(bi);
>  
> +	if (symbol_fixup_support || auto_label_aliases)
> +		generate_label_node(bi->dt, bi->dt);
> +
> +	if (symbol_fixup_support)
> +		generate_fixups_node(bi->dt, bi->dt);
> +
>  	if (streq(outname, "-")) {
>  		outf = stdout;
>  	} else {
> diff --git a/dtc.h b/dtc.h
> index 56212c8..d025111 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -20,7 +20,7 @@
>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>   *                                                                   USA
>   */
> -
> +#define _GNU_SOURCE

Ugh.. I'm at least trying to keep dtc compilable on FreeBSD, so I'd
prefer to avoid using GNU extensions.  What's the feature you needed
this for?

>  #include <stdio.h>
>  #include <string.h>
>  #include <stdlib.h>
> @@ -54,6 +54,14 @@ extern int reservenum;		/* Number of memory reservation slots */
>  extern int minsize;		/* Minimum blob size */
>  extern int padsize;		/* Additional padding to blob */
>  extern int phandle_format;	/* Use linux,phandle or phandle properties */
> +extern int symbol_fixup_support;/* enable symbols & fixup support */
> +extern int auto_label_aliases;	/* auto generate labels -> aliases */
> +
> +/*
> + * Tree source globals
> + */
> +extern bool source_is_plugin;

I think it makes sense to put this flag into the_boot_info, rather
than adding a new global.

> +extern bool deprecated_plugin_syntax_warning;

And as noted above, I don't think you need this one at all.

>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
>  struct node *name_node(struct node *node, char *name);
>  struct node *chain_node(struct node *first, struct node *list);
>  struct node *merge_nodes(struct node *old_node, struct node *new_node);
> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>  
>  void add_property(struct node *node, struct property *prop);
>  void delete_property_by_name(struct node *node, char *name);
> @@ -244,6 +253,8 @@ struct boot_info {
>  struct boot_info *build_boot_info(struct reserve_info *reservelist,
>  				  struct node *tree, uint32_t boot_cpuid_phys);
>  void sort_tree(struct boot_info *bi);
> +void generate_label_node(struct node *node, struct node *dt);
> +void generate_fixups_node(struct node *node, struct node *dt);
>  
>  /* Checks */
>  
> diff --git a/livetree.c b/livetree.c
> index e229b84..1ef9fc4 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -216,6 +216,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  	return old_node;
>  }
>  
> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> +{
> +	static unsigned int next_orphan_fragment = 0;
> +	struct node *ovl = xmalloc(sizeof(*ovl));
> +	struct property *p;
> +	struct data d = empty_data;
> +	char *name;
> +	int ret;
> +
> +	memset(ovl, 0, sizeof(*ovl));
> +
> +	d = data_add_marker(d, REF_PHANDLE, ref);
> +	d = data_append_integer(d, 0xffffffff, 32);
> +	p = build_property("target", d);
> +	add_property(ovl, p);

Hmm, using a phandle (which has to be later mangled) rather than a
string ref directly in the target property seems an unnecessarily
difficult way of doing things, but I guess the format's in use so we
can't fix that now.

> +	ret = asprintf(&name, "fragment@%u",
> +			next_orphan_fragment++);
> +	if (ret == -1)
> +		die("asprintf() failed\n");
> +	name_node(ovl, name);
> +	name_node(new_node, "__overlay__");

It's a bit confusing to me that it's the original node, not 'ovl'
which gets the name "__overlay__".  Maybe a different variable name.

This could produce some confusing results if a plugin source contains
a "base" tree construction.

> +	add_child(dt, ovl);
> +	add_child(ovl, new_node);
> +}
> +
>  struct node *chain_node(struct node *first, struct node *list)
>  {
>  	assert(first->next_sibling == NULL);
> @@ -709,3 +737,177 @@ void sort_tree(struct boot_info *bi)
>  	sort_reserve_entries(bi);
>  	sort_node(bi->dt);
>  }
> +
> +void generate_label_node(struct node *node, struct node *dt)

I prefer to put "context" parameters before more specific parameters,
so I'd like to see dt then node.  Also, maybe call it 'tree', since
that seems to be the more common name in livetree.c.

> +{
> +	struct node *c, *an;
> +	struct property *p;
> +	struct label *l;
> +	int has_label;
> +	char *gen_node_name;
> +
> +	if (auto_label_aliases)
> +		gen_node_name = "aliases";
> +	else
> +		gen_node_name = "__symbols__";

Doing just one or the other here also seems dubious to me, as does
referencing the global here.  I'd prefer to see this as a parameter,
which the main function can pass in.  That way you can also better
handle the case of using both -A and -@ at the same time.

I also think it would be nice to construct the alias/symbols node just
once for the tree then pass the reference to the recursive call.


> +
> +	/* Make sure the label isn't already there */

Comment doesn't match the code, this just tests if the node has a
label at all.  For which 'if (node-labels)' would suffice.

> +	has_label = 0;
> +	for_each_label(node->labels, l) {
> +		has_label = 1;
> +		break;
> +	}
> +
> +	if (has_label) {
> +

Nit: extraneous blank line.

> +		/* an is the aliases/__symbols__ node */
> +		an = get_subnode(dt, gen_node_name);
> +		/* if no node exists, create it */
> +		if (!an) {
> +			an = build_node(NULL, NULL);
> +			name_node(an, gen_node_name);
> +			add_child(dt, an);
> +		}
> +
> +		/* now add the label in the node */
> +		for_each_label(node->labels, l) {
> +			/* check whether the label already exists */
> +			p = get_property(an, l->label);
> +			if (p) {
> +				fprintf(stderr, "WARNING: label %s already"
> +					" exists in /%s", l->label,
> +					gen_node_name);
> +				continue;
> +			}
> +
> +			/* insert it */
> +			p = build_property(l->label,
> +				data_copy_escape_string(node->fullpath,
> +						strlen(node->fullpath)));

fullpath should not contain escape characters, and if it does you
shouldn't be unescaping them, so data_copy_escape_string() is the
wrong tool for this job.

> +			add_property(an, p);
> +		}
> +
> +		/* force allocation of a phandle for this node */
> +		if (symbol_fixup_support)
> +			(void)get_node_phandle(dt, node);

Again, I'd prefer to see a parameter rather than referencing the global.

> +	}
> +
> +	for_each_child(node, c)
> +		generate_label_node(c, dt);
> +}
> +
> +static void add_fixup_entry(struct node *dt, struct node *node,
> +		struct property *prop, struct marker *m)
> +{
> +	struct node *fn;	/* local fixup node */
> +	struct property *p;
> +	char *fixups_name = "__fixups__";
> +	struct data d;
> +	char *entry;
> +	int ret;
> +
> +	/* fn is the node we're putting entries in */
> +	fn = get_subnode(dt, fixups_name);
> +	/* if no node exists, create it */
> +	if (!fn) {
> +		fn = build_node(NULL, NULL);
> +		name_node(fn, fixups_name);
> +		add_child(dt, fn);
> +	}

Again, I'd prefer to see construction and location of the fixups node
factored out.

> +
> +	ret = asprintf(&entry, "%s:%s:%u",
> +			node->fullpath, prop->name, m->offset);

I hate this encoding of things into a string that will have to be
parsed, rather than using the existing mechanisms we have for
structure in the tree.  But, again, the format is in use so I guess
we're stuck with it.

I would at least like to see this throw an error if the path or the
property name include ':' characters that will mess this up.


> +	if (ret == -1)
> +		die("asprintf() failed\n");

Hrm.  We should put an xasprintf function in util.  That also lets us
supply our own implementation when necessary and avoid relying on the
gnu extension.

> +
> +	p = get_property(fn, m->ref);

This doesn't handle the case where m->ref is a path rather than a
label.  It will lead to creating a property with '/' in the name
below, which you really don't want.

> +	d = data_append_data(p ? p->val : empty_data, entry, strlen(entry) + 1);
> +	if (!p)
> +		add_property(fn, build_property(m->ref, d));
> +	else
> +		p->val = d;

I think this would be clearer with just a single if, rather than an if
and the parallel conditional expression.

> +}
> +
> +static void add_local_fixup_entry(struct node *dt, struct node *node,
> +		struct property *prop, struct marker *m,
> +		struct node *refnode)
> +{
> +	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
> +	struct property *p;
> +	struct data d;
> +	char *local_fixups_name = "__local_fixups__";
> +	char *s, *e, *comp;
> +	int len;
> +
> +	/* fn is the node we're putting entries in */
> +	lfn = get_subnode(dt, local_fixups_name);
> +	/* if no node exists, create it */
> +	if (!lfn) {
> +		lfn = build_node(NULL, NULL);
> +		name_node(lfn, local_fixups_name);
> +		add_child(dt, lfn);
> +	}

Again, please factor the node creation.

> +
> +	/* walk the path components creating nodes if they don't exist */

Might be worth making a helper function for this operation.  As it is,
it somewhat obscures the fixup logic that this function is actually
about.

It also seems absurd to me that the local fixups take such a
completely different format from the non-local fixups.  But, yet
again, stuck with it.

> +	comp = NULL;
> +	/* start skipping the first / */
> +	s = node->fullpath + 1;
> +	wn = lfn;
> +	while (*s) {
> +		/* retrieve path component */
> +		e = strchr(s, '/');
> +		if (e == NULL)
> +			e = s + strlen(s);
> +		len = e - s;
> +		comp = xrealloc(comp, len + 1);

You can just allocate comp with strlen(fullpath) bytes in the first
place, rather than realloc()ing on every iteration.

> +		memcpy(comp, s, len);
> +		comp[len] = '\0';
> +
> +		/* if no node exists, create it */
> +		nwn = get_subnode(wn, comp);
> +		if (!nwn) {
> +			nwn = build_node(NULL, NULL);
> +			name_node(nwn, strdup(comp));
> +			add_child(wn, nwn);

This build/name/add tuple appears in a bunch of places in your code,
might be worth a helper function for that too.

> +		}
> +		wn = nwn;
> +
> +		/* last path component */
> +		if (!*e)
> +			break;
> +
> +		/* next path component */
> +		s = e + 1;
> +	}
> +	free(comp);
> +
> +	p = get_property(wn, prop->name);
> +	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
> +	if (!p)
> +		add_property(wn, build_property(prop->name, d));
> +	else
> +		p->val = d;
> +}
> +
> +void generate_fixups_node(struct node *node, struct node *dt)
> +{
> +	struct node *c;
> +	struct property *prop;
> +	struct marker *m;
> +	struct node *refnode;
> +
> +	for_each_property(node, prop) {
> +		m = prop->val.markers;
> +		for_each_marker_of_type(m, REF_PHANDLE) {
> +			refnode = get_node_by_ref(dt, m->ref);
> +			if (!refnode)
> +				add_fixup_entry(dt, node, prop, m);
> +			else
> +				add_local_fixup_entry(dt, node, prop, m,
> +						refnode);

Do you need to consider REF_PATH fixups?

> +		}
> +	}
> +
> +	for_each_child(node, c)
> +		generate_fixups_node(c, dt);
> +}
> diff --git a/treesource.c b/treesource.c
> index a55d1d1..e1d6657 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -28,6 +28,9 @@ extern YYLTYPE yylloc;
>  struct boot_info *the_boot_info;
>  bool treesource_error;
>  
> +bool source_is_plugin;
> +bool deprecated_plugin_syntax_warning;
> +
>  struct boot_info *dt_from_source(const char *fname)
>  {
>  	the_boot_info = NULL;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 1/2] dtc: Plugin and fixup support

[Pantelis Antoniou]

Hi David,

> On Oct 21, 2015, at 05:29 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Wed, Aug 26, 2015 at 09:44:33PM +0300, Pantelis Antoniou wrote:
>> This patch enable the generation of symbols & local fixup information
>> for trees compiled with the -@ (--symbols) option.
>> 
>> Using this patch labels in the tree and their users emit information
>> in __symbols__ and __local_fixups__ nodes.
>> 
>> The __fixups__ node make possible the dynamic resolution of phandle
>> references which are present in the plugin tree but lie in the
>> tree that are applying the overlay against.
>> 
>> panto: The original alternative syntax patch required the use of an empty node
>> which is no longer required.
>> Numbering of fragments in sequence was also implemented.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> Sorry it's taken me so very long to look at this.  I've been a bit
> swamped by critical bugs in my day job.
> 

No worries.

> As I've said before (and say several times below), I don't much like
> the fixups/symbols format, but it's in use so I guess we're stuck with
> it.
> 
> So, the concept and behaviour of this patch seems mostly fine to me.
> I've made a number of comments below.  Some are just trivial nits, but
> a few are important implementation problems that could lead to nasty
> behaviour in edge cases.
> 
>> ---
>> Documentation/manual.txt |  16 ++++
>> checks.c                 |  18 ++++-
>> dtc-lexer.l              |   5 ++
>> dtc-parser.y             |  54 +++++++++++--
>> dtc.c                    |  21 ++++-
>> dtc.h                    |  13 ++-
>> livetree.c               | 202 +++++++++++++++++++++++++++++++++++++++++++++++
>> treesource.c             |   3 +
>> 8 files changed, 321 insertions(+), 11 deletions(-)
>> 
>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>> index 398de32..29682df 100644
>> --- a/Documentation/manual.txt
>> +++ b/Documentation/manual.txt
>> @@ -119,6 +119,20 @@ Options:
>> 	Make space for <number> reserve map entries
>> 	Relevant for dtb and asm output only.
>> 
>> +    -@
>> +        Generates a __symbols__ node at the root node of the resulting blob
> 
> Nit: indentation error here.
> 

OK.

>> +	for any node labels used, and for any local references using phandles
>> +	it also generates a __local_fixups__ node that tracks them.
>> +
>> +	When using the /plugin/ tag all unresolved label references to
>> +	be tracked in the __fixups__ node, making dynamic resolution possible.
>> +
>> +    -A
>> +	Generate automatically aliases for all node labels. This is similar to
>> +	the -@ option (the __symbols__ node contain identical information) but
>> +	the semantics are slightly different since no phandles are automatically
>> +	generated for labeled nodes.
>> +
>>     -S <bytes>
>> 	Ensure the blob at least <bytes> long, adding additional
>> 	space if needed.
>> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
>> 
>>     devicetree:   '/' nodedef
>> 
>> +    plugindecl:   '/' 'plugin' '/' ';'
>> +
>>     nodedef:      '{' list_of_property list_of_subnode '}' ';'
>> 
>>     property:     label PROPNAME '=' propdata ';'
>> diff --git a/checks.c b/checks.c
>> index 0c03ac9..65bf6fd 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -466,8 +466,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>> 
>> 		refnode = get_node_by_ref(dt, m->ref);
>> 		if (! refnode) {
>> -			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
>> -			     m->ref);
>> +			if (!source_is_plugin)
>> +				FAIL(c, "Reference to non-existent node or "
>> +						"label \"%s\"\n", m->ref);
>> +			else /* mark the entry as unresolved */
>> +				*((cell_t *)(prop->val.val + m->offset)) =
>> +					cpu_to_fdt32(0xffffffff);
>> 			continue;
>> 		}
>> 
>> @@ -652,6 +656,15 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
>> }
>> TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
>> 
>> +static void check_deprecated_plugin_syntax(struct check *c,
>> +					   struct node *dt)
>> +{
>> +	if (deprecated_plugin_syntax_warning)
>> +		FAIL(c, "Use '/dts-v1/ /plugin/'; syntax. /dts-v1/; /plugin/; "
>> +				"is going to be removed in next versions");
> 
> Better to put this warning directly where the bad syntax is detected
> in the parser (using ERROR), transferring it to here with the global
> is pretty ugly.
> 
> The checks infrastructure isn't meant to handle all possible error
> conditions - just those that are related to the tree's semantic
> content, rather than pars
> 
> Plus.. the old syntax was never committed upstream, so I'm inclined to
> just drop it now, making this irrelevant.
> 

OK, I will drop the deprecated syntax, but I will maintain an out of tree
patch for people that still keep old style DTSes around.

>> +}
>> +TREE_WARNING(deprecated_plugin_syntax, NULL);
>> +
>> static struct check *check_table[] = {
>> 	&duplicate_node_names, &duplicate_property_names,
>> 	&node_name_chars, &node_name_format, &property_name_chars,
>> @@ -669,6 +682,7 @@ static struct check *check_table[] = {
>> 
>> 	&avoid_default_addr_size,
>> 	&obsolete_chosen_interrupt_controller,
>> +	&deprecated_plugin_syntax,
>> 
>> 	&always_fail,
>> };
>> diff --git a/dtc-lexer.l b/dtc-lexer.l
>> index 0ee1caf..dd44ba2 100644
>> --- a/dtc-lexer.l
>> +++ b/dtc-lexer.l
>> @@ -113,6 +113,11 @@ static void lexical_error(const char *fmt, ...);
>> 			return DT_V1;
>> 		}
>> 
>> +<*>"/plugin/"	{
>> +			DPRINT("Keyword: /plugin/\n");
>> +			return DT_PLUGIN;
>> +		}
>> +
>> <*>"/memreserve/"	{
>> 			DPRINT("Keyword: /memreserve/\n");
>> 			BEGIN_DEFAULT();
>> diff --git a/dtc-parser.y b/dtc-parser.y
>> index 5a897e3..accccba 100644
>> --- a/dtc-parser.y
>> +++ b/dtc-parser.y
>> @@ -19,6 +19,7 @@
>>  */
>> %{
>> #include <stdio.h>
>> +#include <inttypes.h>
>> 
>> #include "dtc.h"
>> #include "srcpos.h"
>> @@ -52,9 +53,11 @@ extern bool treesource_error;
>> 	struct node *nodelist;
>> 	struct reserve_info *re;
>> 	uint64_t integer;
>> +	bool is_plugin;
>> }
>> 
>> %token DT_V1
>> +%token DT_PLUGIN
>> %token DT_MEMRESERVE
>> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>> %token DT_BITS
>> @@ -71,6 +74,7 @@ extern bool treesource_error;
>> 
>> %type <data> propdata
>> %type <data> propdataprefix
>> +%type <is_plugin> plugindecl
>> %type <re> memreserve
>> %type <re> memreserves
>> %type <array> arrayprefix
>> @@ -101,10 +105,39 @@ extern bool treesource_error;
>> %%
>> 
>> sourcefile:
>> -	  DT_V1 ';' memreserves devicetree
>> +	    basesource
>> +	  | pluginsource
>> +	  ;
>> +
>> +basesource:
>> +	  DT_V1 ';' plugindecl memreserves devicetree
>> +		{
>> +			source_is_plugin = $3;
>> +			if (source_is_plugin)
>> +				deprecated_plugin_syntax_warning = true;
>> +			the_boot_info = build_boot_info($4, $5,
>> +							guess_boot_cpuid($5));
>> +		}
>> +	;
>> +
>> +plugindecl:
>> +	/* empty */
>> +		{
>> +			$$ = false;
>> +		}
>> +	| DT_PLUGIN ';'
>> +		{
>> +			$$ = true;
>> +		}
>> +	;
>> +
>> +pluginsource:
>> +	DT_V1 DT_PLUGIN ';' memreserves devicetree
>> 		{
>> -			the_boot_info = build_boot_info($3, $4,
>> -							guess_boot_cpuid($4));
>> +			source_is_plugin = true;
>> +			deprecated_plugin_syntax_warning = false;
>> +			the_boot_info = build_boot_info($4, $5,
>> +							guess_boot_cpuid($5));
>> 		}
> 
> I think the above will be clearer if you make a new non-terminal, say
> 'versioninfo', that incorporates the DT_V1 and (optionally) the
> /plugin/ tag.  We can extend that with other global flags if we ever
> need them.
> 

OK.

>> 	;
>> 
>> @@ -156,10 +189,14 @@ devicetree:
>> 		{
>> 			struct node *target = get_node_by_ref($1, $2);
>> 
>> -			if (target)
>> +			if (target) {
>> 				merge_nodes(target, $3);
>> -			else
>> -				ERROR(&@2, "Label or path %s not found", $2);
>> +			} else {
>> +				if (symbol_fixup_support)
>> +					add_orphan_node($1, $3, $2);
>> +				else
>> +					ERROR(&@2, "Label or path %s not found", $2);
>> +			}
>> 			$$ = $1;
>> 		}
>> 	| devicetree DT_DEL_NODE DT_REF ';'
>> @@ -174,6 +211,11 @@ devicetree:
>> 
>> 			$$ = $1;
>> 		}
>> +	| /* empty */
>> +		{
>> +			/* build empty node */
>> +			$$ = name_node(build_node(NULL, NULL), "");
>> +		}
>> 	;
> 
> What's the importance of this new rule?  It's connection to plugins
> isn't obvious to me.
> 

It is required when you use the syntactic sugar version of an overlay definition. 

&target {
	foo;
};

>> 
>> nodedef:
>> diff --git a/dtc.c b/dtc.c
>> index 5fa23c4..ee37be9 100644
>> --- a/dtc.c
>> +++ b/dtc.c
>> @@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
>> int minsize;		/* Minimum blob size */
>> int padsize;		/* Additional padding to blob */
>> int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
>> +int symbol_fixup_support;
>> +int auto_label_aliases;
>> 
>> static void fill_fullpaths(struct node *tree, const char *prefix)
>> {
>> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>> #define FDT_VERSION(version)	_FDT_VERSION(version)
>> #define _FDT_VERSION(version)	#version
>> static const char usage_synopsis[] = "dtc [options] <input file>";
>> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
>> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
>> static struct option const usage_long_opts[] = {
>> 	{"quiet",            no_argument, NULL, 'q'},
>> 	{"in-format",         a_argument, NULL, 'I'},
>> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
>> 	{"phandle",           a_argument, NULL, 'H'},
>> 	{"warning",           a_argument, NULL, 'W'},
>> 	{"error",             a_argument, NULL, 'E'},
>> +	{"symbols",	     no_argument, NULL, '@'},
>> +	{"auto-alias",       no_argument, NULL, 'A'},
>> 	{"help",             no_argument, NULL, 'h'},
>> 	{"version",          no_argument, NULL, 'v'},
>> 	{NULL,               no_argument, NULL, 0x0},
>> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
>> 	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
>> 	"\n\tEnable/disable warnings (prefix with \"no-\")",
>> 	"\n\tEnable/disable errors (prefix with \"no-\")",
>> +	"\n\tEnable symbols/fixup support",
>> +	"\n\tEnable auto-alias of labels",
>> 	"\n\tPrint this help and exit",
>> 	"\n\tPrint version and exit",
>> 	NULL,
>> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
>> 		case 'E':
>> 			parse_checks_option(false, true, optarg);
>> 			break;
>> -
>> +		case '@':
>> +			symbol_fixup_support = 1;
>> +			break;
>> +		case 'A':
>> +			auto_label_aliases = 1;
>> +			break;
>> 		case 'h':
>> 			usage(NULL);
>> 		default:
>> @@ -294,6 +305,12 @@ int main(int argc, char *argv[])
>> 	if (sort)
>> 		sort_tree(bi);
>> 
>> +	if (symbol_fixup_support || auto_label_aliases)
>> +		generate_label_node(bi->dt, bi->dt);
>> +
>> +	if (symbol_fixup_support)
>> +		generate_fixups_node(bi->dt, bi->dt);
>> +
>> 	if (streq(outname, "-")) {
>> 		outf = stdout;
>> 	} else {
>> diff --git a/dtc.h b/dtc.h
>> index 56212c8..d025111 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -20,7 +20,7 @@
>>  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>>  *                                                                   USA
>>  */
>> -
>> +#define _GNU_SOURCE
> 
> Ugh.. I'm at least trying to keep dtc compilable on FreeBSD, so I'd
> prefer to avoid using GNU extensions.  What's the feature you needed
> this for?
> 

Hmm, it’s for using asprintf.

Funnily asprintf is already used in the srcpos.c file, which is used for the
converter. I can easily switch to sprintf with some hit to readability.

>> #include <stdio.h>
>> #include <string.h>
>> #include <stdlib.h>
>> @@ -54,6 +54,14 @@ extern int reservenum;		/* Number of memory reservation slots */
>> extern int minsize;		/* Minimum blob size */
>> extern int padsize;		/* Additional padding to blob */
>> extern int phandle_format;	/* Use linux,phandle or phandle properties */
>> +extern int symbol_fixup_support;/* enable symbols & fixup support */
>> +extern int auto_label_aliases;	/* auto generate labels -> aliases */
>> +
>> +/*
>> + * Tree source globals
>> + */
>> +extern bool source_is_plugin;
> 
> I think it makes sense to put this flag into the_boot_info, rather
> than adding a new global.

OK

> 
>> +extern bool deprecated_plugin_syntax_warning;
> 
> And as noted above, I don't think you need this one at all.
> 

OK.

>> #define PHANDLE_LEGACY	0x1
>> #define PHANDLE_EPAPR	0x2
>> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
>> struct node *name_node(struct node *node, char *name);
>> struct node *chain_node(struct node *first, struct node *list);
>> struct node *merge_nodes(struct node *old_node, struct node *new_node);
>> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>> 
>> void add_property(struct node *node, struct property *prop);
>> void delete_property_by_name(struct node *node, char *name);
>> @@ -244,6 +253,8 @@ struct boot_info {
>> struct boot_info *build_boot_info(struct reserve_info *reservelist,
>> 				  struct node *tree, uint32_t boot_cpuid_phys);
>> void sort_tree(struct boot_info *bi);
>> +void generate_label_node(struct node *node, struct node *dt);
>> +void generate_fixups_node(struct node *node, struct node *dt);
>> 
>> /* Checks */
>> 
>> diff --git a/livetree.c b/livetree.c
>> index e229b84..1ef9fc4 100644
>> --- a/livetree.c
>> +++ b/livetree.c
>> @@ -216,6 +216,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>> 	return old_node;
>> }
>> 
>> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
>> +{
>> +	static unsigned int next_orphan_fragment = 0;
>> +	struct node *ovl = xmalloc(sizeof(*ovl));
>> +	struct property *p;
>> +	struct data d = empty_data;
>> +	char *name;
>> +	int ret;
>> +
>> +	memset(ovl, 0, sizeof(*ovl));
>> +
>> +	d = data_add_marker(d, REF_PHANDLE, ref);
>> +	d = data_append_integer(d, 0xffffffff, 32);
>> +	p = build_property("target", d);
>> +	add_property(ovl, p);
> 
> Hmm, using a phandle (which has to be later mangled) rather than a
> string ref directly in the target property seems an unnecessarily
> difficult way of doing things, but I guess the format's in use so we
> can't fix that now.

It makes sense in practice though and it makes this to work:

&target {
	foo;
};

> 
>> +	ret = asprintf(&name, "fragment@%u",
>> +			next_orphan_fragment++);
>> +	if (ret == -1)
>> +		die("asprintf() failed\n");
>> +	name_node(ovl, name);
>> +	name_node(new_node, "__overlay__");
> 
> It's a bit confusing to me that it's the original node, not 'ovl'
> which gets the name "__overlay__".  Maybe a different variable name.
> 
> This could produce some confusing results if a plugin source contains
> a "base" tree construction.
> 

This is part of the syntactic sugar part. I will change the names of
the variables to make things clearer.

>> +	add_child(dt, ovl);
>> +	add_child(ovl, new_node);
>> +}
>> +
>> struct node *chain_node(struct node *first, struct node *list)
>> {
>> 	assert(first->next_sibling == NULL);
>> @@ -709,3 +737,177 @@ void sort_tree(struct boot_info *bi)
>> 	sort_reserve_entries(bi);
>> 	sort_node(bi->dt);
>> }
>> +
>> +void generate_label_node(struct node *node, struct node *dt)
> 
> I prefer to put "context" parameters before more specific parameters,
> so I'd like to see dt then node.  Also, maybe call it 'tree', since
> that seems to be the more common name in livetree.c.
> 

OK.

>> +{
>> +	struct node *c, *an;
>> +	struct property *p;
>> +	struct label *l;
>> +	int has_label;
>> +	char *gen_node_name;
>> +
>> +	if (auto_label_aliases)
>> +		gen_node_name = "aliases";
>> +	else
>> +		gen_node_name = "__symbols__";
> 
> Doing just one or the other here also seems dubious to me, as does
> referencing the global here.  I'd prefer to see this as a parameter,
> which the main function can pass in.  That way you can also better
> handle the case of using both -A and -@ at the same time.
> 

I’ll see what I can do… there were some complications when I tried it.

> I also think it would be nice to construct the alias/symbols node just
> once for the tree then pass the reference to the recursive call.
> 

Hmm… maybe. I’ll give it a whirl.

> 
>> +
>> +	/* Make sure the label isn't already there */
> 
> Comment doesn't match the code, this just tests if the node has a
> label at all.  For which 'if (node-labels)' would suffice.
> 

Comment will be deleted. Probably a leftover.

>> +	has_label = 0;
>> +	for_each_label(node->labels, l) {
>> +		has_label = 1;
>> +		break;
>> +	}
>> +
>> +	if (has_label) {
>> +
> 
> Nit: extraneous blank line.
> 

OK. (geeze)

>> +		/* an is the aliases/__symbols__ node */
>> +		an = get_subnode(dt, gen_node_name);
>> +		/* if no node exists, create it */
>> +		if (!an) {
>> +			an = build_node(NULL, NULL);
>> +			name_node(an, gen_node_name);
>> +			add_child(dt, an);
>> +		}
>> +
>> +		/* now add the label in the node */
>> +		for_each_label(node->labels, l) {
>> +			/* check whether the label already exists */
>> +			p = get_property(an, l->label);
>> +			if (p) {
>> +				fprintf(stderr, "WARNING: label %s already"
>> +					" exists in /%s", l->label,
>> +					gen_node_name);
>> +				continue;
>> +			}
>> +
>> +			/* insert it */
>> +			p = build_property(l->label,
>> +				data_copy_escape_string(node->fullpath,
>> +						strlen(node->fullpath)));
> 
> fullpath should not contain escape characters, and if it does you
> shouldn't be unescaping them, so data_copy_escape_string() is the
> wrong tool for this job.
> 

OK.

>> +			add_property(an, p);
>> +		}
>> +
>> +		/* force allocation of a phandle for this node */
>> +		if (symbol_fixup_support)
>> +			(void)get_node_phandle(dt, node);
> 
> Again, I'd prefer to see a parameter rather than referencing the global.
> 

OK

>> +	}
>> +
>> +	for_each_child(node, c)
>> +		generate_label_node(c, dt);
>> +}
>> +
>> +static void add_fixup_entry(struct node *dt, struct node *node,
>> +		struct property *prop, struct marker *m)
>> +{
>> +	struct node *fn;	/* local fixup node */
>> +	struct property *p;
>> +	char *fixups_name = "__fixups__";
>> +	struct data d;
>> +	char *entry;
>> +	int ret;
>> +
>> +	/* fn is the node we're putting entries in */
>> +	fn = get_subnode(dt, fixups_name);
>> +	/* if no node exists, create it */
>> +	if (!fn) {
>> +		fn = build_node(NULL, NULL);
>> +		name_node(fn, fixups_name);
>> +		add_child(dt, fn);
>> +	}
> 
> Again, I'd prefer to see construction and location of the fixups node
> factored out.
> 

OK.

>> +
>> +	ret = asprintf(&entry, "%s:%s:%u",
>> +			node->fullpath, prop->name, m->offset);
> 
> I hate this encoding of things into a string that will have to be
> parsed, rather than using the existing mechanisms we have for
> structure in the tree.  But, again, the format is in use so I guess
> we're stuck with it.
> 
> I would at least like to see this throw an error if the path or the
> property name include ':' characters that will mess this up.
> 

Will do.

> 
>> +	if (ret == -1)
>> +		die("asprintf() failed\n");
> 
> Hrm.  We should put an xasprintf function in util.  That also lets us
> supply our own implementation when necessary and avoid relying on the
> gnu extension.
> 

I think that would be best.

>> +
>> +	p = get_property(fn, m->ref);
> 
> This doesn't handle the case where m->ref is a path rather than a
> label.  It will lead to creating a property with '/' in the name
> below, which you really don't want.
> 

I haven’t thought of the m->ref being a path. Will guard against it.

>> +	d = data_append_data(p ? p->val : empty_data, entry, strlen(entry) + 1);
>> +	if (!p)
>> +		add_property(fn, build_property(m->ref, d));
>> +	else
>> +		p->val = d;
> 
> I think this would be clearer with just a single if, rather than an if
> and the parallel conditional expression.
> 

OK.

>> +}
>> +
>> +static void add_local_fixup_entry(struct node *dt, struct node *node,
>> +		struct property *prop, struct marker *m,
>> +		struct node *refnode)
>> +{
>> +	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
>> +	struct property *p;
>> +	struct data d;
>> +	char *local_fixups_name = "__local_fixups__";
>> +	char *s, *e, *comp;
>> +	int len;
>> +
>> +	/* fn is the node we're putting entries in */
>> +	lfn = get_subnode(dt, local_fixups_name);
>> +	/* if no node exists, create it */
>> +	if (!lfn) {
>> +		lfn = build_node(NULL, NULL);
>> +		name_node(lfn, local_fixups_name);
>> +		add_child(dt, lfn);
>> +	}
> 
> Again, please factor the node creation.
> 

OK

>> +
>> +	/* walk the path components creating nodes if they don't exist */
> 
> Might be worth making a helper function for this operation.  As it is,
> it somewhat obscures the fixup logic that this function is actually
> about.
> 

OK

> It also seems absurd to me that the local fixups take such a
> completely different format from the non-local fixups.  But, yet
> again, stuck with it.
> 

I explained before, there’s a reason for it :)

>> +	comp = NULL;
>> +	/* start skipping the first / */
>> +	s = node->fullpath + 1;
>> +	wn = lfn;
>> +	while (*s) {
>> +		/* retrieve path component */
>> +		e = strchr(s, '/');
>> +		if (e == NULL)
>> +			e = s + strlen(s);
>> +		len = e - s;
>> +		comp = xrealloc(comp, len + 1);
> 
> You can just allocate comp with strlen(fullpath) bytes in the first
> place, rather than realloc()ing on every iteration.
> 

OK

>> +		memcpy(comp, s, len);
>> +		comp[len] = '\0';
>> +
>> +		/* if no node exists, create it */
>> +		nwn = get_subnode(wn, comp);
>> +		if (!nwn) {
>> +			nwn = build_node(NULL, NULL);
>> +			name_node(nwn, strdup(comp));
>> +			add_child(wn, nwn);
> 
> This build/name/add tuple appears in a bunch of places in your code,
> might be worth a helper function for that too.
> 

OK

>> +		}
>> +		wn = nwn;
>> +
>> +		/* last path component */
>> +		if (!*e)
>> +			break;
>> +
>> +		/* next path component */
>> +		s = e + 1;
>> +	}
>> +	free(comp);
>> +
>> +	p = get_property(wn, prop->name);
>> +	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
>> +	if (!p)
>> +		add_property(wn, build_property(prop->name, d));
>> +	else
>> +		p->val = d;
>> +}
>> +
>> +void generate_fixups_node(struct node *node, struct node *dt)
>> +{
>> +	struct node *c;
>> +	struct property *prop;
>> +	struct marker *m;
>> +	struct node *refnode;
>> +
>> +	for_each_property(node, prop) {
>> +		m = prop->val.markers;
>> +		for_each_marker_of_type(m, REF_PHANDLE) {
>> +			refnode = get_node_by_ref(dt, m->ref);
>> +			if (!refnode)
>> +				add_fixup_entry(dt, node, prop, m);
>> +			else
>> +				add_local_fixup_entry(dt, node, prop, m,
>> +						refnode);
> 
> Do you need to consider REF_PATH fixups?
> 

I don’t think so, but I will take a look.

>> +		}
>> +	}
>> +
>> +	for_each_child(node, c)
>> +		generate_fixups_node(c, dt);
>> +}
>> diff --git a/treesource.c b/treesource.c
>> index a55d1d1..e1d6657 100644
>> --- a/treesource.c
>> +++ b/treesource.c
>> @@ -28,6 +28,9 @@ extern YYLTYPE yylloc;
>> struct boot_info *the_boot_info;
>> bool treesource_error;
>> 
>> +bool source_is_plugin;
>> +bool deprecated_plugin_syntax_warning;
>> +
>> struct boot_info *dt_from_source(const char *fname)
>> {
>> 	the_boot_info = NULL;
> 

It’s going to take a couple of days for the updated patch to be sent.

Thanks for the review.

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards

— Pantelis

Re: [PATCH v5 1/2] dtc: Plugin and fixup support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7172 bytes --]

On Wed, Oct 21, 2015 at 04:52:25PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Oct 21, 2015, at 05:29 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Wed, Aug 26, 2015 at 09:44:33PM +0300, Pantelis Antoniou wrote:
> >> This patch enable the generation of symbols & local fixup information
> >> for trees compiled with the -@ (--symbols) option.
> >> 
> >> Using this patch labels in the tree and their users emit information
> >> in __symbols__ and __local_fixups__ nodes.
> >> 
> >> The __fixups__ node make possible the dynamic resolution of phandle
> >> references which are present in the plugin tree but lie in the
> >> tree that are applying the overlay against.
> >> 
> >> panto: The original alternative syntax patch required the use of an empty node
> >> which is no longer required.
> >> Numbering of fragments in sequence was also implemented.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > 
> > Sorry it's taken me so very long to look at this.  I've been a bit
> > swamped by critical bugs in my day job.
> > 
> 
> No worries.
> 
> > As I've said before (and say several times below), I don't much like
> > the fixups/symbols format, but it's in use so I guess we're stuck with
> > it.
> > 
> > So, the concept and behaviour of this patch seems mostly fine to me.
> > I've made a number of comments below.  Some are just trivial nits, but
> > a few are important implementation problems that could lead to nasty
> > behaviour in edge cases.

[snip]
> >> @@ -652,6 +656,15 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
> >> }
> >> TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
> >> 
> >> +static void check_deprecated_plugin_syntax(struct check *c,
> >> +					   struct node *dt)
> >> +{
> >> +	if (deprecated_plugin_syntax_warning)
> >> +		FAIL(c, "Use '/dts-v1/ /plugin/'; syntax. /dts-v1/; /plugin/; "
> >> +				"is going to be removed in next versions");
> > 
> > Better to put this warning directly where the bad syntax is detected
> > in the parser (using ERROR), transferring it to here with the global
> > is pretty ugly.
> > 
> > The checks infrastructure isn't meant to handle all possible error
> > conditions - just those that are related to the tree's semantic
> > content, rather than pars
> > 
> > Plus.. the old syntax was never committed upstream, so I'm inclined to
> > just drop it now, making this irrelevant.
> > 
> 
> OK, I will drop the deprecated syntax, but I will maintain an out of tree
> patch for people that still keep old style DTSes around.

Ok.  If you think there are a reasonable number of people out there
with the old style dts, then I'll re-consider merging the support for
the older syntax.

[snip]
> >> @@ -156,10 +189,14 @@ devicetree:
> >> 		{
> >> 			struct node *target = get_node_by_ref($1, $2);
> >> 
> >> -			if (target)
> >> +			if (target) {
> >> 				merge_nodes(target, $3);
> >> -			else
> >> -				ERROR(&@2, "Label or path %s not found", $2);
> >> +			} else {
> >> +				if (symbol_fixup_support)
> >> +					add_orphan_node($1, $3, $2);
> >> +				else
> >> +					ERROR(&@2, "Label or path %s not found", $2);
> >> +			}
> >> 			$$ = $1;
> >> 		}
> >> 	| devicetree DT_DEL_NODE DT_REF ';'
> >> @@ -174,6 +211,11 @@ devicetree:
> >> 
> >> 			$$ = $1;
> >> 		}
> >> +	| /* empty */
> >> +		{
> >> +			/* build empty node */
> >> +			$$ = name_node(build_node(NULL, NULL), "");
> >> +		}
> >> 	;
> > 
> > What's the importance of this new rule?  It's connection to plugins
> > isn't obvious to me.
> > 
> 
> It is required when you use the syntactic sugar version of an overlay definition. 
> 
> &target {
> 	foo;
> };

Ah, good point.  With the empty production in place, we should be able
to remove the
	'/' nodedef
production.  In fact, I'm a little surprised we're not getting bison
warnings, since this definitely makes the grammar ambiguous.

[snip]
> >> diff --git a/dtc.h b/dtc.h
> >> index 56212c8..d025111 100644
> >> --- a/dtc.h
> >> +++ b/dtc.h
> >> @@ -20,7 +20,7 @@
> >>  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> >>  *                                                                   USA
> >>  */
> >> -
> >> +#define _GNU_SOURCE
> > 
> > Ugh.. I'm at least trying to keep dtc compilable on FreeBSD, so I'd
> > prefer to avoid using GNU extensions.  What's the feature you needed
> > this for?
> > 
> 
> Hmm, it’s for using asprintf.
> 
> Funnily asprintf is already used in the srcpos.c file, which is used for the
> converter. I can easily switch to sprintf with some hit to readability.

Yeah, I cleaned a bunch of stuff to get things compiling on FreeBSD,
but then I think it bitrotted again.  Mind you I suspect FreeBSD has
asprintf, just maybe in a different header.

I kind of want to include an asprintf implementation which we use if
the system library doesn't have it, except that involves adding some
kind of configure step that I've so far been able to avoid.

Hmm.. for now, I think use asprintf, putting any _GNU_SOURCE that's
necessary into individual C files, rather than dtc.h.  We can think
about cleaning that up some other time.

[snip]
> >> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> >> +{
> >> +	static unsigned int next_orphan_fragment = 0;
> >> +	struct node *ovl = xmalloc(sizeof(*ovl));
> >> +	struct property *p;
> >> +	struct data d = empty_data;
> >> +	char *name;
> >> +	int ret;
> >> +
> >> +	memset(ovl, 0, sizeof(*ovl));
> >> +
> >> +	d = data_add_marker(d, REF_PHANDLE, ref);
> >> +	d = data_append_integer(d, 0xffffffff, 32);
> >> +	p = build_property("target", d);
> >> +	add_property(ovl, p);
> > 
> > Hmm, using a phandle (which has to be later mangled) rather than a
> > string ref directly in the target property seems an unnecessarily
> > difficult way of doing things, but I guess the format's in use so we
> > can't fix that now.
> 
> It makes sense in practice though and it makes this to work:
> 
> &target {
> 	foo;
> };

Yeah, but that could have been implemented by putting the label string
straight into the "target" property on the overlay blob, rather than
putting a necessarily meaningless phandle which later gets resolved.

Oh well, too late now.

[snip]
> >> +	has_label = 0;
> >> +	for_each_label(node->labels, l) {
> >> +		has_label = 1;
> >> +		break;
> >> +	}
> >> +
> >> +	if (has_label) {
> >> +
> > 
> > Nit: extraneous blank line.
> > 
> 
> OK. (geeze)

Yeah, that was a nitpick.  Wouldn't mention it if there weren't
already other reasons for a respin.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v5 2/2] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Provides the document explaining the internal mechanics of
plugins and options.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/dt-object-internal.txt | 317 +++++++++++++++++++++++++++++++++++
 1 file changed, 317 insertions(+)
 create mode 100644 Documentation/dt-object-internal.txt

diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
new file mode 100644
index 0000000..38a47b6
--- /dev/null
+++ b/Documentation/dt-object-internal.txt
@@ -0,0 +1,317 @@
+Device Tree Dynamic Object format internals
+-------------------------------------------
+
+The Device Tree for most platforms is a static representation of
+the hardware capabilities. This is insufficient for many platforms
+that need to dynamically insert device tree fragments to the
+running kernel's live tree.
+
+This document explains the the device tree object format and the
+modifications made to the device tree compiler, which make it possible.
+
+1. Simplified Problem Definition
+--------------------------------
+
+Assume we have a platform which boots using following simplified device tree.
+
+---- foo.dts -----------------------------------------------------------------
+	/* FOO platform */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+		}
+	};
+---- foo.dts -----------------------------------------------------------------
+
+We have a number of peripherals that after probing (using some undefined method)
+should result in different device tree configuration.
+
+We cannot boot with this static tree because due to the configuration of the
+foo platform there exist multiple conficting peripherals DT fragments.
+
+So for the bar peripheral we would have this:
+
+---- foo+bar.dts -------------------------------------------------------------
+	/* FOO platform + bar peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		}
+	};
+---- foo+bar.dts -------------------------------------------------------------
+
+While for the baz peripheral we would have this:
+
+---- foo+baz.dts -------------------------------------------------------------
+	/* FOO platform + baz peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+			/* baz resources */
+			baz_res: res_baz { ... };
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* baz peripheral */
+			baz {
+				compatible = "corp,baz";
+				/* reference to another point in the tree */
+				ref-to-res = <&baz_res>;
+				... /* various properties and child nodes */
+			}
+		}
+	};
+---- foo+baz.dts -------------------------------------------------------------
+
+We note that the baz case is more complicated, since the baz peripheral needs to
+reference another node in the DT tree.
+
+2. Device Tree Object Format Requirements
+-----------------------------------------
+
+Since the device tree is used for booting a number of very different hardware
+platforms it is imperative that we tread very carefully.
+
+2.a) No changes to the Device Tree binary format. We cannot modify the tree
+format at all and all the information we require should be encoded using device
+tree itself. We can add nodes that can be safely ignored by both bootloaders and
+the kernel.
+
+2.b) Changes to the DTS source format should be absolutely minimal, and should
+only be needed for the DT fragment definitions, and not the base boot DT.
+
+2.c) An explicit option should be used to instruct DTC to generate the required
+information needed for object resolution. Platforms that don't use the
+dynamic object format can safely ignore it.
+
+2.d) Finally, DT syntax changes should be kept to a minimum. It should be
+possible to express everything using the existing DT syntax.
+
+3. Implementation
+-----------------
+
+The basic unit of addressing in Device Tree is the phandle. Turns out it's
+relatively simple to extend the way phandles are generated and referenced
+so that it's possible to dynamically convert symbolic references (labels)
+to phandle values.
+
+We can roughly divide the operation into two steps.
+
+3.a) Compilation of the base board DTS file using the '-@' option
+generates a valid DT blob with an added __symbols__ node at the root node,
+containing a list of all nodes that are marked with a label.
+
+Using the foo.dts file above the following node will be generated;
+
+$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
+$ fdtdump foo.dtb
+...
+/ {
+	...
+	res {
+		...
+		linux,phandle = <0x00000001>;
+		phandle = <0x00000001>;
+		...
+	};
+	ocp {
+		...
+		linux,phandle = <0x00000002>;
+		phandle = <0x00000002>;
+		...
+	};
+	__symbols__ {
+		res="/res";
+		ocp="/ocp";
+	};
+};
+
+Notice that all the nodes that had a label have been recorded, and that
+phandles have been generated for them.
+
+This blob can be used to boot the board normally, the __symbols__ node will
+be safely ignored both by the bootloader and the kernel (the only loss will
+be a few bytes of memory and disk space).
+
+3.b) The Device Tree fragments must be compiled with the same option but they
+must also have a tag (/plugin/) that allows undefined references to labels
+that are not present at compilation time to be recorded so that the runtime
+loader can fix them.
+
+So the bar peripheral's DTS format would be of the form:
+
+/dts-v1/ /plugin/;	/* allow undefined label references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&ocp>;
+		__overlay__ {
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+
+Note that there's a target property that specifies the location where the
+contents of the overlay node will be placed, and it references the label
+in the foo.dts file.
+
+$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
+$ fdtdump bar.dtbo
+...
+/ {
+	... /* properties */
+	fragment@0 {
+		target = <0xdeadbeef>;
+		__overlay__ {
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+	__fixups__ {
+	    ocp = "/fragment@0:target:0";
+	};
+};
+
+No __symbols__ has been generated (no label in bar.dts).
+Note that the target's ocp label is undefined, so the phandle handle
+value is filled with the illegal value '0xdeadbeef', while a __fixups__
+node has been generated, which marks the location in the tree where
+the label lookup should store the runtime phandle value of the ocp node.
+
+The format of the __fixups__ node entry is
+
+	<label> = "<local-full-path>:<property-name>:<offset>";
+
+<label> 		Is the label we're referring
+<local-full-path>	Is the full path of the node the reference is
+<property-name>		Is the name of the property containing the
+			reference
+<offset>		The offset (in bytes) of where the property's
+			phandle value is located.
+
+Doing the same with the baz peripheral's DTS format is a little bit more
+involved, since baz contains references to local labels which require
+local fixups.
+
+/dts-v1/ /plugin/;	/* allow undefined label references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&res>;
+		__overlay__ {
+			/* baz resources */
+			baz_res: res_baz { ... };
+		};
+	};
+	fragment@1 {
+		target = <&ocp>;
+		__overlay__ {
+			/* baz peripheral */
+			baz {
+				compatible = "corp,baz";
+				/* reference to another point in the tree */
+				ref-to-res = <&baz_res>;
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+
+Note that &bar_res reference.
+
+$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
+$ fdtdump baz.dtbo
+...
+/ {
+	... /* properties */
+	fragment@0 {
+		target = <0xdeadbeef>;
+		__overlay__ {
+			res_baz {
+				....
+				linux,phandle = <0x00000001>;
+				phandle = <0x00000001>;
+			};
+		};
+	};
+	fragment@1 {
+		target = <0xdeadbeef>;
+		__overlay__ {
+			baz {
+				compatible = "corp,baz";
+				... /* various properties and child nodes */
+				ref-to-res = <0x00000001>;
+			}
+		};
+	};
+	__fixups__ {
+		res = "/fragment@0:target:0";
+		ocp = "/fragment@1:target:0";
+	};
+	__local_fixups__ {
+		fragment@1 {
+			__overlay__ {
+				baz {
+					ref-to-res = <0>;
+				};
+			};
+		};
+	};
+};
+
+This is similar to the bar case, but the reference of a local label by the
+baz node generates a __local_fixups__ entry that records the place that the
+local reference is being made. No matter how phandles are allocated from dtc
+the run time loader must apply an offset to each phandle in every dynamic
+DT object loaded. The __local_fixups__ node records the place of every
+local reference so that the loader can apply the offset.
+
+There is an alternative syntax to the expanded form for overlays with phandle
+targets which makes the format similar to the one using in .dtsi include files.
+
+So for the &ocp target example above one can simply write:
+
+/dts-v1/ /plugin/;
+&ocp {
+	/* bar peripheral */
+	bar {
+		compatible = "corp,bar";
+		... /* various properties and child nodes */
+	}
+};
+
+The resulting dtb object is identical.
-- 
1.7.12

Re: [PATCH v5 2/2] dtc: Document the dynamic plugin internals

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 11991 bytes --]

On Wed, Aug 26, 2015 at 09:44:34PM +0300, Pantelis Antoniou wrote:
> Provides the document explaining the internal mechanics of
> plugins and options.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/dt-object-internal.txt | 317 +++++++++++++++++++++++++++++++++++
>  1 file changed, 317 insertions(+)
>  create mode 100644 Documentation/dt-object-internal.txt
> 
> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> new file mode 100644
> index 0000000..38a47b6
> --- /dev/null
> +++ b/Documentation/dt-object-internal.txt
> @@ -0,0 +1,317 @@
> +Device Tree Dynamic Object format internals
> +-------------------------------------------
> +
> +The Device Tree for most platforms is a static representation of
> +the hardware capabilities. This is insufficient for many platforms
> +that need to dynamically insert device tree fragments to the
> +running kernel's live tree.
> +
> +This document explains the the device tree object format and the
> +modifications made to the device tree compiler, which make it possible.
> +
> +1. Simplified Problem Definition
> +--------------------------------
> +
> +Assume we have a platform which boots using following simplified device tree.
> +
> +---- foo.dts -----------------------------------------------------------------
> +	/* FOO platform */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +		}
> +	};
> +---- foo.dts -----------------------------------------------------------------
> +
> +We have a number of peripherals that after probing (using some undefined method)
> +should result in different device tree configuration.
> +
> +We cannot boot with this static tree because due to the configuration of the
> +foo platform there exist multiple conficting peripherals DT fragments.
> +
> +So for the bar peripheral we would have this:
> +
> +---- foo+bar.dts -------------------------------------------------------------
> +	/* FOO platform + bar peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		}
> +	};
> +---- foo+bar.dts -------------------------------------------------------------
> +
> +While for the baz peripheral we would have this:
> +
> +---- foo+baz.dts -------------------------------------------------------------
> +	/* FOO platform + baz peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +			/* baz resources */
> +			baz_res: res_baz { ... };
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* baz peripheral */
> +			baz {
> +				compatible = "corp,baz";
> +				/* reference to another point in the tree */
> +				ref-to-res = <&baz_res>;
> +				... /* various properties and child nodes */
> +			}
> +		}
> +	};
> +---- foo+baz.dts -------------------------------------------------------------
> +
> +We note that the baz case is more complicated, since the baz peripheral needs to
> +reference another node in the DT tree.
> +
> +2. Device Tree Object Format Requirements
> +-----------------------------------------
> +
> +Since the device tree is used for booting a number of very different hardware
> +platforms it is imperative that we tread very carefully.
> +
> +2.a) No changes to the Device Tree binary format. We cannot modify the tree
> +format at all and all the information we require should be encoded using device
> +tree itself. We can add nodes that can be safely ignored by both bootloaders and
> +the kernel.

I think this is a bit misguided.  For the base tree with extra symbols
information, this makes sense.  But for the actual plugin framents,
it's basically impossible for the kernel to use them without knowing
that they're a plugin and how to apply them.

So I think plugin "dtb"s should be given a new magic number.  I guess
we'll need shims for existing fragments with the old magic number.
But I still think a new magic number should be the default and
recommended approach.

> +2.b) Changes to the DTS source format should be absolutely minimal, and should
> +only be needed for the DT fragment definitions, and not the base boot DT.
> +
> +2.c) An explicit option should be used to instruct DTC to generate the required
> +information needed for object resolution. Platforms that don't use the
> +dynamic object format can safely ignore it.
> +
> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> +possible to express everything using the existing DT syntax.
> +
> +3. Implementation
> +-----------------
> +
> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> +relatively simple to extend the way phandles are generated and referenced
> +so that it's possible to dynamically convert symbolic references (labels)
> +to phandle values.

At least provided the dts author always uses reference syntax and
never explicitly assigns phandles.  Unlikely to be an issue with most
hand-authored dts files, but could crop up with decompiled files.

> +We can roughly divide the operation into two steps.
> +
> +3.a) Compilation of the base board DTS file using the '-@' option
> +generates a valid DT blob with an added __symbols__ node at the root node,
> +containing a list of all nodes that are marked with a label.
> +
> +Using the foo.dts file above the following node will be generated;
> +
> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> +$ fdtdump foo.dtb
> +...
> +/ {
> +	...
> +	res {
> +		...
> +		linux,phandle = <0x00000001>;
> +		phandle = <0x00000001>;

Probably not useful to include both forms of the phandle property in
the example.

> +		...
> +	};
> +	ocp {
> +		...
> +		linux,phandle = <0x00000002>;
> +		phandle = <0x00000002>;
> +		...
> +	};
> +	__symbols__ {
> +		res="/res";
> +		ocp="/ocp";
> +	};
> +};
> +
> +Notice that all the nodes that had a label have been recorded, and that
> +phandles have been generated for them.

Um.. except that your example _doesn't_ show any labels, just node names.

> +This blob can be used to boot the board normally, the __symbols__ node will
> +be safely ignored both by the bootloader and the kernel (the only loss will
> +be a few bytes of memory and disk space).
> +
> +3.b) The Device Tree fragments must be compiled with the same option but they
> +must also have a tag (/plugin/) that allows undefined references to labels
> +that are not present at compilation time to be recorded so that the runtime
> +loader can fix them.
> +
> +So the bar peripheral's DTS format would be of the form:
> +
> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc. */
> +	fragment@0 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +};
> +
> +Note that there's a target property that specifies the location where the
> +contents of the overlay node will be placed, and it references the label
> +in the foo.dts file.
> +
> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
> +$ fdtdump bar.dtbo
> +...
> +/ {
> +	... /* properties */
> +	fragment@0 {
> +		target = <0xdeadbeef>;
> +		__overlay__ {
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +	__fixups__ {
> +	    ocp = "/fragment@0:target:0";
> +	};
> +};
> +
> +No __symbols__ has been generated (no label in bar.dts).

I'd actually prefer to see all the "special" nodes be created
unconditionally with -@, even if some of them are empty.  Makes it
easier to spot when plugins are in play.

> +Note that the target's ocp label is undefined, so the phandle handle
> +value is filled with the illegal value '0xdeadbeef', while a __fixups__

No longer matches the code which (correctly) uses 0xffffffff instead
of 0xdeafbeef.

> +node has been generated, which marks the location in the tree where
> +the label lookup should store the runtime phandle value of the ocp node.
> +
> +The format of the __fixups__ node entry is
> +
> +	<label> = "<local-full-path>:<property-name>:<offset>";
> +
> +<label> 		Is the label we're referring
> +<local-full-path>	Is the full path of the node the reference is
> +<property-name>		Is the name of the property containing the
> +			reference
> +<offset>		The offset (in bytes) of where the property's
> +			phandle value is located.
> +
> +Doing the same with the baz peripheral's DTS format is a little bit more
> +involved, since baz contains references to local labels which require
> +local fixups.
> +
> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc. */
> +	fragment@0 {
> +		target = <&res>;
> +		__overlay__ {
> +			/* baz resources */
> +			baz_res: res_baz { ... };
> +		};
> +	};
> +	fragment@1 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* baz peripheral */
> +			baz {
> +				compatible = "corp,baz";
> +				/* reference to another point in the tree */
> +				ref-to-res = <&baz_res>;
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +};
> +
> +Note that &bar_res reference.
> +
> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
> +$ fdtdump baz.dtbo
> +...
> +/ {
> +	... /* properties */
> +	fragment@0 {
> +		target = <0xdeadbeef>;
> +		__overlay__ {
> +			res_baz {
> +				....
> +				linux,phandle = <0x00000001>;
> +				phandle = <0x00000001>;
> +			};
> +		};
> +	};
> +	fragment@1 {
> +		target = <0xdeadbeef>;
> +		__overlay__ {
> +			baz {
> +				compatible = "corp,baz";
> +				... /* various properties and child nodes */
> +				ref-to-res = <0x00000001>;
> +			}
> +		};
> +	};
> +	__fixups__ {
> +		res = "/fragment@0:target:0";
> +		ocp = "/fragment@1:target:0";
> +	};
> +	__local_fixups__ {
> +		fragment@1 {
> +			__overlay__ {
> +				baz {
> +					ref-to-res = <0>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +This is similar to the bar case, but the reference of a local label by the
> +baz node generates a __local_fixups__ entry that records the place that the
> +local reference is being made. No matter how phandles are allocated from dtc
> +the run time loader must apply an offset to each phandle in every dynamic
> +DT object loaded. The __local_fixups__ node records the place of every
> +local reference so that the loader can apply the offset.
> +
> +There is an alternative syntax to the expanded form for overlays with phandle
> +targets which makes the format similar to the one using in .dtsi include files.
> +
> +So for the &ocp target example above one can simply write:
> +
> +/dts-v1/ /plugin/;
> +&ocp {
> +	/* bar peripheral */
> +	bar {
> +		compatible = "corp,bar";
> +		... /* various properties and child nodes */
> +	}
> +};
> +
> +The resulting dtb object is identical.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 0/2] dtc: Dynamic DT support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On Wed, Aug 26, 2015 at 09:44:32PM +0300, Pantelis Antoniou wrote:
> Enable the generation of symbol & fixup information for
> use with dynamic DT loading.
> 
> Changes since v4:
> * Rebase to latest dtc version.
> * Completely redesigned the generation of resolution data.
> Now instead of being generated as part of blob generation
> they are created in the live tree.
> * Consequently the patchset is much smaller.
> * Added -A auto-label alias generation option.
> * Addressed maintainer comments.
> * Added syntactic sugar for overlays in the form of .dtsi
> * Added /dts-v1/ /plugin/ preferred plugin form and deprecate
> the previous form (although still works for backward compatibility)
> 
> Changes since v3:
> * Rebase to latest dtc version.
> 
> Changes since v2:
> * Split single patch to a patchset.
> * Updated to dtc mainline.
> * Changed __local_fixups__ format
> * Clean up for better legibility.

Sorry it's taken me so long to get to this.  I was sick after the
conference, and then had backlog of day-job work to do.  I haven't had
time to look at this yet, but I haven't forgotten it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 0/2] dtc: Dynamic DT support

[Pantelis Antoniou]

Hi David,

> On Sep 8, 2015, at 07:00 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Wed, Aug 26, 2015 at 09:44:32PM +0300, Pantelis Antoniou wrote:
>> Enable the generation of symbol & fixup information for
>> use with dynamic DT loading.
>> 
>> Changes since v4:
>> * Rebase to latest dtc version.
>> * Completely redesigned the generation of resolution data.
>> Now instead of being generated as part of blob generation
>> they are created in the live tree.
>> * Consequently the patchset is much smaller.
>> * Added -A auto-label alias generation option.
>> * Addressed maintainer comments.
>> * Added syntactic sugar for overlays in the form of .dtsi
>> * Added /dts-v1/ /plugin/ preferred plugin form and deprecate
>> the previous form (although still works for backward compatibility)
>> 
>> Changes since v3:
>> * Rebase to latest dtc version.
>> 
>> Changes since v2:
>> * Split single patch to a patchset.
>> * Updated to dtc mainline.
>> * Changed __local_fixups__ format
>> * Clean up for better legibility.
> 
> Sorry it's taken me so long to get to this.  I was sick after the
> conference, and then had backlog of day-job work to do.  I haven't had
> time to look at this yet, but I haven't forgotten it.
> 

No worries.

Conference flu is a real killer.

Regards

— Pantelis

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [PATCH v5 0/2] dtc: Dynamic DT support

[Pantelis Antoniou]

Hi David,

> On Sep 8, 2015, at 07:00 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Wed, Aug 26, 2015 at 09:44:32PM +0300, Pantelis Antoniou wrote:
>> Enable the generation of symbol & fixup information for
>> use with dynamic DT loading.
>> 
>> Changes since v4:
>> * Rebase to latest dtc version.
>> * Completely redesigned the generation of resolution data.
>> Now instead of being generated as part of blob generation
>> they are created in the live tree.
>> * Consequently the patchset is much smaller.
>> * Added -A auto-label alias generation option.
>> * Addressed maintainer comments.
>> * Added syntactic sugar for overlays in the form of .dtsi
>> * Added /dts-v1/ /plugin/ preferred plugin form and deprecate
>> the previous form (although still works for backward compatibility)
>> 
>> Changes since v3:
>> * Rebase to latest dtc version.
>> 
>> Changes since v2:
>> * Split single patch to a patchset.
>> * Updated to dtc mainline.
>> * Changed __local_fixups__ format
>> * Clean up for better legibility.
> 
> Sorry it's taken me so long to get to this.  I was sick after the
> conference, and then had backlog of day-job work to do.  I haven't had
> time to look at this yet, but I haven't forgotten it.
> 

No worries.

Conference flu is a real killer.

Regards

— Pantelis

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson