<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 1, 2016 at 11:31 PM, Joel Stanley <span dir="ltr"><<a href="mailto:joel@jms.id.au" target="_blank">joel@jms.id.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Rick,<br>
<span><br>
On Wed, Nov 2, 2016 at 3:56 PM, Rick Altherr <<a href="mailto:raltherr@google.com" target="_blank">raltherr@google.com</a>> wrote:<br>
> FIT is the modern u-boot native image format for kernels, device trees,<br>
> and ramdisks.  Enabling FIT only compiles in support for the image<br>
> format.  For these devices, the kernel+dtb and ramdisk are loaded from<br>
> separate locations in flash and can be any mix of legacy or FIT images.<br>
> When using FIT images, the dtb is stored as a separate entry that<br>
> requires CONFIG_OF_LIBFDT to load it into RAM and pass it to the kernel.<br>
><br>
> Tested under qemu with both legacy and FIT kernel+dtb images for<br>
> palmetto and witherspoon.<br>
<br>
</span>The changes look good to me.<br>
<br>
Is the FIT support in v2016.07 new enough for us? Is there any reason<br>
to move to a v2016.09 base?<br>
<br></blockquote><div><br></div><div>AFAIK, v2016.07 is new enough.  It certainly includes FIT, FDT loading, and FIT signature checking.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It does introduce a warning in a hack we carry for loading the<br>
initramfs, which I think points to a legitimate issue:<br>
<br>
common/image.c: In function ‘boot_get_ramdisk’:<br>
common/image.c:1078:4: warning: ‘rd_load’ may be used uninitialized in<br>
this function [-Wmaybe-uninitialized]<br>
    memmove ((void *)rd_load, (uchar *)rd_data, rd_len);<br>
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<wbr>~~~~~~~~~~~~~~~~~~~~~<br>
<br>
This code an Aspeed hack we carry. It is required to copy the<br>
initramfs from flash. Cedric was doing some investigation into why we<br>
need the hack, and it appears to be an upstream bug.<br>
<br>
If I read the code correctly, prior to enabling FIT we only accepted<br>
IMAGE_FORMAT_LEGACY. If we move the hack to be part of that case we<br>
should be ok, as we don't need to do the hack when loading from FIT.<br>
<br>
Can you try that and add a second patch to your series that moves the hack?<br>
<br></blockquote><div><br></div><div>Looks like Cedric has a patch that just removes the hack.  I'll look into this more and add it to my series.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Finally, I took a look at the impact on code size. I used<br>
arm-linux-gnueabi-gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005 from<br>
Ubuntu 16.10.<br>
<br>
$ size u-boot<br>
   text   data    bss    dec    hex filename<br>
 151636   4576  61620 217832  352e8 u-boot<br>
 222302   7952  62768 293022  4789e u-boot<br>
<br>
Wow, the FIT image is fatter. What does FIT support pull in that<br>
increases the code size so much?<br>
<br>
The on disk size suggests that size isn't lying:<br>
<br>
Unfit: 156012<br>
FIT: 230056<br>
<br>
Did we expect the change to be that large?<br>
<br>
This is not a blocker, as we currently set aside 384KB for u-boot.<br></blockquote><div><br></div><div>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).  </div><div><br></div><div>Palmetto orig: <span style="color:rgb(0,0,0)">165336</span></div><div><span style="color:rgb(0,0,0)">Palmetto FIT: </span><font color="#000000">201356</font></div><div><span style="color:rgb(0,0,0)"></span><span style="color:rgb(0,0,0)">Palmetto FIT w/ verbose: </span><font color="#000000">201720</font></div><div><span style="color:rgb(0,0,0)">Palmetto FIT w/ verbose+FDT: </span><font color="#000000">238976</font><br></div><div><font color="#000000">Palmetto FIT w/ verbose+FDT-EFI: 223380</font></div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Cheers,<br>
<br>
Joel<br>
<div><div class="gmail-m_-5140069579929286666gmail-m_8960449125377829950m_8480033853154680325gmail-m_1903516065268380482h5"><br>
<br>
><br>
> Signed-off-by: Rick Altherr <<a href="mailto:raltherr@google.com" target="_blank">raltherr@google.com</a>><br>
> ---<br>
> Changes since v1:<br>
> - Fixed commit message grammar<br>
> - Clarified config option needed for FDT loading<br>
> ---<br>
>  configs/ast_g4_ncsi_defconfig | 3 +++<br>
>  configs/ast_g4_phy_defconfig  | 3 +++<br>
>  configs/ast_g5_ncsi_defconfig | 3 +++<br>
>  configs/ast_g5_phy_defconfig  | 3 +++<br>
>  4 files changed, 12 insertions(+)<br>
><br>
> diff --git a/configs/ast_g4_ncsi_defconfi<wbr>g b/configs/ast_g4_ncsi_defconfi<wbr>g<br>
> index 4ee71c5..70cd3c2 100644<br>
> --- a/configs/ast_g4_ncsi_defconfi<wbr>g<br>
> +++ b/configs/ast_g4_ncsi_defconfi<wbr>g<br>
> @@ -3,5 +3,8 @@ CONFIG_TARGET_AST_G4=y<br>
>  CONFIG_SYS_PROMPT="ast# "<br>
>  CONFIG_CMD_DHCP=y<br>
>  CONFIG_CMD_PING=y<br>
> +CONFIG_FIT=y<br>
> +CONFIG_FIT_VERBOSE=y<br>
> +CONFIG_OF_LIBFDT=y<br>
>  CONFIG_SPI_FLASH=y<br>
>  CONFIG_SYS_NS16550=y<br>
> diff --git a/configs/ast_g4_phy_defconfig b/configs/ast_g4_phy_defconfig<br>
> index 61fd69b..468fbc4 100644<br>
> --- a/configs/ast_g4_phy_defconfig<br>
> +++ b/configs/ast_g4_phy_defconfig<br>
> @@ -4,5 +4,8 @@ CONFIG_ASPEED_NET_PHY=y<br>
>  CONFIG_SYS_PROMPT="ast# "<br>
>  CONFIG_CMD_DHCP=y<br>
>  CONFIG_CMD_PING=y<br>
> +CONFIG_FIT=y<br>
> +CONFIG_FIT_VERBOSE=y<br>
> +CONFIG_OF_LIBFDT=y<br>
>  CONFIG_SPI_FLASH=y<br>
>  CONFIG_SYS_NS16550=y<br>
> diff --git a/configs/ast_g5_ncsi_defconfi<wbr>g b/configs/ast_g5_ncsi_defconfi<wbr>g<br>
> index 6d11afb..8a9c297 100644<br>
> --- a/configs/ast_g5_ncsi_defconfi<wbr>g<br>
> +++ b/configs/ast_g5_ncsi_defconfi<wbr>g<br>
> @@ -3,5 +3,8 @@ CONFIG_TARGET_AST_G5=y<br>
>  CONFIG_SYS_PROMPT="ast# "<br>
>  CONFIG_CMD_DHCP=y<br>
>  CONFIG_CMD_PING=y<br>
> +CONFIG_FIT=y<br>
> +CONFIG_FIT_VERBOSE=y<br>
> +CONFIG_OF_LIBFDT=y<br>
>  CONFIG_SPI_FLASH=y<br>
>  CONFIG_SYS_NS16550=y<br>
> diff --git a/configs/ast_g5_phy_defconfig b/configs/ast_g5_phy_defconfig<br>
> index 20f62e0..fd450b9 100644<br>
> --- a/configs/ast_g5_phy_defconfig<br>
> +++ b/configs/ast_g5_phy_defconfig<br>
> @@ -4,5 +4,8 @@ CONFIG_ASPEED_NET_PHY=y<br>
>  CONFIG_SYS_PROMPT="ast# "<br>
>  CONFIG_CMD_DHCP=y<br>
>  CONFIG_CMD_PING=y<br>
> +CONFIG_FIT=y<br>
> +CONFIG_FIT_VERBOSE=y<br>
> +CONFIG_OF_LIBFDT=y<br>
>  CONFIG_SPI_FLASH=y<br>
>  CONFIG_SYS_NS16550=y<br>
> --<br>
> 2.8.0.rc3.226.g39d4020<br>
><br>
</div></div>> ______________________________<wbr>_________________<br>
> openbmc mailing list<br>
> <a href="mailto:openbmc@lists.ozlabs.org" target="_blank">openbmc@lists.ozlabs.org</a><br>
> <a href="https://lists.ozlabs.org/listinfo/openbmc" rel="noreferrer" target="_blank">https://lists.ozlabs.org/listi<wbr>nfo/openbmc</a><br>
</blockquote></div><br></div></div>