On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxxxxxxxx>
>
> 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 <frank.rowand@xxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
