Re: [U-Boot] [PATCH 7/8] tools: mkimage: Add: Kirkwood Boot Image support (kwbimage)

Dear Prafulla Wadaskar,

In message 1248804270-13715-7-git-send-email-prafulla@marvell.com you wrote:

This is Third step towards cleaning mkimage code for kwbimage support in clean way.

Umm... The "Third step" was already in patch 4/8. I guess you have to check your commit messages better...

diff --git a/tools/Makefile b/tools/Makefile index 6ef15d9..07a1c27 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -172,6 +172,7 @@ $(obj)img2srec$(SFX): $(obj)img2srec.o

$(obj)mkimage$(SFX): $(obj)mkimage.o $(obj)crc32.o $(obj)image.o $(obj)md5.o \ $(obj)default_image.o \

• 	$(obj)kwbimage.o \
  $(obj)sha1.o $(LIBFDT_OBJS) $(obj)os_support.o
  

Please keep sorted.

--- /dev/null +++ b/tools/kwbimage.c @@ -0,0 +1,364 @@ +/*

• • (C) Copyright 2008
• • Marvell Semiconductor <www.marvell.com>
• • Written-by: Prafulla Wadaskar prafulla@marvell.com
• • See file CREDITS for list of people who contributed to this
• • project.
• • This program is free software; you can redistribute it and/or
• • modify it under the terms of the GNU General Public License as
• • published by the Free Software Foundation; either version 2 of
• • the License, or (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+#include "mkimage.h" +#include <image.h> +#include "kwbimage.h"

+/*

• • Suported commands for configuration file
• */

+static table_entry_t kwbimage_cmds[] = {

• { CMD_BOOT_FROM, "BOOT_FROM", "", },
• { CMD_NAND_ECC_MODE, "NAND_ECC_MODE", "", },
• { CMD_NAND_PAGE_SIZE, "NAND_PAGE_SIZE", "", },
• { CMD_SATA_PIO_MODE, "SATA_PIO_MODE", "", },
• { CMD_DDR_INIT_DELAY, "DDR_INIT_DELAY", "", },
• { CMD_DATA, "DATA", "", },
• { CMD_INVALID, "", "", },

+};

Why don't these entries have any printable names associated?

+/*

• • Suported Boot options for configuration file

Typo.

...

+/*

• • Suported NAND ecc options configuration file
• */

+static table_entry_t kwbimage_eccmodes[] = {

• { IBR_HDR_ECC_DEFAULT, "default", "", },
• { IBR_HDR_ECC_FORCED_HAMMING,"hamming", "", },
• { IBR_HDR_ECC_FORCED_RS, "rs", "", },
• { IBR_HDR_ECC_DISABLED, "disabled", "", },
• { -1, "", "", },

+};

Why don't these entries have any printable names associated?

+uint32_t check_get_hexval (char *token) +{

• uint32_t hexval;
• if (!sscanf (token, "%x", &hexval)) {

• printf ("Error:%s[%d] - Invalid hex data\n", fname, lineno);
  

• exit (EXIT_FAILURE);
  

• }
• return hexval;

+}

scanf() is not a good or reliable conversion tool. Better avoid it. Use strto[u]l() instead.

In the error case, please also print the string that you cannot convert.

+/*

• • Generates 8 bit checksum
• */

+uint8_t kwbimage_checksum8 (void *start, uint32_t len, uint8_t csum) +{

• register uint8_t sum = csum;
• volatile uint8_t *startp = (volatile uint8_t *)start;
• do {

• sum += *startp;
  

• startp++;
  

• } while (--len);

"startp" is incorrectly named. It is only _start_ at the beginning. Just call it "p" :-)

Maybe you should add handling for the "len=0" case.

+/*

• • Generates 32 bit checksum
• */

+uint32_t kwbimage_checksum32 (uint32_t *start, uint32_t len, uint32_t csum) +{

• register uint32_t sum = csum;
• volatile uint32_t *startp = start;
• do {

• sum += *startp;
  

• startp++;
  

• len -= sizeof(uint32_t);
  

• } while (len > 0);
• return (sum);

You should error handling for the cas ethet len is not an even multiple of sizeof(uint32_t).

Maybe you should add handling for the "len=0" case.

...

• case CMD_NAND_PAGE_SIZE:

• mhdr->nandpagesize =
  

• 	(uint16_t) check_get_hexval (token);
  

• 	printf ("Nand page size = 0x%x\n",
  

• 			mhdr->nandpagesize);
  

Indentation incorrect.

• break;
  

• case CMD_SATA_PIO_MODE:

• mhdr->satapiomode =
  

• 	(uint8_t) check_get_hexval (token);
  

• 	printf ("Sata PIO mode = 0x%x\n",
  

• 			mhdr->satapiomode);
  

Indentation incorrect.

• break;
  

• case CMD_DDR_INIT_DELAY:

• mhdr->ddrinitdelay =
  

• 	(uint16_t) check_get_hexval (token);
  

• printf ("DDR init delay = %d msec\n",
  

• 			mhdr->ddrinitdelay);
  

Indentation incorrect.

• break;
  

• case CMD_DATA:

• exthdr->rcfg[datacmd_cnt].raddr =
  

• 	(uint32_t) check_get_hexval (token);
  

Umm... why are all these casts needed?

Please get rid of these.

• break;
  

• case CMD_INVALID:
• default:

+INVL_DATA:

• printf ("Error:%s[%d] - Invalid data\n", fname, lineno);
  

• exit (EXIT_FAILURE);
  

• }

I really dislike this jumping into another case. Please don;t. Move this code out of the switch().

+}

+void kwdimage_set_ext_header (struct kwb_header *kwbhdr, char* name) {

How about some comments what all these functions are supposed to do?

• 	case CFG_DATA1:
  

• 		if (cmd != CMD_DATA)
  

• 			goto INVL_CMD;
  

• 		exthdr->rcfg[datacmd_cnt].rdata =
  

• 			(uint32_t) check_get_hexval (token);
  

• 		if (datacmd_cnt > KWBIMAGE_MAX_CONFIG ) {
  

• 			printf ("Error:%s[%d] - Found more "
  

• 				"than max(%d) allowed "
  

• 				"data configurations\n",
  

• 				fname, lineno,
  

• 				KWBIMAGE_MAX_CONFIG);
  

• 		exit (EXIT_FAILURE);
  

Indentation incorrect.

• 		} else
  

• 			datacmd_cnt++;
  

• 		break;
  

• 	default:
  

• 		/*
  

• 		 * Command format permits maximum three
  

• 		 * strings on a line, i.e. command and data
  

• 		 * if more than two are observed, then error
  

• 		 * will be reported
  

• 		 */
  

That does not make sense. If three three stings are permitted, then why throwing an error when there are three?

And "three strings on a line, i.e. command and data" sounds incorrect, too.

+INVL_CMD:

• 		printf ("Error:%s[%d] - Invalid command\n",
  

• 			fname, lineno);
  

• 		exit (EXIT_FAILURE);
  

Please do not jump right in the middle of a switch. Move this error code out of the switch.

+/* -l support functions */ +int kwbimage_verify_header (char *ptr, int image_size,

• 	struct mkimage_params *params)
  

+{

• struct kwb_header *hdr = (struct kwb_header *)ptr;
• bhr_t *mhdr = &hdr->kwb_hdr;
• extbhr_t *exthdr = &hdr->kwb_exthdr;
• uint8_t calc_hdrcsum;
• uint8_t calc_exthdrcsum;
• calc_hdrcsum = kwbimage_checksum8 ((void *)mhdr,

• 	sizeof(bhr_t) - sizeof(uint8_t), 0);
  

• if (calc_hdrcsum != mhdr->checkSum) {

• return -FDT_ERR_BADSTRUCTURE;	/* mhdr csum not matched */
  

• }

No braces needed for single line statements.

• calc_exthdrcsum = kwbimage_checksum8 ((void *)exthdr,

• 	sizeof(extbhr_t) - sizeof(uint8_t), 0);
  

• if (calc_hdrcsum != mhdr->checkSum) {

• return -FDT_ERR_BADSTRUCTURE; /* exthdr csum not matched */
  

• }

No braces needed for single line statements.

+void kwbimage_print_header (char *ptr) +{

• struct kwb_header *hdr = (struct kwb_header *) ptr;
• bhr_t *mhdr = &hdr->kwb_hdr;
• printf ("Image Type: Kirkwood Boot from %s Image\n",

• get_table_entry_name (kwbimage_bootops,
  

• 	"Kwbimage boot option",
  

• 	(int) mhdr->blockid));
  

Indentation looks ugly.

...

diff --git a/tools/mkimage.c b/tools/mkimage.c index 3a3cb19..7e29610 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -25,6 +25,7 @@ #include "mkimage.h" #include <image.h> #include "default_image.h" +#include "kwbimage.h"

static void copy_file(int, const char *, int); static void usage(void); @@ -59,6 +60,7 @@ static struct image_functions image_ftable[] = { {image_get_tparams,}, /* for IH_TYPE_SCRIPT */ {image_get_tparams,}, /* for IH_TYPE_STANDALONE */ {fitimage_get_tparams,}, /* for IH_TYPE_FLATDT */

• {kwbimage_get_tparams,}, /* for IH_TYPE_KWBIMAGE */

};

int @@ -111,7 +113,7 @@ main (int argc, char **argv) (params.opt_type = genimg_get_type_id (*++argv)) < 0) usage ();

• 		if (image_ftable_size <= params.opt_type) {
  

• 		if (image_ftable_size < params.opt_type) {
  

Ah! What's going on here????

Best regards,

Wolfgang Denk