[PATCH] DTC: Polish up the DTS Version 1 implementation.

[PATCH] DTC: Polish up the DTS Version 1 implementation.

[Jon Loeliger]

From: Jon Loeliger <jdl@freescale.com>

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 <jdl@freescale.com>
---

David,

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

jdl



 dtc-lexer.l  |    4 ++++
 dtc-parser.y |   13 ++++++++++---
 dtc.c        |    2 +-
 dtc.h        |    2 +-
 treesource.c |   39 +++++++++++++++++++++++++++------------
 5 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 1c262d2..b18c0d1 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -190,6 +190,10 @@ static int dts_version; /* = 0 */
 <*>.		{
 			yylloc.filenum = srcpos_filenum;
 			yylloc.first_line = yylineno;
+			if (yytext[0] == '[') {
+				DPRINT("BYTESTRING\n");
+				BEGIN(BYTESTRING);
+			}
 			if ((yytext[0] == '{')
 			    || (yytext[0] == ';')) {
 				DPRINT("<PROPNODENAME>\n");
diff --git a/dtc-parser.y b/dtc-parser.y
index ffb2299..652199f 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -297,17 +297,23 @@ label:
 
 %%
 
-void yyerror (char const *s)
+void yyerror(char const *s)
 {
 	const char *fname = srcpos_filename_for_num(yylloc.filenum);
 
 	if (strcmp(fname, "-") == 0)
 		fname = "stdin";
 
-	fprintf(stderr, "%s:%d %s\n",
+	fprintf(stderr, "%s:%d Error: %s\n",
 		fname, yylloc.first_line, s);
 }
 
+/*
+ * 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)))
+	else if (errno == ERANGE)
 		yyerror("literal out of range");
 	else if (errno != 0)
 		yyerror("bad literal");
+
 	return val;
 }
diff --git a/dtc.c b/dtc.c
index bbef829..38313f5 100644
--- a/dtc.c
+++ b/dtc.c
@@ -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")) {
diff --git a/dtc.h b/dtc.h
index d080153..ba2027b 100644
--- a/dtc.h
+++ b/dtc.h
@@ -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 */
diff --git a/treesource.c b/treesource.c
index 376ebc8..ac0c777 100644
--- a/treesource.c
+++ b/treesource.c
@@ -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;
 	char *bp = val.val;
 
 	fprintf(f, " = [");
 	for (;;) {
-		fprintf(f, "%02hhx", *bp++);
+		if (dts_version == 0) {
+			fprintf(f, "%02hhx", *bp++);
+		} else {
+			fprintf(f, "0x%02hhx", *bp++);
+		}
 		if ((void *)bp >= propend)
 			break;
 		fprintf(f, " ");
@@ -170,7 +178,7 @@ static void write_propval_bytes(FILE *f, struct data val)
 	fprintf(f, "];\n");
 }
 
-static void write_tree_source_node(FILE *f, struct node *tree, int level)
+static void write_tree_source_node(FILE *f, struct node *tree, int level, int dts_version)
 {
 	struct property *prop;
 	struct node *child;
@@ -202,35 +210,42 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 			break;
 
 		case PROP_CELLS:
-			write_propval_cells(f, prop->val);
+			write_propval_cells(f, prop->val, dts_version);
 			break;
 
 		case PROP_BYTES:
-			write_propval_bytes(f, prop->val);
+			write_propval_bytes(f, prop->val, dts_version);
 			break;
 		}
 	}
 	for_each_child(tree, child) {
 		fprintf(f, "\n");
-		write_tree_source_node(f, child, level+1);
+		write_tree_source_node(f, child, level + 1, dts_version);
 	}
 	write_prefix(f, level);
 	fprintf(f, "};\n");
 }
 
 
-void dt_to_source(FILE *f, struct boot_info *bi)
+void dt_to_source(FILE *f, struct boot_info *bi, int dts_version)
 {
 	struct reserve_info *re;
+	const char *fmt_str;
 
+	if (dts_version > 0) {
+		fprintf(f, "/dts-v1/;\n\n");
+		fmt_str = "/memreserve/\t0x%016llx 0x%016llx;\n";
+	} else {
+		fmt_str = "/memreserve/\t%016llx %016llx;\n";
+	}
 	for (re = bi->reservelist; re; re = re->next) {
 		if (re->label)
 			fprintf(f, "%s: ", re->label);
-		fprintf(f, "/memreserve/\t%016llx-%016llx;\n",
+		fprintf(f, fmt_str,
 			(unsigned long long)re->re.address,
 			(unsigned long long)(re->re.address + re->re.size - 1));
 	}
+	fprintf(f, "\n");
 
-	write_tree_source_node(f, bi->dt, 0);
+	write_tree_source_node(f, bi->dt, 0, dts_version);
 }
-
-- 
1.5.3.1.139.g9346b

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[David Gibson]

On Tue, Nov 06, 2007 at 04:19:19PM -0600, Jon Loeliger wrote:
> From: Jon Loeliger <jdl@freescale.com>
> 
> 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.  Coverting backwards is far more complicated, because as well
as the actualy incompatible version changes, there's the question of
which dtc version supported which dts features.  Not worth it, I
think.

> Skirts around a range check problem in eval_literal() for now.
> 
> Signed-off-by: Jon Loeliger <jdl@freescale.com>
> ---
> 
> David,
> 
> 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?

> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 1c262d2..b18c0d1 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -190,6 +190,10 @@ static int dts_version; /* = 0 */
>  <*>.		{
>  			yylloc.filenum = srcpos_filenum;
>  			yylloc.first_line = yylineno;
> +			if (yytext[0] == '[') {
> +				DPRINT("BYTESTRING\n");
> +				BEGIN(BYTESTRING);
> +			}

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.

>  			if ((yytext[0] == '{')
>  			    || (yytext[0] == ';')) {
>  				DPRINT("<PROPNODENAME>\n");
> diff --git a/dtc-parser.y b/dtc-parser.y
> index ffb2299..652199f 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -297,17 +297,23 @@ label:
>  
>  %%
>  
> -void yyerror (char const *s)
> +void yyerror(char const *s)
>  {
>  	const char *fname = srcpos_filename_for_num(yylloc.filenum);
>  
>  	if (strcmp(fname, "-") == 0)
>  		fname = "stdin";
>  
> -	fprintf(stderr, "%s:%d %s\n",
> +	fprintf(stderr, "%s:%d Error: %s\n",
>  		fname, yylloc.first_line, s);

Not really related changes, but whatever..

>  }
>  
> +/*
> + * 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)))
> +	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.

>  	return val;
>  }
> diff --git a/dtc.c b/dtc.c
> index bbef829..38313f5 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -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")) {
> diff --git a/dtc.h b/dtc.h
> index d080153..ba2027b 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -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 */
> diff --git a/treesource.c b/treesource.c
> index 376ebc8..ac0c777 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -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;
>  	char *bp = val.val;
>  
>  	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.

-- 
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

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[Jon Loeliger]

So, like, the other day David Gibson mumbled:
> On Tue, Nov 06, 2007 at 04:19:19PM -0600, Jon Loeliger wrote:
> > From: Jon Loeliger <jdl@freescale.com>
> > 
> > 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.

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.

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

Oh, I see.  Thank you.

> > 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.

> 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. :-)

> Not really related changes, but whatever..

Uh, yeah.  Leftoverish.  Sorry.


> > +/*
> > + * 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)))
> > +	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.

Here's the problem:

---------------- snip to get foo.c ----------------
#include "stdio.h"

int bits = 64;
unsigned long long val = 0xabcd1234;

main()
{
    int i;

    for (i = 1; i <= 64; i++) {
        unsigned long long r =  (1ULL << i) - 1;
        printf("range at %d is 0x%016llx\n", i, r);
    }

    printf("val   is 0x%016llx\n", val);

    if (val > ((1ULL << bits) - 1)) {
        printf("val out of dynamic range\n");
    } else {
        printf("val is ok with dynamic range
    }

    if (val > ((1ULL << 64) - 1)) {
        printf("val out of static range\n");
    } else {
        printf("val is ok with static range\n");
    }
}
---------------- snip to get foo.c ----------------

Yielding output:

    [ snip ]

    range at 60 is 0x0fffffffffffffff
    range at 61 is 0x1fffffffffffffff
    range at 62 is 0x3fffffffffffffff
    range at 63 is 0x7fffffffffffffff
    range at 64 is 0x0000000000000000
    val   is 0x00000000abcd1234
    val out of dynamic range
    val is ok with static range

This is:

    jdl.com 1069 % gcc -v
    Using built-in specs.
    Target: i486-linux-gnu
    Configured with: ../src/configure -v
    --enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr
    --enable-shared --with-system-zlib --libexecdir=/usr/lib
    --without-included-gettext --enable-threads=posix --enable-nls
    --with-gxx-include-dir=/usr/include/c++/4.1.3 --program-suffix=-4.1
    --enable-__cxa_atexit --enable-clocale=gnu --enable-libstdcxx-debug
    --enable-mpfr --with-tune=i686 --enable-checking=release
    i486-linux-gnu
    Thread model: posix
    gcc version 4.1.3 20070629 (prerelease) (Debian 4.1.2-13)

Which seems at least reasonable, though perhaps wrong. :-)

> > -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;
> >  
> >  	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.

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

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

> but in any case input and output should match.

Oops.  That they should.

> 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.

Thanks,
jdl

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[David Gibson]

On Wed, Nov 07, 2007 at 08:14:29AM -0600, Jon Loeliger wrote:
> So, like, the other day David Gibson mumbled:
> > On Tue, Nov 06, 2007 at 04:19:19PM -0600, Jon Loeliger wrote:
> > > From: Jon Loeliger <jdl@freescale.com>
> > > 
> > > 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.
> 
> 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.

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

> > Having dtc be able to convert dts versions forward is easy and
> > useful.
> 
> Oh, I see.  Thank you.
> 
> > > 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.
> 
> > 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. :-)

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

> > Not really related changes, but whatever..
> 
> Uh, yeah.  Leftoverish.  Sorry.
> 
> 
> > > +/*
> > > + * 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)))
> > > +	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.

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.  So I guess (1 << 64) is just returning garbage - I
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))))

[snip]
> > > -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;
> > >  
> > >  	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.

Well... depends on how you think about it.  If you think of [] as a
list of really small integers, then it's inconsistent.  But, if you
think of [] as a way of doing really long hex literals..

> > We can argue about whether that's a good idea,
> > if you like,
> 
> 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 in any case input and output should match.
> 
> Oops.  That they should.
> 
> > 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.

-- 
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

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[Jon Loeliger]

David Gibson wrote:
> 
> 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.

>
>> 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.



>> 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.

> 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.

> 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? :-)

> So I guess we need
> 	else if ((errno == ERANGE)
> 		 || ((bits < 64) && (val >= (1ULL << bits))))

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


>> 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.

jdl

DTS Bytestrings Representation in /dts-v1/ files

[Jon Loeliger]

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

Re: DTS Bytestrings Representation in /dts-v1/ files

[Josh Boyer]

On Thu, 08 Nov 2007 13:18:50 -0600
Jon Loeliger <jdl@jdl.com> wrote:

> 
> 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?

My off-the-cuff opinion is to leave them as they are today.  They seem
to mostly be used for MAC addresses, and you don't really see a whole
lot of those with the 0x prefix before every number.

At the same time, inconsistency sucks.

josh

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[David Gibson]

On Thu, Nov 08, 2007 at 08:13:13AM -0600, Jon Loeliger wrote:
> David Gibson wrote:
> > 
> > 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.
> 
> >> 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.

Ok.

> >> 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.
> 
> > 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.
> 
> > 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? :-)

Actually I was working from home on an x86 machine when I wrote that,
so I think it must have been the -O0.

> > So I guess we need
> > 	else if ((errno == ERANGE)
> > 		 || ((bits < 64) && (val >= (1ULL << bits))))
> 
> Sounds good.  I'll commit --amend that into the patch!
> 
> 
> >> 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.

-- 
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

Re: DTS Bytestrings Representation in /dts-v1/ files

[David Gibson]

On Thu, Nov 08, 2007 at 01:18:50PM -0600, Jon Loeliger wrote:
> 
> 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 ];

Or, equivalently, like this:
	stuff = [0b3122deeaadbeef];

I think it's important to be aware of the more compact form when
considering whether to change the representation.

> 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

-- 
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

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[Jon Loeliger]

So, like, the other day David Gibson mumbled:
> 
> 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.


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?

Thanks,
jdl

Re: [PATCH] DTC: Polish up the DTS Version 1 implementation.

[David Gibson]

On Fri, Nov 09, 2007 at 08:32:57AM -0600, Jon Loeliger wrote:
> So, like, the other day David Gibson mumbled:
> > 
> > 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.
> 
> 
> 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.

Hrm.. I think I'm not getting all the allusions here.

> 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. :-)

Works for me.  And we even have some ideas on how to fix it, if we
have to, without too much horror, so that seems like a reasonable
position to me.

> And with that issue behind us....
> 
> I'm going to post these patches to introduce the new DTS format!
> Any last straggler comments?

Hurrah.  Let's do it!

-- 
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