[PATCH] [v3] add array parsing to dts

[Ward Vandewege]

With a lot of help from Stepan at the summit, this patch adds array parsing
support to the dts, specifically for the 'unwanted_vpci' field.

The code is a bit of a hack - it matches against the field name to change
interpretation to array mode. Ideally we'd add some more type information
after the parsing of the dts, and use that to write out statictree.c.

The dts parsing/generation code needs some love, so I'm not too worried about
the hackishness of this patch at this point. But if someone wants to improve
it, be my guest :)

Thanks,
Ward.

[Carl-Daniel Hailfinger]

On 10.04.2008 02:51, Ward Vandewege wrote:
> With a lot of help from Stepan at the summit, this patch adds array parsing
> support to the dts, specifically for the 'unwanted_vpci' field.
>
> The code is a bit of a hack - it matches against the field name to change
> interpretation to array mode. Ideally we'd add some more type information
> after the parsing of the dts, and use that to write out statictree.c.
>
> The dts parsing/generation code needs some love, so I'm not too worried about
> the hackishness of this patch at this point. But if someone wants to improve
> it, be my guest :)

It's great to have working code to demonstrate an idea.

However, I strongly disagree with the hardcoding of the string
"unwanted_vpci" in several places in the code. Can't you use the
"must_be_cells" designation in the prop_checker_table in
util/dtc/livetree.c to handle this transparently?

Regards,
Carl-Daniel

[Ward Vandewege]

On Thu, Apr 10, 2008 at 03:51:15AM +0200, Carl-Daniel Hailfinger wrote:
> On 10.04.2008 02:51, Ward Vandewege wrote:
> > With a lot of help from Stepan at the summit, this patch adds array parsing
> > support to the dts, specifically for the 'unwanted_vpci' field.
> >
> > The code is a bit of a hack - it matches against the field name to change
> > interpretation to array mode. Ideally we'd add some more type information
> > after the parsing of the dts, and use that to write out statictree.c.
> >
> > The dts parsing/generation code needs some love, so I'm not too worried about
> > the hackishness of this patch at this point. But if someone wants to improve
> > it, be my guest :)
>
> It's great to have working code to demonstrate an idea.
>
> However, I strongly disagree with the hardcoding of the string
> "unwanted_vpci" in several places in the code. Can't you use the
> "must_be_cells" designation in the prop_checker_table in
> util/dtc/livetree.c to handle this transparently?

Actually, the proper way to do it is to look at what we're parsing in the dts
files, store the type of the field in the 'data' struct, and write out the
proper statictree.s and statictree.h files based on that.

The attached patch does just that.

Note that for 'byte' properties, it just printes 'UNIMPLEMENTED, FIXME',
which will break the statictree output files. We don't use 'byte' properties
right now, and I'm not sure what to write out. Suggestions?

Thanks,
Ward.

[Peter Stuge]

On Sat, Apr 12, 2008 at 10:11:18PM -0400, Ward Vandewege wrote:
> ..byte properties..

I guess they should be unsigned chars?
> Index: flattree.c
>
> --- util/dtc/flattree.c	(revision 656)
> +++ util/dtc/flattree.c	(working copy)
> @@ -452,9 +452,24 @@
>    	return;
>
> cleanname = clean(p->name, 1);
> fprintf(f, "\t.%s = ", cleanname);
> if (d.type == 'S') {
> // Standard property (scalar)
> fprintf(f, "\t.%s = ", cleanname);
> fprintf(f, "0x%lx,\n", strtoul((char *)d.val, 0, 0));
> } else if (d.type == 'C') {
> // 'Cell' property (array of 4-byte elements)
> fprintf(f, "\t.%s = {\n", cleanname, d.len/4);

Why the last parameter d.len/4? It's not used, right?
> int i;
>  for (i = 0; (i < d.len) && (0 != *(u32 *)(d.val+i)); i = i+4) {
> 	fprintf(f, "\t\t[%d] = 0x%08X,\n",i/4,*(u32 *)(d.val+i));
> } 

Looks like there's some strange whitespace here.
> fprintf(f, "\t\t[%d] = 0x00000000,\n",i/4);	// Make sure to end our array with a zero element

Perhaps use 0x0 or 0 to show that this last entry is not generated
the same way as the previous ones.
> @@ -785,7 +800,16 @@
>    		if (streq(prop->name, "device_operations")) /* this is special */
>    			continue;
>    		cleanname = clean(prop->name, 0);
> 	fprintf(f, "\tu32 %s;\n", cleanname);
> 	if (prop->val.type == 'S') {
> 		// Standard property, scalar
> 		fprintf(f, "\tu32 %s;\n", cleanname);
> 	} else if (prop->val.type == 'C') {
> 		// 'Cell' property (array of 4-byte elements)
> 		fprintf(f, "\tu32 %s[];\n", cleanname);

Will this always work? An empty array like this must be last in the
containing struct, and there can only ever be one in each struct.

//Peter

[Ward Vandewege]

On Wed, Apr 16, 2008 at 05:02:47AM +0200, Peter Stuge wrote:
> On Sat, Apr 12, 2008 at 10:11:18PM -0400, Ward Vandewege wrote:
> > ..byte properties..
>
> I guess they should be unsigned chars?

Maybe - the DTS syntax suggests this is an array; so an array of unsigned
chars? The example in the test.dts file in util/dtc goes like this:

randomnode {
    string = "\xff\0stuffstuff\t\t\t\n\n\n";
    blob = [0a 0b 0c 0d de ea ad be ef];
    ref = < &/memory@0 >;
  };

That 'blob' property is what's called a 'byte' property.
> > Index: flattree.c
> >
> > --- util/dtc/flattree.c	(revision 656)
> > +++ util/dtc/flattree.c	(working copy)
> > @@ -452,9 +452,24 @@
> >    	return;
> >
> > cleanname = clean(p->name, 1);
> > fprintf(f, "\t.%s = ", cleanname);
> > if (d.type == 'S') {
> > // Standard property (scalar)
> > fprintf(f, "\t.%s = ", cleanname);
> > fprintf(f, "0x%lx,\n", strtoul((char *)d.val, 0, 0));
> > } else if (d.type == 'C') {
> > // 'Cell' property (array of 4-byte elements)
> > fprintf(f, "\t.%s = {\n", cleanname, d.len/4);
>
> Why the last parameter d.len/4? It's not used, right?

Right, that was leftover from an earlier version of the patch. Dropped.
> > int i;
> >  for (i = 0; (i < d.len) && (0 != *(u32 *)(d.val+i)); i = i+4) {
> > 	fprintf(f, "\t\t[%d] = 0x%08X,\n",i/4,*(u32 *)(d.val+i));
> > } 
>
> Looks like there's some strange whitespace here.

Good catch, fixed.
> > fprintf(f, "\t\t[%d] = 0x00000000,\n",i/4);	// Make sure to end our array with a zero element
>
> Perhaps use 0x0 or 0 to show that this last entry is not generated
> the same way as the previous ones.

Good idea, done.
> > @@ -785,7 +800,16 @@
> >    		if (streq(prop->name, "device_operations")) /* this is special */
> >    			continue;
> >    		cleanname = clean(prop->name, 0);
> > 	fprintf(f, "\tu32 %s;\n", cleanname);
> > 	if (prop->val.type == 'S') {
> > 		// Standard property, scalar
> > 		fprintf(f, "\tu32 %s;\n", cleanname);
> > 	} else if (prop->val.type == 'C') {
> > 		// 'Cell' property (array of 4-byte elements)
> > 		fprintf(f, "\tu32 %s[];\n", cleanname);
>
> Will this always work? An empty array like this must be last in the
> containing struct, and there can only ever be one in each struct.

Ah, that's a good point. Since we know at this point how long the array is
going to be, I've just added that to the struct definition.

Is the attached better?

Thanks,
Ward.

add array parsing to dts

[Jordan Crouse]

On 12/04/08 22:11 -0400, Ward Vandewege wrote:
> On Thu, Apr 10, 2008 at 03:51:15AM +0200, Carl-Daniel Hailfinger wrote:
> > On 10.04.2008 02:51, Ward Vandewege wrote:
> > > With a lot of help from Stepan at the summit, this patch adds array parsing
> > > support to the dts, specifically for the 'unwanted_vpci' field.
> > >
> > > The code is a bit of a hack - it matches against the field name to change
> > > interpretation to array mode. Ideally we'd add some more type information
> > > after the parsing of the dts, and use that to write out statictree.c.
> > >
> > > The dts parsing/generation code needs some love, so I'm not too worried about
> > > the hackishness of this patch at this point. But if someone wants to improve
> > > it, be my guest :)
> >
> > It's great to have working code to demonstrate an idea.
> >
> > However, I strongly disagree with the hardcoding of the string
> > "unwanted_vpci" in several places in the code. Can't you use the
> > "must_be_cells" designation in the prop_checker_table in
> > util/dtc/livetree.c to handle this transparently?
>
> Actually, the proper way to do it is to look at what we're parsing in the dts
> files, store the type of the field in the 'data' struct, and write out the
> proper statictree.s and statictree.h files based on that.
>
> The attached patch does just that.
>
> Note that for 'byte' properties, it just printes 'UNIMPLEMENTED, FIXME',
> which will break the statictree output files. We don't use 'byte' properties
> right now, and I'm not sure what to write out. Suggestions?
>
> Thanks,
> Ward.
> Add generic array support to the coreboot dts output code.
>
> This is necessary for the 'unwanted_vpci' field on geode-based boards.
>
> Signed-off-by: Ward Vandewege

If this is the same patch as
http://www.coreboot.org/pipermail/coreboot/2008-April/033385.html
Then Acked-by: Jordan Crouse

If not, then I ack that patch. :)
> Index: flattree.c
>
> --- util/dtc/flattree.c	(revision 656)
> +++ util/dtc/flattree.c	(working copy)
> @@ -452,9 +452,24 @@
>    	return;
>
> cleanname = clean(p->name, 1);
> fprintf(f, "\t.%s = ", cleanname);
> if (d.type == 'S') {
> // Standard property (scalar)
> fprintf(f, "\t.%s = ", cleanname);
> fprintf(f, "0x%lx,\n", strtoul((char *)d.val, 0, 0));
> } else if (d.type == 'C') {
> // 'Cell' property (array of 4-byte elements)
> fprintf(f, "\t.%s = {\n", cleanname, d.len/4);
> int i;
>  for (i = 0; (i < d.len) && (0 != *(u32 *)(d.val+i)); i = i+4) {
> 	fprintf(f, "\t\t[%d] = 0x%08X,\n",i/4,*(u32 *)(d.val+i));
> } 
> fprintf(f, "\t\t[%d] = 0x00000000,\n",i/4);	// Make sure to end our array with a zero element
> fprintf(f, "\t},\n");
> } else if (d.type == 'B') {
> fprintf(f, "\tUNIMPLEMENTED: FIXME\n");
> }
> free(cleanname);
> fprintf(f, "0x%lx,\n", strtoul((char *)d.val, 0, 0));
>
> #if 0
>    /* sorry, but right now, u32 is all you get */
>    fprintf(f, "0");
> @@ -785,7 +800,16 @@
>    		if (streq(prop->name, "device_operations")) /* this is special */
>    			continue;
>    		cleanname = clean(prop->name, 0);
> 	fprintf(f, "\tu32 %s;\n", cleanname);
> 	if (prop->val.type == 'S') {
> 		// Standard property, scalar
> 		fprintf(f, "\tu32 %s;\n", cleanname);
> 	} else if (prop->val.type == 'C') {
> 		// 'Cell' property (array of 4-byte elements)
> 		fprintf(f, "\tu32 %s[];\n", cleanname);
> 	} else if (prop->val.type == 'B') {
> 		// Byte property
> 		fprintf(f, "\tUNIMPLEMENTED: FIXME\n");
> 	}
> free(cleanname);
>
> }
> Index: data.c
>
> --- util/dtc/data.c	(revision 656)
> +++ util/dtc/data.c	(working copy)
> @@ -64,6 +64,7 @@
>    nd.asize = newsize;
>    nd.val = xrealloc(d.val, newsize);
>    nd.len = d.len;
>
> nd.type = d.type;
> nd.refs = d.refs;
>
> assert(nd.asize >= (d.len + xlen));
>
> @@ -199,7 +200,11 @@
>
> struct data data_append_cell(struct data d, cell_t word)
>  {
> cell_t beword = cpu_to_be32(word);
>
> // Don't do system/network order byte translation. We don't do it for scalars either.
>
> //cell_t beword = cpu_to_be32(word);
>
> cell_t beword = word;
>
> // Mark this property as being of the 'cell' type
>
> d.type = 'C';
>
> return data_append_data(d, &beword, sizeof(beword));
>
> }
> @@ -223,6 +228,8 @@
>
> struct data data_append_byte(struct data d, uint8_t byte)
>  {
> // Mark this property as being of the 'byte' type
> d.type = 'B';
> return data_append_data(d, &byte, 1);
>
> }
> Index: livetree.c
>
> --- util/dtc/livetree.c	(revision 656)
> +++ util/dtc/livetree.c	(working copy)
> @@ -32,6 +32,9 @@
>
> new->name = name;
>    new->val = val;
>
> if (new->val.type == NULL) {
> new->val.type = 'S'; // Default to 'scalar' type; if this is a cell or byte value, type will already be set
>
> }
>
> new->next = NULL;
>
> @@ -301,7 +304,9 @@
>  } prop_checker_table[] = {
>    {"name", must_be_string},
>    {"name", name_prop_check},
> -/* we don't care about these things now  -- we think */
> /* unwanted_vpci must be a cells field (i.e. an array) */
> {"unwanted_vpci", must_be_cells},
> /* we don't care about these things now  -- we think */
> {"linux,phandle", must_be_one_cell},
> {"#address-cells", must_be_one_cell},
> {"#size-cells", must_be_one_cell},
> Index: dtc.h
>
> --- util/dtc/dtc.h	(revision 656)
> +++ util/dtc/dtc.h	(working copy)
> @@ -105,6 +105,7 @@
>  struct data {
>    int len;
>    unsigned char *val;
> unsigned char type;
> int asize;
> struct fixup *refs;
>
> };
>

add array parsing to dts

[Ward Vandewege]

On Thu, Apr 17, 2008 at 10:01:38AM -0600, Jordan Crouse wrote:
> > Add generic array support to the coreboot dts output code.
> >
> > This is necessary for the 'unwanted_vpci' field on geode-based boards.
> >
> > Signed-off-by: Ward Vandewege
>
> If this is the same patch as
> http://www.coreboot.org/pipermail/coreboot/2008-April/033385.html
> Then Acked-by: Jordan Crouse

It is the same patch.

r661

Thanks!
Ward.