[U-Boot-Users] [PATCH] Add Flat OF tree support.
Wolfgang Denk
wd at denx.de
Sun Sep 4 11:36:39 CEST 2005
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.
More information about the U-Boot
mailing list