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