On 12/29/2010 01:49 PM, Jason Liu wrote:
This patch add the MX53 boot image support.
This patch has been tested on Freescale MX53EVK board and MX51EVK board.
Signed-off-by: Jason Liu r64343@freescale.com
Hi Jason,
diff --git a/board/freescale/mx51evk/imximage.cfg b/board/freescale/mx51evk/imximage.cfg index a875e8f..11825fb 100644 --- a/board/freescale/mx51evk/imximage.cfg +++ b/board/freescale/mx51evk/imximage.cfg @@ -25,6 +25,10 @@ # # The syntax is taken as close as possible with the kwbimage
+# Imximage version
+IMXIMAGE_VERSION 1
The name is not consistent with the other commands, as they do not use the IMXIMAGE_ prefix (we do not have such as IMXIMAGE_BOOT_FROM). Please replace with VERSION.
Not change the mx51evk file. The code should take VERSION=1 as default, and we do not need to change the actual boards.
diff --git a/board/ttcontrol/vision2/imximage_hynix.cfg b/board/ttcontrol/vision2/imximage_hynix.cfg index ed531db..cdc533d 100644 --- a/board/ttcontrol/vision2/imximage_hynix.cfg +++ b/board/ttcontrol/vision2/imximage_hynix.cfg @@ -28,6 +28,10 @@ # # The syntax is taken as close as possible with the kwbimage
+# Imximage version
+IMXIMAGE_VERSION 2
...and this is wrong, as vision is a MX51 board.
diff --git a/doc/README.imximage b/doc/README.imximage index 3378f7e..48ac466 100644 --- a/doc/README.imximage +++ b/doc/README.imximage @@ -57,6 +57,11 @@ Configuration command line syntax: 2. Following are the valid command strings and associated data strings:- Command string data string
- IMXIMAGE_VERSION 1/2
1 is for mx25/mx35/mx51 compatible,2 is for mx53 compatible,others is invalid and error is generated.
As the VERSION selects the type of the header, I do not think it could be set in any place of the file, or the code must be aware of it. Please add a note in the documentation and raise an error in code if the VERSION command is read after any other suitable commands.
What happens for example if VERSION is set at the end of imximage.cfg file ?
diff --git a/tools/imximage.c b/tools/imximage.c index 39f89c2..2bbc4a6 100644 --- a/tools/imximage.c +++ b/tools/imximage.c @@ -36,6 +36,7 @@
- Supported commands for configuration file
*/ static table_entry_t imximage_cmds[] = {
- {CMD_IMXIMAGE_VERSION, "IMXIMAGE_VERSION", "imximage version", },
Change IMXIMAGE_VERSION simply to VERSION
+static void err_imximage_version(int version) +{
- fprintf(stderr,
"Error: Unsuported imximage version:%d\n", version);
^--typo, unsupported
+static void set_dcd_value_v1(struct imx_header *imxhdr, char *name, int lineno,
int fld, uint32_t value, uint32_t off)+{
- dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
- switch (fld) {
- case CFG_REG_SIZE:
/* Byte, halfword, word */if ((value != 1) && (value != 2) && (value != 4)) {fprintf(stderr, "Error: %s[%d] - ""Invalid register size " "(%d)\n",name, lineno, value);exit(EXIT_FAILURE);}dcd_v1->addr_data[off].type = value;break;- case CFG_REG_ADDRESS:
dcd_v1->addr_data[off].addr = value;break;- case CFG_REG_VALUE:
dcd_v1->addr_data[off].value = value;break;- default:
break;- }
+}
+static void set_dcd_value_v2(struct imx_header *imxhdr, char *name, int lineno,
int fld, uint32_t value, uint32_t off)+{
- dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
- switch (fld) {
- case CFG_REG_ADDRESS:
dcd_v2->addr_data[off].addr = cpu_to_be32(value);break;- case CFG_REG_VALUE:
dcd_v2->addr_data[off].value = cpu_to_be32(value);break;- default:
break;- }
+}
+static void set_dcd_value(struct imx_header *imxhdr, char *name, int lineno,
int fld, uint32_t value, uint32_t off)+{
- switch (imxhdr->imximage_version) {
- case IMXIMAGE_V1:
set_dcd_value_v1(imxhdr, name, lineno, fld, value, off);break;- case IMXIMAGE_V2:
set_dcd_value_v2(imxhdr, name, lineno, fld, value, off);break;- default:
err_imximage_version(imxhdr->imximage_version);break;- }
+}
You inserted several help functions (set_dcd_value, print_...,..) and all of them implement a switch case to reindirect to the specific function. What about to drop them and to use function pointers, set when the version of header is recognized ?
@@ -82,44 +213,91 @@ static int imximage_check_image_types(uint8_t type) static int imximage_verify_header(unsigned char *ptr, int image_size, struct mkimage_params *params) {
- struct imx_header *imx_hdr = (struct imx_header *) ptr;
- flash_header_t *hdr = &imx_hdr->fhdr;
- flash_header_v1_t *fhdr = &imx_hdr->header.hdr_v1.fhdr;
- if (imx_hdr->imximage_version != IMXIMAGE_V1)
return 0;
I think this is not correct. mkimage can be called with "-l" option to check the correctness of the produced image without an imximage.cfg file, and from your code it seems to me that actual images are not recognized anymore. In fact, you recognize the header from a version field that you add in the header, but it is not actually provided.
You should find the version of the header checking its structure, for example recognizing APP_CODE_BARKER and DCD_BARKER for V1, and for example DCD_HEADER_TAG for V2.
Please note that print_header and verify_header are part of the mkimage API (in imximage_params table), and they are called by mkimage independently without parsing any configuration file.
+static void imximage_print_header(const void *ptr) +{
- struct imx_header *imx_hdr = (struct imx_header *) ptr;
- switch (imx_hdr->imximage_version) {
As I said, this does not work with actual images. The tool should be able to recognize the version directly from its structure (we have enough magic numbers, or better, barkers, to do it), and not storing the version into the header.
Best regards, Stefano Babic