DTC: Polish up the DTS Version 1 implementation.

From: Jon Loeliger <[email protected]>

Fixes BYTESTRING lexing.
Allows -O dts output to be emitted in a given (1) format version.

Skirts around a range check problem in eval_literal() for now.

Signed-off-by Jon Loeliger <[email protected]>

---

David,

This patch is directly on top of your prior two patches. Lemme know what you think.

dtc-lexer.l +0 -0

 			if ((yytext[0] == '{')
 			    || (yytext[0] == ';')) {
 				DPRINT("<PROPNODENAME>\n");

dtc-parser.y +0 -1

index ffb2299..652199f 100644
@@ -297,17 +297,23 @@ label:
 
 %%
 
-void yyerror (char const *s)

dtc-parser.y +6 -0

 }
 
+/*
+ * bits is unused, but it should be 32 or 64 as needed.
+ *
+ * FIXME: However, with bits == 64, ((1ULL << bits) - 1)
+ * overflows before you can actually do the range test.
+ */
 unsigned long long eval_literal(const char *s, int base, int bits)
 {
 	unsigned long long val;

dtc-parser.y +0 -0

 	return val;
 }

dtc.c +1 -1

index bbef829..38313f5 100644
@@ -225,7 +225,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (streq(outform, "dts")) {
-		dt_to_source(outf, bi);
+		dt_to_source(outf, bi, 1);
 	} else if (streq(outform, "dtb")) {
 		dt_to_blob(outf, bi, outversion, boot_cpuid_phys);
 	} else if (streq(outform, "asm")) {

dtc.h +1 -1

index d080153..ba2027b 100644
@@ -238,7 +238,7 @@ struct boot_info *dt_from_blob(FILE *f);
 
 /* Tree source */
 
-void dt_to_source(FILE *f, struct boot_info *bi);
+void dt_to_source(FILE *f, struct boot_info *bi, int dts_version);
 struct boot_info *dt_from_source(const char *f);
 
 /* FS trees */

treesource.c +7 -3

index 376ebc8..ac0c777 100644
@@ -140,14 +140,18 @@ static void write_propval_string(FILE *f, struct data val)
 	fprintf(f, "\";\n");
 }
 
-static void write_propval_cells(FILE *f, struct data val)
+static void write_propval_cells(FILE *f, struct data val, int dts_version)
 {
 	void *propend = val.val + val.len;
 	cell_t *cp = (cell_t *)val.val;
 
 	fprintf(f, " = <");
 	for (;;) {
-		fprintf(f, "%x", be32_to_cpu(*cp++));
+		if (dts_version == 0) {
+			fprintf(f, "%x", be32_to_cpu(*cp++));
+		} else {
+			fprintf(f, "0x%x", be32_to_cpu(*cp++));
+		}
 		if ((void *)cp >= propend)
 			break;
 		fprintf(f, " ");

@@ -155,14 +159,18 @@ static void write_propval_cells(FILE *f, struct data val)
 	fprintf(f, ">;\n");
 }
 
-static void write_propval_bytes(FILE *f, struct data val)
+static void write_propval_bytes(FILE *f, struct data val, int dts_version)
 {
 	void *propend = val.val + val.len;

Quoted 4 lines

From: Jon Loeliger <[email protected]>

Fixes BYTESTRING lexing.
Allows -O dts output to be emitted in a given (1) format version.

Ok... I'd actually be more inclined to remove the option and just have -Odts *always* use the newer version.

Having dtc be able to convert dts versions forward is easy and useful.

Quoted 2 lines

This patch is directly on top of your prior two patches. Lemme know what you think.

On top of my two dts-v1 patches that is, I assume?

Ow... that's rather embarrassing, that I didn't even notice I'd totally broken bytestrings. Really must add some bytestrings to test_tree1.dts so this actually gets checked by the testsuite.

Not really related changes, but whatever..

Quoted 14 lines

dtc-parser.y +6 -1

+/*
+ * bits is unused, but it should be 32 or 64 as needed.
+ *
+ * FIXME: However, with bits == 64, ((1ULL << bits) - 1)
+ * overflows before you can actually do the range test.
+ */
 unsigned long long eval_literal(const char *s, int base, int bits)
 {
 	unsigned long long val;

@@ -317,9 +323,10 @@ unsigned long long eval_literal(const char *s, int base, int bits)
 	val = strtoull(s, &e, base);
 	if (*e)
 		yyerror("bad characters in literal");
-	else if ((errno == ERANGE) || (val > ((1ULL << bits)-1)))

dtc-parser.y unsigned long long eval_literal(const char *s, int base, int bits) · line 330

Quoted 5 lines

dtc-parser.y +2 -0

+	else if (errno == ERANGE)
 		yyerror("literal out of range");
 	else if (errno != 0)
 		yyerror("bad literal");
+

Ok.. I don't understand why you've pulled out the range checking against bits here.

Quoted 6 lines

treesource.c +1 -1

-static void write_propval_bytes(FILE *f, struct data val)
+static void write_propval_bytes(FILE *f, struct data val, int dts_version)
 {

treesource.c +0 -0

 	void *propend = val.val + val.len;
 	char *bp = val.val;

treesource.c static void write_propval_cells(FILE *f, struct data val) · line 173

Quoted 8 lines

treesource.c +5 -1

 	fprintf(f, " = [");
 	for (;;) {
-		fprintf(f, "%02hhx", *bp++);
+		if (dts_version == 0) {
+			fprintf(f, "%02hhx", *bp++);
+		} else {
+			fprintf(f, "0x%02hhx", *bp++);
+		}

Uh.. not quite right. My patch (intentionally) leaves bytestrings as pure hex, without 0x.

We can argue about whether that's a good idea,
if you like,

but in any case input and output should match.

To avoid this sort of problem, I suggest before we apply the dts-v1 transition, we apply the patch I'm working on (I'll try to get it out today), which adds a bunch of extra testcases checking that using dtc to go dtb->dts->dtb preserves everything.

So, like, the other day David Gibson mumbled:

Quoted 6 lines · Jon Loeliger

Quoted 4 lines

From: Jon Loeliger <[email protected]>

Fixes BYTESTRING lexing.
Allows -O dts output to be emitted in a given (1) format version.

Quoted 2 lines

Ok... I'd actually be more inclined to remove the option and just have -Odts *always* use the newer version.

You didn't read the code. :-) That's what it does! There is no option, it is just parameterized in the code. It _always_ forward re-writes as the latest version.

Quoted 2 lines

Having dtc be able to convert dts versions forward is easy and useful.

Oh, I see. Thank you.

Quoted 4 lines

Quoted 2 lines

This patch is directly on top of your prior two patches. Lemme know what you think.

On top of my two dts-v1 patches that is, I assume?

Right.

Quoted 3 lines

Ow... that's rather embarrassing, that I didn't even notice I'd totally broken bytestrings. Really must add some bytestrings to test_tree1.dts so this actually gets checked by the testsuite.

I ran my "old versus new DTC" comparison script and found it. :-)

Quoted 1 line

Not really related changes, but whatever..

Uh, yeah. Leftoverish. Sorry.

Quoted 14 lines

Quoted 14 lines

dtc-parser.y +6 -1

+/*
+ * bits is unused, but it should be 32 or 64 as needed.
+ *
+ * FIXME: However, with bits == 64, ((1ULL << bits) - 1)
+ * overflows before you can actually do the range test.
+ */
 unsigned long long eval_literal(const char *s, int base, int bits)
 {
 	unsigned long long val;

@@ -317,9 +323,10 @@ unsigned long long eval_literal(const char *s, int base, int bits)
 	val = strtoull(s, &e, base);
 	if (*e)
 		yyerror("bad characters in literal");
-	else if ((errno == ERANGE) || (val > ((1ULL << bits)-1)))

Quoted 8 lines

dtc-parser.y unsigned long long eval_literal(const char *s, int base, int bits) · line 330

Quoted 5 lines

dtc-parser.y +2 -0

+	else if (errno == ERANGE)
 		yyerror("literal out of range");
 	else if (errno != 0)
 		yyerror("bad literal");
+

Ok.. I don't understand why you've pulled out the range checking against bits here.

Because it wasn't working, as explained in the comment I added. Specifically, (1<<bits), with bits==64 overflowed and yielded the value 0.

Quoted 6 lines

Quoted 6 lines

treesource.c +1 -1

-static void write_propval_bytes(FILE *f, struct data val)
+static void write_propval_bytes(FILE *f, struct data val, int dts_version)
 {
 	void *propend = val.val + val.len;
 	char *bp = val.val;

Quoted 11 lines

treesource.c static void write_propval_cells(FILE *f, struct data val) · line 173

Quoted 8 lines

treesource.c +5 -1

 	fprintf(f, " = [");
 	for (;;) {
-		fprintf(f, "%02hhx", *bp++);
+		if (dts_version == 0) {
+			fprintf(f, "%02hhx", *bp++);
+		} else {
+			fprintf(f, "0x%02hhx", *bp++);
+		}

Uh.. not quite right. My patch (intentionally) leaves bytestrings as pure hex, without 0x.

Ugh. That seems inconsistent and wrong to me.

Quoted 2 lines

We can argue about whether that's a good idea,
if you like,

And in the blue corner, touting consistent hex forms, ...

Quoted 1 line

but in any case input and output should match.

Oops. That they should.

Quoted 4 lines

To avoid this sort of problem, I suggest before we apply the dts-v1 transition, we apply the patch I'm working on (I'll try to get it out today), which adds a bunch of extra testcases checking that using dtc to go dtb->dts->dtb preserves everything.

Yeah, I've been doing that too... Sounds good.

Yes, I know, but I don't think it's even worth having the unused internal parameterization.

Quoted 2 lines

I ran my "old versus new DTC" comparison script and found it. :-)

Heh, we should fold that into the testsuite, too.

Quoted 3 lines

Because it wasn't working, as explained in the comment I added. Specifically, (1<<bits), with bits==64 overflowed and yielded the value 0.

Ah...

Well, I assumed (1ULL << 64) would equal 0, which is why the comparison has the (-1) - I was expecting for bits == 64 it would end up being against -1, i.e. 0xffffffffffffffff. This appears to work on the systems I've been using.

But I just remembered that (x << n) has undefined behaviour in C when n >= word size.

suspect I didn't catch it because I've been building with -O0 for gdb-ability, which might change the behaviour of corner cases like that.

So I guess we need

	else if ((errno == ERANGE)
		 || ((bits < 64) && (val >= (1ULL << bits))))

Quoted 1 line

And in the blue corner, touting consistent hex forms, ...

And in the red, compact bytestring representations.

No, seriously, the inconsistency bothers me too. But so does the fact that using 0x in the bytestring would double the minimum size for representing bytestrings, somewhat changing the flavour of [] as well (because spaces are no longer optional). I'm looking for a killer argument one way or the other, but I haven't found it yet.

Quoted 3 lines · David Gibson

Yes, I know, but I don't think it's even worth having the unused internal parameterization.

OK. We can eliminate it then; no problem.

Quoted 3 lines

Quoted 1 line

I ran my "old versus new DTC" comparison script and found it. :-)

Heh, we should fold that into the testsuite, too.

I'll start by simply adding the script to the test directory then.

Quoted 10 lines

Quoted 3 lines

Because it wasn't working, as explained in the comment I added. Specifically, (1<<bits), with bits==64 overflowed and yielded the value 0.

Ah...

Well, I assumed (1ULL << 64) would equal 0, which is why the comparison has the (-1) - I was expecting for bits == 64 it would end up being against -1, i.e. 0xffffffffffffffff. This appears to work on the systems I've been using.

But not on an x86 system.

Quoted 2 lines

But I just remembered that (x << n) has undefined behaviour in C when n >= word size.

Exactly. In fact, I think x86 takes the shift value, bit-wise ANDs it with 63 internally, and then shifts left by that value.

So I guess (1 << 64) is just returning garbage - I

In fact, it is yielding 1 on an x86 machine.

Quoted 3 lines

suspect I didn't catch it because I've been building with -O0 for gdb-ability, which might change the behaviour of corner cases like that.

Or works on a PPC machine? :-)

Quoted 3 lines

So I guess we need

	else if ((errno == ERANGE)
		 || ((bits < 64) && (val >= (1ULL << bits))))

Sounds good. I'll commit --amend that into the patch!

Quoted 9 lines

Quoted 1 line

And in the blue corner, touting consistent hex forms, ...

And in the red, compact bytestring representations.

No, seriously, the inconsistency bothers me too. But so does the fact that using 0x in the bytestring would double the minimum size for representing bytestrings, somewhat changing the flavour of [] as well (because spaces are no longer optional). I'm looking for a killer argument one way or the other, but I haven't found it yet.

But why does it even have to be hex numbers at all? I guess my point is that they could just be expressions. You could use 0x31 or 49 or '1' or 061, whichever made more sense in some application. You don't necessarily take a representational size hit.

But you do take a hit w.r.t. *minimum* representation size - there's no form amongst all the possibilities here more compact than pure hex. Especially since spaces are optional in the old form. The fact that [ab cd 00] and [abcd00] are equivalent was a deliberate choice in the original form.

The point of [] is for random binary data which is neither strings (even with the odd strange character) nor sensibly organized into 32-bit (or larger) integers. Wanting something other than hex here is much rarer than in the < > case.

You're seeing < > and [ ] as basically the same thing - a list of values - with the only difference being the size of those values. That's not wrong, but it's not the only way to look at it - and it's not the way I was thinking of [ ] when I invented it. Your proposal makes perfect sense while you think of [] as a list of values - but not so much when it's thought of as a direct binary representation.

So I'm thinking perhaps we need two different things here: a "list of values" representation, which can accomodate expressions and can also have multiple sizes (because expressions which are evaluated to a 16-bit or 64-bit value could also be useful under the right circumstances), and the [ ] "bytestring
literal" representation. Perhaps something like:

(32-bit values)

	<0xdeadbeef (1+1)>

or <.32 0xdeadbeef (1+1)>

(64-bit values)

	<.64 (0xdeadbeef << 32) (-1)>
(8-bit values)
	<.8 0x00 0x0a 0xe4 0x2c 0x23 (0x10 + n)>

i.e. < > is list of values form, with size of each value as a sort of parameter (defaulting to 32-bit, of course). I'm not sure I like that particular syntax, it's just the first thing I came up with to demonstrate the idea.

So, like, the other day David Gibson mumbled:

Quoted 31 lines

But you do take a hit w.r.t. *minimum* representation size - there's no form amongst all the possibilities here more compact than pure hex. Especially since spaces are optional in the old form. The fact that [ab cd 00] and [abcd00] are equivalent was a deliberate choice in the original form.

The point of [] is for random binary data which is neither strings (even with the odd strange character) nor sensibly organized into 32-bit (or larger) integers. Wanting something other than hex here is much rarer than in the < > case.

You're seeing < > and [ ] as basically the same thing - a list of values - with the only difference being the size of those values. That's not wrong, but it's not the only way to look at it - and it's not the way I was thinking of [ ] when I invented it. Your proposal makes perfect sense while you think of [] as a list of values - but not so much when it's thought of as a direct binary representation.

So I'm thinking perhaps we need two different things here: a "list of values" representation, which can accomodate expressions and can also have multiple sizes (because expressions which are evaluated to a 16-bit or 64-bit value could also be useful under the right circumstances), and the [ ] "bytestring
literal" representation. Perhaps something like:

(32-bit values)

	<0xdeadbeef (1+1)>

or <.32 0xdeadbeef (1+1)>

(64-bit values)

Quoted 8 lines

	<.64 (0xdeadbeef << 32) (-1)>
(8-bit values)
	<.8 0x00 0x0a 0xe4 0x2c 0x23 (0x10 + n)>

i.e. < > is list of values form, with size of each value as a sort of parameter (defaulting to 32-bit, of course). I'm not sure I like that particular syntax, it's just the first thing I came up with to demonstrate the idea.

Ah ha! I see. You want this, then:

    x = <.srec 0000 C001C0DEGE75BABE F1>

OK. That was entirely joking. We all know that
the cool code does NOT get the babe.

Seriously though, I see your point, and I don't really have a strong opinion here, so in the interest of making some headway, we can just leave it as is for now.

If it turns out to be a bad decision later, we'll fix it. :-)

And with that issue behind us....

I'm going to post these patches to introduce the new DTS format! Any last straggler comments?

Folks,

When the new DTS /dts-v1/ support is released Real Soon Now, it will support C-like literal constants. Hex values will be prefixed with 0x, binary with 0b, and bare numbers will be decimal unless they start with a leading 0.

One outstanding question on which I'd like some feedback is the issue of bytestring value representation.

Currently they look like this:

    stuff = [ 0b 31 22 de ea ad be ef ];

One opinion is to have them continue to look like that and be in hex only.

Another opinion is to allow the new, consistent C-style literals and expressions so that one could have:

    new_stuff = [ 0x31 49 '1' 23 17 ];

Opinions?

Folks,

When the new DTS /dts-v1/ support is released Real Soon Now, it will support C-like literal constants. Hex values will be prefixed with 0x, binary with 0b, and bare numbers will be decimal unless they start with a leading 0.

One outstanding question on which I'd like some feedback is the issue of bytestring value representation.

Currently they look like this:

    stuff = [ 0b 31 22 de ea ad be ef ];

One opinion is to have them continue to look like that and be in hex only.

Another opinion is to allow the new, consistent C-style literals and expressions so that one could have:

    new_stuff = [ 0x31 49 '1' 23 17 ];

Opinions?

Thanks,
jdl