[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Dirk Behme]

The flash header v2 supports different flash offsets for different
boot devices. E.g. parallel NOR or OneNAND use a different offset
than FLASH_OFFSET_STANDARD (== 0x400).

The flash offset is correctly read from the configuration in
parse_cfg_cmd(). But is then overwritten wrongly in set_imx_hdr_v2().

Fix this by removing this overwriting. Use the flash offset
correclty read from the configuration, instead.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
CC: Jason Liu <liu.h.jason@gmail.com>
CC: Stefano Babic <sbabic@denx.de>
---
 tools/imximage.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/tools/imximage.c b/tools/imximage.c
index 1e0f5d4..e9947f1 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -253,9 +253,6 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
 	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
 
-	/* Set default offset */
-	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
-
 	/* Set magic number */
 	fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
 	fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t));
-- 
1.7.0.4

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Stefano Babic]

On 21/02/2012 10:02, Dirk Behme wrote:
> The flash header v2 supports different flash offsets for different
> boot devices. E.g. parallel NOR or OneNAND use a different offset
> than FLASH_OFFSET_STANDARD (== 0x400).
> 
> The flash offset is correctly read from the configuration in
> parse_cfg_cmd(). But is then overwritten wrongly in set_imx_hdr_v2().
> 
> Fix this by removing this overwriting. Use the flash offset
> correclty read from the configuration, instead.
> 
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> CC: Jason Liu <liu.h.jason@gmail.com>
> CC: Stefano Babic <sbabic@denx.de>
> ---

Hi Dirk,

>  tools/imximage.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 1e0f5d4..e9947f1 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -253,9 +253,6 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>  	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
>  	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
>  
> -	/* Set default offset */
> -	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
> -
>  	/* Set magic number */
>  	fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
>  	fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t));


I think we have then a problem when there is not a BOOT_FROM statement
in the configuration file, that let  imxhdr->flash_offset unset.

We need to set it with the default value at the beginning of the
processing or to raise an error if we want to make this statement
mandatory in the configuration file.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Dirk Behme]

On 21.02.2012 18:57, Stefano Babic wrote:
> On 21/02/2012 10:02, Dirk Behme wrote:
>> The flash header v2 supports different flash offsets for different
>> boot devices. E.g. parallel NOR or OneNAND use a different offset
>> than FLASH_OFFSET_STANDARD (== 0x400).
>>
>> The flash offset is correctly read from the configuration in
>> parse_cfg_cmd(). But is then overwritten wrongly in set_imx_hdr_v2().
>>
>> Fix this by removing this overwriting. Use the flash offset
>> correclty read from the configuration, instead.
>>
>> Signed-off-by: Dirk Behme<dirk.behme@de.bosch.com>
>> CC: Jason Liu<liu.h.jason@gmail.com>
>> CC: Stefano Babic<sbabic@denx.de>
>> ---
>
> Hi Dirk,
>
>>   tools/imximage.c |    3 ---
>>   1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 1e0f5d4..e9947f1 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -253,9 +253,6 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>   	imx_header_v2_t *hdr_v2 =&imxhdr->header.hdr_v2;
>>   	flash_header_v2_t *fhdr_v2 =&hdr_v2->fhdr;
>>
>> -	/* Set default offset */
>> -	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
>> -
>>   	/* Set magic number */
>>   	fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
>>   	fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t));
>
>
> I think we have then a problem when there is not a BOOT_FROM statement
> in the configuration file, that let  imxhdr->flash_offset unset.
>
> We need to set it with the default value

Hmm, my understanding is that there can't be any default value? What 
do you think would be a default value? The one for SD boot? The one 
for NAND? Or? Who will decide what the default should be?

> at the beginning of the
> processing or to raise an error if we want to make this statement
> mandatory in the configuration file.

If there can't be a default value, I think to raise an error if no 
BOOT_FROM statement is there is the only option.

Best regards

Dirk

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[stefano babic]

Am 21/02/2012 20:18, schrieb Dirk Behme:

>> I think we have then a problem when there is not a BOOT_FROM statement
>> in the configuration file, that let  imxhdr->flash_offset unset.
>>
>> We need to set it with the default value
> 
> Hmm, my understanding is that there can't be any default value? 

This is desirable, but it does not correspond to the code.

> What do
> you think would be a default value? The one for SD boot? The one for
> NAND? Or? Who will decide what the default should be?

Whatever we choose, it is wrong. We can select SD, and one board has not
a SD interface. The same with NAND, ...

The best one is we have not a default value.

> 
>> at the beginning of the
>> processing or to raise an error if we want to make this statement
>> mandatory in the configuration file.
> 
> If there can't be a default value, I think to raise an error if no
> BOOT_FROM statement is there is the only option.

Agree - BOOT_FROM must be mandatory.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Dirk Behme]

On 21.02.2012 22:49, stefano babic wrote:
> Am 21/02/2012 20:18, schrieb Dirk Behme:
> 
>>> I think we have then a problem when there is not a BOOT_FROM statement
>>> in the configuration file, that let  imxhdr->flash_offset unset.
>>>
>>> We need to set it with the default value
>> Hmm, my understanding is that there can't be any default value? 
> 
> This is desirable, but it does not correspond to the code.
> 
>> What do
>> you think would be a default value? The one for SD boot? The one for
>> NAND? Or? Who will decide what the default should be?
> 
> Whatever we choose, it is wrong. We can select SD, and one board has not
> a SD interface. The same with NAND, ...
> 
> The best one is we have not a default value.

Yes, agree.

>>> at the beginning of the
>>> processing or to raise an error if we want to make this statement
>>> mandatory in the configuration file.
>> If there can't be a default value, I think to raise an error if no
>> BOOT_FROM statement is there is the only option.
> 
> Agree - BOOT_FROM must be mandatory.

What do you think about anything like below then [1]?

I looked through the imximage.c code and, well, due to the mixture to 
support the v1 and v2 header format, the execution path isn't the 
cleanest one. So, while it doesn't seem to be the cleanest way to exit 
directly in set_imx_hdr_v2, it seems to be the easiest and best place to 
add this check. Some other functions have some exit() calls, too, so it 
seems to be common practice in this code.

If this is ok, I will send a v2 of the patch.

Best regards

Dirk

[1]

--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -253,8 +253,12 @@ static void set_imx_hdr_v2(struct imx_header 
*imxhdr, uint32_t dcd_len,
  	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
  	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;

-	/* Set default offset */
-	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
+	/* Exit if there is no BOOT_FROM field specifying the flash_offset */
+	if(imxhdr->flash_offset == 0) {
+		fprintf(stderr, "Error: Header v2: No BOOT_FROM tag in %s\n",
+			params->imagename);
+		exit(EXIT_FAILURE);
+	}

  	/* Set magic number */
  	fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Stefano Babic]

On 22/02/2012 09:14, Dirk Behme wrote:
> On 21.02.2012 22:49, stefano babic wrote:
>> Am 21/02/2012 20:18, schrieb Dirk Behme:
>>

Hi Dirk,

> What do you think about anything like below then [1]?
> 
> I looked through the imximage.c code and, well, due to the mixture to
> support the v1 and v2 header format, the execution path isn't the
> cleanest one. So, while it doesn't seem to be the cleanest way to exit
> directly in set_imx_hdr_v2, it seems to be the easiest and best place to
> add this check. Some other functions have some exit() calls, too, so it
> seems to be common practice in this code.

It is common in all mkimage - when there is an error, it makes no sense
to go on.

You must also fix this issue for V1 in set_imx_hdr_v1() as well, because
we do not want default value at all. I suggest also you do not check
with if(imxhdr->flash_offset == 0), in case Freescale will put a SOC
without an offset in the future. But it is easy to add a value that is
not allowed. If we add something like

	FLASH_OFFSET_UNDEFINED = 0xFF

or whatever you want that is not 32-bit aligned, we are on the safest side.

> 
> If this is ok, I will send a v2 of the patch.

Ok, thanks.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Dirk Behme]

On 22.02.2012 10:29, Stefano Babic wrote:
> On 22/02/2012 09:14, Dirk Behme wrote:
>> On 21.02.2012 22:49, stefano babic wrote:
>>> Am 21/02/2012 20:18, schrieb Dirk Behme:
>>>
> 
> Hi Dirk,
> 
>> What do you think about anything like below then [1]?
>>
>> I looked through the imximage.c code and, well, due to the mixture to
>> support the v1 and v2 header format, the execution path isn't the
>> cleanest one. So, while it doesn't seem to be the cleanest way to exit
>> directly in set_imx_hdr_v2, it seems to be the easiest and best place to
>> add this check. Some other functions have some exit() calls, too, so it
>> seems to be common practice in this code.
> 
> It is common in all mkimage - when there is an error, it makes no sense
> to go on.
> 
> You must also fix this issue for V1 in set_imx_hdr_v1() as well, because
> we do not want default value at all.

Ok, the V1 topic is new.

I can't touch V1 because I don't know anything about it. And I don't 
have any hardware to test anything V1 related.

Even though the V1 code might have a similar issue, it's my 
understanding that it doesn't hurt there as in V1 there are no 
flash_offsets != FLASH_OFFSET_STANDARD. Therefore in V1 the existing 
code works fine (?). Same as the V2 code before Freescale introduced 
flash offsets which are not FLASH_OFFSET_STANDARD (== 0x400).

> I suggest also you do not check
> with if(imxhdr->flash_offset == 0), in case Freescale will put a SOC
> without an offset in the future. But it is easy to add a value that is
> not allowed. If we add something like
> 
> 	FLASH_OFFSET_UNDEFINED = 0xFF
> 
> or whatever you want that is not 32-bit aligned, we are on the safest side.

I will look where the correct location might be to add this.

>> If this is ok, I will send a v2 of the patch.

I will try to update the V2 header with something like 
FLASH_OFFSET_UNDEFINED as proposed above and then send a v2 of the patch.

Best regards

Dirk

[U-Boot] [PATCH] imximage: header v2: Remove overwriting of flash_offset

[Stefano Babic]

On 22/02/2012 10:40, Dirk Behme wrote:

>> You must also fix this issue for V1 in set_imx_hdr_v1() as well, because
>> we do not want default value at all.
> 
> Ok, the V1 topic is new.
> 
> I can't touch V1 because I don't know anything about it. And I don't
> have any hardware to test anything V1 related.
> 

It is enough if you add the same check in set_imx_hdr_v1() you want to
put in  set_imx_hdr_v2().

> Even though the V1 code might have a similar issue, it's my
> understanding that it doesn't hurt there as in V1 there are no
> flash_offsets != FLASH_OFFSET_STANDARD.

This is not correct. For the MX51, there is a different offset for the
onenand device. But there is not a board booting from onenand in mainline.

However, we want that BOOT_FROM is mandatory to avoid confusion. All V1
boards in mainline have BOOT_FROM in their imximage file, so it is
enough to test if we can build the boards.

> Therefore in V1 the existing
> code works fine (?). Same as the V2 code before Freescale introduced
> flash offsets which are not FLASH_OFFSET_STANDARD (== 0x400).

It is the same with both versions - also with V1 there are different
offset, but SD / NAND / SPI share the same offset.

> 
>> I suggest also you do not check
>> with if(imxhdr->flash_offset == 0), in case Freescale will put a SOC
>> without an offset in the future. But it is easy to add a value that is
>> not allowed. If we add something like
>>
>>     FLASH_OFFSET_UNDEFINED = 0xFF
>>
>> or whatever you want that is not 32-bit aligned, we are on the safest
>> side.
> 
> I will look where the correct location might be to add this.

Add it into imximage.h

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================