[PATCH v4 0/3] dtc: Dynamic DT support

[PATCH v4 0/3] dtc: Dynamic DT support

[Pantelis Antoniou]

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

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 (3):
  dtc: Symbol and local fixup generation support
  dtc: Plugin (object) device tree support.
  dtc: Document the dynamic plugin internals

 Documentation/dt-object-internal.txt | 301 +++++++++++++++++++++++++++++++++++
 Documentation/manual.txt             |  10 ++
 checks.c                             | 106 +++++++++++-
 dtc-lexer.l                          |   5 +
 dtc-parser.y                         |  22 ++-
 dtc.c                                |   9 +-
 dtc.h                                |  40 +++++
 flattree.c                           | 202 +++++++++++++++++++++++
 8 files changed, 688 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/dt-object-internal.txt

-- 
1.7.12

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

[PATCH v4 1/3] dtc: Symbol and local fixup generation support

[Pantelis Antoniou]

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. Using this information
it is possible to implement run time dynamic tree loading.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/manual.txt |   5 ++
 checks.c                 |  61 +++++++++++++++++++++
 dtc.c                    |   9 ++-
 dtc.h                    |  28 ++++++++++
 flattree.c               | 139 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 240 insertions(+), 2 deletions(-)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 398de32..4359958 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -119,6 +119,11 @@ 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.
+
     -S <bytes>
 	Ensure the blob at least <bytes> long, adding additional
 	space if needed.
diff --git a/checks.c b/checks.c
index e81a8c7..103ca52 100644
--- a/checks.c
+++ b/checks.c
@@ -458,6 +458,7 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
 				     struct node *node, struct property *prop)
 {
 	struct marker *m = prop->val.markers;
+	struct fixup_entry *fe, **fep;
 	struct node *refnode;
 	cell_t phandle;
 
@@ -471,9 +472,28 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
 			continue;
 		}
 
+		/* if it's a local reference, we need to record it */
+		if (symbol_fixup_support) {
+
+			/* allocate a new local fixup entry */
+			fe = xmalloc(sizeof(*fe));
+
+			fe->node = node;
+			fe->prop = prop;
+			fe->offset = m->offset;
+			fe->next = NULL;
+
+			/* append it to the local fixups */
+			fep = &dt->local_fixups;
+			while (*fep)
+				fep = &(*fep)->next;
+			*fep = fe;
+		}
+
 		phandle = get_node_phandle(dt, refnode);
 		*((cell_t *)(prop->val.val + m->offset)) = cpu_to_fdt32(phandle);
 	}
+
 }
 ERROR(phandle_references, NULL, NULL, fixup_phandle_references, NULL,
       &duplicate_node_names, &explicit_phandles);
@@ -652,6 +672,45 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
 }
 TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
 
+static void check_auto_label_phandles(struct check *c, struct node *dt,
+				       struct node *node)
+{
+	struct label *l;
+	struct symbol *s, **sp;
+	int has_label;
+
+	if (!symbol_fixup_support)
+		return;
+
+	has_label = 0;
+	for_each_label(node->labels, l) {
+		has_label = 1;
+		break;
+	}
+
+	if (!has_label)
+		return;
+
+	/* force allocation of a phandle for this node */
+	(void)get_node_phandle(dt, node);
+
+	/* add the symbol */
+	for_each_label(node->labels, l) {
+
+		s = xmalloc(sizeof(*s));
+		s->label = l;
+		s->node = node;
+		s->next = NULL;
+
+		/* add it to the symbols list */
+		sp = &dt->symbols;
+		while (*sp)
+			sp = &((*sp)->next);
+		*sp = s;
+	}
+}
+NODE_WARNING(auto_label_phandles, NULL);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,
@@ -670,6 +729,8 @@ static struct check *check_table[] = {
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
+	&auto_label_phandles,
+
 	&always_fail,
 };
 
diff --git a/dtc.c b/dtc.c
index 8c4add6..91e91e7 100644
--- a/dtc.c
+++ b/dtc.c
@@ -29,6 +29,7 @@ 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 = 0;
 
 static void fill_fullpaths(struct node *tree, const char *prefix)
 {
@@ -51,7 +52,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:@hv";
 static struct option const usage_long_opts[] = {
 	{"quiet",            no_argument, NULL, 'q'},
 	{"in-format",         a_argument, NULL, 'I'},
@@ -69,6 +70,7 @@ 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, '@'},
 	{"help",             no_argument, NULL, 'h'},
 	{"version",          no_argument, NULL, 'v'},
 	{NULL,               no_argument, NULL, 0x0},
@@ -99,6 +101,7 @@ 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\tPrint this help and exit",
 	"\n\tPrint version and exit",
 	NULL,
@@ -186,7 +189,9 @@ int main(int argc, char *argv[])
 		case 'E':
 			parse_checks_option(false, true, optarg);
 			break;
-
+		case '@':
+			symbol_fixup_support = 1;
+			break;
 		case 'h':
 			usage(NULL);
 		default:
diff --git a/dtc.h b/dtc.h
index 56212c8..16354fa 100644
--- a/dtc.h
+++ b/dtc.h
@@ -54,6 +54,7 @@ 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 */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
@@ -132,6 +133,20 @@ struct label {
 	struct label *next;
 };
 
+struct fixup_entry {
+	int offset;
+	struct node *node;
+	struct property *prop;
+	struct fixup_entry *next;
+	bool local_fixup_generated;
+};
+
+struct symbol {
+	struct label *label;
+	struct node *node;
+	struct symbol *next;
+};
+
 struct property {
 	bool deleted;
 	char *name;
@@ -158,6 +173,10 @@ struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+
+	struct symbol *symbols;
+	struct fixup_entry *local_fixups;
+	bool emit_local_fixup_node;
 };
 
 #define for_each_label_withdel(l0, l) \
@@ -181,6 +200,15 @@ struct node {
 	for_each_child_withdel(n, c) \
 		if (!(c)->deleted)
 
+#define for_each_fixup_entry(f, fe) \
+	for ((fe) = (f)->entries; (fe); (fe) = (fe)->next)
+
+#define for_each_symbol(n, s) \
+	for ((s) = (n)->symbols; (s); (s) = (s)->next)
+
+#define for_each_local_fixup_entry(n, fe) \
+	for ((fe) = (n)->local_fixups; (fe); (fe) = (fe)->next)
+
 void add_label(struct label **labels, char *label);
 void delete_labels(struct label **labels);
 
diff --git a/flattree.c b/flattree.c
index bd99fa2..3a58949 100644
--- a/flattree.c
+++ b/flattree.c
@@ -255,6 +255,142 @@ static int stringtable_insert(struct data *d, const char *str)
 	return i;
 }
 
+static void emit_local_fixups(struct node *tree, struct emitter *emit,
+		void *etarget, struct data *strbuf, struct version_info *vi,
+		struct node *node)
+{
+	struct fixup_entry *fe, *fen;
+	struct node *child;
+	int nameoff, count;
+	cell_t *buf;
+	struct data d;
+
+	if (node->emit_local_fixup_node) {
+
+		/* emit the external fixups (do not emit /) */
+		if (node != tree) {
+			emit->beginnode(etarget, NULL);
+			emit->string(etarget, node->name, 0);
+			emit->align(etarget, sizeof(cell_t));
+		}
+
+		for_each_local_fixup_entry(tree, fe) {
+			if (fe->node != node || fe->local_fixup_generated)
+				continue;
+
+			/* count the number of fixup entries */
+			count = 0;
+			for_each_local_fixup_entry(tree, fen) {
+				if (fen->prop != fe->prop)
+					continue;
+				fen->local_fixup_generated = true;
+				count++;
+			}
+
+			/* allocate buffer */
+			buf = xmalloc(count * sizeof(cell_t));
+
+			/* collect all the offsets in buffer */
+			count = 0;
+			for_each_local_fixup_entry(tree, fen) {
+				if (fen->prop != fe->prop)
+					continue;
+				fen->local_fixup_generated = true;
+				buf[count++] = cpu_to_fdt32(fen->offset);
+			}
+			d = empty_data;
+			d.len = count * sizeof(cell_t);
+			d.val = (char *)buf;
+
+			nameoff = stringtable_insert(strbuf, fe->prop->name);
+			emit->property(etarget, fe->prop->labels);
+			emit->cell(etarget, count * sizeof(cell_t));
+			emit->cell(etarget, nameoff);
+
+			if ((vi->flags & FTF_VARALIGN) &&
+					(count * sizeof(cell_t)) >= 8)
+				emit->align(etarget, 8);
+
+			emit->data(etarget, d);
+			emit->align(etarget, sizeof(cell_t));
+
+			free(buf);
+		}
+	}
+
+	for_each_child(node, child)
+		emit_local_fixups(tree, emit, etarget, strbuf, vi, child);
+
+	if (node->emit_local_fixup_node && node != tree)
+		emit->endnode(etarget, tree->labels);
+}
+
+static void emit_symbols_node(struct node *tree, struct emitter *emit,
+			      void *etarget, struct data *strbuf,
+			      struct version_info *vi)
+{
+	struct symbol *sym;
+	int nameoff, vallen;
+
+	/* do nothing if no symbols */
+	if (!tree->symbols)
+		return;
+
+	emit->beginnode(etarget, NULL);
+	emit->string(etarget, "__symbols__", 0);
+	emit->align(etarget, sizeof(cell_t));
+
+	for_each_symbol(tree, sym) {
+
+		vallen = strlen(sym->node->fullpath);
+
+		nameoff = stringtable_insert(strbuf, sym->label->label);
+
+		emit->property(etarget, NULL);
+		emit->cell(etarget, vallen + 1);
+		emit->cell(etarget, nameoff);
+
+		if ((vi->flags & FTF_VARALIGN) && vallen >= 8)
+			emit->align(etarget, 8);
+
+		emit->string(etarget, sym->node->fullpath,
+				strlen(sym->node->fullpath));
+		emit->align(etarget, sizeof(cell_t));
+	}
+
+	emit->endnode(etarget, NULL);
+}
+
+static void emit_local_fixups_node(struct node *tree, struct emitter *emit,
+				   void *etarget, struct data *strbuf,
+				   struct version_info *vi)
+{
+	struct fixup_entry *fe;
+	struct node *node;
+
+	/* do nothing if no local fixups */
+	if (!tree->local_fixups)
+		return;
+
+	/* mark all nodes that need a local fixup generated (and parents) */
+	for_each_local_fixup_entry(tree, fe) {
+		node = fe->node;
+		while (node != NULL && !node->emit_local_fixup_node) {
+			node->emit_local_fixup_node = true;
+			node = node->parent;
+		}
+	}
+
+	/* emit the local fixups node now */
+	emit->beginnode(etarget, NULL);
+	emit->string(etarget, "__local_fixups__", 0);
+	emit->align(etarget, sizeof(cell_t));
+
+	emit_local_fixups(tree, emit, etarget, strbuf, vi, tree);
+
+	emit->endnode(etarget, tree->labels);
+}
+
 static void flatten_tree(struct node *tree, struct emitter *emit,
 			 void *etarget, struct data *strbuf,
 			 struct version_info *vi)
@@ -310,6 +446,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 		flatten_tree(child, emit, etarget, strbuf, vi);
 	}
 
+	emit_symbols_node(tree, emit, etarget, strbuf, vi);
+	emit_local_fixups_node(tree, emit, etarget, strbuf, vi);
+
 	emit->endnode(etarget, tree->labels);
 }
 
-- 
1.7.12

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

Re: [PATCH v4 1/3] dtc: Symbol and local fixup generation support

[David Gibson]

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

On Fri, Feb 27, 2015 at 08:55:44PM +0200, Pantelis Antoniou wrote:
> 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. Using this information
> it is possible to implement run time dynamic tree loading.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/manual.txt |   5 ++
>  checks.c                 |  61 +++++++++++++++++++++
>  dtc.c                    |   9 ++-
>  dtc.h                    |  28 ++++++++++
>  flattree.c               | 139 +++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 240 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 398de32..4359958 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -119,6 +119,11 @@ 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.
> +
>      -S <bytes>
>  	Ensure the blob at least <bytes> long, adding additional
>  	space if needed.
> diff --git a/checks.c b/checks.c
> index e81a8c7..103ca52 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -458,6 +458,7 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>  				     struct node *node, struct property *prop)
>  {
>  	struct marker *m = prop->val.markers;
> +	struct fixup_entry *fe, **fep;
>  	struct node *refnode;
>  	cell_t phandle;
>  
> @@ -471,9 +472,28 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>  			continue;
>  		}
>  
> +		/* if it's a local reference, we need to record it */
> +		if (symbol_fixup_support) {
> +
> +			/* allocate a new local fixup entry */
> +			fe = xmalloc(sizeof(*fe));
> +
> +			fe->node = node;
> +			fe->prop = prop;
> +			fe->offset = m->offset;
> +			fe->next = NULL;

You're not initializing the local_fixup_generated field.

> +
> +			/* append it to the local fixups */
> +			fep = &dt->local_fixups;
> +			while (*fep)
> +				fep = &(*fep)->next;
> +			*fep = fe;
> +		}
> +
>  		phandle = get_node_phandle(dt, refnode);
>  		*((cell_t *)(prop->val.val + m->offset)) = cpu_to_fdt32(phandle);
>  	}
> +
>  }
>  ERROR(phandle_references, NULL, NULL, fixup_phandle_references, NULL,
>        &duplicate_node_names, &explicit_phandles);
> @@ -652,6 +672,45 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
>  }
>  TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
>  
> +static void check_auto_label_phandles(struct check *c, struct node *dt,
> +				       struct node *node)

This should be called fixup_<something>.  The naming convention is to
distinguish checks that actually check something, from fixups which
use the check infrastructure to traverse the tree and adjust things
but don't actually check anything.

> +{
> +	struct label *l;
> +	struct symbol *s, **sp;
> +	int has_label;
> +
> +	if (!symbol_fixup_support)
> +		return;
> +
> +	has_label = 0;
> +	for_each_label(node->labels, l) {
> +		has_label = 1;
> +		break;
> +	}
> +
> +	if (!has_label)
> +		return;
> +
> +	/* force allocation of a phandle for this node */
> +	(void)get_node_phandle(dt, node);
> +
> +	/* add the symbol */
> +	for_each_label(node->labels, l) {
> +
> +		s = xmalloc(sizeof(*s));
> +		s->label = l;
> +		s->node = node;
> +		s->next = NULL;
> +
> +		/* add it to the symbols list */
> +		sp = &dt->symbols;
> +		while (*sp)
> +			sp = &((*sp)->next);
> +		*sp = s;
> +	}
> +}
> +NODE_WARNING(auto_label_phandles, NULL);

This almost certainly should have some prereq checks/fixups.

>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
>  	&node_name_chars, &node_name_format, &property_name_chars,
> @@ -670,6 +729,8 @@ static struct check *check_table[] = {
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
>  
> +	&auto_label_phandles,
> +
>  	&always_fail,
>  };
>  
> diff --git a/dtc.c b/dtc.c
> index 8c4add6..91e91e7 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -29,6 +29,7 @@ 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 = 0;
>  
>  static void fill_fullpaths(struct node *tree, const char *prefix)
>  {
> @@ -51,7 +52,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:@hv";
>  static struct option const usage_long_opts[] = {
>  	{"quiet",            no_argument, NULL, 'q'},
>  	{"in-format",         a_argument, NULL, 'I'},
> @@ -69,6 +70,7 @@ 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, '@'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
>  	{NULL,               no_argument, NULL, 0x0},
> @@ -99,6 +101,7 @@ 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\tPrint this help and exit",
>  	"\n\tPrint version and exit",
>  	NULL,
> @@ -186,7 +189,9 @@ int main(int argc, char *argv[])
>  		case 'E':
>  			parse_checks_option(false, true, optarg);
>  			break;
> -
> +		case '@':
> +			symbol_fixup_support = 1;
> +			break;
>  		case 'h':
>  			usage(NULL);
>  		default:
> diff --git a/dtc.h b/dtc.h
> index 56212c8..16354fa 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -54,6 +54,7 @@ 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 */
>  
>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> @@ -132,6 +133,20 @@ struct label {
>  	struct label *next;
>  };
>  
> +struct fixup_entry {
> +	int offset;
> +	struct node *node;
> +	struct property *prop;
> +	struct fixup_entry *next;
> +	bool local_fixup_generated;
> +};
> +
> +struct symbol {
> +	struct label *label;
> +	struct node *node;
> +	struct symbol *next;
> +};
> +
>  struct property {
>  	bool deleted;
>  	char *name;
> @@ -158,6 +173,10 @@ struct node {
>  	int addr_cells, size_cells;
>  
>  	struct label *labels;
> +
> +	struct symbol *symbols;
> +	struct fixup_entry *local_fixups;

You've put these fields in the structure for every node, but you're
only using them on the root node.

> +	bool emit_local_fixup_node;
>  };
>  
>  #define for_each_label_withdel(l0, l) \
> @@ -181,6 +200,15 @@ struct node {
>  	for_each_child_withdel(n, c) \
>  		if (!(c)->deleted)
>  
> +#define for_each_fixup_entry(f, fe) \
> +	for ((fe) = (f)->entries; (fe); (fe) = (fe)->next)

This macro is never used.

> +#define for_each_symbol(n, s) \
> +	for ((s) = (n)->symbols; (s); (s) = (s)->next)
> +
> +#define for_each_local_fixup_entry(n, fe) \
> +	for ((fe) = (n)->local_fixups; (fe); (fe) = (fe)->next)
> +
>  void add_label(struct label **labels, char *label);
>  void delete_labels(struct label **labels);
>  
> diff --git a/flattree.c b/flattree.c
> index bd99fa2..3a58949 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -255,6 +255,142 @@ static int stringtable_insert(struct data *d, const char *str)
>  	return i;
>  }
>  
> +static void emit_local_fixups(struct node *tree, struct emitter *emit,
> +		void *etarget, struct data *strbuf, struct version_info *vi,
> +		struct node *node)

Constructing these nodes during the emit code in flattree.c makes life
unnecessarily difficult, and means they'll be missing from -O dts
output.  Instead you should construct the fixup nodes in the live
tree, then emit them in the usual way.

> +{
> +	struct fixup_entry *fe, *fen;
> +	struct node *child;
> +	int nameoff, count;
> +	cell_t *buf;
> +	struct data d;
> +
> +	if (node->emit_local_fixup_node) {
> +
> +		/* emit the external fixups (do not emit /) */
> +		if (node != tree) {
> +			emit->beginnode(etarget, NULL);
> +			emit->string(etarget, node->name, 0);
> +			emit->align(etarget, sizeof(cell_t));
> +		}
> +
> +		for_each_local_fixup_entry(tree, fe) {
> +			if (fe->node != node || fe->local_fixup_generated)
> +				continue;
> +
> +			/* count the number of fixup entries */
> +			count = 0;
> +			for_each_local_fixup_entry(tree, fen) {
> +				if (fen->prop != fe->prop)
> +					continue;
> +				fen->local_fixup_generated = true;
> +				count++;
> +			}
> +
> +			/* allocate buffer */
> +			buf = xmalloc(count * sizeof(cell_t));
> +
> +			/* collect all the offsets in buffer */
> +			count = 0;
> +			for_each_local_fixup_entry(tree, fen) {
> +				if (fen->prop != fe->prop)
> +					continue;
> +				fen->local_fixup_generated = true;
> +				buf[count++] = cpu_to_fdt32(fen->offset);
> +			}
> +			d = empty_data;
> +			d.len = count * sizeof(cell_t);
> +			d.val = (char *)buf;
> +
> +			nameoff = stringtable_insert(strbuf, fe->prop->name);
> +			emit->property(etarget, fe->prop->labels);
> +			emit->cell(etarget, count * sizeof(cell_t));
> +			emit->cell(etarget, nameoff);
> +
> +			if ((vi->flags & FTF_VARALIGN) &&
> +					(count * sizeof(cell_t)) >= 8)
> +				emit->align(etarget, 8);
> +
> +			emit->data(etarget, d);
> +			emit->align(etarget, sizeof(cell_t));
> +
> +			free(buf);
> +		}
> +	}
> +
> +	for_each_child(node, child)
> +		emit_local_fixups(tree, emit, etarget, strbuf, vi, child);
> +
> +	if (node->emit_local_fixup_node && node != tree)
> +		emit->endnode(etarget, tree->labels);
> +}
> +
> +static void emit_symbols_node(struct node *tree, struct emitter *emit,
> +			      void *etarget, struct data *strbuf,
> +			      struct version_info *vi)
> +{
> +	struct symbol *sym;
> +	int nameoff, vallen;
> +
> +	/* do nothing if no symbols */
> +	if (!tree->symbols)
> +		return;
> +
> +	emit->beginnode(etarget, NULL);
> +	emit->string(etarget, "__symbols__", 0);
> +	emit->align(etarget, sizeof(cell_t));
> +
> +	for_each_symbol(tree, sym) {
> +
> +		vallen = strlen(sym->node->fullpath);
> +
> +		nameoff = stringtable_insert(strbuf, sym->label->label);
> +
> +		emit->property(etarget, NULL);
> +		emit->cell(etarget, vallen + 1);
> +		emit->cell(etarget, nameoff);
> +
> +		if ((vi->flags & FTF_VARALIGN) && vallen >= 8)
> +			emit->align(etarget, 8);
> +
> +		emit->string(etarget, sym->node->fullpath,
> +				strlen(sym->node->fullpath));
> +		emit->align(etarget, sizeof(cell_t));
> +	}
> +
> +	emit->endnode(etarget, NULL);
> +}
> +
> +static void emit_local_fixups_node(struct node *tree, struct emitter *emit,
> +				   void *etarget, struct data *strbuf,
> +				   struct version_info *vi)
> +{
> +	struct fixup_entry *fe;
> +	struct node *node;
> +
> +	/* do nothing if no local fixups */
> +	if (!tree->local_fixups)
> +		return;
> +
> +	/* mark all nodes that need a local fixup generated (and parents) */
> +	for_each_local_fixup_entry(tree, fe) {
> +		node = fe->node;
> +		while (node != NULL && !node->emit_local_fixup_node) {
> +			node->emit_local_fixup_node = true;
> +			node = node->parent;
> +		}
> +	}
> +
> +	/* emit the local fixups node now */
> +	emit->beginnode(etarget, NULL);
> +	emit->string(etarget, "__local_fixups__", 0);
> +	emit->align(etarget, sizeof(cell_t));
> +
> +	emit_local_fixups(tree, emit, etarget, strbuf, vi, tree);
> +
> +	emit->endnode(etarget, tree->labels);
> +}
> +
>  static void flatten_tree(struct node *tree, struct emitter *emit,
>  			 void *etarget, struct data *strbuf,
>  			 struct version_info *vi)
> @@ -310,6 +446,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
>  		flatten_tree(child, emit, etarget, strbuf, vi);
>  	}
>  
> +	emit_symbols_node(tree, emit, etarget, strbuf, vi);
> +	emit_local_fixups_node(tree, emit, etarget, strbuf, vi);
> +
>  	emit->endnode(etarget, tree->labels);
>  }
>  

-- 
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 --]

[PATCH v4 2/3] dtc: Plugin (object) device tree support.

[Pantelis Antoniou]

Enables the generation of a __fixups__ node for trees compiled
using the -@ option that are using the /plugin/ tag.

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.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/manual.txt |  5 ++++
 checks.c                 | 45 ++++++++++++++++++++++++++++++++--
 dtc-lexer.l              |  5 ++++
 dtc-parser.y             | 22 ++++++++++++++---
 dtc.h                    | 12 +++++++++
 flattree.c               | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 147 insertions(+), 5 deletions(-)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 4359958..18d1333 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -124,6 +124,9 @@ Options:
 	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.
+
     -S <bytes>
 	Ensure the blob at least <bytes> long, adding additional
 	space if needed.
@@ -158,6 +161,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 103ca52..4be5233 100644
--- a/checks.c
+++ b/checks.c
@@ -458,6 +458,7 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
 				     struct node *node, struct property *prop)
 {
 	struct marker *m = prop->val.markers;
+	struct fixup *f, **fp;
 	struct fixup_entry *fe, **fep;
 	struct node *refnode;
 	cell_t phandle;
@@ -467,8 +468,48 @@ 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 (!dt->is_plugin) {
+				FAIL(c, "Reference to non-existent node or label \"%s\"\n",
+					m->ref);
+				continue;
+			}
+
+			/* allocate fixup entry */
+			fe = xmalloc(sizeof(*fe));
+
+			fe->node = node;
+			fe->prop = prop;
+			fe->offset = m->offset;
+			fe->next = NULL;
+
+			/* search for an already existing fixup */
+			for_each_fixup(dt, f)
+				if (strcmp(f->ref, m->ref) == 0)
+					break;
+
+			/* no fixup found, add new */
+			if (f == NULL) {
+				f = xmalloc(sizeof(*f));
+				f->ref = m->ref;
+				f->entries = NULL;
+				f->next = NULL;
+
+				/* add it to the tree */
+				fp = &dt->fixups;
+				while (*fp)
+					fp = &(*fp)->next;
+				*fp = f;
+			}
+
+			/* and now append fixup entry */
+			fep = &f->entries;
+			while (*fep)
+				fep = &(*fep)->next;
+			*fep = fe;
+
+			/* mark the entry as unresolved */
+			*((cell_t *)(prop->val.val + m->offset)) =
+				cpu_to_fdt32(0xdeadbeef);
 			continue;
 		}
 
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..d23927d 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,22 @@ extern bool treesource_error;
 %%
 
 sourcefile:
-	  DT_V1 ';' memreserves devicetree
+	  DT_V1 ';' plugindecl memreserves devicetree
 		{
-			the_boot_info = build_boot_info($3, $4,
-							guess_boot_cpuid($4));
+			$5->is_plugin = $3;
+			the_boot_info = build_boot_info($4, $5,
+							guess_boot_cpuid($5));
+		}
+	;
+
+plugindecl:
+	/* empty */
+		{
+			$$ = false;
+		}
+	| DT_PLUGIN ';'
+		{
+			$$ = true;
 		}
 	;
 
diff --git a/dtc.h b/dtc.h
index 16354fa..f163b22 100644
--- a/dtc.h
+++ b/dtc.h
@@ -141,6 +141,12 @@ struct fixup_entry {
 	bool local_fixup_generated;
 };
 
+struct fixup {
+	char *ref;
+	struct fixup_entry *entries;
+	struct fixup *next;
+};
+
 struct symbol {
 	struct label *label;
 	struct node *node;
@@ -177,6 +183,9 @@ struct node {
 	struct symbol *symbols;
 	struct fixup_entry *local_fixups;
 	bool emit_local_fixup_node;
+
+	bool is_plugin;
+	struct fixup *fixups;
 };
 
 #define for_each_label_withdel(l0, l) \
@@ -200,6 +209,9 @@ struct node {
 	for_each_child_withdel(n, c) \
 		if (!(c)->deleted)
 
+#define for_each_fixup(n, f) \
+	for ((f) = (n)->fixups; (f); (f) = (f)->next)
+
 #define for_each_fixup_entry(f, fe) \
 	for ((fe) = (f)->entries; (fe); (fe) = (fe)->next)
 
diff --git a/flattree.c b/flattree.c
index 3a58949..2385137 100644
--- a/flattree.c
+++ b/flattree.c
@@ -391,6 +391,68 @@ static void emit_local_fixups_node(struct node *tree, struct emitter *emit,
 	emit->endnode(etarget, tree->labels);
 }
 
+static void emit_fixups_node(struct node *tree, struct emitter *emit,
+			     void *etarget, struct data *strbuf,
+			     struct version_info *vi)
+{
+	struct fixup *f;
+	struct fixup_entry *fe;
+	char *name, *s;
+	const char *fullpath;
+	int namesz, nameoff, vallen;
+
+	/* do nothing if no fixups */
+	if (!tree->fixups)
+		return;
+
+	/* emit the external fixups */
+	emit->beginnode(etarget, NULL);
+	emit->string(etarget, "__fixups__", 0);
+	emit->align(etarget, sizeof(cell_t));
+
+	for_each_fixup(tree, f) {
+
+		namesz = 0;
+		for_each_fixup_entry(f, fe) {
+			fullpath = fe->node->fullpath;
+			if (fullpath[0] == '\0')
+				fullpath = "/";
+			namesz += strlen(fullpath) + 1;
+			namesz += strlen(fe->prop->name) + 1;
+			namesz += 32;	/* space for :<number> + '\0' */
+		}
+
+		name = xmalloc(namesz);
+
+		s = name;
+		for_each_fixup_entry(f, fe) {
+			fullpath = fe->node->fullpath;
+			if (fullpath[0] == '\0')
+				fullpath = "/";
+			snprintf(s, name + namesz - s, "%s:%s:%d", fullpath,
+					fe->prop->name, fe->offset);
+			s += strlen(s) + 1;
+		}
+
+		nameoff = stringtable_insert(strbuf, f->ref);
+		vallen = s - name - 1;
+
+		emit->property(etarget, NULL);
+		emit->cell(etarget, vallen + 1);
+		emit->cell(etarget, nameoff);
+
+		if ((vi->flags & FTF_VARALIGN) && vallen >= 8)
+			emit->align(etarget, 8);
+
+		emit->string(etarget, name, vallen);
+		emit->align(etarget, sizeof(cell_t));
+
+		free(name);
+	}
+
+	emit->endnode(etarget, tree->labels);
+}
+
 static void flatten_tree(struct node *tree, struct emitter *emit,
 			 void *etarget, struct data *strbuf,
 			 struct version_info *vi)
@@ -448,6 +510,7 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 
 	emit_symbols_node(tree, emit, etarget, strbuf, vi);
 	emit_local_fixups_node(tree, emit, etarget, strbuf, vi);
+	emit_fixups_node(tree, emit, etarget, strbuf, vi);
 
 	emit->endnode(etarget, tree->labels);
 }
-- 
1.7.12

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

Re: [PATCH v4 2/3] dtc: Plugin (object) device tree support.

[David Gibson]

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

On Fri, Feb 27, 2015 at 08:55:45PM +0200, Pantelis Antoniou wrote:
> Enables the generation of a __fixups__ node for trees compiled
> using the -@ option that are using the /plugin/ tag.
> 
> 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.

It seems to me you've really missed an opportunity in designing the
plugin syntax.  You have dtc generating the fixups nodes, but you
still have to manually lay out your fragments in the right format, and 
manually construct the target properties.  Instead you could re-use
the existing dts overlay syntax.  For a plugin tree, you wouldn't need
the base tree piece. So, allow something like:

/dts-v1/ /plugin/;

&res {
	baz_res: baz_res { ... };
};

&ocp {
	baz { ... };
};

> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/manual.txt |  5 ++++
>  checks.c                 | 45 ++++++++++++++++++++++++++++++++--
>  dtc-lexer.l              |  5 ++++
>  dtc-parser.y             | 22 ++++++++++++++---
>  dtc.h                    | 12 +++++++++
>  flattree.c               | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 147 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 4359958..18d1333 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -124,6 +124,9 @@ Options:
>  	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.
> +
>      -S <bytes>
>  	Ensure the blob at least <bytes> long, adding additional
>  	space if needed.
> @@ -158,6 +161,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 103ca52..4be5233 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -458,6 +458,7 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>  				     struct node *node, struct property *prop)
>  {
>  	struct marker *m = prop->val.markers;
> +	struct fixup *f, **fp;
>  	struct fixup_entry *fe, **fep;
>  	struct node *refnode;
>  	cell_t phandle;
> @@ -467,8 +468,48 @@ 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 (!dt->is_plugin) {
> +				FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> +					m->ref);
> +				continue;
> +			}
> +
> +			/* allocate fixup entry */
> +			fe = xmalloc(sizeof(*fe));
> +
> +			fe->node = node;
> +			fe->prop = prop;
> +			fe->offset = m->offset;
> +			fe->next = NULL;
> +
> +			/* search for an already existing fixup */
> +			for_each_fixup(dt, f)
> +				if (strcmp(f->ref, m->ref) == 0)
> +					break;
> +
> +			/* no fixup found, add new */
> +			if (f == NULL) {
> +				f = xmalloc(sizeof(*f));
> +				f->ref = m->ref;
> +				f->entries = NULL;
> +				f->next = NULL;
> +
> +				/* add it to the tree */
> +				fp = &dt->fixups;
> +				while (*fp)
> +					fp = &(*fp)->next;
> +				*fp = f;
> +			}
> +
> +			/* and now append fixup entry */
> +			fep = &f->entries;
> +			while (*fep)
> +				fep = &(*fep)->next;
> +			*fep = fe;
> +
> +			/* mark the entry as unresolved */
> +			*((cell_t *)(prop->val.val + m->offset)) =
> +				cpu_to_fdt32(0xdeadbeef);
>  			continue;
>  		}
>  
> 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;

The /plugin/ tag affects how the whole tree is interpreted.  I'd
therefore recommend folding it into the version declaration.  So,
something like

/dts-v1-plugin/;

or

/dts-v1/ /plugin/;

That ensures that this globally relevant tag appears right at the top.

> +		}
> +
>  <*>"/memreserve/"	{
>  			DPRINT("Keyword: /memreserve/\n");
>  			BEGIN_DEFAULT();
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 5a897e3..d23927d 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,22 @@ extern bool treesource_error;
>  %%
>  
>  sourcefile:
> -	  DT_V1 ';' memreserves devicetree
> +	  DT_V1 ';' plugindecl memreserves devicetree
>  		{
> -			the_boot_info = build_boot_info($3, $4,
> -							guess_boot_cpuid($4));
> +			$5->is_plugin = $3;
> +			the_boot_info = build_boot_info($4, $5,
> +							guess_boot_cpuid($5));
> +		}
> +	;
> +
> +plugindecl:
> +	/* empty */
> +		{
> +			$$ = false;
> +		}
> +	| DT_PLUGIN ';'
> +		{
> +			$$ = true;
>  		}
>  	;
>  
> diff --git a/dtc.h b/dtc.h
> index 16354fa..f163b22 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -141,6 +141,12 @@ struct fixup_entry {
>  	bool local_fixup_generated;
>  };
>  
> +struct fixup {
> +	char *ref;
> +	struct fixup_entry *entries;
> +	struct fixup *next;
> +};
> +
>  struct symbol {
>  	struct label *label;
>  	struct node *node;
> @@ -177,6 +183,9 @@ struct node {
>  	struct symbol *symbols;
>  	struct fixup_entry *local_fixups;
>  	bool emit_local_fixup_node;
> +
> +	bool is_plugin;
> +	struct fixup *fixups;

Again, you've put these fields in every node, but they're only used in
the root node.

>  };
>  
>  #define for_each_label_withdel(l0, l) \
> @@ -200,6 +209,9 @@ struct node {
>  	for_each_child_withdel(n, c) \
>  		if (!(c)->deleted)
>  
> +#define for_each_fixup(n, f) \
> +	for ((f) = (n)->fixups; (f); (f) = (f)->next)
> +
>  #define for_each_fixup_entry(f, fe) \
>  	for ((fe) = (f)->entries; (fe); (fe) = (fe)->next)
>  
> diff --git a/flattree.c b/flattree.c
> index 3a58949..2385137 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -391,6 +391,68 @@ static void emit_local_fixups_node(struct node *tree, struct emitter *emit,
>  	emit->endnode(etarget, tree->labels);
>  }
>  
> +static void emit_fixups_node(struct node *tree, struct emitter *emit,
> +			     void *etarget, struct data *strbuf,
> +			     struct version_info *vi)

And again, constructing the node in the flattree code is not the best idea.

> +{
> +	struct fixup *f;
> +	struct fixup_entry *fe;
> +	char *name, *s;
> +	const char *fullpath;
> +	int namesz, nameoff, vallen;
> +
> +	/* do nothing if no fixups */
> +	if (!tree->fixups)
> +		return;
> +
> +	/* emit the external fixups */
> +	emit->beginnode(etarget, NULL);
> +	emit->string(etarget, "__fixups__", 0);
> +	emit->align(etarget, sizeof(cell_t));
> +
> +	for_each_fixup(tree, f) {
> +
> +		namesz = 0;
> +		for_each_fixup_entry(f, fe) {
> +			fullpath = fe->node->fullpath;
> +			if (fullpath[0] == '\0')
> +				fullpath = "/";
> +			namesz += strlen(fullpath) + 1;
> +			namesz += strlen(fe->prop->name) + 1;
> +			namesz += 32;	/* space for :<number> + '\0' */
> +		}
> +
> +		name = xmalloc(namesz);
> +
> +		s = name;
> +		for_each_fixup_entry(f, fe) {
> +			fullpath = fe->node->fullpath;
> +			if (fullpath[0] == '\0')
> +				fullpath = "/";
> +			snprintf(s, name + namesz - s, "%s:%s:%d", fullpath,
> +					fe->prop->name, fe->offset);
> +			s += strlen(s) + 1;
> +		}
> +
> +		nameoff = stringtable_insert(strbuf, f->ref);
> +		vallen = s - name - 1;
> +
> +		emit->property(etarget, NULL);
> +		emit->cell(etarget, vallen + 1);
> +		emit->cell(etarget, nameoff);
> +
> +		if ((vi->flags & FTF_VARALIGN) && vallen >= 8)
> +			emit->align(etarget, 8);
> +
> +		emit->string(etarget, name, vallen);
> +		emit->align(etarget, sizeof(cell_t));
> +
> +		free(name);
> +	}
> +
> +	emit->endnode(etarget, tree->labels);
> +}
> +
>  static void flatten_tree(struct node *tree, struct emitter *emit,
>  			 void *etarget, struct data *strbuf,
>  			 struct version_info *vi)
> @@ -448,6 +510,7 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
>  
>  	emit_symbols_node(tree, emit, etarget, strbuf, vi);
>  	emit_local_fixups_node(tree, emit, etarget, strbuf, vi);
> +	emit_fixups_node(tree, emit, etarget, strbuf, vi);
>  
>  	emit->endnode(etarget, tree->labels);
>  }

-- 
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 v4 2/3] dtc: Plugin (object) device tree support.

[Jan Lübbe]

On Do, 2015-03-05 at 10:34 +1100, David Gibson wrote:
> On Fri, Feb 27, 2015 at 08:55:45PM +0200, Pantelis Antoniou wrote:
> > Enables the generation of a __fixups__ node for trees compiled
> > using the -@ option that are using the /plugin/ tag.
> > 
> > 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.
> 
> It seems to me you've really missed an opportunity in designing the
> plugin syntax.  You have dtc generating the fixups nodes, but you
> still have to manually lay out your fragments in the right format,
> and 
> manually construct the target properties.  Instead you could re-use
> the existing dts overlay syntax.  For a plugin tree, you wouldn't need
> the base tree piece. So, allow something like:
> 
> /dts-v1/ /plugin/;
> 
> &res {
>         baz_res: baz_res { ... };
> };
> 
> &ocp {
>         baz { ... };
> };

During our initial experiments with overlays in Barebox, Sascha Hauer
did a simple patch to DTC to implement this. I've recently rebased it on
Pantelis' series. It makes writing overlays much more natural:

From e7082dd7160082c64d242b4ed2ebb9eb0b8aad42 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Date: Thu, 8 Aug 2013 23:08:31 +0200
Subject: [PATCH] dtc: convert unresolved labels into overlay nodes

Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 dtc-parser.y | 10 +++++++---
 dtc.h        |  3 ++-
 livetree.c   | 23 +++++++++++++++++++++++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/dtc-parser.y b/dtc-parser.y
index d23927d99215..f431bdd6e57c 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -172,10 +172,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 ';'
diff --git a/dtc.h b/dtc.h
index f163b22b14b8..fc7330e9013e 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>
@@ -234,6 +234,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);
diff --git a/livetree.c b/livetree.c
index e229b84432f9..ed16b8c216f4 100644
--- a/livetree.c
+++ b/livetree.c
@@ -216,6 +216,29 @@ 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)
+{
+	struct node *ovl = xmalloc(sizeof(*ovl));
+	struct property *p;
+	struct data d = empty_data;
+	char *name;
+
+	memset(ovl, 0, sizeof(*ovl));
+
+	d = data_add_marker(d, REF_PHANDLE, ref);
+	d = data_append_integer(d, 0xdeadbeef, 32);
+
+	p = build_property("target", d);
+	add_property(ovl, p);
+
+	asprintf(&name, "fragment@%s", ref);
+	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);
-- 
2.1.4



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH v4 2/3] dtc: Plugin (object) device tree support.

[David Gibson]

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

On Fri, Mar 06, 2015 at 06:55:14PM +0100, Jan Lübbe wrote:
> On Do, 2015-03-05 at 10:34 +1100, David Gibson wrote:
> > On Fri, Feb 27, 2015 at 08:55:45PM +0200, Pantelis Antoniou wrote:
> > > Enables the generation of a __fixups__ node for trees compiled
> > > using the -@ option that are using the /plugin/ tag.
> > > 
> > > 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.
> > 
> > It seems to me you've really missed an opportunity in designing the
> > plugin syntax.  You have dtc generating the fixups nodes, but you
> > still have to manually lay out your fragments in the right format,
> > and 
> > manually construct the target properties.  Instead you could re-use
> > the existing dts overlay syntax.  For a plugin tree, you wouldn't need
> > the base tree piece. So, allow something like:
> > 
> > /dts-v1/ /plugin/;
> > 
> > &res {
> >         baz_res: baz_res { ... };
> > };
> > 
> > &ocp {
> >         baz { ... };
> > };
> 
> During our initial experiments with overlays in Barebox, Sascha Hauer
> did a simple patch to DTC to implement this. I've recently rebased it on
> Pantelis' series. It makes writing overlays much more natural:

Ok, well I'd like to see this merged with the basic patch series

> >From e7082dd7160082c64d242b4ed2ebb9eb0b8aad42 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Date: Thu, 8 Aug 2013 23:08:31 +0200
> Subject: [PATCH] dtc: convert unresolved labels into overlay nodes
> 
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  dtc-parser.y | 10 +++++++---
>  dtc.h        |  3 ++-
>  livetree.c   | 23 +++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index d23927d99215..f431bdd6e57c 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -172,10 +172,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 ';'
> diff --git a/dtc.h b/dtc.h
> index f163b22b14b8..fc7330e9013e 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>
> @@ -234,6 +234,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);
> diff --git a/livetree.c b/livetree.c
> index e229b84432f9..ed16b8c216f4 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -216,6 +216,29 @@ 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)
> +{
> +	struct node *ovl = xmalloc(sizeof(*ovl));
> +	struct property *p;
> +	struct data d = empty_data;
> +	char *name;
> +
> +	memset(ovl, 0, sizeof(*ovl));
> +
> +	d = data_add_marker(d, REF_PHANDLE, ref);
> +	d = data_append_integer(d, 0xdeadbeef, 32);
> +
> +	p = build_property("target", d);
> +	add_property(ovl, p);
> +
> +	asprintf(&name, "fragment@%s", ref);
> +	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);

-- 
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 --]

[PATCH v4 3/3] 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 | 301 +++++++++++++++++++++++++++++++++++
 1 file changed, 301 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..b5ce9b4
--- /dev/null
+++ b/Documentation/dt-object-internal.txt
@@ -0,0 +1,301 @@
+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:
+
+/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.
+
+/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. Since phandles are allocated starting at 1
+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.
-- 
1.7.12

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

Re: [PATCH v4 3/3] dtc: Document the dynamic plugin internals

[Jan Lübbe]

Hi Pantelis,

On Fr, 2015-02-27 at 20:55 +0200, Pantelis Antoniou wrote:
> +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:
> +
> +/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.

The kernel overlay code also supports using 'target-path="/path";'
instead of 'target=<phandle>;' to work with base DTBs which have no
__symbols__ node. Shouldn't this be mentioned here as well?

Thanks,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH v4 3/3] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Hi Jan,

> On Mar 3, 2015, at 15:42 , Jan Lübbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> 
> Hi Pantelis,
> 
> On Fr, 2015-02-27 at 20:55 +0200, Pantelis Antoniou wrote:
>> +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:
>> +
>> +/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.
> 
> The kernel overlay code also supports using 'target-path="/path";'
> instead of 'target=<phandle>;' to work with base DTBs which have no
> __symbols__ node. Shouldn't this be mentioned here as well?
> 

The kernel documentation entries contain all relevant information. This is just the DTC
support patch, and as such they only need a rough outline of what the actual in-kernel
resolver and overlay handler code does.

tl;dr DTC has no notion of either target or target-path properties.

> Thanks,
> Jan
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Regards

— Pantelis

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

Re: [PATCH v4 3/3] dtc: Document the dynamic plugin internals

[David Gibson]

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

On Fri, Feb 27, 2015 at 08:55:46PM +0200, 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 | 301 +++++++++++++++++++++++++++++++++++
>  1 file changed, 301 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..b5ce9b4
> --- /dev/null
> +++ b/Documentation/dt-object-internal.txt
> @@ -0,0 +1,301 @@
> +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 really don't understand this requirement.  I mean, obviously it
makes sense for the "base" dt, since that can be used as-is, ignoring
the overlay stuff.  But the overlay fragments themselves are useless
if not properly processed, in which case what's the point of
maintaining the same dtb format.

It really seems like you're trying to force out-of-band information
into band.  The dt is flexible enough that you *can* do that, but that
doesn't mean it's a good idea.


> +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";


So, the __symbols__ node looks an awful lot like the OF-standard
aliases node.  Is there any reason you can't use /aliases for this
instead?  (Automatically generating aliases for nodes with a label is
a dtc feature I've had vaguely in mind since forever).


> +	};
> +};
> +
> +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:
> +
> +/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>;

The target property is specific to the overlay format.  So specifying
it as a nonsense phandle, which must then be fixed up is silly.  It
should be encoded directly as a symbolic name.


> +		__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',

0xdeadbeef is not an illegal phandle value.  Use either 0 or -1 if you
need an illegal phandle value.

> 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>";

Encoding multiple things into parsed text strings, and encoding
numbers into text really doesn't sit well with normal conventions for
encoding dt properties.  You can mix \0 terminated strings and binary
encoded numbers into a single property if necessary, or use multiple
properties.

Worse, this encoding doesn't allow multiple overlay properties to be
fixed up to point to the same label.


> +<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.
> +
> +/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. Since phandles are allocated
> starting at 1

That's not guaranteed, it's just what the current dtc implementation does.

> +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.

The __local_fixups__ has a totally different format from __fixups__
for doing a very similar operation.  You should be able to unify the
encoding for local and non-local fixups.

-- 
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 v4 3/3] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Hi David,

> On Mar 4, 2015, at 13:29 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Fri, Feb 27, 2015 at 08:55:46PM +0200, 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 | 301 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 301 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..b5ce9b4
>> --- /dev/null
>> +++ b/Documentation/dt-object-internal.txt
>> @@ -0,0 +1,301 @@
>> +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 really don't understand this requirement.  I mean, obviously it
> makes sense for the "base" dt, since that can be used as-is, ignoring
> the overlay stuff.  But the overlay fragments themselves are useless
> if not properly processed, in which case what's the point of
> maintaining the same dtb format.
> 
> It really seems like you're trying to force out-of-band information
> into band.  The dt is flexible enough that you *can* do that, but that
> doesn't mean it's a good idea.
> 

It means that every piece of software that understands DT can be used as is
without any changes to the format.

And IMHO all information that is related to DT should be expressed using DT only
constructs and not out of band data like memory ranges.

The existence of out-of-band data is owed to the easier processing of them by
non-DT aware bootloaders, but that that’s anymore the case.

> 
>> +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";
> 
> 
> So, the __symbols__ node looks an awful lot like the OF-standard
> aliases node.  Is there any reason you can't use /aliases for this
> instead?  (Automatically generating aliases for nodes with a label is
> a dtc feature I've had vaguely in mind since forever).
> 

The point of the aliases node is to add explicitly defined shortcut 
for locations in the tree, when the symbols node records all the label
instances. If we were to go with the implicit aliases node fill up,
should the existing aliases node definition by the user continue? Cause that
might cause conflicts.

> 
>> +	};
>> +};
>> +
>> +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:
>> +
>> +/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>;
> 
> The target property is specific to the overlay format.  So specifying
> it as a nonsense phandle, which must then be fixed up is silly.  It
> should be encoded directly as a symbolic name.
> 

There is nothing specific to the target property for the overlay format per-se.
The 0xdeadbeef value is used as an easily visually identified value that will
be fixed up by resolution. 

> 
>> +		__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',
> 
> 0xdeadbeef is not an illegal phandle value.  Use either 0 or -1 if you
> need an illegal phandle value.

0xdeadbeef is just used to be easily identified. 0 or -1 can easily be
substituted.
 
> 
>> 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>";
> 
> Encoding multiple things into parsed text strings, and encoding
> numbers into text really doesn't sit well with normal conventions for
> encoding dt properties.  You can mix \0 terminated strings and binary
> encoded numbers into a single property if necessary, or use multiple
> properties.
> 
> Worse, this encoding doesn't allow multiple overlay properties to be
> fixed up to point to the same label.
> 

My mistake, the documentation entry is not exactly accurate, there are
multiple entries by label encoded as multiple zero encoded strings.

So that’s

<label> = "<local-full-path>:<property-name>:<offset>”
	[, "<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.
>> +
>> +/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. Since phandles are allocated
>> starting at 1
> 
> That's not guaranteed, it's just what the current dtc implementation does.
> 

OK, I will remove the reference to the ‘1’ number. It’s only there to illustrate
the theory of operation.

>> +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.
> 
> The __local_fixups__ has a totally different format from __fixups__
> for doing a very similar operation.  You should be able to unify the
> encoding for local and non-local fixups.
> 

The __fixups__ and __local_fixups__ are used for completely different operations.

__local_fixups__ contain pointers in the local tree where phandle references are
been taken, while __fixups__ contains pointers to the local tree where phandle
resolution takes place.

So no, they perform completely different operations.

> -- 
> 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

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

Re: [PATCH v4 3/3] dtc: Document the dynamic plugin internals

[David Gibson]

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

On Wed, Mar 04, 2015 at 03:16:32PM +0200, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Mar 4, 2015, at 13:29 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Fri, Feb 27, 2015 at 08:55:46PM +0200, 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 | 301 +++++++++++++++++++++++++++++++++++
> >> 1 file changed, 301 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..b5ce9b4
> >> --- /dev/null
> >> +++ b/Documentation/dt-object-internal.txt
> >> @@ -0,0 +1,301 @@
> >> +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 really don't understand this requirement.  I mean, obviously it
> > makes sense for the "base" dt, since that can be used as-is, ignoring
> > the overlay stuff.  But the overlay fragments themselves are useless
> > if not properly processed, in which case what's the point of
> > maintaining the same dtb format.
> > 
> > It really seems like you're trying to force out-of-band information
> > into band.  The dt is flexible enough that you *can* do that, but that
> > doesn't mean it's a good idea.
> > 
> 
> It means that every piece of software that understands DT can be used as is
> without any changes to the format.

No, only those pieces of software that process the dtb format, but
don't actually do anything with the tree content continue to work.
Software which wants to actually make sense of the tree will *think*
they understand the format, but be horribly mistaken.

So, apart from dtc itself, what software in the first category did
you have in mind.

> And IMHO all information that is related to DT should be expressed using DT only
> constructs and not out of band data like memory ranges.

You're going to have to back up your HO with better reasons that
you've given so far..

> The existence of out-of-band data is owed to the easier processing of them by
> non-DT aware bootloaders, but that that’s anymore the case.
> 
> > 
> >> +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";
> > 
> > 
> > So, the __symbols__ node looks an awful lot like the OF-standard
> > aliases node.  Is there any reason you can't use /aliases for this
> > instead?  (Automatically generating aliases for nodes with a label is
> > a dtc feature I've had vaguely in mind since forever).
> > 
> 
> The point of the aliases node is to add explicitly defined shortcut 
> for locations in the tree, when the symbols node records all the label
> instances. If we were to go with the implicit aliases node fill up,
> should the existing aliases node definition by the user continue? Cause that
> might cause conflicts.

Automatically putting every node label into aliases might cause
problems, yes.  But it's also not clear that making every label
available for use by overlays is a good idea.  What I had in mind was
some way of tagging labels to say that they're exported, meaning they
go into aliases.

> 
> > 
> >> +	};
> >> +};
> >> +
> >> +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:
> >> +
> >> +/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>;
> > 
> > The target property is specific to the overlay format.  So specifying
> > it as a nonsense phandle, which must then be fixed up is silly.  It
> > should be encoded directly as a symbolic name.
> 
> There is nothing specific to the target property for the overlay format per-se.
> The 0xdeadbeef value is used as an easily visually identified value that will
> be fixed up by resolution.

What I mean is that there isn't a previous standard defining the
format of the target property, which means you can format it however
you like.  In which case making it a phandle which will always needing
fixing up is stupid.  Make it be a symbolic name which you can resolve
directly.

> 
> > 
> >> +		__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',
> > 
> > 0xdeadbeef is not an illegal phandle value.  Use either 0 or -1 if you
> > need an illegal phandle value.
> 
> 0xdeadbeef is just used to be easily identified. 0 or -1 can easily be
> substituted.
>  
> > 
> >> 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>";
> > 
> > Encoding multiple things into parsed text strings, and encoding
> > numbers into text really doesn't sit well with normal conventions for
> > encoding dt properties.  You can mix \0 terminated strings and binary
> > encoded numbers into a single property if necessary, or use multiple
> > properties.
> > 
> > Worse, this encoding doesn't allow multiple overlay properties to be
> > fixed up to point to the same label.
> > 
> 
> My mistake, the documentation entry is not exactly accurate, there are
> multiple entries by label encoded as multiple zero encoded strings.
> 
> So that’s
> 
> <label> = "<local-full-path>:<property-name>:<offset>”
> 	[, "<local-full-path>:<property-name>:<offset>” …] ;

Ok.  The format still doesn't sit well with DT conventions.

> > 
> >> +<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.
> >> +
> >> +/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. Since phandles are allocated
> >> starting at 1
> > 
> > That's not guaranteed, it's just what the current dtc implementation does.
> > 
> 
> OK, I will remove the reference to the ‘1’ number. It’s only there to illustrate
> the theory of operation.
> 
> >> +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.
> > 
> > The __local_fixups__ has a totally different format from __fixups__
> > for doing a very similar operation.  You should be able to unify the
> > encoding for local and non-local fixups.
> > 
> 
> The __fixups__ and __local_fixups__ are used for completely different operations.
> 
> __local_fixups__ contain pointers in the local tree where phandle references are
> been taken, while __fixups__ contains pointers to the local tree where phandle
> resolution takes place.

> So no, they perform completely different operations.

I'm not sure quite what distinction you're making between "references
are been taken" and "phandle resolution".  But as far as I can tell
both local and non-local fixups involve taking a phandle value in the
local fragment and correcting it in some way, which sound like pretty
similar operations to me.

But even to the extent that they're different, your own description
above says both involve a pointer to the local tree.  But you've
encoded that pointer to the local tree in a completely different way
in the two cases, which is idiotic.

-- 
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 v4 0/3] dtc: Dynamic DT support

[David Gibson]

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

On Fri, Feb 27, 2015 at 08:55:43PM +0200, Pantelis Antoniou wrote:
> Enable the generation of symbol & fixup information for
> use with dynamic DT loading.

This still needs a lot of work before it's close to ready.  I'm mildly
dubious about the basic premise, but more importantly there are
serious flaws in the current implementation.  More details coming once
I have a chance.

-- 
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 --]