[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