[U-Boot] [PATCH v2] image: Fix Android boot image support

[Simon Glass]

Hi Ahmad,

On 21 October 2014 10:55, Ahmad Draidi <ar2000jp at gmail.com> wrote:
> This patch makes the following changes:
> - Set kernel entry point correctly
> - Append bootargs from image to global bootargs instead
>         of replacing them
> - Return end address instead of size from android_image_get_end()
> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
>
> Signed-off-by: Ahmad Draidi <ar2000jp at gmail.com>
> Cc: Tom Rini <trini at ti.com>
> ---
> Changes for v2:
>    - Cleanup bootargs copying code
>    - Coding Style cleanup

One little nit below but it looks OK to me. I'm assume that no one
would want to replace the command line completely?

I hope you can write a test too. Re your comment about not wanting to
change the code much - if we go that way the code will get really
ugly. When you add features you often need to refactor. When there are
tests, it becomes easier to know you have not broken things.

Reviewed-by: Simon Glass <sjg at chromium.org>

> ---
>  common/bootm.c         |  4 ++--
>  common/image-android.c | 45 ++++++++++++++++++++++++++++++++++-----------
>  common/image.c         |  3 ++-
>  3 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index ff81a27..c04a3b0 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -144,11 +144,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
>                 images.os.type = IH_TYPE_KERNEL;
>                 images.os.comp = IH_COMP_NONE;
>                 images.os.os = IH_OS_LINUX;
> -               images.ep = images.os.load;
> -               ep_found = true;
>
>                 images.os.end = android_image_get_end(os_hdr);
>                 images.os.load = android_image_get_kload(os_hdr);
> +               images.ep = images.os.load;
> +               ep_found = true;
>                 break;
>  #endif
>         default:
> diff --git a/common/image-android.c b/common/image-android.c
> index 6ded7e2..673cbca 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <image.h>
>  #include <android_image.h>
> +#include <malloc.h>
>
>  static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
>
> @@ -25,12 +26,33 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>
>         printf("Kernel load addr 0x%08x size %u KiB\n",
>                hdr->kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
> -       strncpy(andr_tmp_str, hdr->cmdline, ANDR_BOOT_ARGS_SIZE);
> -       andr_tmp_str[ANDR_BOOT_ARGS_SIZE] = '\0';
> -       if (strlen(andr_tmp_str)) {
> -               printf("Kernel command line: %s\n", andr_tmp_str);
> -               setenv("bootargs", andr_tmp_str);
> +
> +       int len = 0;
> +       if (*hdr->cmdline) {
> +               printf("Kernel command line: %s\n", hdr->cmdline);
> +               len += strlen(hdr->cmdline);
> +       }
> +
> +       char *bootargs = getenv("bootargs");
> +       if (bootargs)
> +               len += strlen(bootargs);
> +
> +       char *newbootargs = malloc(len + 2);
> +       if (!newbootargs) {
> +               puts("Error: malloc in android_image_get_kernel failed!\n");
> +               return -1;

nit: return -ENOMEM - suggest adding a comment to
android_image_get_kernel() so its args and return value are
documented.

>         }
> +       *newbootargs = '\0';
> +
> +       if (bootargs) {
> +               strcpy(newbootargs, bootargs);
> +               strcat(newbootargs, " ");
> +       }
> +       if (*hdr->cmdline)
> +               strcat(newbootargs, hdr->cmdline);
> +
> +       setenv("bootargs", newbootargs);
> +
>         if (hdr->ramdisk_size)
>                 printf("RAM disk load addr 0x%08x size %u KiB\n",
>                        hdr->ramdisk_addr,
> @@ -52,17 +74,18 @@ int android_image_check_header(const struct andr_img_hdr *hdr)
>
>  ulong android_image_get_end(const struct andr_img_hdr *hdr)
>  {
> -       u32 size = 0;
> +       ulong end;
>         /*
>          * The header takes a full page, the remaining components are aligned
>          * on page boundary
>          */
> -       size += hdr->page_size;
> -       size += ALIGN(hdr->kernel_size, hdr->page_size);
> -       size += ALIGN(hdr->ramdisk_size, hdr->page_size);
> -       size += ALIGN(hdr->second_size, hdr->page_size);
> +       end = (ulong)hdr;
> +       end += hdr->page_size;
> +       end += ALIGN(hdr->kernel_size, hdr->page_size);
> +       end += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +       end += ALIGN(hdr->second_size, hdr->page_size);
>
> -       return size;
> +       return end;
>  }
>
>  ulong android_image_get_kload(const struct andr_img_hdr *hdr)
> diff --git a/common/image.c b/common/image.c
> index 085771c..e21c848 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -1009,7 +1009,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
>                 image_multi_getimg(images->legacy_hdr_os, 1, &rd_data, &rd_len);
>         }
>  #ifdef CONFIG_ANDROID_BOOT_IMAGE
> -       else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
> +       else if ((genimg_get_format((void *)images->os.start)
> +                       == IMAGE_FORMAT_ANDROID) &&
>                  (!android_image_get_ramdisk((void *)images->os.start,
>                  &rd_data, &rd_len))) {
>                 /* empty */
> --
> 2.1.1

Regards,
Simon