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

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1248804270-13715-7-git-send-email-prafulla at 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 at 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When choosing between two evils, I always like to take the  one  I've
never tried before.                     -- Mae West, "Klondike Annie"