[U-Boot] [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts

[Tom Warren]

Albert,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Friday, April 05, 2013 8:59 AM
> To: Tom Rini
> Cc: Albert ARIBAUD; Scott Wood; u-boot at lists.denx.de;
> Stephen at theia.denx.de; Tom Warren; Simon Glass; Allen Martin
> Subject: Re: [U-Boot] [PATCH] ARM: Fix __bss_start and __bss_end in linker
> scripts

I just fetched & merged TOT u-boot-arm (getting ready for a PR from u-boot-tegra -> u-boot-arm), and did a test build of all Tegra boards.

I get the following linker (ld) error on all builds due to this commit (abbecf4c8,  ARM: Fix __bss_start and __bss_end in linker scripts):

 arm-linux-gnueabi-ld.bfd:u-boot.lds:44: syntax error

This is pointing to the 'HIDDEN(__bss_base = .);' lines added in this change (in the generated u-boot.lds in the root dir).

I'm seeing this with both gcc 4.4.1/ld 2.19.51  and gcc 4.6.2/ld 2.22 tools.

What compiler/binutils did you use to build/test this?

Tom
> 
> On 04/05/2013 07:53 AM, Tom Rini wrote:
> > On Fri, Apr 05, 2013 at 08:00:43AM +0200, Albert ARIBAUD wrote:
> >> Hi Beno??t,
> >>
> >> CC:ing Stephen Warren who wrote commit 2b7818d4 which git blame tells
> >> me added the ASSERT() to arch/arm/cpu/u-boot.lds, and Tom Rini to
> >> help decide what to do.
> > [snip]
> >>>>>> Looks good, but what about the __bss_end in ASSERT(__bss_end <
> >>>>>> (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image
> too
> >>>>>> big"); ?
> >>>>>>
> >>>>>> Shouldn't it be replaced with __bss_limit, or even better, with
> >>>>>> __image_copy_end (why should BSS be taken into account for SPL
> >>>>>> image size?)?
> >>>>>
> >>>>> I meant __bss_base, not __image_copy_end.
> >>>>
> >>>> Part of SPL can use BSS, once board_init_f() has handed things over
> >>>> to board_init_r(),
> >>>
> >>> I agree with that.
> >>>
> >>>> and this test is meant to ensure that .text+.data+.bss fits in the
> >>>> RAM available to SPL. This ASSERT() compares the upper limit of
> >>>> this RAM to the upper limit of the SPL needs, i.e. __bss_end.
> >>>>
> >>>> IOW, this ASSERT is about how much memory SPL will use when it
> >>>> runs,
> >>>
> >>> This is where there might be an issue with the definition / usage of
> >>> CONFIG_SPL_MAX_SIZE.
> >>>
> >>> I had understood that this is the purpose of this assert, but this
> >>> usage of CONFIG_SPL_MAX_SIZE, while frequent on arm, conflicts with
> >>> its definition in README, as well as with some arm and other usages,
> >>> e.g. in include/configs/am335x_evm.h or
> >>> include/configs/p1_p2_rdb_pc.h.
> >>>
> >>> The README's definition is also how I understood this config when
> >>> discussing it with Scott about my 16/30, which led to the #error
> >>> test that was added at some point to this patch.
> >>
> >> Sorry, I'd missed that. Adding Stephen for any comment regarding the
> >> ASSERT() and CONFIG_SPL_MAX_SIZE semantics.
> >
> > Yes.  I'm a little confused about this one as well now.
> ...
> > If I read all of the tegra files correctly, the way they work is that
> > SPL and U-Boot are both loaded into memory, and thus they're ensuring
> > that the BSS won't overlap with the full U-Boot that's right behind.
> 
> Yes exactly.
> 
> I'll explain the whole situation just so there's something to refer to in the
> archives.
> 
> Tegra has two different types of CPU. As far as boot process goes, the AVP
> (Audio/Video Processor) is what is released from reset at system boot time,
> and what runs the built-in Tegra boot ROM and the U-Boot SPL. The AVP is an
> ARM7TDMI. The SPL initializes the main CPU complex (A9/A15), and causes it
> to execute the main U-Boot.
> 
> The boot process is:
> 
> * Entire U-Boot binary (SPL+pad+U-Boot) sits in boot flash.
> * Boot ROM code running on AVP:
> ** Initializes DRAM controller.
> ** Copies entire SPL+pad+U-Boot to DRAM byte-for-byte.
> ** Jumps to U-Boot SPL.
> * U-Boot SPL running on AVP:
> ** Initializes clocks/... required to boot A9 CPUs.
> ** Set up the A9 reset vector to point at the main U-Boot code,
>    and releases A9 CPUs from reset.
> * Main U-Boot running on A9:
> ** Runs in the typical fashion, including regular relocation.
> 
> Thus, the in-flash layout of SPL+U-Boot must match the in-DRAM layout,
> since the boot ROM just copies it byte-for-byte, and the SPL does nothing to
> move the appended main U-Boot out-of-the-way before executing.
> 
> The gap between SPL and concatenated U-Boot must be large enough to
> hold the BSS (since SPL will use BSS without moving U-boot), but is also larger
> so that we can hard-code the TEXT_BASE of the main U-Boot to some stable
> value, without having to move it about often if SPL changes size.
> 
> So the U-Boot binary layout in flash/DRAM is:
> 
> +---------------------+
> | SPL                 |
> +---------------------+
> | Pad, including      |
> | space for BSS       |
> +---------------------+
> | Main U-Boot         |
> +---------------------+
> 
> The assert in question ensures that the start of the "Main U-Boot" in the
> diagram above is at a large enough address that it doesn't overlap the SPL's
> BSS usage in RAM.
> 
> It's possible that I chose the wrong defines to validate when writing that
> assert. As long as the assert continues to ensure the above image layout in
> the generated u-boot*.bin, I have no problems with it being changed.
--
nvpublic