[U-Boot-Users] [PATCH] Add Flat OF tree support.

[Wolfgang Denk]

In message <200509041132.18328.pantelis.antoniou at gmail.com> you wrote:
>
> > > +	/*
> > > +	 * Linux Kernel Parameters:
> > > +	 *   r3: ptr to board info data
> > > +	 *   r4: initrd_start or 0 if no initrd
> > > +	 *   r5: initrd_end - unused if r4 is 0
> > > +	 *   r6: Start of command line string
> > > +	 *   r7: End   of command line string
> > > +	 */
> > > +	(*kernel) ((bd_t *)of_flat_tree, initrd_start, initrd_end, cmd_start,
> > > cmd_end);
> >
> > Is it board info data or an OF tree? I think the comment needs to  be
> > adjusted to match the code.
> 
> It's both, after the OF tree the bd_t is pasted.

Where is this done? I don't see it. And "after" does not  help  -  an
old  kernel  is still expecting it right at the address pointed to by
r3.

> We'll have a nice transition period when both are present.

Please explain how this is  supposed  to  work.  You  code  does  not
contain  much  comments or explanations for the implementation (which
is IMHO bad), but at first glance I had the impression that with your
patch there are two situations:

1) CONFIG_OF_FLAT_TREE is not defined; we pass a bd_t to the  kernel,
   and everything is as before. Only kernels expecting an bd_t can be
   booted.

2) CONFIG_OF_FLAT_TREE is defined; we pass an OF tree to the  kernel.
   No  backward  compatibility is provided: kernels expecting an bd_t
   will crash miserably. Only kernels expecting an  OF  tree  can  be
   booted.

> The target is for bd_t to be gone completely.

Yes, but I don't see where the "transition period" is implemented.

I would expect that (when CONFIG_OF_FLAT_TREE is enabled) there is an
environment variable that allows to switch  between  boot  interfaces
(similar  to  the  clock_in_mhz  variable),  so that one and the same
U-Boot image can boot both older and newer kernels?

Am I missing something?

> > I understand that *all* of this is PPC specific, so why do you #ifdef
> > just this part? IMHO all of this should  disappear  for  all  non-PPC
> > versions?
> 
> There's nothing PPC specific about the patch. Endianess issues are handled.

Come on, be realistic. There is not the slightest hint  that  ARM  or
MIPS considered using this.

All this *is* PPC specific code only.

And by the way: the whole function do_bootm_linux()  you're  patching
is  already  enclosed  in "#ifdef CONFIG_PPC" - ARM and MIPS etc. use
different implementations of this function which you did not modify.

Also, in the rest of the code we use "#ifdef CONFIG_PPC",  so  please
use this consistently (instead of "#ifdef __powerpc__".

> OF trees are a superset of all the current ways that firmware passes
> settings to the kernel. In theory other architectures can use it for the
> same reason.

The key words here are "in theory". Do you have any information  that
there might be a realistic chance they would?


> > While we are at it: did you check how much this  patch  adds  to  the
> > memory footprint?
> 
> It is controlled by a define, CONFIG_OF_FLAT_TREE, so if you don't use it
> there's no overhead.

That was not the question.

> The difference between a build with and without was about 5.5K.
> About 1K was the OF tree proper, which will grow when we start populating 
> it with the device tree.

6.5k or typically 5 % of the total size of U-Boot, tendency growing?

Outch...

There was discussion that we could leave out the OFT builder and just
"load" a pre-built blob.Do you intend to implement this, too?


> > > +#if defined(CONFIG_MPC5xxx)
> > > +	p = ft_get_prop(blob, "/u-boot-bd_t/mbar_base", &len);
> > > +	if (p != NULL)
> > > +		*p = cpu_to_be32(bd->bi_mbar_base);
> > > +#endif
> >
> > ...
> >
> > > +#if defined(CONFIG_MPC5xxx)
> > > +
> > > +	p = ft_get_prop(blob, "/u-boot-bd_t/ipbfreq", &len);
> > > +	if (p != NULL)
> > > +		*p = cpu_to_be32(bd->bi_ipbfreq);
> > > +
> > > +	p = ft_get_prop(blob, "/u-boot-bd_t/pcifreq", &len);
> > > +	if (p != NULL)
> > > +		*p = cpu_to_be32(bd->bi_pcifreq);
> > > +#endif
> >
> > Maybe you can sort these things to minimize the  groups  of  #ifdef's
> > needed?
> 
> ???

Why must you have several blocks "#if defined(CONFIG_MPC5xxx)" ?  Put
all this code in a single block.

BTW: where is the name "u-boot-bd_t" coming from? I think it's  ugly.
Can we shorten this to either "u-boot" or "bd_t", please?


Now   you   submit    two    patches:    "of-uboot-base.patch"    and
"of-uboot-stxxtc.patch"  and  a  separate  "of-uboot-changelog.patch"
which does not mention a bit about what the second patch does.

Each patch needs it's own description and it's  own  CHANGELOG  entry
(commit  message).  Also,  as  documented  by  the  RAEDME, CHANGELOG
entries are required as plain text.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I can't understand it. I can't even understand  the  people  who  can
understand it.                    - Queen Juliana of the Netherlands.