Hello and thanks for the feedback.
On Wed, Sep 19, 2018 at 6:47 AM David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 13, 2018 at 11:32:13AM +0200, Fredrik Markstrom wrote:
> > Commit 8f1b3 (Correct overlay syntactic sugar for generating target-path fragments)
> > changes what is produced when using label references. Instead of merging the nodes
> > it creates a new fragment. This patch reverts that behavior for labels but still
> > generates separate fragments for path references.
> >
> > Previously the dts below would generate two fragments:
> >
> > /dts-v1/;
> > /plugin/;
> > &x { a: a@0 {};};
> > &a { b {}; };
> >
> > I've kept DT_REF upper case in this patch to keep the patch small and readable,
> > if it is decided to merge this I'll create a v3 where that is fixed
> >
> > Signed-off-by: Fredrik Markstrom <fredrik.markstrom@xxxxxxxxx>
>
> I think the patch is correct, however there are a couple of style
> issues.
>
> More importantly, though, I'm still not sure what the concrete benefit
> of this is - which ought to be in the commit message.
>
> At present, the structure of the dts file will be precisely reflected
> in the structure of the fragments in the dtbo, which is a nice
> property. That's not a *strong* reason to keep it the way it is, but
> it needs some kind of counter-argument to revert it.
The main reason is backwards compatibility. There exists dtb
consumers, most notably the 4.1 linux kernel, that doesn't resolve
target references between fragments in the same dtbo. So fragments
with targets pointing at something in the same dtbo will fail to load
(silently in 4.1). This is working with dtc 1.4.6 (since it merges),
but did break with commit 8f1b3 that was included in 1.4.7. (I have
not checked if newer kernels can handle this case)
In addition, isn't it good to do stuff we can do at compile time
instead of run time (for performance and footprint). In our simplest
use case the dtbo increases ~7% in size with 8f1b3 (without this
patch).
If we decide that this change is good, I will prepare another version
of the patch with the changes you suggest below and a more elaborate
commit description including the information above.
>
> > ---
> > dtc-lexer.l | 4 ++--
> > dtc-parser.y | 27 +++++++++++++++++++++++++--
> > 2 files changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/dtc-lexer.l b/dtc-lexer.l
> > index 615b7ec..06c0409 100644
> > --- a/dtc-lexer.l
> > +++ b/dtc-lexer.l
> > @@ -213,14 +213,14 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
> > <*>\&{LABEL} { /* label reference */
> > DPRINT("Ref: %s\n", yytext+1);
> > yylval.labelref = xstrdup(yytext+1);
> > - return DT_REF;
> > + return DT_LABEL_REF;
> > }
> >
> > <*>"&{/"{PATHCHAR}*\} { /* new-style path reference */
> > yytext[yyleng-1] = '\0';
> > DPRINT("Ref: %s\n", yytext+2);
> > yylval.labelref = xstrdup(yytext+2);
> > - return DT_REF;
> > + return DT_PATH_REF;
> > }
> >
> > <BYTESTRING>[0-9a-fA-F]{2} {
> > diff --git a/dtc-parser.y b/dtc-parser.y
> > index 815481a..44748ed 100644
> > --- a/dtc-parser.y
> > +++ b/dtc-parser.y
> > @@ -70,7 +70,8 @@ extern bool treesource_error;
> > %token <byte> DT_BYTE
> > %token <data> DT_STRING
> > %token <labelref> DT_LABEL
> > -%token <labelref> DT_REF
> > +%token <labelref> DT_LABEL_REF
> > +%token <labelref> DT_PATH_REF
> > %token DT_INCBIN
> >
> > %type <data> propdata
> > @@ -83,6 +84,7 @@ extern bool treesource_error;
> > %type <data> bytestring
> > %type <prop> propdef
> > %type <proplist> proplist
> > +%type <labelref> DT_REF
>
> Only terminal symbols should be named with capitals, non-terminals
> (which this now is) should be lower case.
Agreed, I actually mentioned why I kept it upper case in the commit message :)
>
> >
> > %type <node> devicetree
> > %type <node> nodedef
> > @@ -158,6 +160,8 @@ memreserve:
> > }
> > ;
> >
> > +DT_REF: DT_LABEL_REF {} | DT_PATH_REF {};
>
> You don't need empty semantic blocks ({}).
You are right, thanks.
/Fredrik
>
> > devicetree:
> > '/' nodedef
> > {
> > @@ -189,7 +193,7 @@ devicetree:
> > ERROR(&@3, "Label or path %s not found", $3);
> > $$ = $1;
> > }
> > - | devicetree DT_REF nodedef
> > + | devicetree DT_PATH_REF nodedef
> > {
> > /*
> > * We rely on the rule being always:
> > @@ -208,6 +212,25 @@ devicetree:
> > }
> > $$ = $1;
> > }
> > + | devicetree DT_LABEL_REF nodedef
> > + {
> > + struct node *target = get_node_by_ref($1, $2);
> > +
> > + if (target) {
> > + merge_nodes(target, $3);
> > + } else {
> > + /*
> > + * We rely on the rule being always:
> > + * versioninfo plugindecl memreserves devicetree
> > + * so $-1 is what we want (plugindecl)
> > + */
> > + if ($<flags>-1 & DTSF_PLUGIN)
> > + add_orphan_node($1, $3, $2);
> > + else
> > + ERROR(&@2, "Label or path %s not found", $2);
> > + }
> > + $$ = $1;
> > + }
> > | devicetree DT_DEL_NODE DT_REF ';'
> > {
> > struct node *target = get_node_by_ref($1, $3);
>
> --
> 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
--
/Fredrik
