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
