Hi York, On 1 September 2015 at 22:01, York Sun <[email protected]> wrote: > FIT image supports more than 32 bits in addresses by using #address-cell > field. However the address length is not handled when parsing FIT images. > Beside, the variable used to host address has "ulong" type. It is OK for > the target, but not always enough for host tools such as mkimage. This > patch replaces "ulong" with "phys_addr_t" to make sure the address is > correct for both the target and the host.
This looks right to me but I have a few comments. > > Signed-off-by: York Sun <[email protected]> > > --- > > common/bootm.c | 13 +++++++------ > common/image-fit.c | 55 > +++++++++++++++++++++++++++++++++++++--------------- > include/bootm.h | 6 +++--- > include/image.h | 12 ++++++++---- > 4 files changed, 57 insertions(+), 29 deletions(-) > > diff --git a/common/bootm.c b/common/bootm.c > index 667c934..0672e73 100644 > --- a/common/bootm.c > +++ b/common/bootm.c > @@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t > uncomp_size, > return BOOTM_ERR_RESET; > } > > -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, > - void *load_buf, void *image_buf, ulong image_len, > - uint unc_len, ulong *load_end) > +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start, > + int type, void *load_buf, void *image_buf, > + ulong image_len, uint unc_len, ulong *load_end) > { > int ret = 0; > > @@ -883,7 +883,8 @@ void memmove_wd(void *to, void *from, size_t len, ulong > chunksz) > static int bootm_host_load_image(const void *fit, int req_image_type) > { > const char *fit_uname_config = NULL; > - ulong data, len; > + phys_addr_t *data = NULL; > + ulong len; > bootm_headers_t images; > int noffset; > ulong load_end; > @@ -897,7 +898,7 @@ static int bootm_host_load_image(const void *fit, int > req_image_type) > noffset = fit_image_load(&images, (ulong)fit, > NULL, &fit_uname_config, > IH_ARCH_DEFAULT, req_image_type, -1, > - FIT_LOAD_IGNORED, &data, &len); > + FIT_LOAD_IGNORED, data, &len); > if (noffset < 0) > return noffset; > if (fit_image_get_type(fit, noffset, &image_type)) { > @@ -912,7 +913,7 @@ static int bootm_host_load_image(const void *fit, int > req_image_type) > > /* Allow the image to expand by a factor of 4, should be safe */ > load_buf = malloc((1 << 20) + len * 4); > - ret = bootm_decomp_image(imape_comp, 0, data, image_type, load_buf, > + ret = bootm_decomp_image(imape_comp, 0, *data, image_type, load_buf, > (void *)data, len, CONFIG_SYS_BOOTM_LEN, > &load_end); > free(load_buf); > diff --git a/common/image-fit.c b/common/image-fit.c > index 28f7aa8..513cfdc 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -358,7 +358,7 @@ void fit_image_print(const void *fit, int image_noffset, > const char *p) > char *desc; > uint8_t type, arch, os, comp; > size_t size; > - ulong load, entry; > + phys_addr_t load, entry; > const void *data; > int noffset; > int ndepth; > @@ -428,17 +428,17 @@ void fit_image_print(const void *fit, int > image_noffset, const char *p) > if (ret) > printf("unavailable\n"); > else > - printf("0x%08lx\n", load); > + printf("0x%08llx\n", (uint64_t)load); > } > > if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) || > (type == IH_TYPE_RAMDISK)) { > - fit_image_get_entry(fit, image_noffset, &entry); > + ret = fit_image_get_entry(fit, image_noffset, &entry); > printf("%s Entry Point: ", p); > if (ret) > printf("unavailable\n"); > else > - printf("0x%08lx\n", entry); > + printf("0x%08llx\n", (uint64_t)entry); If the address is 32-bit we cast it to 64-bit and print 8 digits. If it is 64-bit we print as many digits as we can find. I think this behaviour is reasonable - and avoids hopelessly confusing 16-character hex strings with lots of leading zeros. But the code looks a bit odd. Do you think we should add a % formatter to print a phys_addr_t? > } > > /* Process all hash subnodes of the component image node */ > @@ -679,7 +679,7 @@ int fit_image_get_comp(const void *fit, int noffset, > uint8_t *comp) > * fit_image_get_load() - get load addr property for 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 > + * @load: pointer to the phys_addr_t, will hold load address > * > * fit_image_get_load() finds load address property in a given component > * image node. If the property is found, its value is returned to the caller. > @@ -688,7 +688,7 @@ int fit_image_get_comp(const void *fit, int noffset, > uint8_t *comp) > * 0, on success > * -1, on failure > */ > -int fit_image_get_load(const void *fit, int noffset, ulong *load) > +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load) > { > int len; > const uint32_t *data; > @@ -699,7 +699,18 @@ int fit_image_get_load(const void *fit, int noffset, > ulong *load) > return -1; > } > > - *load = uimage_to_cpu(*data); > + if (len == 4) { > + *load = uimage_to_cpu(*data); > + } else if (len == 8 && sizeof(*load) >= 8) { > + *load = uimage_to_cpu(data[0]); > + *load <<= 32; > + *load |= uimage_to_cpu(data[1]); > + } else { > + printf("Unsupported load address length %d (%zd)\n", > + len, sizeof(*load)); > + return -1; Can this use fdtdec_get_number()? I'm not sure we need this error message. > + } > + > return 0; > } > > @@ -707,7 +718,7 @@ int fit_image_get_load(const void *fit, int noffset, > ulong *load) > * fit_image_get_entry() - get entry point address property > * @fit: pointer to the FIT format image header > * @noffset: component image node offset > - * @entry: pointer to the uint32_t, will hold entry point address > + * @entry: pointer to the phys_addr_t, will hold entry point address > * > * This gets the entry point address property for a given component image > * node. > @@ -720,7 +731,7 @@ int fit_image_get_load(const void *fit, int noffset, > ulong *load) > * 0, on success > * -1, on failure > */ > -int fit_image_get_entry(const void *fit, int noffset, ulong *entry) > +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry) > { > int len; > const uint32_t *data; > @@ -731,7 +742,18 @@ int fit_image_get_entry(const void *fit, int noffset, > ulong *entry) > return -1; > } > > - *entry = uimage_to_cpu(*data); > + if (len == 4) { > + *entry = uimage_to_cpu(*data); > + } else if (len == 8 && sizeof(*entry) >= 8) { > + *entry = uimage_to_cpu(data[0]); > + *entry <<= 32; > + *entry |= uimage_to_cpu(data[1]); > + } else { > + printf("Unsupported entry address length %d (%zd)\n", > + len, sizeof(*entry)); > + return -1; Ditto - in fact this code seems duplicated. > + } > + > return 0; > } > > @@ -1554,7 +1576,7 @@ static const char *fit_get_image_type_property(int type) > int fit_image_load(bootm_headers_t *images, ulong addr, > const char **fit_unamep, const char **fit_uname_configp, > int arch, int image_type, int bootstage_id, > - enum fit_load_op load_op, ulong *datap, ulong *lenp) > + enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp) > { > int cfg_noffset, noffset; > const char *fit_uname; > @@ -1563,7 +1585,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > const void *buf; > size_t size; > int type_ok, os_ok; > - ulong load, data, len; > + phys_addr_t load; > + ulong data, len; > uint8_t os; > const char *prop_name; > int ret; > @@ -1719,7 +1742,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > } > } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { > ulong image_start, image_end; > - ulong load_end; > + phys_addr_t load_end; > void *dst; > > /* > @@ -1736,8 +1759,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > return -EXDEV; > } > > - printf(" Loading %s from 0x%08lx to 0x%08lx\n", > - prop_name, data, load); > + printf(" Loading %s from 0x%08lx to %08llx\n", > + prop_name, data, (uint64_t)load); > > dst = map_sysmem(load, len); > memmove(dst, buf, len); > @@ -1756,7 +1779,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > } > > int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch, > - ulong *setup_start, ulong *setup_len) > + phys_addr_t *setup_start, ulong *setup_len) > { > int noffset; > ulong addr; > diff --git a/include/bootm.h b/include/bootm.h > index 4981377..f280ace 100644 > --- a/include/bootm.h > +++ b/include/bootm.h > @@ -69,8 +69,8 @@ void arch_preboot_os(void); > * @unc_len: Available space for decompression > * @return 0 if OK, -ve on error (BOOTM_ERR_...) > */ > -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, > - void *load_buf, void *image_buf, ulong image_len, > - uint unc_len, ulong *load_end); > +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start, > + int type, void *load_buf, void *image_buf, > + ulong image_len, uint unc_len, ulong *load_end); > > #endif > diff --git a/include/image.h b/include/image.h > index 63c3d37..bc41ecb 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -33,11 +33,15 @@ struct lmb; > #define IMAGE_ENABLE_IGNORE 0 > #define IMAGE_INDENT_STRING "" > > +/* Be able to hold physical address */ > +typedef unsigned long long phys_addr_t; > + > #else > > #include <lmb.h> > #include <asm/u-boot.h> > #include <command.h> > +#include <linux/types.h> > > /* Take notice of the 'ignore' property for hashes */ > #define IMAGE_ENABLE_IGNORE 1 > @@ -494,7 +498,7 @@ int boot_get_loadable(int argc, char * const argv[], > bootm_headers_t *images, > #endif /* !USE_HOSTCC */ > > int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch, > - ulong *setup_start, ulong *setup_len); > + phys_addr_t *setup_start, ulong *setup_len); > > /** > * fit_image_load() - load an image from a FIT > @@ -529,7 +533,7 @@ int boot_get_setup_fit(bootm_headers_t *images, uint8_t > arch, > int fit_image_load(bootm_headers_t *images, ulong addr, > const char **fit_unamep, const char **fit_uname_configp, > int arch, int image_type, int bootstage_id, > - enum fit_load_op load_op, ulong *datap, ulong *lenp); > + enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp); > > #ifndef USE_HOSTCC > /** > @@ -840,8 +844,8 @@ int fit_image_get_os(const void *fit, int noffset, > uint8_t *os); > int fit_image_get_arch(const void *fit, int noffset, uint8_t *arch); > int fit_image_get_type(const void *fit, int noffset, uint8_t *type); > int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp); > -int fit_image_get_load(const void *fit, int noffset, ulong *load); > -int fit_image_get_entry(const void *fit, int noffset, ulong *entry); > +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load); > +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry); > int fit_image_get_data(const void *fit, int noffset, > const void **data, size_t *size); > > -- > 1.7.9.5 > Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
