Re: [PATCH 12/13] dtc: Merge the fragments of a dts file after parsing

[Herve Codina <herve.codina@bootlin.com>]

On Sun,  1 Nov 2026 09:37:06 -0700
Simon Glass <sjg@chromium.org> wrote:

> We would like to deal with each top-level fragment of a dts file before
> it is merged into the tree, but that requires the fragments to be kept
> apart until the whole file is parsed. At present the parser merges each
> one as soon as it is parsed. The grammar actions also need to know
> whether the file is a plugin, which they find using $<flags>-1
> 
> Have the grammar build a list of the fragments instead, in source order,
> and merge them with a new merge_fragments() once the whole file is
> parsed. This calls merge_fragment() for each one, so the output does not
> change, and frees each fragment once it is merged.
> 
> The $<flags>-1 term is no longer needed. Within a devicetree action it
> reaches back up the parser stack to the value of headers, i.e. the flags
> from /dts-v1/ and /plugin/. The sourcefile action passes the same value
> to build_dt_info(), which stores it in dti->dtsflags, so
> merge_fragments() can use that instead.
> 
> There are two differences in the errors. A syntax error later in the
> file now stops dtc before any earlier reference errors are reported, and
> reference errors now come after any other errors which the parser
> reports, such as a division by zero, rather than in source order.
> 
> dtc gives the same output and messages as before for all 4162 .dts and
> .dtso files in Linux v7.3-12284-g3799eeb906c9, and for the dts files in
> tests/
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Link: https://lore.kernel.org/devicetree-compiler/auK0ERvxGCJGSc3p@gractus.seuss/
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  dtc-parser.y | 43 +++++++++++++------------------------------
>  dtc.h        |  2 +-
>  livetree.c   | 36 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 48 insertions(+), 33 deletions(-)
> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 1ed9731..c1aa4ad 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -24,22 +24,6 @@ extern void yyerror(char const *s);
>  extern struct dt_info *parser_output;
>  extern bool treesource_error;
>  
> -/*
> - * Merge a fragment into the tree as soon as it is parsed. The flags come
> - * from the actions using $<flags>-1, which relies on the rule being always:
> - *   headers memreserves devicetree
> - * so $-1 is the value of headers
> - */
> -static struct node *merge_parsed_fragment(struct node *dt, struct fragment *f,
> -					  unsigned int flags)
> -{
> -	if (merge_fragment(&dt, f, flags & DTSF_PLUGIN))
> -		treesource_error = true;
> -	free_fragment(f);
> -
> -	return dt;
> -}
> -
>  %}
>  
>  %union {
> @@ -94,7 +78,7 @@ static struct node *merge_parsed_fragment(struct node *dt, struct fragment *f,
>  %type <labelref> dt_ref
>  %type <fragment> fragment_ref
>  
> -%type <node> devicetree
> +%type <fragment> devicetree
>  %type <node> nodedef
>  %type <node> subnode
>  %type <nodelist> subnodes
> @@ -119,8 +103,10 @@ static struct node *merge_parsed_fragment(struct node *dt, struct fragment *f,
>  sourcefile:
>  	  headers memreserves devicetree
>  		{
> -			parser_output = build_dt_info($1, $2, $3,
> -			                              guess_boot_cpuid($3));
> +			parser_output = build_dt_info($1, $2, NULL, 0);
> +			parser_output->fragments = $3;
> +			if (merge_fragments(parser_output))
> +				treesource_error = true;

Why not just pass the fragment list to build_dt_info() instead of using the 
parser_output->fragments and later in merge_fragments() setting this same field
to NULL once the merge is done?

IHMO, the struct dt_info doesn't need the fragments fiels (already mentionned
in patch 2).

If build_dt_info() has too much parameters, a struct dt_info_def could be
created to group all those parameters in one structure.

>  		}
>  	;
>  
> @@ -184,42 +170,39 @@ fragment_ref:
>  devicetree:
>  	  '/' nodedef
>  		{
> -			struct fragment *f = build_fragment(FRAGMENT_ROOT, $2);
> -
> -			$$ = merge_parsed_fragment(NULL, f, $<flags>-1);
> +			$$ = build_fragment(FRAGMENT_ROOT, $2);
>  		}
>  	| devicetree '/' nodedef
>  		{
> -			struct fragment *f = build_fragment(FRAGMENT_ROOT, $3);
> -
> -			$$ = merge_parsed_fragment($1, f, $<flags>-1);
> +			$$ = append_fragment($1, build_fragment(FRAGMENT_ROOT,
> +								$3));
>  		}
>  	| fragment_ref nodedef
>  		{
>  			$1->node = $2;
> -			$$ = merge_parsed_fragment(NULL, $1, $<flags>-1);
> +			$$ = $1;
>  		}
>  	| devicetree DT_LABEL fragment_ref nodedef
>  		{
>  			$3->label = $2;
>  			$3->label_srcpos = srcpos_copy(&@2);
>  			$3->node = $4;
> -			$$ = merge_parsed_fragment($1, $3, $<flags>-1);
> +			$$ = append_fragment($1, $3);
>  		}
>  	| devicetree fragment_ref nodedef
>  		{
>  			$2->node = $3;
> -			$$ = merge_parsed_fragment($1, $2, $<flags>-1);
> +			$$ = append_fragment($1, $2);
>  		}
>  	| devicetree DT_DEL_NODE fragment_ref ';'
>  		{
>  			$3->type = FRAGMENT_DELETE_NODE;
> -			$$ = merge_parsed_fragment($1, $3, $<flags>-1);
> +			$$ = append_fragment($1, $3);
>  		}
>  	| devicetree DT_OMIT_NO_REF fragment_ref ';'
>  		{
>  			$3->type = FRAGMENT_OMIT_IF_NO_REF;
> -			$$ = merge_parsed_fragment($1, $3, $<flags>-1);
> +			$$ = append_fragment($1, $3);
>  		}
>  	;
>  
> diff --git a/dtc.h b/dtc.h
> index 41013f5..fdd0d75 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -368,7 +368,6 @@ struct fragment *build_fragment_ref(char *ref, bool path_ref,
>  				    struct srcpos *srcpos);
>  struct fragment *append_fragment(struct fragment *list, struct fragment *new);
>  void free_fragment(struct fragment *f);
> -bool merge_fragment(struct node **dtp, struct fragment *f, bool plugin);
>  
>  struct dt_info {
>  	unsigned int dtsflags;
> @@ -386,6 +385,7 @@ struct dt_info {
>  struct dt_info *build_dt_info(unsigned int dtsflags,
>  			      struct reserve_info *reservelist,
>  			      struct node *tree, uint32_t boot_cpuid_phys);
> +bool merge_fragments(struct dt_info *dti);
>  void sort_tree(struct dt_info *dti);
>  void generate_labels_from_tree(struct dt_info *dti, const char *name);
>  void generate_label_tree(struct dt_info *dti, const char *name, bool allocph);
> diff --git a/livetree.c b/livetree.c
> index 1e012d4..68ad3c3 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -537,7 +537,8 @@ struct dt_info *build_dt_info(unsigned int dtsflags,
>  	dti->fragments = NULL;
>  	dti->boot_cpuid_phys = boot_cpuid_phys;
>  
> -	fill_fullpaths(dti->dt, "");
> +	if (tree)
> +		fill_fullpaths(dti->dt, "");
>  
>  	return dti;
>  }
> @@ -718,7 +719,8 @@ static bool merge_omit_if_no_ref(struct node *dt, struct fragment *f)
>   *
>   * Returns true if there is an error, which has been reported.
>   */
> -bool merge_fragment(struct node **dtp, struct fragment *f, bool plugin)
> +static bool merge_fragment(struct node **dtp, struct fragment *f,
> +			   bool plugin)
>  {
>  	struct node *dt = *dtp;
>  
> @@ -750,6 +752,36 @@ bool merge_fragment(struct node **dtp, struct fragment *f, bool plugin)
>  	return false;
>  }
>  
> +/*
> + * Merge the fragments of a dts file into a single tree, in source order.
> + *
> + * Returns true if there are errors, which have been reported.
> + */
> +bool merge_fragments(struct dt_info *dti)
> +{
> +	bool plugin = dti->dtsflags & DTSF_PLUGIN;
> +	struct fragment *f, *next;
> +	struct node *dt = NULL;
> +	bool error = false;
> +
> +	if (!dti->fragments)
> +		return false;
> +
> +	for (f = dti->fragments; f; f = next) {
> +		next = f->next;
> +		if (merge_fragment(&dt, f, plugin))
> +			error = true;
> +		free_fragment(f);
> +	}
> +
> +	dti->dt = dt;
> +	dti->fragments = NULL;

This field is really a temporary field. Should be removed from the
struct dt_info.


> +	dti->boot_cpuid_phys = guess_boot_cpuid(dt);
> +	fill_fullpaths(dti->dt, "");

fill_fullpaths() should be called only once in build_dt_info() as well as
setting dti->boot_cpuid_phys.

They have nothing to do with fragments.

> +
> +	return error;
> +}
> +
>  /*
>   * Tree accessor functions
>   */

Best regards,
Hervé