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

[Tom Rini]

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.

> > According to README, CONFIG_SPL_MAX_SIZE is for text + data + rodata (i.e. for
> > the size of the binary image file to be programmed on the target device), while
> > CONFIG_SPL_BSS_MAX_SIZE is dedicated to the BSS. Hence, in order to comply with
> > this definition, that assert should either use __bss_base/__bss_start, or
> > compare against CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE, depending on
> > whether it's supposed to test the binary image size or the memory footprint.
> > There could even be asserts testing both. This would anyway be a new patch
> > separate from this one.
> 
> It would indeed be a different patch.
> 
> IIUC, this future patch would increase the limit for SPL run-time size,
> as the constant against which the ASS tests __bss_end for would
> necessarily be greater than it is now. Correct? If so, this future
> patch should not break any target, as it would loosen the constraint,
> not tighten it.
> 
> Tom, would such a patch, if posted today, be acceptable for 2013.04?

So, lets talk things out.  On (setting aside davinci) TI platforms,
SPL's text/data/rodata is sitting in SRAM (starting at 0x4...) and we
say BSS shall live in DDR (starting at 0x8...) and we configure DDR
before clearing and using BSS.  The "max" we set is mostly about trying
to not possibly step on ourselves later on (SPL malloc, full U-Boot,
etc).

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.

So yes, a patch to correct the behavior is fine, and we can build test
that easily enough as well.

-- 
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/20130405/a2f0d12e/attachment.pgp>