[PATCH u-boot v2] Enable FIT image support and FDT loading for AST2400/AST2500

Thu Nov 10 11:00:09 AEDT 2016

On Tue, Nov 1, 2016 at 11:31 PM, Joel Stanley <joel at jms.id.au> wrote:

> Hi Rick,
>
> On Wed, Nov 2, 2016 at 3:56 PM, Rick Altherr <raltherr at google.com> wrote:
> > FIT is the modern u-boot native image format for kernels, device trees,
> > and ramdisks.  Enabling FIT only compiles in support for the image
> > format.  For these devices, the kernel+dtb and ramdisk are loaded from
> > separate locations in flash and can be any mix of legacy or FIT images.
> > When using FIT images, the dtb is stored as a separate entry that
> > requires CONFIG_OF_LIBFDT to load it into RAM and pass it to the kernel.
> >
> > Tested under qemu with both legacy and FIT kernel+dtb images for
> > palmetto and witherspoon.
>
> The changes look good to me.
>
> Is the FIT support in v2016.07 new enough for us? Is there any reason
> to move to a v2016.09 base?
>
>
AFAIK, v2016.07 is new enough.  It certainly includes FIT, FDT loading, and
FIT signature checking.

> It does introduce a warning in a hack we carry for loading the
> initramfs, which I think points to a legitimate issue:
>
> common/image.c: In function ‘boot_get_ramdisk’:
> common/image.c:1078:4: warning: ‘rd_load’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>     memmove ((void *)rd_load, (uchar *)rd_data, rd_len);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This code an Aspeed hack we carry. It is required to copy the
> initramfs from flash. Cedric was doing some investigation into why we
> need the hack, and it appears to be an upstream bug.
>
> If I read the code correctly, prior to enabling FIT we only accepted
> IMAGE_FORMAT_LEGACY. If we move the hack to be part of that case we
> should be ok, as we don't need to do the hack when loading from FIT.
>
> Can you try that and add a second patch to your series that moves the hack?
>
>
Looks like Cedric has a patch that just removes the hack.  I'll look into
this more and add it to my series.

> Finally, I took a look at the impact on code size. I used
> arm-linux-gnueabi-gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005 from
> Ubuntu 16.10.
>
> $ size u-boot
>    text   data    bss    dec    hex filename
>  151636   4576  61620 217832  352e8 u-boot
>  222302   7952  62768 293022  4789e u-boot
>
> Wow, the FIT image is fatter. What does FIT support pull in that
> increases the code size so much?
>
> The on disk size suggests that size isn't lying:
>
> Unfit: 156012
> FIT: 230056
>
> Did we expect the change to be that large?
>
> This is not a blocker, as we currently set aside 384KB for u-boot.
>

I tried a few variations to see where the increase comes from.  Looks like
it is an even split between the FIT image format support and the FDT
loading support.  I expected a sizable increase from libfdt (brought in by
both FIT and FDT options) and additional hash algorithms (sha1, sha256).

Palmetto orig: 165336
Palmetto FIT: 201356
Palmetto FIT w/ verbose: 201720
Palmetto FIT w/ verbose+FDT: 238976
Palmetto FIT w/ verbose+FDT-EFI: 223380

Enabling FDT seems to enable EFI by default as well.  Explicitly turning
off EFI loading support saves ~13k.  I'm uncertain if it is reasonable to
say that EFI support should be turned off by default.  OpenBMC certainly
doesn't need it but the u-boot configuration is ideally more generic.

> Cheers,
>
> Joel
>
>
> >
> > Signed-off-by: Rick Altherr <raltherr at google.com>
> > ---
> > Changes since v1:
> > - Fixed commit message grammar
> > - Clarified config option needed for FDT loading
> > ---
> >  configs/ast_g4_ncsi_defconfig | 3 +++
> >  configs/ast_g4_phy_defconfig  | 3 +++
> >  configs/ast_g5_ncsi_defconfig | 3 +++
> >  configs/ast_g5_phy_defconfig  | 3 +++
> >  4 files changed, 12 insertions(+)
> >
> > diff --git a/configs/ast_g4_ncsi_defconfig
> b/configs/ast_g4_ncsi_defconfig
> > index 4ee71c5..70cd3c2 100644
> > --- a/configs/ast_g4_ncsi_defconfig
> > +++ b/configs/ast_g4_ncsi_defconfig
> > @@ -3,5 +3,8 @@ CONFIG_TARGET_AST_G4=y
> >  CONFIG_SYS_PROMPT="ast# "
> >  CONFIG_CMD_DHCP=y
> >  CONFIG_CMD_PING=y
> > +CONFIG_FIT=y
> > +CONFIG_FIT_VERBOSE=y
> > +CONFIG_OF_LIBFDT=y
> >  CONFIG_SPI_FLASH=y
> >  CONFIG_SYS_NS16550=y
> > diff --git a/configs/ast_g4_phy_defconfig b/configs/ast_g4_phy_defconfig
> > index 61fd69b..468fbc4 100644
> > --- a/configs/ast_g4_phy_defconfig
> > +++ b/configs/ast_g4_phy_defconfig
> > @@ -4,5 +4,8 @@ CONFIG_ASPEED_NET_PHY=y
> >  CONFIG_SYS_PROMPT="ast# "
> >  CONFIG_CMD_DHCP=y
> >  CONFIG_CMD_PING=y
> > +CONFIG_FIT=y
> > +CONFIG_FIT_VERBOSE=y
> > +CONFIG_OF_LIBFDT=y
> >  CONFIG_SPI_FLASH=y
> >  CONFIG_SYS_NS16550=y
> > diff --git a/configs/ast_g5_ncsi_defconfig
> b/configs/ast_g5_ncsi_defconfig
> > index 6d11afb..8a9c297 100644
> > --- a/configs/ast_g5_ncsi_defconfig
> > +++ b/configs/ast_g5_ncsi_defconfig
> > @@ -3,5 +3,8 @@ CONFIG_TARGET_AST_G5=y
> >  CONFIG_SYS_PROMPT="ast# "
> >  CONFIG_CMD_DHCP=y
> >  CONFIG_CMD_PING=y
> > +CONFIG_FIT=y
> > +CONFIG_FIT_VERBOSE=y
> > +CONFIG_OF_LIBFDT=y
> >  CONFIG_SPI_FLASH=y
> >  CONFIG_SYS_NS16550=y
> > diff --git a/configs/ast_g5_phy_defconfig b/configs/ast_g5_phy_defconfig
> > index 20f62e0..fd450b9 100644
> > --- a/configs/ast_g5_phy_defconfig
> > +++ b/configs/ast_g5_phy_defconfig
> > @@ -4,5 +4,8 @@ CONFIG_ASPEED_NET_PHY=y
> >  CONFIG_SYS_PROMPT="ast# "
> >  CONFIG_CMD_DHCP=y
> >  CONFIG_CMD_PING=y
> > +CONFIG_FIT=y
> > +CONFIG_FIT_VERBOSE=y
> > +CONFIG_OF_LIBFDT=y
> >  CONFIG_SPI_FLASH=y
> >  CONFIG_SYS_NS16550=y
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> > _______________________________________________
> > openbmc mailing list
> > openbmc at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161109/5bfd8a51/attachment-0001.html>