[U-Boot] [PATCH v7 13/19] Makefile: u-boot-with-spl.bin: Fix SPL padding

[Benoît Thébaudeau]

Hi Scott,

On Monday, February 18, 2013 8:11:22 PM, Scott Wood wrote:
> On 02/18/2013 12:52:51 PM, Benoît Thébaudeau wrote:
> > On Monday, February 18, 2013 7:24:51 PM, Scott Wood wrote:
> > > On 02/18/2013 12:22:58 PM, Benoît Thébaudeau wrote:
> > > > On Monday, February 18, 2013 7:02:49 PM, Scott Wood wrote:
> > > > > On 02/18/2013 12:00:52 PM, Benoît Thébaudeau wrote:
> > > > > > The only question is if we may need to have an empty gap
> > between
> > > > the
> > > > > > SPL and
> > > > > > U-Boot within the resulting image. I don't think so since that
> > > > would
> > > > > > mean that
> > > > > > the target memory device has an area that is not really
> > available
> > > > at
> > > > > > the
> > > > > > location of this gap.
> > > > >
> > > > > Why not allow that possibility?
> > > >
> > > > To save a config setting (there are already many for SPL) if this
> > is
> > > > not
> > > > strictly required, but also for the reason below.
> > > >
> > > > >  Maybe it's easier for the SPL to load
> > > > > from a particular offset (e.g. NAND starting at the beginning
> > of a
> > > > > block)?
> > > >
> > > > CONFIG_SPL_MAX_SIZE would be closer to a NAND mapping in that case
> > > > (e.g. size of
> > > > 1 NAND Flash block) than CONFIG_SPL_PAD_TO (address within RAM
> > that
> > > > should be
> > > > considered relatively to CONFIG_SPL_TEXT_BASE to get the NAND
> > offset).
> > >
> > > CONFIG_SPL_PAD_TO is for the placement of the payload
> > 
> > Correct.
> > 
> > > -- and it's not a
> > > RAM address.
> > 
> > It doesn't have to be, but it may be for some configs.
> 
> Right.  My point is it shouldn't be defined as a RAM address.
> 
> > >  Currently it is a link address (or zero if the linker
> > > script handles padding, or padding is not required for other
> > reasons).
> > 
> > Correct.
> > 
> > > With your patch it it is a file offset, IIUC.
> > 
> > With my patch, it is nothing at all since only CONFIG_SPL_MAX_SIZE is
> > used.
> 
> Sorry, I just meant with your change to how objcopy is invoked.  What
> you pass into objcopy is a file offset.
> 
> > > CONFIG_SPL_MAX_SIZE is what it says -- the maximum size that the SPL
> > > may be, ideally to be enforced by the linker script.
> > 
> > Correct.
> > 
> > > They are different.  An SPL wanting the payload to begin as a block
> > > boundary does not mean the hardware is suddenly capable of loading
> > an
> > > entire block of SPL.
> > 
> > Sure, but my question is: Why would you want to have a 2-kiB SPL
> > followed by a
> > 126-kiB gap before the payload? Why couldn't you place the payload on
> > the 1st
> > page boundary after the SPL?
> 
> You can, and we usually do.  But size-limited SPLs may want to simplify
> (e.g. bad block detection needs some special logic to handle beginning
> inside a block), and it may not always be 126 KiB.
> 
> E.g. MPC8313ERDB uses small-page NAND, so it's only 12KiB that gets
> wasted.  It currently has MAX_SIZE of 4KiB but PAD_TO of base plus
> 16KiB.
> 
> > If there are hardware constraints or something that make
> > CONFIG_SPL_PAD_TO
> > useful in some cases, then let's use it, but otherwise, why keep it?
> 
> It's easier to mainain orthogonality (and use defaults for simplicity)
> than to restore it after the fact if we need it later.
> 
> > And if we keep it, do we change it to an image offset, or do we keep
> > it as a
> > link address?
> 
> Changing to an image offset sounds good.
> 
> > > > Also, CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE depend on each
> > other:
> > > > If both
> > > > can be defined, you may change one forgetting the other one, which
> > > > could e.g.
> > > > result in an overlapping of SPL and U-Boot that won't show up at
> > > > build time
> > > > (with CONFIG_SPL_MAX_SIZE = 0x1000 and CONFIG_SPL_PAD_TO =
> > > > CONFIG_SPL_TEXT_BASE
> > > > + 0x800, the SPL would build fine, and objcopy wouldn't complain).
> > >
> > > So add a check that CONFIG_SPL_PAD_TO >= CONFIG_SPL_MAX_SIZE
> > (assuming
> > > the new interpretation of CONFIG_SPL_PAD_TO as a file offset), and
> > let
> > > CONFIG_SPL_PAD_TO default to CONFIG_SPL_MAX_SIZE if not set.
> > 
> > That would make sense. The current default value of 0 for
> > CONFIG_SPL_PAD_TO does
> > not make sense since it means that the SPL can't know where the
> > payload is
> > located within the image.
> 
> CONFIG_SPL_PAD_TO is not the mechanism that is used for finding the
> payload.  On mpc85xx it is unnecessary because the SPL will always be
> fixed size, because the reset vector goes at the end.  It's also
> possible that some SPLs could use linker symbols to find the end of the
> SPL, if they want to pack more tightly.

Thanks for all the clarifications.

So I will make a v8 with CONFIG_SPL_PAD_TO as an image offset, and use it to
generate u-boot-with-spl.bin. But first, I will wait for more feedback on v7
(Fabio should give his test results this week), and for Stefano to re-sync
u-boot-imx/master with u-boot/master.

Best regards,
Benoît