[U-Boot] [PATCH 3/3] imximage: Add MX53 boot image support

[Jason Liu]

Hi, Stefano,

2010/12/27 Stefano Babic <sbabic at denx.de>:
> On 12/22/2010 02:23 PM, Jason Liu wrote:
>> This patch add the MX53 boot image support
>>
>> This patch has been tested on Freescale MX53EVK board.
>>
>> Signed-off-by: Jason Liu <r64343 at freescale.com>
>> ---
>>  board/freescale/mx53evk/config.mk    |   25 ++++++++
>>  board/freescale/mx53evk/imximage.cfg |  108 ++++++++++++++++++++++++++++++++++
>>  tools/imximage.c                     |  101 +++++++++++++++++++++++++++++--
>>  tools/imximage.h                     |   75 ++++++++++++++++++++---
>
> You mixed two patches with different meaning. You add support for
> mx53evk board (and patches for config.mk and imximage.cfg must belong to
> this patch) and you modify imximage to support the MX53 processor, not
> related to the mx53evk board. Please split this patch into two separate
> patches.

OK,  I will put config.mk and imximage.cfg to mx53evk board support patch.

>
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 39f89c2..884feff 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -82,7 +82,7 @@ static int imximage_check_image_types(uint8_t type)
>>  static int imximage_verify_header(unsigned char *ptr, int image_size,
>>                       struct mkimage_params *params)
>>  {
>> -
>> +#ifdef CONFIG_MX51
>
> NAK. This is wrong: mkimage is a tool running on host and must be
> possible to include it in a distro. It must be able (as it now does) to
> produce the correct image at runtime. For this reason there are no
> #ifdef in the actual code. If a different behavior is required, this
> must be added extending the syntax of the imximage.cfg file, for example
> adding a processor type (or a version number, or whatever is needed).

Sorry, I don't notice that. If that, I have to add extending syntax to
support mx53 since
mx53 is different with mx51 for ROM boot structure.

I would like to add one process type into the imximage.cfg file, what
do you think of that?
Any suggestions? Thanks,

>
>> +#if defined(CONFIG_MX51)
>>  static void imximage_print_header(const void *ptr)
>>  {
>>       struct imx_header *imx_hdr = (struct imx_header *) ptr;
>>       flash_header_t *hdr = &imx_hdr->fhdr;
>>       uint32_t size;
>>       uint32_t length;
>> -     dcd_t *dcd = &imx_hdr->dcd_table;
>>
>> +     dcd_t *dcd = &imx_hdr->dcd_table;
>
> I do not see any changes on these lines. Please add only changes where
> they are required.

I don't know why this get into the patch. I will take care later.

>
> 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 mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>