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

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@gmail.com>
Cc: Tom Rini <trini@ti.com>
---
Changes for v2:
   - Cleanup bootargs copying code
   - Coding Style cleanup
---
 common/bootm.c         |  4 ++--
 common/image-android.c | 45 ++++++++++++++++++++++++++++++++++-----------
 common/image.c         |  3 ++-
 3 files changed, 38 insertions(+), 14 deletions(-)

Hi Ahmad,

On 21 October 2014 10:55, Ahmad Draidi <ar2000jp@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@gmail.com>
> Cc: Tom Rini <trini@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@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

Hello, Mr. Glass.

On Wednesday 22 October 2014 11:29:00 AM Simon Glass wrote:
> One little nit below but it looks OK to me. I'm assume that no one
> would want to replace the command line completely?
> 

In some setups, one image can be used with several versions, or even
    different boards of hardware. The bootloader passes a command line
    argument to specify this. Also, the header file for the mkbootimg
    specifies that the "kernel_args[] is appended to the kernel
    commandline". So I thought this would make it consistent with other
    bootloaders, and the spec.

> 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.

Thanks for the tip.
I'll try to write a test if I get the time.
One thing comes to mind. To be able to write a test, we'll need an image
    to test against. I think pulling in the mkbootimg tool is the right move.
    The other option I can think of is bundling a small dummy image with
    U-Boot, which I think is a bit ugly. What do you think?

> Reviewed-by: Simon Glass <sjg@chromium.org>
> nit: return -ENOMEM - suggest adding a comment to
> android_image_get_kernel() so its args and return value are
> documented.
> 

Thank you. I'll send in a new version soon.

> Regards,
> Simon

Regards,
Ahmad Draidi

Hi Ahmad,

On 23 October 2014 00:08, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> Hello, Mr. Glass.
>
> On Wednesday 22 October 2014 11:29:00 AM Simon Glass wrote:
>> One little nit below but it looks OK to me. I'm assume that no one
>> would want to replace the command line completely?
>>
>
> In some setups, one image can be used with several versions, or even
>     different boards of hardware. The bootloader passes a command line
>     argument to specify this. Also, the header file for the mkbootimg
>     specifies that the "kernel_args[] is appended to the kernel
>     commandline". So I thought this would make it consistent with other
>     bootloaders, and the spec.
>
>> 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.
>
> Thanks for the tip.
> I'll try to write a test if I get the time.
> One thing comes to mind. To be able to write a test, we'll need an image
>     to test against. I think pulling in the mkbootimg tool is the right move.
>     The other option I can think of is bundling a small dummy image with
>     U-Boot, which I think is a bit ugly. What do you think?

Yes bringing in the tool sounds good. It should probably be integrated
as just another output from mkimage.

>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> nit: return -ENOMEM - suggest adding a comment to
>> android_image_get_kernel() so its args and return value are
>> documented.
>>
>
> Thank you. I'll send in a new version soon.
>
>> Regards,
>> Simon
>
> Regards,
> Ahmad Draidi

Regards.
Simon