[U-Boot] [PATCH v3 3/5] image: add support for Android's boot image format

[Marek Vasut <marex@denx.de>]

On Saturday, April 12, 2014 at 11:54:10 PM, Rob Herring wrote:
> On Fri, Apr 11, 2014 at 4:44 PM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, April 10, 2014 at 09:18:05 PM, Rob Herring wrote:
> >> From: Sebastian Siewior <bigeasy@linutronix.de>
> >> 
> >> This patch adds support for the Android boot-image format. The header
> >> file is from the Android project and got slightly alterted so the struct
> >> + its defines are not generic but have something like a namespace. The
> >> header file is from bootloader/legacy/include/boot/bootimg.h. The
> >> header parsing has been written from scratch and I looked at
> >> bootloader/legacy/usbloader/usbloader.c for some details.
> >> The image contains the physical address (load address) of the kernel and
> >> ramdisk. This address is considered only for the kernel image.
> >> The "second image" is currently ignored. I haven't found anything that
> >> is creating this.
> > 
> > Can you please elaborate on those last two sentences ?
> 
> The android header has 3 images: kernel, ramdisk and "second". I think
> this is for secondary bootloader, but I'll have to investigate.

Viva hardcoded b/s :'-(

> >> v3 (Rob Herring):
> >> This is based on http://patchwork.ozlabs.org/patch/126797/ with the
> >> following changes:
> >> - Rebased to current mainline
> >> - Moved android image handling to separate functions in
> >> 
> >>   common/image-android.c
> >> 
> >> - s/u8/char/ in header to fix string function warnings
> >> - Use SPDX identifiers for licenses
> >> 
> >> - Cleaned-up file source information:
> >>   android_image.h is from file include/boot/bootimg.h in repository:
> >>   https://android.googlesource.com/platform/bootable/bootloader/legacy
> >>   The git commit hash is 4205b865141ff2e255fe1d3bd16de18e217ef06a
> >>   usbloader.c would be from the same commit, but it does not appear
> >>   to have been used for any actual code.
> > 
> > [...]
> > 
> >> @@ -293,7 +306,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag,
> >> int argc, return 1;
> >> 
> >>               }
> >>  
> >>  #endif
> >> 
> >> -     } else {
> >> +     } else if (images.ep == ~0UL) {
> > 
> > I don't find this really portable. While it's unlikely the kernel will
> > have the EP here, don't we have a better solution than using special
> > value?
> 
> This is to address Wolfgang's prior comments about moving setting of
> images.ep into the switch statement above. Do you like the previous
> version better?

I think I might be missing that part of the discussion. My point is just that 
you might rather have a separate variable to detect error instead of having a 
special value like this.

Does my rant make sense to you please ?

> > [...]
> > 
> >> @@ -949,7 +951,15 @@ int boot_get_ramdisk(int argc, char * const argv[],
> >> bootm_headers_t *images, (ulong)images->legacy_hdr_os);
> >> 
> >>               image_multi_getimg(images->legacy_hdr_os, 1, &rd_data,
> >>               &rd_len);
> >> 
> >> -     } else {
> >> +     }
> >> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
> >> +     else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
> >> +              (!andriod_image_get_ramdisk((void *)images->os.start,
> > 
> > andriod_image_get_ramdisk() ? There is a typo in the function name, did
> > you actually ever even compile this patchset please ?
> > [...]
> 
> Of course it compiles as the same typo is everywhere for this
> function... :) I've compiled and run this (although I have not tested
> with a ramdisk).

Okay, thanks for clearing this up (and fixing this in v4)!