| Message ID | 1317412383-7731-1-git-send-email-swarren@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Stephen Warren wrote at Friday, September 30, 2011 1:53 PM: > uImage files contain absolute "load" and "entry" addresses. Such a concept > is incompatible with using the same kernel image on multiple SoCs, each with > a potentially different SDRAM base. To support that, augment the FIT image > syntax with a "relative-addresses" property, which indicates that the "load" > and "entry" properties are an offset from SDRAM, rather than an absolute > address. Wolfgang, does this change look like a reasonable approach? > In theory, a similar change could be made to the legacy uImage format. > However, representing the a "relative-addresses" flag in that format is > problematic, so I have ignored that possibility for now. What are your thoughts on the legacy format; should we ignore it? We could probably steal some bit in one of the fields for this flag, although technically that would be a change to the format, with potential compatibility issues. Thanks.
Dear Stephen Warren, In message <1317412383-7731-1-git-send-email-swarren@nvidia.com> you wrote: > uImage files contain absolute "load" and "entry" addresses. Such a concept > is incompatible with using the same kernel image on multiple SoCs, each with > a potentially different SDRAM base. To support that, augment the FIT image > syntax with a "relative-addresses" property, which indicates that the "load" > and "entry" properties are an offset from SDRAM, rather than an absolute > address. > > In theory, a similar change could be made to the legacy uImage format. > However, representing the a "relative-addresses" flag in that format is > problematic, so I have ignored that possibility for now. ... > + - relative-addresses : Indicates the the values of the load and entry > + properties are to be interpreted as relative to the base of SDRAM, rather > + than as an absolute values. How is "base of SDRAM" defined, for example in the case of several non-contiguously mapped banks of RAM? Please add this to the comment. Best regards, Wolfgang Denk
Dear Stephen Warren, In message <74CDBE0F657A3D45AFBB94109FB122FF173A2C7568@HQMAIL01.nvidia.com> you wrote: > > Wolfgang, does this change look like a reasonable approach? Yes, looks good to me - with the nitpick of a bit more explicit comment. See previous message. > What are your thoughts on the legacy format; should we ignore it? We > could probably steal some bit in one of the fields for this flag, although > technically that would be a change to the format, with potential > compatibility issues. I would really like to see this supported as well - so far only a minority of users use FIT images. I suggest we define a new image type (say, IH_TYPE_KERNEL_REL) for this purpose. This would probably cause the least conflicts. Best regards, Wolfgang Denk
Wolfgang Denk wrote at Thursday, October 06, 2011 1:20 PM: > In message <74CDBE0F657A3D45AFBB94109FB122FF173A2C7568@HQMAIL01.nvidia.com> you wrote: > > > > Wolfgang, does this change look like a reasonable approach? > > Yes, looks good to me - with the nitpick of a bit more explicit > comment. See previous message. > > > What are your thoughts on the legacy format; should we ignore it? We > > could probably steal some bit in one of the fields for this flag, although > > technically that would be a change to the format, with potential > > compatibility issues. > > I would really like to see this supported as well - so far only a > minority of users use FIT images. > > I suggest we define a new image type (say, IH_TYPE_KERNEL_REL) for > this purpose. This would probably cause the least conflicts. OK. Wouldn't we also need e.g. IH_TYPE_STANDALONE_REL, IH_TYPE_RAMDISK_REL and perhaps other too?
Dear Stephen Warren, In message <74CDBE0F657A3D45AFBB94109FB122FF173A2C75C4@HQMAIL01.nvidia.com> you wrote: > > > I suggest we define a new image type (say, IH_TYPE_KERNEL_REL) for > > this purpose. This would probably cause the least conflicts. > > OK. Wouldn't we also need e.g. IH_TYPE_STANDALONE_REL, IH_TYPE_RAMDISK_REL > and perhaps other too? I don't think so - either these don't have (and use) load and entry point addesses (like in the ramdisk case), or they are not easily relocatable in the same way as the Linux kernel. No, at the moment only the Linux kernel images need to support tis property. OK, eventually this might be useful for multi-part images, too, but these have been deprecated for a long time, and here the user should just use a FIT image instead, Best regards, Wolfgang Denk
Wolfgang Denk wrote at Thursday, October 06, 2011 2:52 PM: > In message <74CDBE0F657A3D45AFBB94109FB122FF173A2C75C4@HQMAIL01.nvidia.com> you wrote: > > > > > I suggest we define a new image type (say, IH_TYPE_KERNEL_REL) for > > > this purpose. This would probably cause the least conflicts. > > > > OK. Wouldn't we also need e.g. IH_TYPE_STANDALONE_REL, IH_TYPE_RAMDISK_REL > > and perhaps other too? > > I don't think so - either these don't have (and use) load and entry > point addesses (like in the ramdisk case), or they are not easily > relocatable in the same way as the Linux kernel. Sorry, I don't quite understand then; in both boot_get_ramdisk() and boot_get_fdt(), I see calls to image_get_load() and fit_image_get_load(). I guess in the ramdisk case, this just sets rd_load, which isn't used, but in the FDT case, it sets load_start, which is used to derive load_end and fdt_blob. > No, at the moment only the Linux kernel images need to support tis > property. > > OK, eventually this might be useful for multi-part images, too, but > these have been deprecated for a long time, and here the user should > just use a FIT image instead,
Dear Stephen Warren, In message <74CDBE0F657A3D45AFBB94109FB122FF173A2C7619@HQMAIL01.nvidia.com> you wrote: > > > > OK. Wouldn't we also need e.g. IH_TYPE_STANDALONE_REL, IH_TYPE_RAMDISK_REL > > > and perhaps other too? > > > > I don't think so - either these don't have (and use) load and entry > > point addesses (like in the ramdisk case), or they are not easily > > relocatable in the same way as the Linux kernel. > > Sorry, I don't quite understand then; in both boot_get_ramdisk() and > boot_get_fdt(), I see calls to image_get_load() and fit_image_get_load(). Agreed for FDT images, but you wrote IH_TYPE_STANDALONE_REL - standanole images are something differend, and usually not relocatable. And ramdisks have neither a load address nor an entry point associated with them; no matter where they are located in memory, we just pass their address to the Linux kernel. > I guess in the ramdisk case, this just sets rd_load, which isn't used, > but in the FDT case, it sets load_start, which is used to derive load_end > and fdt_blob. Agreed. But that's not what you wrote above. Best regards, Wolfgang Denk
diff --git a/common/image.c b/common/image.c index d38ce4a..e62fc76 100644 --- a/common/image.c +++ b/common/image.c @@ -2299,6 +2299,13 @@ int fit_image_get_load (const void *fit, int noffset, ulong *load) } *load = uimage_to_cpu (*data); + +#ifndef USE_HOSTCC + data = fdt_getprop(fit, noffset, FIT_REL_ADDRS_PROP, &len); + if (data != NULL) + *load += getenv_bootm_low(); +#endif + return 0; } @@ -2327,6 +2334,13 @@ int fit_image_get_entry (const void *fit, int noffset, ulong *entry) } *entry = uimage_to_cpu (*data); + +#ifndef USE_HOSTCC + data = fdt_getprop(fit, noffset, FIT_REL_ADDRS_PROP, &len); + if (data != NULL) + *entry += getenv_bootm_low(); +#endif + return 0; } diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt index 6d20707..c2ae67e 100644 --- a/doc/uImage.FIT/source_file_format.txt +++ b/doc/uImage.FIT/source_file_format.txt @@ -183,6 +183,9 @@ the '/images' node should have the following layout: Optional nodes: - hash@1 : Each hash sub-node represents separate hash or checksum calculated for node's data according to specified algorithm. + - relative-addresses : Indicates the the values of the load and entry + properties are to be interpreted as relative to the base of SDRAM, rather + than as an absolute values. 5) Hash nodes diff --git a/include/image.h b/include/image.h index 352e4a0..aefba5d 100644 --- a/include/image.h +++ b/include/image.h @@ -538,6 +538,7 @@ static inline int image_check_target_arch (const image_header_t *hdr) #define FIT_COMP_PROP "compression" #define FIT_ENTRY_PROP "entry" #define FIT_LOAD_PROP "load" +#define FIT_REL_ADDRS_PROP "relative-addresses" /* configuration node */ #define FIT_KERNEL_PROP "kernel"
uImage files contain absolute "load" and "entry" addresses. Such a concept is incompatible with using the same kernel image on multiple SoCs, each with a potentially different SDRAM base. To support that, augment the FIT image syntax with a "relative-addresses" property, which indicates that the "load" and "entry" properties are an offset from SDRAM, rather than an absolute address. In theory, a similar change could be made to the legacy uImage format. However, representing the a "relative-addresses" flag in that format is problematic, so I have ignored that possibility for now. Signed-off-by: Stephen Warren <swarren@nvidia.com> --- common/image.c | 14 ++++++++++++++ doc/uImage.FIT/source_file_format.txt | 3 +++ include/image.h | 1 + 3 files changed, 18 insertions(+), 0 deletions(-)