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 <swar...@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
>From what I can tell it doesn't seem like the order matters here. You
could perhaps move the last ones up into the main area, to avoid the
new code in image_check_image_types().
Also I don't think the #ifdef really helps here, except perhaps for
testing that you haven't left something bad in the code. It might be
better to display an error like 'relative images not supported' when
CONFIG_SYS_RELATIVE_IMAGES is not defined.
> @@ -372,10 +384,25 @@ image_get_hdr_l(magic) /* image_get_magic */
> image_get_hdr_l(hcrc) /* image_get_hcrc */
> image_get_hdr_l(time) /* image_get_time */
> image_get_hdr_l(size) /* image_get_size */
> -image_get_hdr_l(load) /* image_get_load */
> -image_get_hdr_l(ep) /* image_get_ep */
> +image_get_hdr_l(load_raw) /* image_get_load_raw */
> +image_get_hdr_l(ep_raw) /* image_get_ep_raw */
might need another tab here (although it isn't visible on email)
> @@ -430,8 +457,8 @@ image_set_hdr_l(magic) /* image_set_magic */
> image_set_hdr_l(hcrc) /* image_set_hcrc */
> image_set_hdr_l(time) /* image_set_time */
> image_set_hdr_l(size) /* image_set_size */
> -image_set_hdr_l(load) /* image_set_load */
> -image_set_hdr_l(ep) /* image_set_ep */
> +image_set_hdr_l(load_raw) /* image_set_load_raw */
> +image_set_hdr_l(ep_raw) /* image_set_ep_raw */
might need another tab here (although it isn't visible on email)
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
