Patchwork [U-Boot,1/2] image: Support relative-addresses property in FIT images

login
register
mail settings
Submitter Stephen Warren
Date Sept. 30, 2011, 7:53 p.m.
Message ID <1317412383-7731-1-git-send-email-swarren@nvidia.com>
Download mbox | patch
Permalink /patch/117197/
State Changes Requested
Headers show

Comments

Stephen Warren - Sept. 30, 2011, 7:53 p.m.
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(-)
Stephen Warren - Oct. 6, 2011, 6:07 p.m.
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.
Wolfgang Denk - Oct. 6, 2011, 7:17 p.m.
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
Wolfgang Denk - Oct. 6, 2011, 7:20 p.m.
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
Stephen Warren - Oct. 6, 2011, 7:59 p.m.
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?
Wolfgang Denk - Oct. 6, 2011, 8:51 p.m.
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
Stephen Warren - Oct. 6, 2011, 9:03 p.m.
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,
Wolfgang Denk - Oct. 13, 2011, 7:56 p.m.
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

Patch

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"