[U-Boot] [PATCH] imximage: Fix imximage IVT bug for EIM-NOR boot

[U-Boot] [PATCH] imximage: Fix imximage IVT bug for EIM-NOR boot

[Ye.Li]

From: "Ye.Li" <Ye.Li@freescale.com>

The load region size of EIM-NOR are defined to 0. For this case,
the parameter "imximage_init_loadsize" must be calculated.
The imximage tool implements the calculation in the "imximage_generate"
function, but the following function "imximage_set_header" resets the value
and not calculate. This bug cause some fields of IVT head are not
correct, for example the boot_data and DCD overlay the application area.

Signed-off-by: Ye.Li <B37916@freescale.com>
---
 tools/imximage.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/imximage.c b/tools/imximage.c
index 18dc051..faba238 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -568,6 +568,13 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	/* Parse dcd configuration file */
 	dcd_len = parse_cfg_file(imxhdr, params->imagename);
 
+	if (imximage_version == IMXIMAGE_V2) {
+		if (imximage_init_loadsize < imximage_ivt_offset +
+			sizeof(imx_header_v2_t))
+				imximage_init_loadsize = imximage_ivt_offset +
+					sizeof(imx_header_v2_t);
+	}
+
 	/* Set the imx header */
 	(*set_imx_hdr)(imxhdr, dcd_len, params->ep, imximage_ivt_offset);
 
-- 
1.7.9.5

[U-Boot] [PATCH] imximage: Fix imximage IVT bug for EIM-NOR boot

[Stefano Babic]

Hi Ye,

On 20/08/2014 10:55, Ye.Li wrote:
> From: "Ye.Li" <Ye.Li@freescale.com>
> 
> The load region size of EIM-NOR are defined to 0. For this case,
> the parameter "imximage_init_loadsize" must be calculated.
> The imximage tool implements the calculation in the "imximage_generate"
> function, but the following function "imximage_set_header" resets the value
> and not calculate. This bug cause some fields of IVT head are not
> correct, for example the boot_data and DCD overlay the application area.
> 
> Signed-off-by: Ye.Li <B37916@freescale.com>
> ---
>  tools/imximage.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 18dc051..faba238 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -568,6 +568,13 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	/* Parse dcd configuration file */
>  	dcd_len = parse_cfg_file(imxhdr, params->imagename);
>  
> +	if (imximage_version == IMXIMAGE_V2) {
> +		if (imximage_init_loadsize < imximage_ivt_offset +
> +			sizeof(imx_header_v2_t))
> +				imximage_init_loadsize = imximage_ivt_offset +
> +					sizeof(imx_header_v2_t);
> +	}
> +

Agree that this must be fixed for NOR flash. I am checking where the fix
should be done.

As far as I see, there are multiple entry involved in the computation:

387       if (imximage_init_loadsize < imximage_ivt_offset)
388          imximage_init_loadsize = imximage_ivt_offset;

It seems more logical that the check should be here.

The other point is:

#define FLASH_LOADSIZE_NOR              0x0 /* entire image */

Well, if the comment is correct (with NOR the SOC can load the entire
image and there is no limit as with other storage), setting it to "0"
has nothing to do with it.
Then it seems to me clearer something at line 372:

if (!imximage_init_loadsize && imximage_version == IMXIMAGE_V2)
			imximage_init_loadsize = imximage_ivt_offset +
				sizeof(imx_header_v2_t);

What do you think ?

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-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

[U-Boot] [PATCH] imximage: Fix imximage IVT bug for EIM-NOR boot

[Li Ye-B37916]

Hi Stefano Babic,

On 8/20/2014 5:44 PM, Stefano Babic wrote:
> Hi Ye,
>
> On 20/08/2014 10:55, Ye.Li wrote:
>> From: "Ye.Li" <Ye.Li@freescale.com>
>>
>> The load region size of EIM-NOR are defined to 0. For this case,
>> the parameter "imximage_init_loadsize" must be calculated.
>> The imximage tool implements the calculation in the "imximage_generate"
>> function, but the following function "imximage_set_header" resets the value
>> and not calculate. This bug cause some fields of IVT head are not
>> correct, for example the boot_data and DCD overlay the application area.
>>
>> Signed-off-by: Ye.Li <B37916@freescale.com>
>> ---
>>  tools/imximage.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 18dc051..faba238 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -568,6 +568,13 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>>  	/* Parse dcd configuration file */
>>  	dcd_len = parse_cfg_file(imxhdr, params->imagename);
>>  
>> +	if (imximage_version == IMXIMAGE_V2) {
>> +		if (imximage_init_loadsize < imximage_ivt_offset +
>> +			sizeof(imx_header_v2_t))
>> +				imximage_init_loadsize = imximage_ivt_offset +
>> +					sizeof(imx_header_v2_t);
>> +	}
>> +
> Agree that this must be fixed for NOR flash. I am checking where the fix
> should be done.
>
> As far as I see, there are multiple entry involved in the computation:
>
> 387       if (imximage_init_loadsize < imximage_ivt_offset)
> 388          imximage_init_loadsize = imximage_ivt_offset;
>
> It seems more logical that the check should be here.
>
> The other point is:
>
> #define FLASH_LOADSIZE_NOR              0x0 /* entire image */
>
> Well, if the comment is correct (with NOR the SOC can load the entire
> image and there is no limit as with other storage), setting it to "0"
> has nothing to do with it.
> Then it seems to me clearer something at line 372:
>
> if (!imximage_init_loadsize && imximage_version == IMXIMAGE_V2)
> 			imximage_init_loadsize = imximage_ivt_offset +
> 				sizeof(imx_header_v2_t);
>
> What do you think ?
>
> Best regards,
> Stefano Babic
>
>
>
>
There are two minor impacts if putting the check in the function "parse_cfg_cmd":

1. The "imximage_version" must be got before parsing a CMD_BOOT_FROM command. This compels the CMD_IMAGE_VERSION preceding the CMD_BOOT_FROM in script.  imximage_init_loadsize is only needed by V2 version.

2. Since the "imximage_generate" function already implements post fixing for imximage_init_loadsize, this post fixing needs be removed.

Actually, putting the check in the parsing or post the parsing are ok for me. Both can resolve the issue.
The comment for "FLASH_LOADSIZE_NOR" sources from iMX reference manual, it is correct.

Best regards,
Ye Li

[U-Boot] [PATCH] imximage: Fix imximage IVT bug for EIM-NOR boot

[Stefano Babic]

Hi Ye,

On 21/08/2014 09:04, Li Ye-B37916 wrote:

> There are two minor impacts if putting the check in the function
> "parse_cfg_cmd":
> 
> 1. The "imximage_version" must be got before parsing a CMD_BOOT_FROM
> command. This compels the CMD_IMAGE_VERSION preceding the
> CMD_BOOT_FROM in script.  imximage_init_loadsize is only needed by V2
> version.

I know abot this, and this is a minor impact. It is common that a
version number of a document must be set first in the document.

> 
> 2. Since the "imximage_generate" function already implements post
> fixing for imximage_init_loadsize, this post fixing needs be
> removed.
> 
> Actually, putting the check in the parsing or post the parsing are ok
> for me. Both can resolve the issue. The comment for
> "FLASH_LOADSIZE_NOR" sources from iMX reference manual, it is
> correct.

Well, I think it is clear for both of us because we worked with i-IM
images. But setting a size to 0x0 with the comment that this is the size
of whole image can be confusing (I think I was the author of this
comment, so I am guilty for that). Anyway, it is a very minor issue.

I hope that people will read carefully the manual together with code.

Anyway, comments are very minor issues - I will apply the patch.

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-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================