dt compiler patch

[ron minnich]

This has been tested and builds a working boot-to-linux coreboot on
the dbe62 (old style dts) and builds a serengeti target (new dts, but
that new dts not submitted yet).

ron

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of ron minnich
> Sent: Monday, November 24, 2008 11:06 AM
> To: Coreboot
> Subject: [coreboot] dt compiler patch
>
> This has been tested and builds a working boot-to-linux coreboot on
> the dbe62 (old style dts) and builds a serengeti target (new dts, but
> that new dts not submitted yet).

Before and after your patch statictree.c is identical without dts changes.
That seems like a good thing.  I'm interested in a modified dts to try it
on.

I know I should just generate one, but I'm feeling lazy.

Thanks,
Myles

[ron minnich]

see other note. I am glad they are the same -- that's important.

ron

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of ron minnich
> Sent: Monday, November 24, 2008 11:06 AM
> To: Coreboot
> Subject: [coreboot] dt compiler patch
>
> This has been tested and builds a working boot-to-linux coreboot on
> the dbe62 (old style dts) and builds a serengeti target (new dts, but
> that new dts not submitted yet).
>
> ron

Here's my first pass.

Thanks,
Myles
> I took the opportunity to do some cleanup.
>
> Signed-off-by: Ronald G. Minnich 
> Index: util/dtc/flattree.c
> ===================================================================
> --- util/dtc/flattree.c	(revision 1046)
> +++ util/dtc/flattree.c	(working copy)
> @@ -443,10 +449,7 @@
>  {
>    struct data d = p->val;
>    FILE *f = e;
> int i;
> char *cleanname;
> int vallen = d.len > 4 ? 4 : d.len;

Lets take out the commented code that uses these variables at the same time.
> /* nothing to do? */
>    if (d.len == 0)
>    	return;
> @@ -865,7 +875,7 @@
>
> if (tree->config){
>    	int i;
> -//		treename = clean(tree->label, 0);
> +//		treename = clean(tree->dtslabel, 0);

Maybe just remove this line.
> treename = toname(tree->config->label, "_config");
> for(i = 0; i < emitted_names_count; i++)
> 	if (!strcmp(treename, emitted_names[i]))
> Index: util/dtc/livetree.c
>
> --- util/dtc/livetree.c	(revision 1046)
> +++ util/dtc/livetree.c	(working copy)
> @@ -43,6 +44,74 @@
>    return new;
>  }
>
> +/* XXX HACK */
> +void move_proplist_to_config(struct node *f)
> +{
> struct node *n = f;
> while (n) {
> n->config = n->proplist; 
> n->proplist = NULL;
> move_proplist_to_config(n->children);
> n = n->next_sibling;
> }
>
> +}
>
> +/**
> Given a set of nodes from a chip dts, and a set of nodes from the

mainboard dts,
> update the chip dts nodes with values from the mainboard dts.
> All this work is so that a chip dts can set default values and a

mainboard
> can override them.
> Node definitions come from the chip dts.
> These definitions have properties that are used in turn to create the
> C structures.
> Changing the vlaues of properties in the node definitions is done by
> creating nodes in the mainboard dts.
> For each of the node defintions from 'config', we
> have to match the node definitions in 'subnodes',
> that have matching labels. For each matching label, we modify the
> property settings for the node in config.
> Finally, for all subnodes that are leftover, i.e. did not match

anything in the
> config, we simply append them to the list of subnodes from config.
> Later, it might be an error to have a missing match.
> */
>
> +void
> +fixup_properties(struct node *chipnodes, struct node *mainboardnodes)
> +{
> struct node *c;
> struct node *m = mainboardnodes;
> struct node *del = NULL;
> struct node *head = mainboardnodes;
> while (m) {
> for (c = chipnodes; c; c = c->next) {
> 	if (strcmp(c->dtslabel, m->dtslabel)) {
> 		continue;
> 	}
> 	/* match */
> 	/* the chip properties are in c->config list
> 	 * the dts properties are in the m->proplist. 
> 	 * So copy the mainboard properties to the chip

proplist.
> 	 */
> 	c->proplist = m->proplist;

This is a replacement, shouldn't it be an append?
> 	c->children = m->children;

Again, this is a replacement, shouldn't it be an append?
> 	/* since we matched this node we have to delete it.

*/

The delete function says you can't have children, so the children link needs
to be NULL here.
> 	del = m;
> 	break;
> }
> /* must track head node for append_node below. */
> if (m == head)
> 	head = m->next;
> m = m->next;
> if (del)
> 	del_node(del);
> del = NULL;
> }
> /* backwards compatibility: anything left gets appended to the chip

list */
> if (head)
> append_node(chipnodes, head);
>
> +}
>
> struct property *chain_property(struct property *first, struct property

*list)
> {
>    assert(first->next == NULL);
> @@ -51,12 +120,9 @@
>    return first;      
>  }
>
> -struct node *build_node(struct property *config, struct property

*proplist, struct node *children)
> +struct node *new_node(struct property *config, struct property *proplist,

struct node *children)
> {
>
> static struct node *last_node = NULL;
>
> struct node *new = xmalloc(sizeof(*new));
>
> struct node *child;
>
> memset(new, 0, sizeof(*new));
>
> @@ -64,6 +130,39 @@
>    new->proplist = proplist;
>    new->children = children;
> return new;
>
> +}
>
> +/* we should only ever delete simple nodes: nodes with no kids. */
> +void del_node(struct node *node)
> +{
> struct node *n;
> assert(! node->children);
> if (first_node == node) {
> first_node = node->next;
> free(node);
> return;
> }
> for_all_nodes(n) {
> if (n->next != node)
> 	continue;
> n->next = node->next;
> n->next_sibling = node->next_sibling;

Will this always be true?  It seems like you need to go through again to do
the sibling links right.
> if (node == last_node)
> 	last_node = n;
> break;
> }
> free(node);
>
> +}
>
> +struct node *build_node(struct property *config, struct property

*proplist, struct node *children)
> +{
> struct node *new;
> struct node *child;
> new = new_node(config, proplist, children);
> for_each_child(new, child) {
>   child->parent = new;
>  }
>
> @@ -85,7 +184,7 @@
>
> node->name = name;
> node->label = label;
>
> node->dtslabel = label;
>
> return node;
>
> }
> @@ -98,6 +197,16 @@
>    return first;
>  }

Do we guarantee that you will never be appending to a NULL list (first ==
NULL), I didn't see that check.
> +struct node *append_node(struct node *first, struct node *list)
> +{
> struct node *n = first;
> while(n->next_sibling) {
> n = n->next_sibling;
> }
> n->next_sibling = list;
> return first;
>
> +}
>
> void add_property(struct node *node, struct property *prop)
>  {
>    struct property **p;

[ron minnich]

On Tue, Nov 25, 2008 at 6:28 AM, Myles Watson  wrote:
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]]
> > On Behalf Of ron minnich
> > Sent: Monday, November 24, 2008 11:06 AM
> > To: Coreboot
> > Subject: [coreboot] dt compiler patch
> >
> > This has been tested and builds a working boot-to-linux coreboot on
> > the dbe62 (old style dts) and builds a serengeti target (new dts, but
> > that new dts not submitted yet).
> >
> > ron
>
> Here's my first pass.
>
> Thanks,
> Myles
> > I took the opportunity to do some cleanup.
> >
> > Signed-off-by: Ronald G. Minnich 
> > Index: util/dtc/flattree.c
> > ===================================================================
> > --- util/dtc/flattree.c       (revision 1046)
> > +++ util/dtc/flattree.c       (working copy)
> > @@ -443,10 +449,7 @@
> >  {
> >       struct data d = p->val;
> >       FILE *f = e;
> > int i;
> > char *cleanname;
> > int vallen = d.len > 4 ? 4 : d.len;
>
> Lets take out the commented code that uses these variables at the same time.

done
> >   /* nothing to do? */
> >   if (d.len == 0)
> >           return;
> >
> > @@ -865,7 +875,7 @@
> >   if (tree->config){
> >           int i;
> >
> > -//           treename = clean(tree->label, 0);
> > +//           treename = clean(tree->dtslabel, 0);
>
> Maybe just remove this line.

done
> >           treename = toname(tree->config->label, "_config");
> >           for(i = 0; i < emitted_names_count; i++)
> >                   if (!strcmp(treename, emitted_names[i]))
> > Index: util/dtc/livetree.c
> >
> > --- util/dtc/livetree.c       (revision 1046)
> > +++ util/dtc/livetree.c       (working copy)
> >                 /* match */
> >                 /* the chip properties are in c->config list
> >                  * the dts properties are in the m->proplist.
> >                  * So copy the mainboard properties to the chip
>
> proplist.
> >                  */
> >                 c->proplist = m->proplist;
>
> This is a replacement, shouldn't it be an append?

no, because at this point, c->proplist is empty. c->configlist has the
properties.
> >                 c->children = m->children;
>
> Again, this is a replacement, shouldn't it be an append?

ah, there is a question. Yes, it should be. Fixed.

Let's see what it breaks for you :-)
> >                 /* since we matched this node we have to delete it.
>
> */
>
> The delete function says you can't have children, so the children link needs
> to be NULL here.

Fixed.
> > +/* we should only ever delete simple nodes: nodes with no kids. */
> > +void del_node(struct node *node)
> > +{
> > struct node *n;
> > assert(! node->children);
> > if (first_node == node) {
> >         first_node = node->next;
> >         free(node);
> >         return;
> > }
> > for_all_nodes(n) {
> >         if (n->next != node)
> >                 continue;
> >         n->next = node->next;
> >         n->next_sibling = node->next_sibling;
>
> Will this always be true?  It seems like you need to go through again to do
> the sibling links right.

I don't think so because, at this point, n is prev(node to be deleted).

so I am setting prev(node)->next and next_sibling to node->next etc.
Am I missing something?
> Do we guarantee that you will never be appending to a NULL list (first ==
> NULL), I didn't see that check.

Here's my thinking on this. The guarantee is that there is always a
root node -- we don't ever remove that.

New diff attached, with issue from your next email managed as well.

Signed-off-by: Ronald G. Minnich

[Myles Watson]

> > > +/* we should only ever delete simple nodes: nodes with no kids. */
> > > +void del_node(struct node *node)
> > > +{
> > > struct node *n;
> > > assert(! node->children);
> > > if (first_node == node) {
> > >         first_node = node->next;
> > >         free(node);
> > >         return;
> > > }
> > > for_all_nodes(n) {
> > >         if (n->next != node)
> > >                 continue;
> > >         n->next = node->next;
> > >         n->next_sibling = node->next_sibling;
> >
> > Will this always be true?  It seems like you need to go through again to
>
> do
> > the sibling links right.
>
> I don't think so because, at this point, n is prev(node to be deleted).
>
> so I am setting prev(node)->next and next_sibling to node->next etc.
> Am I missing something?

I was thinking about the case where n wasn't the sibling of the node being
deleted.  For example, a first child being deleted.

It seems like you should at least check that n->next_sibling was node before
setting it to node->next_sibling.
> > Do we guarantee that you will never be appending to a NULL list (first
>
> ==
> > NULL), I didn't see that check.
>
> Here's my thinking on this. The guarantee is that there is always a
> root node -- we don't ever remove that.

I guess I'm confused here.  I was talking about when we call it from
fixup_properties with chipnodes as the list.  It looks like we could delete
all of them.  Do I have it backward?

thanks,
Myles
> New diff attached, with issue from your next email managed as well.
>
> Signed-off-by: Ronald G. Minnich

[ron minnich]

On Tue, Nov 25, 2008 at 8:19 AM, Myles Watson  wrote:
> > > > +/* we should only ever delete simple nodes: nodes with no kids. */
> > > > +void del_node(struct node *node)
> > > > +{
> > > > struct node *n;
> > > > assert(! node->children);
> > > > if (first_node == node) {
> > > >         first_node = node->next;
> > > >         free(node);
> > > >         return;
> > > > }
> > > > for_all_nodes(n) {
> > > >         if (n->next != node)
> > > >                 continue;
> > > >         n->next = node->next;
> > > >         n->next_sibling = node->next_sibling;
> > >
> > > Will this always be true?  It seems like you need to go through again to
> >
> > do
> > > the sibling links right.
> >
> > I don't think so because, at this point, n is prev(node to be deleted).
> >
> > so I am setting prev(node)->next and next_sibling to node->next etc.
> > Am I missing something?
>
> I was thinking about the case where n wasn't the sibling of the node being
> deleted.  For example, a first child being deleted.
>
> It seems like you should at least check that n->next_sibling was node before
> setting it to node->next_sibling.

I have code whiteout, can you write this correctly for me :-)
> > > Do we guarantee that you will never be appending to a NULL list (first
> >
> > ==
> > > NULL), I didn't see that check.
> >
> > Here's my thinking on this. The guarantee is that there is always a
> > root node -- we don't ever remove that.
>
> I guess I'm confused here.  I was talking about when we call it from
> fixup_properties with chipnodes as the list.  It looks like we could delete
> all of them.  Do I have it backward?

yes. We should only ever delete mainboard nodes, not chip nodes.

ron

[Myles Watson]

On Tue, Nov 25, 2008 at 9:22 AM, ron minnich  wrote:
> On Tue, Nov 25, 2008 at 8:19 AM, Myles Watson  wrote:
> > > > > +/* we should only ever delete simple nodes: nodes with no kids. */
> > > > > +void del_node(struct node *node)
> > > > > +{
> > > > > struct node *n;
> > > > > assert(! node->children);
> > > > > if (first_node == node) {
> > > > >         first_node = node->next;
> > > > >         free(node);
> > > > >         return;
> > > > > }
> > > > > for_all_nodes(n) {
> > > > >         if (n->next != node)
> > > > >                 continue;
> > > > >         n->next = node->next;
> > > > >         n->next_sibling = node->next_sibling;
> > > >
> > > > Will this always be true?  It seems like you need to go through again
>
> to
> > > do
> > > > the sibling links right.
> > >
> > > I don't think so because, at this point, n is prev(node to be deleted).
> > >
> > > so I am setting prev(node)->next and next_sibling to node->next etc.
> > > Am I missing something?
> >
> > I was thinking about the case where n wasn't the sibling of the node
>
> being
> > deleted.  For example, a first child being deleted.
> >
> > It seems like you should at least check that n->next_sibling was node
>
> before
> > setting it to node->next_sibling.
>
> I have code whiteout, can you write this correctly for me :-)

I don't know if it's correct, but here's a first try:

/* we should only ever delete simple nodes: nodes with no kids. */
void del_node(struct node *node)
{
    struct node *n, *s;
    assert(! node->children);
    if (first_node == node) {
        first_node = node->next;
        free(node);
        return;
    }

/* If the node is the first child, set parent->children link. */
    if (node->parent->children == node)
        node->parent->children = node->next_sibling;
    else {
        /* Remove from sibling list. */
        for (n=node->parent->children; n->next_sibling;
             n=n->next_sibling)
            if (n->next_sibling == node)
                break;
        n->next_sibling = node->next_sibling;
    }

for_all_nodes(n) {
        if (n->next != node)
            continue;
        n->next = node->next;
        if (node == last_node)
            last_node = n;
        break;
    }
    free(node);
}

It doesn't check to see if the child is really a child of its parent.  I
figured that was a given.
> > > > Do we guarantee that you will never be appending to a NULL list (first
> > >
> > > ==
> > > > NULL), I didn't see that check.
> > >
> > > Here's my thinking on this. The guarantee is that there is always a
> > > root node -- we don't ever remove that.
> >
> > I guess I'm confused here.  I was talking about when we call it from
> > fixup_properties with chipnodes as the list.  It looks like we could
>
> delete
> > all of them.  Do I have it backward?
>
> yes. We should only ever delete mainboard nodes, not chip nodes.

Right.

Thanks,

Myles

[ron minnich]

looks right to me.

how close are we to committing all this stuff?

ron

[Myles Watson]

On Tue, Nov 25, 2008 at 10:01 AM, ron minnich  wrote:
> looks right to me.
>
> how close are we to committing all this stuff?

You mean because patches on patches are getting unwieldy :) ?

I'm comfortable with the idea that this is the way we want to go, but it
doesn't work for me yet.  We could commit to make patching easier.  I guess
it's up to you how functional you want it before committing.

Thanks,
Myles

[ron minnich]

On Tue, Nov 25, 2008 at 9:04 AM, Myles Watson  wrote:
> I'm comfortable with the idea that this is the way we want to go, but it
> doesn't work for me yet.  We could commit to make patching easier.  I guess
> it's up to you how functional you want it before committing.

We'll commit when
1. it works for you
2. I can build a working dbe62 target (which I still can)

You have the token for now.

ron

[Myles Watson]

On Tue, Nov 25, 2008 at 10:06 AM, ron minnich  wrote:
> On Tue, Nov 25, 2008 at 9:04 AM, Myles Watson  wrote:
> > I'm comfortable with the idea that this is the way we want to go, but it
> > doesn't work for me yet.  We could commit to make patching easier.  I
>
> guess
> > it's up to you how functional you want it before committing.
>
> We'll commit when
> it works for you
> I can build a working dbe62 target (which I still can)
>
> You have the token for now.

I'm not sure that's wise.  I'm breaking more than fixing right now.

Here's the latest snippet:
  DTC     mainboard/amd/serengeti/dts (dts->lbh)
name_node: pci@0,0(usb11)
name_node: pci@0,1(usb12)
name_node: pci@0,2(usb2)
name_node: pci@1,0(nic)
name_node: pci@0,0(multilevel_root)
name_node: pci@1,0(lpc)
name_node: pci@1,1(ide)
name_node: pci@1,2(smbus)
name_node: pci@1,3(acpi)
name_node: pci@1,5(ac97audio)
name_node: pci@1,6(ac97modem)
name_node: pci@1,7(dev7)
name_node: NULL(nic)
name_node: NULL(ide)
name_node: ioport@2e(NULL)
name_node: NULL(lpc)
fixup_properties: before fixups mainboard tree nic()
NULL(nic)
fixup_properties: before fixups chipnodes tree multilevel_root(pci@0,0)
pci@0,0(multilevel_root)
 pci@0,0(usb11)
 pci@0,1(usb12)
 pci@0,2(usb2)
 pci@1,0(nic)
fixup_properties: checking nic() and multilevel_root(pci@0,0)
fixup_properties: checking nic() and lpc(pci@1,0)
fixup_properties: checking nic() and ide(pci@1,1)
fixup_properties: checking nic() and smbus(pci@1,2)
fixup_properties: checking nic() and acpi(pci@1,3)
fixup_properties: checking nic() and ac97audio(pci@1,5)
fixup_properties: checking nic() and ac97modem(pci@1,6)
fixup_properties: checking nic() and dev7(pci@1,7)
fixup_properties: checking nic() and nic()
fixup_properties: last_child NULL()
fixup_properties: matched and deleting nic((null))
del_node: matched and deleting nic((null))
del_node: expect seg fault nic((null))
/bin/sh: line 1:  3615 Segmentation fault
/home/myles/buildrom/try/buildrom/buildrom-devel/work/coreboot-v3/svn/build/util/dtc/dtc
-O lbh mainboard/amd/serengeti/dts >/tmp/statictree.h.$
make: ***
[/home/myles/buildrom/try/buildrom/buildrom-devel/work/coreboot-v3/svn/build/statictree.h]
Error 139

The "expect seg fault" line means that del_node got passed a node with no
parent.  That means I don't understand enough what's going on here.  I was
expecting the parent to be set to pci@0,0 (from the mainboard dts), but it
hasn't been made yet (no name_node for it yet)

Help me understand.  Should we just take out the sibling links all together
from del?  Have they not been added yet?

Thanks,
Myles
> ron

[Myles Watson]

One more:
> +void
> +fixup_properties(struct node *chipnodes, struct node *mainboardnodes)
> +{
> struct node *c;
> struct node *m = mainboardnodes;
> struct node *del = NULL;
> struct node *head = mainboardnodes;
> while (m) {
> for (c = chipnodes; c; c = c->next) {
> 	if (strcmp(c->dtslabel, m->dtslabel)) {

This strcmp seg faults if the node wasn't labeled.  I'd suggest:
> 	if (!c->dtslabel || !m->dtslabel ||

strcmp(c->dtslabel, m->dtslabel)) {
> 		continue;
> 	}