| Message ID | 1305472900-4004-19-git-send-email-aneesh@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Aneesh V, In message <1305472900-4004-19-git-send-email-aneesh@ti.com> you wrote: > Embed the u-boot flash image size at a known offset from the > start of u-boot so that SPL can use it while loading u-boot > from a non-XIP media. I don't think this is a good idea. What you are doing here is defining an image format. Such an image format must be good enough not only for OMAP4 and for loading U-Boot as second stage, but for all other architectures and use cases as well. We therefore should be very careful not to oversimplify things here. I suggest NOT to pot this information into the image itself, but rather to prepend it (for example, using the mkimage tool), so the actual payload image needs not to be modified. Best regards, Wolfgang Denk
On Sun, 15 May 2011 20:51:36 +0530 Aneesh V <aneesh@ti.com> wrote: > Embed the u-boot flash image size at a known offset from the > start of u-boot so that SPL can use it while loading u-boot > from a non-XIP media. > > Signed-off-by: Aneesh V <aneesh@ti.com> > V2: > * Removed the linker script label '__flash_image_end' and its usage. > Instead '_end' is used now > --- > arch/arm/cpu/armv7/start.S | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S > index f92c6d9..f676d7d 100644 > --- a/arch/arm/cpu/armv7/start.S > +++ b/arch/arm/cpu/armv7/start.S > @@ -64,8 +64,12 @@ _pad: .word 0x12345678 /* now 16*4=64 */ > > .global _end_vect > _end_vect: > +.global _u_boot_size > +_u_boot_size: > + .word 0xDEADBEEF > + .word _end - _start 0xdeadbeef does not seem like a good magic value to identify this header format -- especially since it looks like that may have been the value present in the older images that don't have this header (depending on whether the .balignl needed to pad). -Scott
Hi Scott, On Tuesday 17 May 2011 12:26 AM, Scott Wood wrote: > On Sun, 15 May 2011 20:51:36 +0530 > Aneesh V<aneesh@ti.com> wrote: > >> Embed the u-boot flash image size at a known offset from the >> start of u-boot so that SPL can use it while loading u-boot >> from a non-XIP media. >> >> Signed-off-by: Aneesh V<aneesh@ti.com> >> V2: >> * Removed the linker script label '__flash_image_end' and its usage. >> Instead '_end' is used now >> --- >> arch/arm/cpu/armv7/start.S | 6 +++++- >> 1 files changed, 5 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S >> index f92c6d9..f676d7d 100644 >> --- a/arch/arm/cpu/armv7/start.S >> +++ b/arch/arm/cpu/armv7/start.S >> @@ -64,8 +64,12 @@ _pad: .word 0x12345678 /* now 16*4=64 */ >> >> .global _end_vect >> _end_vect: >> +.global _u_boot_size >> +_u_boot_size: >> + .word 0xDEADBEEF >> + .word _end - _start > > 0xdeadbeef does not seem like a good magic value to identify this header > format -- especially since it looks like that may have been the value > present in the older images that don't have this header (depending on > whether the .balignl needed to pad). Thanks for pointing this out. I will change it to something different. best regards, Aneesh
Hi Wolfgang, On Monday 16 May 2011 01:39 AM, Wolfgang Denk wrote: > Dear Aneesh V, > > In message<1305472900-4004-19-git-send-email-aneesh@ti.com> you wrote: >> Embed the u-boot flash image size at a known offset from the >> start of u-boot so that SPL can use it while loading u-boot >> from a non-XIP media. > > I don't think this is a good idea. > > What you are doing here is defining an image format. Such an image > format must be good enough not only for OMAP4 and for loading U-Boot > as second stage, but for all other architectures and use cases as > well. I am not defining and publishing a format for the external world. It's just an internal information much like any other info embedded in start.S such as _bss_end_ofs, _end_ofs etc. It's just that SPL, being an insider, can take advantage of it. I feel creating a new mkimage target just for this will be an overkill. I will be happy loading the maximum image size always. best regards, Aneesh
Dear Aneesh V, In message <4DD352EA.3090007@ti.com> you wrote: > > > What you are doing here is defining an image format. Such an image > > format must be good enough not only for OMAP4 and for loading U-Boot > > as second stage, but for all other architectures and use cases as > > well. > > I am not defining and publishing a format for the external world. It's In the essence this is what you are doing: you are defining an interface how the payload has to look like. Anything not meeting this "specification" or "image format" or "ABI" or whatever you call it will not be directly loadable by the SPL. But I want to make sure that we can load arbitrary payloads as second stage, not only U-Boot images. Because we cannot guarantee that we can embed such information into other payloads we may want to load, we must prepend or append such information, but not embed it somewhere within the image itself. > just an internal information much like any other info embedded in > start.S such as _bss_end_ofs, _end_ofs etc. It's just that SPL, being > an insider, can take advantage of it. This is not quite correct. SPL needs this information. We are defining an API here, and we should make sure it fits the use cases we can see now (and ideally is flexible enough so we can adjust/extend it for future use). > I feel creating a new mkimage target just for this will be an overkill. We don't have to create a new format. We can, for example, use the existing format with a prepended 64 byte header. The only thing that needs changes is that we now have to consider the header when loading the image, but the same problem will always exist when we accept that we cannot embed the information within the payload. We don't need any new code to implement this solution. We can easily create the images using "mkimage -T firmware -O u-boot". > I will be happy loading the maximum image size always. This makes no sense. What is "the maximum image size always"? 4 MB, so we can also load a Linux kernel as payload? 8 MB, so we can stil do this in a year from now? Best regards, Wolfgang Denk
Hi Wolfgang, On Wednesday 18 May 2011 11:36 AM, Wolfgang Denk wrote: > Dear Aneesh V, > > In message<4DD352EA.3090007@ti.com> you wrote: >> >>> What you are doing here is defining an image format. Such an image >>> format must be good enough not only for OMAP4 and for loading U-Boot >>> as second stage, but for all other architectures and use cases as >>> well. >> >> I am not defining and publishing a format for the external world. It's > > In the essence this is what you are doing: you are defining an > interface how the payload has to look like. Anything not meeting this > "specification" or "image format" or "ABI" or whatever you call it > will not be directly loadable by the SPL. This is not exactly true. I had implemented a fall-back option albeit with a maximum size assumed for U-Boot. > > But I want to make sure that we can load arbitrary payloads as second > stage, not only U-Boot images. Because we cannot guarantee that we > can embed such information into other payloads we may want to load, we > must prepend or append such information, but not embed it somewhere > within the image itself. > >> just an internal information much like any other info embedded in >> start.S such as _bss_end_ofs, _end_ofs etc. It's just that SPL, being >> an insider, can take advantage of it. > > This is not quite correct. SPL needs this information. We are defining > an API here, and we should make sure it fits the use cases we can see > now (and ideally is flexible enough so we can adjust/extend it for > future use). Agree that mkimage approach works better for different types of payloads. > >> I feel creating a new mkimage target just for this will be an overkill. > > We don't have to create a new format. We can, for example, use the > existing format with a prepended 64 byte header. The only thing that > needs changes is that we now have to consider the header when loading > the image, but the same problem will always exist when we accept that > we cannot embed the information within the payload. We don't need any > new code to implement this solution. We can easily create the images > using "mkimage -T firmware -O u-boot". The existing target u-boot.img works for me. Just a couple of questions: 1. I see that size is at offset 0xC in this header. Is this a standard? 2. I see that the header is 64 bytes. Is that again a standard for mkimage. 3. Is it ok to add u-boot.img to the target "ALL"? 4. If not, is it ok to add it to "ALL" when CONFIG_SPL is defined? Something like this: ifeq ($(CONFIG_SPL),y) .PHONEY : SPL -ALL += SPL +ALL += SPL u-boot.img endif Is it ok to add support for kernel payload as a subsequent incremental step. That's, right now I intend to parse the mkimage header, find the size and load address and load the image and pass control to it, but *without* passing any parameters. Is that ok?
Dear Aneesh V, In message <4DDE34C5.1050407@ti.com> you wrote: > > 1. I see that size is at offset 0xC in this header. Is this a standard? > 2. I see that the header is 64 bytes. Is that again a standard for > mkimage. Both are not really "standards" in the sense that any standardization group like ANSI or IEEE has approved this, but these are standard within U-Boot context. The header (struct image_header) is defined in "include/image.h" > 3. Is it ok to add u-boot.img to the target "ALL"? No, because in the general case this is not needed, so it's just a waste of build time and disk space. > 4. If not, is it ok to add it to "ALL" when CONFIG_SPL is defined? > Something like this: > > ifeq ($(CONFIG_SPL),y) > .PHONEY : SPL > -ALL += SPL > +ALL += SPL u-boot.img > endif IN principle this should be OK, but please pay attention not to break out-of-tree builds (You have to prefext target names with "$(obj)"). > Is it ok to add support for kernel payload as a subsequent incremental > step. That's, right now I intend to parse the mkimage header, find the > size and load address and load the image and pass control to it, but > *without* passing any parameters. Is that ok? That's perfectly fine. Best regards, Wolfgang Denk
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index f92c6d9..f676d7d 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -64,8 +64,12 @@ _pad: .word 0x12345678 /* now 16*4=64 */ .global _end_vect _end_vect: +.global _u_boot_size +_u_boot_size: + .word 0xDEADBEEF + .word _end - _start - .balignl 16,0xdeadbeef +.balignl 16,0xdeadbeef /************************************************************************* * * Startup Code (reset vector)
Embed the u-boot flash image size at a known offset from the start of u-boot so that SPL can use it while loading u-boot from a non-XIP media. Signed-off-by: Aneesh V <aneesh@ti.com> V2: * Removed the linker script label '__flash_image_end' and its usage. Instead '_end' is used now --- arch/arm/cpu/armv7/start.S | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)