[U-Boot] [PATCH 00/27] spl: Use linker list and parameters for SPL image loading

[Tom Rini]

On Sun, Sep 18, 2016 at 06:57:01PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On 18 September 2016 at 16:14, Tom Rini <trini at konsulko.com> wrote:
> > On Sun, Sep 18, 2016 at 01:44:49PM -0600, Simon Glass wrote:
> >
> >> At present the SPL code uses a global spl_image variable which is shared
> >> amongst lots of files, some in common/spl and some elsewhere. There is no
> >> need for this to be global, and in fact a parameter makes it easier to
> >> understand what information the functions act on. It also reduces the BSS
> >> use in the SPL (at the expense of stack) which is useful on boards which
> >> don't have BSS available early on.
> >
> > Thanks for taking a stab at cleaning this up.
> 
> I ran into it with the 64-bit x86 stuff and decided to take a detour :-)
> 
> >
> > [snip]
> >> There is a priorty value attached to each loader which should allow the
> >> existing ordering to be maintained.
> >
> > I worry here about some of the corner cases, but it's probably no more
> > or less fragile than currently at least.  I'm actually not sure how
> > that's working in this version of the series, everyone gets priority 0
> > filled in so it seems like we're on linker order.  But I'm also not
> > convinced that it's a problem here.  As far as I can recall, the use
> > cases are that a given binary will support say NAND and MMC, and at run
> > time knows that it came from NAND or MMC, and thus we need to look at
> > the same device for U-Boot itself.  So order doesn't matter there.  So
> > we can just drop that part I think.
> 
> I see in the code that if Ubifs is enabled, it uses that instead of
> NAND and ONENAND. There is another case too - SPI might have a
> board-specific loader which takes precedence (although I suspect in
> the sunxi case the generic loader is not present).
> 
> If these are not needed then we can drop it.
> 
> At present priority is handled by adding the priority value into the
> linker list element name. Since they are sorted in order within the
> image, this gives the required result.

Looking over the code, UBI is setup as mutually exclusive.  It also
looks like that SPL_SPI_BOOT, SPL_SPI_SUNXI and SPL_SPI_LOAD are all
exclusive as well.  So we should clean that up so that we select the
right back end if SPL_SPI is set, and we can drop the order part.

> >> Code size is about 20 bytes larger on average which I think is acceptable.
> >> The BSS size drops by about 64 bytes, but really this just transfers to
> >> the stack.
> >
> > How about the worst case growths in what we have today?
> 
> Ah well that's something buildman cannot tell me! There is a small
> overhead for the lookup code (about 20-30 bytes) and 8-12 bytes for
> each loader method. Here's the buildman summary.
> 
> $ buildman -b spl -s --step 0 -S
> boards.cfg is up to date. Nothing to do.
> Summary of 2 commits for 1196 boards (32 threads, 1 job per thread)
> 01: Makefile: Give a build error if ad-hoc CONFIG options are added
>   blackfin:  +   cm-bf527 bf609-ezkit bf537-stamp
>      sparc:  +   grsim grsim_leon2 gr_cpci_ax2000 gr_xc3s_1500 gr_ep2s60
>      nios2:  +   10m50 3c120
> microblaze:  +   microblaze-generic
>   openrisc:  +   openrisc-generic
> 28: spl: Make spl_boot_list a local variable
>    aarch64: (for 51/51 boards)  spl/u-boot-spl:all +36.8
> spl/u-boot-spl:bss -6.3  spl/u-boot-spl:data +5.6
> spl/u-boot-spl:rodata +7.5  spl/u-boot-spl:text +29.9
>    powerpc: (for 415/415 boards)  spl/u-boot-spl:all -24.7
> spl/u-boot-spl:bss -0.1  spl/u-boot-spl:data -0.1
> spl/u-boot-spl:rodata -15.9  spl/u-boot-spl:text -8.6
>    sandbox: (for 3/3 boards)  spl/u-boot-spl:all +37.3
> spl/u-boot-spl:bss -10.7  spl/u-boot-spl:rodata +10.7
> spl/u-boot-spl:text +37.3
>        arm: (for 552/552 boards)  all -0.8  bss -0.8  data -0.1
> spl/u-boot-spl:all -66.3  spl/u-boot-spl:bss -22.3
> spl/u-boot-spl:data -5.1  spl/u-boot-spl:rodata +0.5
> spl/u-boot-spl:text -39.3

You should have a bit more output :)  Adding in -d will list every board
and then what I do (since I do -SCdvel and -Ssdel after to log it) is
read over the list and manually look at the deviations.

> >> There is an obvious follow-on from this, to move boot_name_table[] into the
> >> same linker list struct (i.e. add a name field to struct spl_image_loader).
> >> The complication here is that we don't want naming if
> >> CONFIG_SPL_LIBCOMMON_SUPPORT is not enabled, since it bloats the code. In
> >> addition I think that common/spl/spl.c can be tidied up a little.
> >
> > Yeah, I worry about size growth in doing that too.  But maybe we can be
> > a little clever when declaring the ll and just not have the strings if
> > !LIBCOMMON ?
> 
> Yes I think so...perhaps someone else might take a look, or I'll wait
> for a rainy day!

Sounds good enough for now, thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160919/d2e063f7/attachment.sig>