[U-Boot] [PATCH v2 REPOST 2/3] image: Implement IH_TYPE_KERNEL_REL

Simon Glass sjg at chromium.org
Wed Oct 19 02:56:44 CEST 2011


Hi Stephen,

Here are a few comments which may or may not be useful. In general it
seems that the need to reduce code size change to absolute minimum is
making the code a bit painful. Maybe relax that by a few 10s of bytes?

The code size increase you see may be section alignment - e.g. if you
increase the size of a section by 4 that might push the next section
up to the next 32-byte boundary.

On Tue, Oct 18, 2011 at 2:11 PM, Stephen Warren <swarren at nvidia.com> wrote:

[snip]
> --- a/arch/arm/cpu/armv7/omap-common/spl.c
> +++ b/arch/arm/cpu/armv7/omap-common/spl.c
> @@ -68,7 +68,7 @@ void spl_parse_image_header(const struct image_header *header)
>
>        if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
>                spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
> -               spl_image.entry_point = __be32_to_cpu(header->ih_load);
> +               spl_image.entry_point = __be32_to_cpu(header->ih_load_raw);

I don't completely understand why raw is used here (and in some other
boards) - is it simply because the abs and raw are always the same in
SPL/these boards, or something else?


> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -272,6 +273,9 @@ static int bootm_start(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]
>        }
>
>        if (((images.os.type == IH_TYPE_KERNEL) ||
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +            (images.os.type == IH_TYPE_KERNEL_REL) ||
> +#endif

Is it worth saving 4 bytes for this #ifdef?


> --- a/common/image.c
> +++ b/common/image.c
> @@ -133,7 +133,14 @@ static const table_entry_t uimage_type[] = {
>        {       IH_TYPE_FILESYSTEM, "filesystem", "Filesystem Image",   },
>        {       IH_TYPE_FIRMWARE,   "firmware",   "Firmware",           },
>        {       IH_TYPE_FLATDT,     "flat_dt",    "Flat Device Tree",   },
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +       {       IH_TYPE_FLATDT_REL, "flat_dt_rel",
> +                                           "Relative Flat Device Tree",},
> +#endif
>        {       IH_TYPE_KERNEL,     "kernel",     "Kernel Image",       },
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +       {       IH_TYPE_KERNEL_REL, "kernel_rel", "Relative Kernel Image",},
> +#endif

You could perhaps put the two relative items together as it doesn't
look like the order matters.

> @@ -188,6 +198,20 @@ int image_check_dcrc(const image_header_t *hdr)
>        return (dcrc == image_get_dcrc(hdr));
>  }
>
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +uint32_t image_get_load_abs(const image_header_t *hdr)
> +{
> +       return image_addr_raw_to_abs(image_get_type(hdr),
> +                                       image_get_load_raw(hdr));
> +}
> +
> +uint32_t image_get_ep_abs(const image_header_t *hdr)
> +{
> +       return image_addr_raw_to_abs(image_get_type(hdr),
> +                                       image_get_ep_raw(hdr));
> +}
> +#endif

Perhaps if you have a function like image_addr_raw_to_abs() for the
fdt case, you could reduce the code in
fit_image_get_load/entry_abs()?

> @@ -314,8 +342,24 @@ void image_print_contents(const void *ptr)
>        image_print_type(hdr);
>        printf("%sData Size:    ", p);
>        genimg_print_size(image_get_data_size(hdr));
> -       printf("%sLoad Address: %08x\n", p, image_get_load(hdr));
> -       printf("%sEntry Point:  %08x\n", p, image_get_ep(hdr));
> +
> +       abs = image_get_load_abs(hdr);
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +       raw = image_get_load_raw(hdr);
> +       if (abs != raw)
> +               printf("%sLoad Address: %08x (relative %08x)\n", p, abs, raw);
> +       else
> +#endif
> +               printf("%sLoad Address: %08x\n", p, abs);
> +
> +       abs = image_get_ep_abs(hdr);
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +       raw = image_get_ep_raw(hdr);
> +       if (abs != raw)
> +               printf("%sEntry Point:  %08x (relative %08x)\n", p, abs, raw);
> +       else
> +#endif

Any mileage in something like this?

print_address(p, "Load", image_get_load_abs(hdr), image_get_load_raw(hdr));
print_address(p, "Entry", image_get_entry_abs(hdr), image_get_entry_raw(hdr));

static void do_address(const char *p, const char *name, ulong abs, ulong raw)
{
printf("%s%s Point:  %08x", p, name, abs);
#ifdef CONFIG_SYS_RELATIVE_IMAGES
 if (abs != raw)
     printf(" (relative %08x)", p, raw);
#endif
puts("\n");
}

> +               printf("%sEntry Point:  %08x\n", p, abs);
>
>        if (image_check_type(hdr, IH_TYPE_MULTI) ||
>                        image_check_type(hdr, IH_TYPE_SCRIPT)) {
> @@ -425,6 +469,10 @@ ulong getenv_bootm_low(void)
>                return tmp;
>        }
>
> +       /*
> +        * If this code changes, please modify the comments in image.h that
> +        * describe IH_TYPE_xxx_REL, in the "Image Types" list.
> +        */
>  #if defined(CONFIG_SYS_SDRAM_BASE)
>        return CONFIG_SYS_SDRAM_BASE;
>  #elif defined(CONFIG_ARM)


> @@ -2003,35 +2079,75 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>                genimg_print_size(size);
>
>        /* Remaining, type dependent properties */
> -       if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
> -           (type == IH_TYPE_RAMDISK) || (type == IH_TYPE_FIRMWARE) ||
> -           (type == IH_TYPE_FLATDT)) {
> +       if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_FLATDT) ||
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +           (type == IH_TYPE_KERNEL_REL) || (type == IH_TYPE_FLATDT_REL) ||
> +#endif
> +           (type == IH_TYPE_STANDALONE) || (type == IH_TYPE_RAMDISK) ||
> +           (type == IH_TYPE_FIRMWARE)) {

Perhaps define a macro like ih_type_has_architecture() in the header?
Not sure that Wolfgang likes that sort of thing though.

>                fit_image_get_arch(fit, image_noffset, &arch);
>                printf("%s  Architecture: %s\n", p, genimg_get_arch_name(arch));
>        }
>
> -       if (type == IH_TYPE_KERNEL) {
> +       if ((type == IH_TYPE_KERNEL)
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +           || (type == IH_TYPE_KERNEL_REL)
> +#endif
> +           ) {
>                fit_image_get_os(fit, image_noffset, &os);
>                printf("%s  OS:           %s\n", p, genimg_get_os_name(os));
>        }
>
> -       if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
> -               (type == IH_TYPE_FIRMWARE)) {
> -               ret = fit_image_get_load(fit, image_noffset, &load);
> +       if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_FLATDT) ||
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +           (type == IH_TYPE_KERNEL_REL) || (type == IH_TYPE_FLATDT_REL) ||
> +#endif

Again here and above wonder whether the code size reduction is worth the #ifdefs

> +           (type == IH_TYPE_STANDALONE) || (type == IH_TYPE_FIRMWARE)) {
> +               ret = fit_image_get_load_abs(fit, image_noffset, &abs);
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +               /*
> +                * fit_image_get_load_* return 0 on success, other on failure.
> +                * Oring two such values together yields something that's
> +                * still 0 on success, other on failure.
> +                */
> +               ret |= fit_image_get_load_raw(fit, image_noffset, &raw);
> +#endif
>                printf("%s  Load Address: ", p);
> -               if (ret)
> +               if (ret) {
>                        printf("unavailable\n");
> -               else
> -                       printf("0x%08lx\n", load);
> +               } else {
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +                       if (abs != raw)
> +                               printf("0x%08lx (relative 0x%08lx)\n",
> +                                       abs, raw);
> +                       else
> +#endif
> +                               printf("0x%08lx\n", abs);
> +               }

Another use for the function perhaps?

>        }
>
> -       if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE)) {
> -               fit_image_get_entry(fit, image_noffset, &entry);
> +       if ((type == IH_TYPE_KERNEL) ||
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +           (type == IH_TYPE_KERNEL_REL) ||
> +#endif
> +           (type == IH_TYPE_STANDALONE)) {
> +               ret = fit_image_get_entry_abs(fit, image_noffset, &abs);
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +               /* See comment about fit_image_get_load_* above */
> +               ret |= fit_image_get_entry_raw(fit, image_noffset, &raw);
> +#endif
>                printf("%s  Entry Point:  ", p);
> -               if (ret)
> +               if (ret) {
>                        printf("unavailable\n");
> -               else
> -                       printf("0x%08lx\n", entry);
> +               } else {
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +                       if (abs != raw)
> +                               printf("0x%08lx (relative 0x%08lx)\n",
> +                                       abs, raw);
> +                       else
> +#endif
> +                               printf("0x%08lx\n", abs);
> +               }

another one!


> @@ -2346,20 +2467,63 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>        return 0;
>  }
>
> -/**
> - * fit_image_get_entry - get entry point address property for a given component image node
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +/*
> + * fit_image_get_load_abs - get absolute load address property for a given
> + * component image node
> + * @fit: pointer to the FIT format image header
> + * @noffset: component image node offset
> + * @load: pointer to the uint32_t, will hold load address
> + *
> + * fit_image_get_load_abs() finds load address property in a given component
> + *  image node. If the property is found, its value is returned to the caller.
> + *
> + * Note that this function returns the absolute value that U-Boot should
> + * use when actually loading images, or relocating them to the load address.
> + *
> + * returns:
> + *     0, on success
> + *     -1, on failure
> + */
> +int fit_image_get_load_abs(const void *fit, int noffset, ulong *load)
> +{
> +       int ret;
> +       ulong raw;
> +       uint8_t type;
> +
> +       ret = fit_image_get_load_raw(fit, noffset, &raw);
> +       if (ret)
> +               return ret;
> +
> +       ret = fit_image_get_type(fit, noffset, &type);
> +       if (ret)
> +               return ret;
> +
> +       *load = image_addr_raw_to_abs(type, raw);
> +       return 0;
> +}
> +#endif

I wonder (given there are so few callers) whether it might be worth
just having a function (for each of raw and abs) that returns both the
load and entry addresses in one shot?


> --- a/include/image.h
> +++ b/include/image.h
> @@ -160,6 +168,10 @@
>  #define IH_TYPE_IMXIMAGE       10      /* Freescale IMXBoot Image      */
>  #define IH_TYPE_UBLIMAGE       11      /* Davinci UBL Image            */
>  #define IH_TYPE_OMAPIMAGE      12      /* TI OMAP Config Header Image  */
> +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> +#define IH_TYPE_KERNEL_REL     13      /* OS Kernel Image              */
> +#define IH_TYPE_FLATDT_REL     14      /* Binary Flat Device Tree Blob */
> +#endif



More information about the U-Boot mailing list