[U-Boot] OMAP (4) boot_params

[Tom Rini]

On Wed, Apr 03, 2013 at 05:34:18PM +0200, Albert ARIBAUD wrote:
> Hi Michael,
> 
> On Wed, 3 Apr 2013 10:59:23 -0400, Michael Cashwell
> <mboards at prograde.net> wrote:
> 
> > >>>> ...Making that:
> > >>>> 
> > >>>> u32 *boot_params_ptr __attribute__ ((section(".data")));
> > >> 
> > >> Yes, that was my thinking too. Surely clearing data after code has set
> > >> it can't be right.
> > > 
> > > With all due respect, the documentation can with greater legitimately
> > > turn your admonition around and ask that you please refrain from
> > > setting BSS or data variables when the C runtime environment has not
> > > been set. :)
> > > 
> > > IOW, what is wrong here is writing to a BSS variable before you're
> > > allowed to as per the rules under which your code is running.
> > 
> > I think we're in agreement, but it's not my code doing it. The code,
> > as it exists in mainline is writing early to space in bss. My change
> > avoids that by moving the variable from the default bss to data:
> 
> ... except, as I said above, at this point your code should not write at
> all, be int in BSS or data, until the C environment is set up. So...

But we have to save this ROM-passed information before we overwrite it
ourselves (by accident or purpose).

> 
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index 6715e0d..1d84535
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -42,7 +42,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >  #define CONFIG_SYS_MONITOR_LEN (200 * 1024)
> >  #endif
> >  
> > -u32 *boot_params_ptr = NULL;
> > +u32 *boot_params_ptr __attribute__ ((section(".data")));
> >  struct spl_image_info spl_image;
> 
> ... NAK. Place this in a fixed section that you'll map somewhere else
> then in BSS or data.
> 
> Also: in the future, avoid pasting a diff directly in a mail to the
> u-boot list if it is not a real patch submission, as our patchwork
> instance at (http://patchwork.ozlabs.org/project/uboot/list/) will get
> confused and record your mail as a legitimate patch.
> 
> >  /* Define board data structure */
> > 
> > >> OK, here we have an unfortunate name overloading. The boot_params here
> > >> is specifically an OMAP handoff from the CPU's internal boot ROM to SPL
> > >> and then from SPL to u-boot. (The same code paths are involved.) It's
> > >> totally unrelated to the the boot_params passed to the Linux kernel.
> > >> 
> > >> Since it's confusing maybe a renaming is called for as well.
> > > 
> > > Indeed. Plus, if it is shared data, it should definitely be mapped at a
> > > fixed memory location or copied from stage to stage (the latter only if
> > > the former is impossible)
> > 
> > Yes, I'm exploring that now. The differences between SPL and U-boot are
> > subtle.
> 
> Actually not that subtle once you get the hang of it: SPL and U-Boot
> are built on the same code base; SPL is the minimal, non-interactive,
> early boot stage which can be loaded and run by ROM code, while U-Boot
> is the full-featured, interactive, too big to boot directly, stage,
> which SPL can chain into.

Part of the confusion here is that I think some TI-isms didn't get
removed from the general code.  jump_to_image_no_args() does not in fact
jump to an image without passing any arguments.  It jumps to an image
passing an argument of where we may have saved some previously passed
paramters (in this case, the format the TI's ROM has defined for a
while).  We also _may_ be in U-Boot without SPL having been run because
U-Boot was given a config header instead.

But I think, and need to re-read this thread a bit more, part of the
solution is to rename jump_to_image_no_args as jump_to_image_uboot, keep
it __weak and provide one that deals with this (and perhaps more cleanly
deals with VIRTIO/ZEBU image_entry).  And after that we can talk about
moving things that can't be in the BSS out of the data section and into
another section.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130403/111267f0/attachment.pgp>