Re: [PATCH v10 3/4] dtc: Plugin and fixup support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi David,

> On Nov 30, 2016, at 03:50 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Tue, Nov 29, 2016 at 01:09:11PM +0200, Pantelis Antoniou wrote:
>> Hi David,
>> 
>>> On Nov 29, 2016, at 04:10 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>> 
>>> On Mon, Nov 28, 2016 at 02:10:35PM +0200, Pantelis Antoniou wrote:
>>>> 
>>>>> On Nov 28, 2016, at 06:12 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>> 
>>>>> On Fri, Nov 25, 2016 at 02:32:10PM +0200, Pantelis Antoniou wrote:
>>>>>> This patch enable the generation of symbols & local fixup information
>>>>>> for trees compiled with the -@ (--symbols) option.
>>>>>> 
>>>>>> Using this patch labels in the tree and their users emit information
>>>>>> in __symbols__ and __local_fixups__ nodes.
>>>>>> 
>>>>>> The __fixups__ node make possible the dynamic resolution of phandle
>>>>>> references which are present in the plugin tree but lie in the
>>>>>> tree that are applying the overlay against.
>>>>>> 
>>>>>> While there is a new magic number for dynamic device tree/overlays blobs
>>>>>> it is by default enabled. Remember to use -M to generate compatible
>>>>>> blobs.
>>>>>> 
>>>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>>>>>> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>>>>>> Signed-off-by: Jan Luebbe <jlu@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> Documentation/manual.txt |  25 +++++-
>>>>>> checks.c                 |   8 +-
>>>>>> dtc-lexer.l              |   5 ++
>>>>>> dtc-parser.y             |  50 +++++++++--
>>>>>> dtc.c                    |  39 +++++++-
>>>>>> dtc.h                    |  20 ++++-
>>>>>> fdtdump.c                |   2 +-
>>>>>> flattree.c               |  17 ++--
>>>>>> fstree.c                 |   2 +-
>>>>>> libfdt/fdt.c             |   2 +-
>>>>>> libfdt/fdt.h             |   3 +-
>>>>>> livetree.c               | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>> tests/mangle-layout.c    |   7 +-
>>>>>> 13 files changed, 375 insertions(+), 30 deletions(-)
>>>>>> 
>>>>>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>>>>>> index 398de32..094893b 100644
>>>>>> --- a/Documentation/manual.txt
>>>>>> +++ b/Documentation/manual.txt
>>>>>> @@ -119,6 +119,24 @@ Options:
>>>>>> 	Make space for <number> reserve map entries
>>>>>> 	Relevant for dtb and asm output only.
>>>>>> 
>>>>>> +    -@
>>>>>> +	Generates a __symbols__ node at the root node of the resulting blob
>>>>>> +	for any node labels used, and for any local references using phandles
>>>>>> +	it also generates a __local_fixups__ node that tracks them.
>>>>>> +
>>>>>> +	When using the /plugin/ tag all unresolved label references to
>>>>>> +	be tracked in the __fixups__ node, making dynamic resolution possible.
>>>>>> +
>>>>>> +    -A
>>>>>> +	Generate automatically aliases for all node labels. This is similar to
>>>>>> +	the -@ option (the __symbols__ node contain identical information) but
>>>>>> +	the semantics are slightly different since no phandles are automatically
>>>>>> +	generated for labeled nodes.
>>>>>> +
>>>>>> +    -M
>>>>>> +	Generate blobs with the old FDT magic number for device tree objects.
>>>>>> +	By default blobs use the DTBO FDT magic number instead.
>>>>>> +
>>>>>>   -S <bytes>
>>>>>> 	Ensure the blob at least <bytes> long, adding additional
>>>>>> 	space if needed.
>>>>>> @@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
>>>>>> Here is a very rough overview of the layout of a DTS source file:
>>>>>> 
>>>>>> 
>>>>>> -    sourcefile:   list_of_memreserve devicetree
>>>>>> +    sourcefile:   versioninfo plugindecl list_of_memreserve devicetree
>>>>>> 
>>>>>>   memreserve:   label 'memreserve' ADDR ADDR ';'
>>>>>> 		| label 'memreserve' ADDR '-' ADDR ';'
>>>>>> 
>>>>>>   devicetree:   '/' nodedef
>>>>>> 
>>>>>> +    versioninfo:  '/' 'dts-v1' '/' ';'
>>>>>> +
>>>>>> +    plugindecl:   '/' 'plugin' '/' ';'
>>>>>> +                | /* empty */
>>>>>> +
>>>>>>   nodedef:      '{' list_of_property list_of_subnode '}' ';'
>>>>>> 
>>>>>>   property:     label PROPNAME '=' propdata ';'
>>>>>> diff --git a/checks.c b/checks.c
>>>>>> index 2bd27a4..4292f4b 100644
>>>>>> --- a/checks.c
>>>>>> +++ b/checks.c
>>>>>> @@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
>>>>>> 
>>>>>> 			refnode = get_node_by_ref(dt, m->ref);
>>>>>> 			if (! refnode) {
>>>>>> -				FAIL(c, "Reference to non-existent node or label \"%s\"\n",
>>>>>> -				     m->ref);
>>>>>> +				if (!(bi->versionflags & VF_PLUGIN))
>>>>>> +					FAIL(c, "Reference to non-existent node or "
>>>>>> +							"label \"%s\"\n", m->ref);
>>>>>> +				else /* mark the entry as unresolved */
>>>>>> +					*((cell_t *)(prop->val.val + m->offset)) =
>>>>>> +						cpu_to_fdt32(0xffffffff);
>>>>>> 				continue;
>>>>>> 			}
>>>>>> 
>>>>>> diff --git a/dtc-lexer.l b/dtc-lexer.l
>>>>>> index 790fbf6..40bbc87 100644
>>>>>> --- a/dtc-lexer.l
>>>>>> +++ b/dtc-lexer.l
>>>>>> @@ -121,6 +121,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 14aaf2e..1a1f660 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"
>>>>>> @@ -33,6 +34,7 @@ extern void yyerror(char const *s);
>>>>>> 
>>>>>> extern struct boot_info *the_boot_info;
>>>>>> extern bool treesource_error;
>>>>>> +
>>>>> 
>>>>> Extraneous whitespace change here
>>>>> 
>>>> 
>>>> OK.
>>>> 
>>>>>> %}
>>>>>> 
>>>>>> %union {
>>>>>> @@ -52,9 +54,11 @@ extern bool treesource_error;
>>>>>> 	struct node *nodelist;
>>>>>> 	struct reserve_info *re;
>>>>>> 	uint64_t integer;
>>>>>> +	unsigned int flags;
>>>>>> }
>>>>>> 
>>>>>> %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 +75,8 @@ extern bool treesource_error;
>>>>>> 
>>>>>> %type <data> propdata
>>>>>> %type <data> propdataprefix
>>>>>> +%type <flags> versioninfo
>>>>>> +%type <flags> plugindecl
>>>>>> %type <re> memreserve
>>>>>> %type <re> memreserves
>>>>>> %type <array> arrayprefix
>>>>>> @@ -101,16 +107,34 @@ extern bool treesource_error;
>>>>>> %%
>>>>>> 
>>>>>> sourcefile:
>>>>>> -	  v1tag memreserves devicetree
>>>>>> +	  versioninfo plugindecl memreserves devicetree
>>>>>> +		{
>>>>>> +			the_boot_info = build_boot_info($1 | $2, $3, $4,
>>>>>> +							guess_boot_cpuid($4));
>>>>>> +		}
>>>>>> +	;
>>>>>> +
>>>>>> +versioninfo:
>>>>>> +	v1tag
>>>>>> 		{
>>>>>> -			the_boot_info = build_boot_info($2, $3,
>>>>>> -							guess_boot_cpuid($3));
>>>>>> +			$$ = VF_DT_V1;
>>>>>> 		}
>>>>>> 	;
>>>>>> 
>>>>>> v1tag:
>>>>>> 	  DT_V1 ';'
>>>>>> +	| DT_V1
>>>>>> 	| DT_V1 ';' v1tag
>>>>>> +
>>>>>> +plugindecl:
>>>>>> +	DT_PLUGIN ';'
>>>>>> +		{
>>>>>> +			$$ = VF_PLUGIN;
>>>>>> +		}
>>>>>> +	| /* empty */
>>>>>> +		{
>>>>>> +			$$ = 0;
>>>>>> +		}
>>>>>> 	;
>>>>>> 
>>>>>> memreserves:
>>>>>> @@ -161,10 +185,19 @@ 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 {
>>>>>> +				/*
>>>>>> +				 * We rely on the rule being always:
>>>>>> +				 *   versioninfo plugindecl memreserves devicetree
>>>>>> +				 * so $-1 is what we want (plugindecl)
>>>>>> +				 */
>>>>>> +				if ($<flags>-1 & VF_PLUGIN)
>>>>> 
>>>>> o_O... ok.  I've never seen negative value references before.  Can you
>>>>> provide a link to some documentation saying this is actually supported
>>>>> usage in bison?  I wasn't able to find it when I looked.
>>>>> 
>>>> 
>>>> There is a section about inherited attributes in the flex & bison book by O’Reily.
>>>> 
>>>> https://books.google.gr/books?id=3Sr1V5J9_qMC&lpg=PP1&dq=flex%20bison&hl=el&pg=PP1#v=onepage&q=flex%20bison&f=false
>>>> 
>>>> There’s a direct link to the 2nd Edition of lex & yacc:
>>>> 
>>>> https://books.google.gr/books?id=fMPxfWfe67EC&lpg=PA183&ots=RcRSji2NAT&dq=yacc%20inherited%20attributes&hl=el&pg=PA183#v=onepage&q=yacc%20inherited%20attributes&f=false
>>> 
>>> Thanks for the link.  I still think moving the fragment assembly out
>>> of the parser will be a better idea long term, but this does address
>>> the main concern I had, so it will do for now.
>>> 
>>>>>> +					add_orphan_node($1, $3, $2);
>>>>>> +				else
>>>>>> +					ERROR(&@2, "Label or path %s not found", $2);
>>>>>> +			}
>>>>>> 			$$ = $1;
>>>>>> 		}
>>>>>> 	| devicetree DT_DEL_NODE DT_REF ';'
>>>>>> @@ -179,6 +212,11 @@ devicetree:
>>>>>> 
>>>>>> 			$$ = $1;
>>>>>> 		}
>>>>>> +	| /* empty */
>>>>>> +		{
>>>>>> +			/* build empty node */
>>>>>> +			$$ = name_node(build_node(NULL, NULL), "");
>>>>>> +		}
>>>>>> 	;
>>>>>> 
>>>>>> nodedef:
>>>>>> diff --git a/dtc.c b/dtc.c
>>>>>> index 9dcf640..06e91bc 100644
>>>>>> --- a/dtc.c
>>>>>> +++ b/dtc.c
>>>>>> @@ -32,6 +32,9 @@ int minsize;		/* Minimum blob size */
>>>>>> int padsize;		/* Additional padding to blob */
>>>>>> int alignsize;		/* Additional padding to blob accroding to the alignsize */
>>>>>> int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
>>>>>> +int symbol_fixup_support;	/* enable symbols & fixup support */
>>>>>> +int auto_label_aliases;		/* auto generate labels -> aliases */
>>>>>> +int no_dtbo_magic;		/* use old FDT magic values for objects */
>>>>>> 
>>>>>> static int is_power_of_2(int x)
>>>>>> {
>>>>>> @@ -59,7 +62,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:a:fb:i:H:sW:E:hv";
>>>>>> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
>>>>>> static struct option const usage_long_opts[] = {
>>>>>> 	{"quiet",            no_argument, NULL, 'q'},
>>>>>> 	{"in-format",         a_argument, NULL, 'I'},
>>>>>> @@ -78,6 +81,9 @@ static struct option const usage_long_opts[] = {
>>>>>> 	{"phandle",           a_argument, NULL, 'H'},
>>>>>> 	{"warning",           a_argument, NULL, 'W'},
>>>>>> 	{"error",             a_argument, NULL, 'E'},
>>>>>> +	{"symbols",	     no_argument, NULL, '@'},
>>>>>> +	{"auto-alias",       no_argument, NULL, 'A'},
>>>>>> +	{"no-dtbo-magic",    no_argument, NULL, 'M'},
>>>>>> 	{"help",             no_argument, NULL, 'h'},
>>>>>> 	{"version",          no_argument, NULL, 'v'},
>>>>>> 	{NULL,               no_argument, NULL, 0x0},
>>>>>> @@ -109,6 +115,9 @@ static const char * const usage_opts_help[] = {
>>>>>> 	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
>>>>>> 	"\n\tEnable/disable warnings (prefix with \"no-\")",
>>>>>> 	"\n\tEnable/disable errors (prefix with \"no-\")",
>>>>>> +	"\n\tEnable symbols/fixup support",
>>>>>> +	"\n\tEnable auto-alias of labels",
>>>>>> +	"\n\tDo not use DTBO magic value for plugin objects",
>>>>>> 	"\n\tPrint this help and exit",
>>>>>> 	"\n\tPrint version and exit",
>>>>>> 	NULL,
>>>>>> @@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
>>>>>> 	fclose(f);
>>>>>> 
>>>>>> 	magic = fdt32_to_cpu(magic);
>>>>>> -	if (magic == FDT_MAGIC)
>>>>>> +	if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
>>>>>> 		return "dtb";
>>>>>> 
>>>>>> 	return guess_type_by_name(fname, fallback);
>>>>>> @@ -172,6 +181,7 @@ int main(int argc, char *argv[])
>>>>>> 	FILE *outf = NULL;
>>>>>> 	int outversion = DEFAULT_FDT_VERSION;
>>>>>> 	long long cmdline_boot_cpuid = -1;
>>>>>> +	fdt32_t out_magic = FDT_MAGIC;
>>>>>> 
>>>>>> 	quiet      = 0;
>>>>>> 	reservenum = 0;
>>>>>> @@ -249,6 +259,16 @@ int main(int argc, char *argv[])
>>>>>> 			parse_checks_option(false, true, optarg);
>>>>>> 			break;
>>>>>> 
>>>>>> +		case '@':
>>>>>> +			symbol_fixup_support = 1;
>>>>>> +			break;
>>>>>> +		case 'A':
>>>>>> +			auto_label_aliases = 1;
>>>>>> +			break;
>>>>>> +		case 'M':
>>>>>> +			no_dtbo_magic = 1;
>>>>>> +			break;
>>>>>> +
>>>>>> 		case 'h':
>>>>>> 			usage(NULL);
>>>>>> 		default:
>>>>>> @@ -306,6 +326,14 @@ int main(int argc, char *argv[])
>>>>>> 	fill_fullpaths(bi->dt, "");
>>>>>> 	process_checks(force, bi);
>>>>>> 
>>>>>> +	if (auto_label_aliases)
>>>>>> +		generate_label_tree(bi->dt, "aliases", false);
>>>>>> +
>>>>>> +	if (symbol_fixup_support) {
>>>>>> +		generate_label_tree(bi->dt, "__symbols__", true);
>>>>>> +		generate_fixups_tree(bi->dt);
>>>>> 
>>>>> Hang on.. this doesn't seem right.  I thought -@ controlled the
>>>>> __symbols__ side (i.e. the part upon which we overlay) rather than the
>>>>> fixups side (the part which overlays).  A dtbo could certainly have
>>>>> both, of course, but for base trees, wouldn't you have symbols without
>>>>> fixups?  And should it be illegal to try to build a /plugin/ without
>>>>> -@?
>>>> 
>>>> It does control both for now. For base trees having the fixup nodes
>>>> will allow us to do probe order dependency tracking in the future.
>>> 
>>> Erm.. how?
>>> 
>>>> For plugins we need the __symbols__ node to support stacked overlays, i.e.
>>>> overlays referring label that were introduced by a previous overlay.
>>> 
>>> Yes, I realise that an overlay may well want __symbols__ as well.  But
>>> they still seem conceptually different.  I think -@ should control
>>> __symbols__ whereas /plugin/ should control __fixups__.
>>> 
>> 
>> It is easily done. Although using /plugin/ as an auto-magic option does both
>> just fine.
> 
> Sorry, I don't follow what you're saying.
> 

The last patch uses the /plugin/ tag to turn on the options that are required
(both symbols & fixups)

>>>> For plugins there is no requirement for now to actually contain references to
>>>> be resolved. It can easily be enforced though.
>>> 
>>> Sure, but I don't see the relevance of that here.  You could just omit
>>> the __fixups__ node if there's nothing to go into them.
>>> 
>> 
>> Hmm, yeah.
>> 
>> 
>> Regards
>> 
>> — Pantelis
>> 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards

— Pantelis

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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux