On Wed, Sep 19, 2018 at 03:26:46PM +0200, Fredrik Markström wrote:
> 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).
Ok, I'm convinced, let's go ahead with this.
> 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.
Great.
>
> >
> > > ---
> > > 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 :)
Missed that, sorry.
> >
> > >
> > > %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
Attachment:
signature.asc
Description: PGP signature
