From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Sun, 13 Apr 2014 18:55:34 +0200 Subject: [U-Boot] [PATCH v3 3/5] image: add support for Android's boot image format In-Reply-To: References: <1397157488-8695-1-git-send-email-robherring2@gmail.com> <201404112344.37141.marex@denx.de> Message-ID: <201404131855.34700.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 wrote: > > On Thursday, April 10, 2014 at 09:18:05 PM, Rob Herring wrote: > >> From: Sebastian Siewior > >> > >> 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)!