On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote: > From: Frank Rowand > > Proof of concept patch. > > Annotates input source file and line number of nodes and properties > as comments in output .dts file when --annotate flag is supplied. > > > A common dts source file convention is for a system .dts file > to include default SOC and/or device .dtsi files and then add > additional system specific properties or over-ride property values > from the .dtsi files. It can be a time consuming and error prone > exercise to determine exactly what nodes, properties, and property > values are in the final .dtb binary blob and where they originated. > > Modify the dtc compiler to read a (possibly cpp pre-processed) .dts > file and for the output .dts annotate each node and property with > the corresponding source location. > > As an example, one device tree node for the dragonboard in the > Linux kernel source tree is: > > ----- long format ----- > > sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14.3-18.5 */ > compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240.4-37 */ > reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241.4-49 */ > reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242.4-37 */ > interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243.4-38 */ > interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244.4-42 */ > clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245.4-36 */ > clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246.4-34 */ > status = "ok"; /* qcom-apq8074-dragonboard.dts:17.4-18 */ > bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15.4-20 */ > non-removable; /* qcom-apq8074-dragonboard.dts:16.4-18 */ > }; /* qcom-apq8074-dragonboard.dts:14.3-18.5 */ > > > ----- short format (requires patch 3) ----- > > sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14 */ > compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240 */ > reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241 */ > reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242 */ > interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243 */ > interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244 */ > clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245 */ > clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246 */ > status = "ok"; /* qcom-apq8074-dragonboard.dts:17 */ > bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15 */ > non-removable; /* qcom-apq8074-dragonboard.dts:16 */ > }; /* qcom-apq8074-dragonboard.dts:18 */ > > > qcom-apq8074-dragonboard.dts: > - last referenced the sdhci node > - changed the value of the "status" property from "disabled" to "ok" > - added two properties, "bus-width" and "non-removable" > > qcom-msm8974.dtsi: > - initially set the value the "status" property to "disabled" > (not visible in the annotated .dts) > - provided all of the other property values > > > When the dtc compiler is run within the Linux kernel build system, > the path of the source files will be the full absolute path, just > as seen for gcc warnings and errors. I always trim away the path > leading up to the Linux kernel source tree by passing the kernel > build output through a sed pipe. I have done the same to the > above example to remove the excessive verbosity in the source paths. > > Implementation notes: > > - The source location of each node and property is saved in the > respective node or property during the parse phase because > the source location information no longer available when the > final values are written out from dt_to_source() and the > functions that it calls. > > - A check is added to dtc.c to ensure that input and output format > are both device tree source. An alternate choice would be to > turn off the --annotate flag if either the input file or the > output file is not device tree source. In the alternate case, > the disabling of --annotate could be silent or a warning could > be issued. > > TODO: > > - More testing. > > Not-signed-off-by: Frank Rowand > --- > dtc-parser.y | 16 ++++++++-------- > dtc.c | 9 +++++++++ > dtc.h | 9 ++++++--- > flattree.c | 2 +- > fstree.c | 7 ++++--- > livetree.c | 20 ++++++++++++++------ > srcpos.c | 35 +++++++++++++++++++++++++++++++++++ > srcpos.h | 2 ++ > treesource.c | 38 +++++++++++++++++++++++++++++++++----- > 9 files changed, 112 insertions(+), 26 deletions(-) > > Index: b/dtc.h > =================================================================== [snip] > Index: b/dtc-parser.y > =================================================================== > --- a/dtc-parser.y > +++ b/dtc-parser.y > @@ -134,11 +134,11 @@ memreserve: > devicetree: > '/' nodedef > { > - $$ = name_node($2, ""); > + $$ = name_node($2, "", &@1); > } > | devicetree '/' nodedef > { > - $$ = merge_nodes($1, $3); > + $$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3)); > } The two branches here aren't quite consistent - the first doesn't include the '/', the second does. You could either change the second to just &@3, or use &@$ for the first. > > | devicetree DT_LABEL DT_REF nodedef > @@ -147,7 +147,7 @@ devicetree: > > add_label(&target->labels, $2); > if (target) > - merge_nodes(target, $4); > + merge_nodes(target, $4, &@4); This one doesn't include the name/label again > else > ERROR(&@3, "Label or path %s not found", $3); > $$ = $1; > @@ -157,7 +157,7 @@ devicetree: > struct node *target = get_node_by_ref($1, $2); > > if (target) > - merge_nodes(target, $3); > + merge_nodes(target, $3, &@3); > else > ERROR(&@2, "Label or path %s not found", $2); > $$ = $1; > @@ -197,11 +197,11 @@ proplist: > propdef: > DT_PROPNODENAME '=' propdata ';' > { > - $$ = build_property($1, $3); > + $$ = build_property($1, $3, &@$); > } > | DT_PROPNODENAME ';' > { > - $$ = build_property($1, empty_data); > + $$ = build_property($1, empty_data, &@$); > } > | DT_DEL_PROP DT_PROPNODENAME ';' > { > @@ -456,11 +456,11 @@ subnodes: > subnode: > DT_PROPNODENAME nodedef > { > - $$ = name_node($2, $1); > + $$ = name_node($2, $1, &@$); .. and this one does. Looking at all of these it's probably going to be simplest not to include the label/name (i.e. just use the srcpos from the nodedef). > } > | DT_DEL_NODE DT_PROPNODENAME ';' > { > - $$ = name_node(build_node_delete(), $2); > + $$ = name_node(build_node_delete(), $2, &@$); > } > | DT_LABEL subnode > { > Index: b/srcpos.c > =================================================================== > --- a/srcpos.c > +++ b/srcpos.c > @@ -246,6 +246,41 @@ srcpos_copy(struct srcpos *pos) > return pos_new; > } > > +struct srcpos * > +srcpos_copy_all(struct srcpos *pos) > +{ > + struct srcpos *pos_new; > + struct srcfile_state *srcfile_state; > + > + if (!pos) > + return NULL; > + > + pos_new = srcpos_copy(pos); > + > + if (pos_new) { > + /* allocate without free */ > + srcfile_state = xmalloc(sizeof(struct srcfile_state)); > + memcpy(srcfile_state, pos->file, sizeof(struct srcfile_state)); > + > + pos_new->file = srcfile_state; > + } > + > + return pos_new; > +} You still don't need this function. srcfile_states have unlimited lifetime. > + > +struct srcpos * > +srcpos_combine(struct srcpos *left_srcpos, struct srcpos *right_srcpos) > +{ > + struct srcpos *pos_new; > + > + pos_new = srcpos_copy(left_srcpos); > + > + pos_new->last_line = right_srcpos->last_line; > + pos_new->last_column = right_srcpos->last_column; > + > + return pos_new; > +} > + > > > void > Index: b/srcpos.h > =================================================================== > --- a/srcpos.h > +++ b/srcpos.h > @@ -104,6 +104,8 @@ extern struct srcpos srcpos_empty; > > extern void srcpos_update(struct srcpos *pos, const char *text, int len); > extern struct srcpos *srcpos_copy(struct srcpos *pos); > +extern struct srcpos *srcpos_copy_all(struct srcpos *pos); > +extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos, struct srcpos *right_srcpos); > extern char *srcpos_string(struct srcpos *pos); > extern void srcpos_dump(struct srcpos *pos); > > Index: b/flattree.c > =================================================================== > --- a/flattree.c > +++ b/flattree.c > @@ -692,7 +692,7 @@ static struct property *flat_read_proper > > val = flat_read_data(dtbuf, proplen); > > - return build_property(name, val); > + return build_property(name, val, NULL); > } > > > Index: b/fstree.c > =================================================================== > --- a/fstree.c > +++ b/fstree.c > @@ -60,7 +60,8 @@ static struct node *read_fstree(const ch > } else { > prop = build_property(xstrdup(de->d_name), > data_copy_file(pfile, > - st.st_size)); > + st.st_size), > + NULL); > add_property(tree, prop); > fclose(pfile); > } > @@ -68,7 +69,7 @@ static struct node *read_fstree(const ch > struct node *newchild; > > newchild = read_fstree(tmpname); > - newchild = name_node(newchild, xstrdup(de->d_name)); > + newchild = name_node(newchild, xstrdup(de->d_name), NULL); > add_child(tree, newchild); > } > > @@ -84,7 +85,7 @@ struct boot_info *dt_from_fs(const char > struct node *tree; > > tree = read_fstree(dirname); > - tree = name_node(tree, ""); > + tree = name_node(tree, "", NULL); > > return build_boot_info(NULL, tree, guess_boot_cpuid(tree)); > } -- 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