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

login
register
mail settings
Submitter Dirk Behme
Date Feb. 21, 2012, 9:02 a.m.
Message ID <1329814920-12295-1-git-send-email-dirk.behme@de.bosch.com>
Download mbox | patch
Permalink /patch/142268/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Dirk Behme - Feb. 21, 2012, 9:02 a.m.
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(-)
Stefano Babic - Feb. 21, 2012, 5:57 p.m.
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
Dirk Behme - Feb. 21, 2012, 7:18 p.m.
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
Stefano Babic - Feb. 21, 2012, 9:49 p.m.
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

Patch

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));