Patchwork [U-Boot,v2,18/22] armv7: embed u-boot size within u-boot for use from SPL

login
register
mail settings
Submitter Aneesh V
Date May 15, 2011, 3:21 p.m.
Message ID <1305472900-4004-19-git-send-email-aneesh@ti.com>
Download mbox | patch
Permalink /patch/95642/
State Changes Requested
Headers show

Comments

Aneesh V - May 15, 2011, 3:21 p.m.
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(-)
Wolfgang Denk - May 15, 2011, 8:09 p.m.
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
Scott Wood - May 16, 2011, 6:56 p.m.
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
Aneesh V - May 18, 2011, 4:49 a.m.
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
Aneesh V - May 18, 2011, 5:02 a.m.
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
Wolfgang Denk - May 18, 2011, 6:06 a.m.
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
Aneesh V - May 26, 2011, 11:08 a.m.
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?
Wolfgang Denk - May 26, 2011, 5:21 p.m.
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

Patch

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)