dtc: some fixes, and make asm labels for data

The following series adds the features of labeling memory reserve slots and labeling specific bytes or cells of property data. When translating from dts to asm, a global symbol is created with the name matching the label in the dts file.

With these features a program does not need to understand the flattened device tree headers, offsets, layouts, or alignments to modify a property's contents. Instead it can read or write to the bytes or words addressed by a global symbol.

In addition the series contains 3 fixes to the compiler (one major
- incorrect output, one recognising an input error, and one compiler warning), a source dtc for use as a testcase, and two formatting changes to the generated assembly.

I verified the label placement in the asm output and compared the dts to dtb output with outptut of dts to asm after cc -c ond objcopy -o binary, the difference was trailing padding of the assembled version. I also compared the dts to dts output with the dtb to dts of the assembled version. Although I did not include the assembled output with the testcase, it should be placed in the test directory when the samples there are upgraded to version 17.

I have not studied the tests directory since the new libfdt test suite was merged to evaluate how to test the labels point at the property.

I choose not to attempt writing the labels when generating dts output at this time, although I did consider it. The code currently resolves node references to phandles creating the properties if needed, so creating dts from dts is already a lossy operation. I felt there was no benefit in preserving the labels until there is a method to read and write some kind of symbol or map file of labels with dtb input and therefore couldn't justify writing the code to split the data as it is output. Such code could be used to avoid formatting a list of strings as list of cells or bytes.

I did not provide a method to label the address and size fields of a memory reserve entry. Although it could be implemented, I didn't deem it necessary as there is no header before the address field and placing a label on the size field of a reserve created with the start-end syntax would be misleading.

I did not include a prefix when generating the labels, matching the existing labels output on node and property tags. If code is later added to change the _dt label prefix used in the header to allow multiple trees to be linked in one program, a prefix should be added to the labels to create a a seperate namespace for each dts.

milton

The version 17 flat device tree format added struct size. When writing versin 17 assembly output the field must be emitted.

---
Found when trying to parse the binary from the assembled output.

Index: dtc/flattree.c
===================================================================
@@ -477,6 +477,10 @@ void dt_to_asm(FILE *f, struct boot_info
 		fprintf(f, "\t.long\t_%s_strings_end - _%s_strings_start\t/* size_dt_strings */\n",
 			symprefix, symprefix);
 
+	if (vi->flags & FTF_STRUCTSIZE)
+		fprintf(f, "\t.long\t_%s_struct_end - _%s_struct_start\t/* size_dt_struct */\n",
+			symprefix, symprefix);
+
 	/*
 	 * Reserve map entries.
 	 * Align the reserve map to a doubleword boundary.

Oops, good catch.

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

yyerror() is used by both dtc-parser.y and dtc-lexer.l, so move the declaration to srcpos.h.

---
eliminates implicit declaration warning.

Index: dtc/dtc-parser.y
===================================================================
@@ -26,7 +26,6 @@
 #include "srcpos.h"
 
 int yylex(void);
-void yyerror(char const *);
 cell_t cell_from_string(char *s, unsigned int base);
 
 extern struct boot_info *the_boot_info;

Index: dtc/srcpos.h
===================================================================
@@ -62,6 +62,7 @@ typedef struct YYLTYPE {
 
 
 
+extern void yyerror(char const *);
 
 extern int srcpos_filenum;
 

Eck. This wasn't necessary: that part of dtc-parser.y is included into dtc-parser.tab.h which in turn is included into dtc-lexer.l. srcpos.h is supposed to be exclusively for, well, the source position stuff.

Please revert.

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

Check that strtoul() parsed the complete string.

As with the number overflow case, write a non-fatal error message to stdout.

---
I saw the FIXME and knew how to fix it.

I think the severity should be higher, but opted for the detailed error message compared to the fixed string of yyerror() and its immediate termination of parsing.

Index: dtc/dtc-parser.y
===================================================================
@@ -192,19 +192,27 @@ void yyerror (char const *s)
  * Convert a string representation of a numeric cell
  * in the given base into a cell.
  *
- * FIXME: The string "abc123", base 10, should be flagged
- *        as an error due to the leading "a", but isn't yet.
+ * FIXME: should these specification errors be fatal instead?
  */
 
 cell_t cell_from_string(char *s, unsigned int base)
 {
 	cell_t c;
+	char *e;
+
+	c = strtoul(s, &e, base);
+	if (*e) {
+		fprintf(stderr,
+			"Line %d: Invalid cell value '%s' : "
+			"%c is not a base %d digit; %d assumed\n",
+			yylloc.first_line, s, *e, base, c);
+	}
 
-	c = strtoul(s, NULL, base);
 	if (errno == EINVAL || errno == ERANGE) {
 		fprintf(stderr,
 			"Line %d: Invalid cell value '%s'; %d assumed\n",
 			yylloc.first_line, s, c);
+		errno = 0;
 	}
 
 	return c;

Yeah, it should really result in a fatal error, but delayed until after parsing and other error checking is complete. Yet another one for the Great Error Handling Rewrite. This is an improvement for now.

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

Allow a label to be placed on a memory reserve entry. Change the parser to recogonise and store them. Emit them when writing assembly output.

---
The infrastructure for storing the label was already present, so the changes are small.

Index: dtc/dtc-parser.y
===================================================================
@@ -92,11 +92,11 @@ memreserves:	memreserve memreserves {
 		}
 	;
 
-memreserve:	DT_MEMRESERVE DT_ADDR DT_ADDR ';' {
-			$$ = build_reserve_entry($2, $3, NULL);
+memreserve:	label DT_MEMRESERVE DT_ADDR DT_ADDR ';' {
+			$$ = build_reserve_entry($3, $4, $1);
 		}
-	|	DT_MEMRESERVE DT_ADDR '-' DT_ADDR ';' {
-			$$ = build_reserve_entry($2, $4 - $2 + 1, NULL);
+	|	label DT_MEMRESERVE DT_ADDR '-' DT_ADDR ';' {
+			$$ = build_reserve_entry($3, $5 - $3 + 1, $1);
 		}
 	;
 

Index: dtc/flattree.c
===================================================================
@@ -497,6 +497,10 @@ void dt_to_asm(FILE *f, struct boot_info
 	 * as it appears .quad isn't available in some assemblers.
 	 */
 	for (re = bi->reservelist; re; re = re->next) {
+		if (re->label) {
+			fprintf(f, "\t.globl\t%s\n", re->label);
+			fprintf(f, "%s:\n", re->label);
+		}
 		fprintf(f, "\t.long\t0x%08x\n\t.long\t0x%08x\n",
 			(unsigned int)(re->re.address >> 32),
 			(unsigned int)(re->re.address & 0xffffffff));

Change the grow_data_for function to copy struct data and modifiy the fields it is updating instead of storing all fields individually to a stack allocated struct.

This reduces maintence for future enhancements as now all instances of struct data are created by modifying a copy of an existing struct data or directly copying empty_data.

---
It took me a bit of time to find the source of the uninitialized pointer that was causing my changes to segfault.

index 5d7db17..3d68792 100644
@@ -57,6 +57,8 @@ struct data data_grow_for(struct data d,
 	if (xlen == 0)
 		return d;
 
+	nd = d;
+
 	newsize = xlen;
 
 	while ((d.len + xlen) > newsize)

@@ -64,8 +66,6 @@ struct data data_grow_for(struct data d,
 
 	nd.asize = newsize;
 	nd.val = xrealloc(d.val, newsize);
-	nd.len = d.len;
-	nd.refs = d.refs;
 
 	assert(nd.asize >= (d.len + xlen));
 

Change the lexer to recognise a label in any context. Place before other celldata and bytestrings to avoid the initial characters being stolen by other matches.

A label is a character sequence starting with an alphabetic or underscore optinally followed by the same plus digits and terminating in a colon.

The included terminating colon will prevent matching hex numbers.

---
The following patches parse matches in CELLDATA and BYTESTRING in addition to the current INITIAL context; matches in MEMRESERVE and INCLUDE context will not be parsed.

index 77ccd54..0e356bb 100644
@@ -107,6 +107,16 @@ #endif
 			BEGIN(INITIAL);
 			return ';';
 		}
+
+<*>[a-zA-Z_][a-zA-Z0-9_]*:	{
+			yylloc.filenum = srcpos_filenum;
+			yylloc.first_line = yylineno;
+			DPRINT("Label: %s\n", yytext);
+			yylval.str = strdup(yytext);
+			yylval.str[yyleng-1] = '\0';
+			return DT_LABEL;
+		}
+
 <CELLDATA>[bodh]# {
 			yylloc.filenum = srcpos_filenum;
 			yylloc.first_line = yylineno;

@@ -189,15 +199,6 @@ #endif
 		}
 
 
-[a-zA-Z_][a-zA-Z0-9_]*:	{
-			yylloc.filenum = srcpos_filenum;
-			yylloc.first_line = yylineno;
-			DPRINT("Label: %s\n", yytext);
-			yylval.str = strdup(yytext);
-			yylval.str[yyleng-1] = '\0';
-			return DT_LABEL;
-		}
-
 <*>{WS}+	/* eat whitespace */
 
 <*>"/*"([^*]|\*+[^*/])*\*+"/"	{

Extend the parser grammer to allow labels before or after any property data (string, cell list, or byte list), and any byte or cell within the property data.

Store the labels using the same linked list structure as node references using in a parallel list.

When writing assembly output emit global labels as offsets from the start of the definition of the data.

Note that the alignment for a cell list is done as part of the opening < delimiter, not the = or , before it. To label a cell after a string or byte list put the label inside the cell list.

For example,

	prop = zero: [ aa bb ], two: < four: 1234 > eight: ;

will produce labels with offsets 0, 2, 4, and 8 bytes from the beginning of the data for property prop.

---
The testcase in the following patch will also note in the source tree the gap between labels two and four above.

I took out the explcit list of fields in emtpy_data because it would have required a third line to add the label field and all fields were initialized to the default value.

Index: dtc/data.c
===================================================================
@@ -29,12 +29,17 @@ void fixup_free(struct fixup *f)
 
 void data_free(struct data d)
 {
-	struct fixup *f;
+	struct fixup *f, *nf;
 
 	f = d.refs;
 	while (f) {
-		struct fixup *nf;
+		nf = f->next;
+		fixup_free(f);
+		f = nf;
+	}
 
+	f = d.labels;
+	while (f) {
 		nf = f->next;
 		fixup_free(f);
 		f = nf;

@@ -198,27 +203,36 @@ struct data data_append_data(struct data
 	return d;
 }
 
-struct data data_merge(struct data d1, struct data d2)
+void fixup_merge(struct fixup **fd, struct fixup **fd2, int d1_len)
 {
-	struct data d;
 	struct fixup **ff;
 	struct fixup *f, *f2;
 
-	d = data_append_data(d1, d2.val, d2.len);
-
 	/* Extract d2's fixups */
-	f2 = d2.refs;
-	d2.refs = NULL;
+	f2 = *fd2;
+	*fd2 = NULL;
 
 	/* Tack them onto d's list of fixups */
-	ff = &d.refs;
+	ff = fd;
 	while (*ff)
 		ff = &((*ff)->next);
 	*ff = f2;
 
 	/* And correct them for their new position */
 	for (f = f2; f; f = f->next)
-		f->offset += d1.len;
+		f->offset += d1_len;
+
+
+}
+
+struct data data_merge(struct data d1, struct data d2)
+{
+	struct data d;
+
+	d = data_append_data(d1, d2.val, d2.len);
+
+	fixup_merge(&d.refs, &d2.refs, d1.len);
+	fixup_merge(&d.labels, &d2.labels, d1.len);
 
 	data_free(d2);
 

@@ -285,6 +299,22 @@ struct data data_add_fixup(struct data d
 	return nd;
 }
 
+struct data data_add_label(struct data d, char *label)
+{
+	struct fixup *f;
+	struct data nd;
+
+	f = xmalloc(sizeof(*f));
+	f->offset = d.len;
+	f->ref = label;
+	f->next = d.labels;
+
+	nd = d;
+	nd.labels = f;
+
+	return nd;
+}
+
 int data_is_one_string(struct data d)
 {
 	int i;

Index: dtc/dtc-parser.y
===================================================================
@@ -131,9 +131,11 @@ propdata:	propdataprefix DT_STRING { $$ 
 			$$ = data_merge(data_append_align($1, sizeof(cell_t)), $3);
 		}
 	|	propdataprefix '[' bytestring ']' { $$ = data_merge($1, $3); }
+	|	propdata DT_LABEL { $$ = data_add_label($1, $2); }
 	;
 
 propdataprefix:	propdata ',' { $$ = $1; }
+	|	propdataprefix DT_LABEL { $$ = data_add_label($1, $2); }
 	|	/* empty */ { $$ = empty_data; }
 	;
 

@@ -150,10 +152,12 @@ celllist:	celllist opt_cell_base DT_CELL
 	|	celllist DT_REF	{
 			$$ = data_append_cell(data_add_fixup($1, $2), -1);
 		}
+	|	celllist DT_LABEL { $$ = data_add_label($1, $2); }
 	|	/* empty */ { $$ = empty_data; }
 	;
 
 bytestring:	bytestring DT_BYTE { $$ = data_append_byte($1, $2); }
+	|	bytestring DT_LABEL { $$ = data_add_label($1, $2); }
 	|	/* empty */ { $$ = empty_data; }
 	;
 

Index: dtc/dtc.h
===================================================================
@@ -111,10 +111,10 @@ struct data {
 	char *val;
 	int asize;
 	struct fixup *refs;
+	struct fixup *labels;
 };
 
-#define empty_data \
-	((struct data){.len = 0, .val = NULL, .asize = 0, .refs = NULL})
+#define empty_data ((struct data){ /* all .members = 0 or NULL */ })
 
 void fixup_free(struct fixup *f);
 void data_free(struct data d);

@@ -135,6 +135,7 @@ struct data data_append_zeroes(struct da
 struct data data_append_align(struct data d, int align);
 
 struct data data_add_fixup(struct data d, char *ref);
+struct data data_add_label(struct data d, char *label);
 
 int data_is_one_string(struct data d);
 

Index: dtc/flattree.c
===================================================================
@@ -121,6 +121,12 @@ static void emit_label(FILE *f, char *pr
 	fprintf(f, "_%s_%s:\n", prefix, label);
 }
 
+static void emit_offset_label(FILE *f, char *label, int offset)
+{
+	fprintf(f, "\t.globl\t%s\n", label);
+	fprintf(f, "%s\t= . + %d\n", label, offset);
+}
+
 static void asm_emit_cell(void *e, cell_t val)
 {
 	FILE *f = e;

@@ -157,6 +163,13 @@ static void asm_emit_data(void *e, struc
 {
 	FILE *f = e;
 	int off = 0;
+	struct fixup *l;
+
+	l = d.labels;
+	while (l) {
+		emit_offset_label(f, l->ref, l->offset);
+		l = l->next;
+	}
 
 	while ((d.len - off) >= sizeof(u32)) {
 		fprintf(f, "\t.long\t0x%x\n",

When adding a label, walk to the end of the list since the label reflects the end of the data.

Since merging data buffers already preserved the order, this will cause the labels to be emitted in order when writing assembly output.

It should also aid emiting labels when writing dts output should that be added in the future (data formatting would need to break at each label).

---
Currently cosmetic as the labels will be on the correct data either way.

Index: dtc/data.c
===================================================================
@@ -301,16 +301,22 @@ struct data data_add_fixup(struct data d
 
 struct data data_add_label(struct data d, char *label)
 {
-	struct fixup *f;
+	struct fixup *f, **p;
 	struct data nd;
 
 	f = xmalloc(sizeof(*f));
 	f->offset = d.len;
 	f->ref = label;
-	f->next = d.labels;
 
 	nd = d;
-	nd.labels = f;
+	p = &nd.labels;
+
+	/* adding to end keeps them sorted */
+	while (*p)
+		p = &((*p)->next);
+
+	f->next = *p;
+	*p = f;
 
 	return nd;
 }

Create a source file with labels for use as a testcase to check parsing dts files.

---
I started with the existing test01.dts and added labels and a labeled subnode.

The labels could instead be added to that file if desired, but that would mean the output of the compiler would not match the input when writing dts from dts or dtb input.

I haven't studied how to use the new testsuite to check the result. Initially I was going to suggest diff on the asm output, but with the libfdt library we could write a test to change the property and reserve contents then use the library to fetch and compare.

Index: dtc/tests/label01.dts
===================================================================
@@ -0,0 +1,60 @@
+/memreserve/ 1000000000000000 0000000002000000;
+memrsv2: /memreserve/ 2000000000000000-20ffffffffffffff;
+/memreserve/ 0-13;
+
+/ {
+	model = "MyBoardName";
+	compatible = "MyBoardName", "MyBoardFamilyName";
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	cpus {
+		linux,phandle = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		PowerPC,970@0 {
+			name = "PowerPC,970";
+			device_type = "cpu";
+			reg = <0>;
+			clock-frequency = <5f5e1000>;
+			timebase-frequency = <1FCA055>;
+			linux,boot-cpu;
+			i-cache-size = <10000>;
+			d-cache-size = <8000>;
+		};
+
+		PowerPC,970@1 {
+			name = "PowerPC,970";
+			device_type = "cpu";
+			reg = <1>;
+			clock-frequency = <5f5e1000>;
+			timebase-frequency = <1FCA055>;
+			i-cache-size = <10000>;
+			d-cache-size = <8000>;
+		};
+
+	};
+
+	node: randomnode {
+		prop: string = data: "\xff\0stuffstuff\t\t\t\n\n\n" data_end: ;
+		blob = [byte: 0a 0b 0c 0d de ea ad be ef byte_end: ];
+		ref = < cell: &/memory@0 cell_end: >;
+		mixed = "abc", pre: [1234] post: , gap: < aligned: a b c>;
+		subnode: child {
+		};
+		/* subnode_end: is auto-generated by node emit */
+	};
+	/* node_end: is auto-generated by node emit */
+
+	memory@0 {
+		device_type = "memory";
+		memreg: reg = <00000000 00000000 00000000 20000000>;
+	};
+
+	chosen {
+		bootargs = "root=/dev/sda2";
+		linux,platform = <00000600>;
+	};
+
+};
+

Insert tabs to align the comments describing the fields of the boot parameters header struct.

---
Previously some of the comments were seperated by one space and others by one tab from the field.

Purely cosmetic, but then so are the comments.

Index: dtc/flattree.c
===================================================================
@@ -469,21 +469,21 @@ void dt_to_asm(FILE *f, struct boot_info
 
 	emit_label(f, symprefix, "blob_start");
 	emit_label(f, symprefix, "header");
-	fprintf(f, "\t.long\tOF_DT_HEADER /* magic */\n");
-	fprintf(f, "\t.long\t_%s_blob_abs_end - _%s_blob_start /* totalsize */\n",
+	fprintf(f, "\t.long\tOF_DT_HEADER\t\t\t\t/* magic */\n");
+	fprintf(f, "\t.long\t_%s_blob_abs_end - _%s_blob_start\t/* totalsize */\n",
 		symprefix, symprefix);
-	fprintf(f, "\t.long\t_%s_struct_start - _%s_blob_start /* off_dt_struct */\n",
+	fprintf(f, "\t.long\t_%s_struct_start - _%s_blob_start\t/* off_dt_struct */\n",
 		symprefix, symprefix);
-	fprintf(f, "\t.long\t_%s_strings_start - _%s_blob_start /* off_dt_strings */\n",
+	fprintf(f, "\t.long\t_%s_strings_start - _%s_blob_start\t/* off_dt_strings */\n",
 		symprefix, symprefix);
-	fprintf(f, "\t.long\t_%s_reserve_map - _%s_blob_start /* off_dt_strings */\n",
+	fprintf(f, "\t.long\t_%s_reserve_map - _%s_blob_start\t/* off_dt_strings */\n",
 		symprefix, symprefix);
-	fprintf(f, "\t.long\t%d /* version */\n", vi->version);
-	fprintf(f, "\t.long\t%d /* last_comp_version */\n",
+	fprintf(f, "\t.long\t%d\t\t\t\t\t/* version */\n", vi->version);
+	fprintf(f, "\t.long\t%d\t\t\t\t\t/* last_comp_version */\n",
 		vi->last_comp_version);
 
 	if (vi->flags & FTF_BOOTCPUID)
-		fprintf(f, "\t.long\t%i\t/*boot_cpuid_phys*/\n",
+		fprintf(f, "\t.long\t%i\t\t\t\t\t/* boot_cpuid_phys */\n",
 			boot_cpuid_phys);
 
 	if (vi->flags & FTF_STRTABSIZE)

When writing the memory reserve table in assembly output, emit both halves of each 64 bit number on a single .long statement. This results in two lines per memory reserve slot instead of four, each line contains one field (start or size).

---
Purely consmentic but aids in showing the data is from a single 64 bit quantity. The end marker and the extra reserved slots were already emitting a pair of zeros per statement.

Index: dtc/flattree.c
===================================================================
@@ -514,10 +514,10 @@ void dt_to_asm(FILE *f, struct boot_info
 			fprintf(f, "\t.globl\t%s\n", re->label);
 			fprintf(f, "%s:\n", re->label);
 		}
-		fprintf(f, "\t.long\t0x%08x\n\t.long\t0x%08x\n",
+		fprintf(f, "\t.long\t0x%08x, 0x%08x\n",
 			(unsigned int)(re->re.address >> 32),
 			(unsigned int)(re->re.address & 0xffffffff));
-		fprintf(f, "\t.long\t0x%08x\n\t.long\t0x%08x\n",
+		fprintf(f, "\t.long\t0x%08x, 0x%08x\n",
 			(unsigned int)(re->re.size >> 32),
 			(unsigned int)(re->re.size & 0xffffffff));
 	}

So, like, the other day Milton Miller mumbled:

Milton,

I applied the whole lot.
Minor English typo corrections in commit log messages.

Also, I added two patches on top of these to reorganize the Makefile into a better top-down layout and added some Version string support, borrowed almost verbatim from the Linux Makefiles. We're going to arbitrarily start with 1.0.0 unless someone has a good argument for something else!

And, for those keeping track, my "To Do" list here still contains the notion of breaking the booting-without-of.txt file into two parts. The technical descriptions of the DTC and its file (blob) layout will move to the DTC repository while the descriptions of nodes and properties will remain in the kernel repository.

Let me know what you think...

Thanks,
jdl