Re: [coreboot] dt compiler patch

[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