[U-Boot] [PATCH v2 2/4] tools: mkimage (type=kwbimage) kirkwood boot image support

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1248215857-11006-2-git-send-email-prafulla at marvell.com> you wrote:
> For more details refer docs/README.kwbimage
> 
> Signed-off-by: Prafulla Wadaskar <prafulla at marvell.com>

> +enum kwbimage_cmd kwbimage_check_cfgcmd(char *token,
> +				struct kwb_header *kwbhdr)
> +{
> +	extbhr_t *exthdr = &kwbhdr->kwb_exthdr;
> +	char tempptr[MAX_TEMPBUF_LEN];
> +
> +	/* command string processing */
> +	if (strcmp(token, "BOOT_FROM") == 0)
> +		return CMD_BOOT_FROM;
> +	else if (strcmp(token, "NAND_ECC_MODE") == 0)
> +		return CMD_NAND_ECC_MODE;
> +	else if (strcmp(token, "NAND_PAGE_SIZE") == 0)
> +		return CMD_NAND_PAGE_SIZE;
> +	else if (strcmp(token, "SATA_PIO_MODE") == 0)
> +		return CMD_SATA_PIO_MODE;
> +	else if (strcmp(token, "DDR_INIT_DELAY") == 0)
> +		return CMD_DDR_INIT_DELAY;
> +	else {
> +		exthdr->rcfg[datacmd_cnt].raddr =
> +			(uint32_t) strtoul (token, tempptr, 16);

No error checking here? What happens if token does not repesent a
valid number format?

> +void kwbimage_check_cfgdata(char *token, enum kwbimage_cmd cmdsw,
> +					struct kwb_header *kwbhdr)
> +{
> +	bhr_t *mhdr = &kwbhdr->kwb_hdr;
> +	extbhr_t *exthdr = &kwbhdr->kwb_exthdr;
> +	char tempptr[MAX_TEMPBUF_LEN];
> +
> +	switch(cmdsw) {
> +	case CMD_BOOT_FROM:
> +		if (strcmp(token, "spi") == 0) {
> +			mhdr->blockid = IBR_HDR_SPI_ID;
> +		} else if (strcmp(token, "nand") == 0) {
> +			mhdr->blockid = IBR_HDR_NAND_ID;
> +		} else if (strcmp(token, "sata") == 0) {
> +			mhdr->blockid = IBR_HDR_SATA_ID;
> +		} else {
> +			printf("Err.. Invalid kwimage data\n");

Well, please print a more helpful error message here: print the line
number where the error occurs, and what's wrong (i. e. boot medium is
neither  "spi" or  "nand" or "sata" but ``token'' instead.

> +	case CMD_NAND_ECC_MODE:
> +		if (strcmp(token, "default") == 0) {
> +			mhdr->nandeccmode = IBR_HDR_ECC_DEFAULT;
> +		} else if (strcmp(token, "hamming") == 0) {
> +			mhdr->nandeccmode =
> +				IBR_HDR_ECC_FORCED_HAMMING;
> +		} else if (strcmp(token, "rs") == 0) {
> +			mhdr->nandeccmode =
> +				IBR_HDR_ECC_FORCED_RS;
> +		} else if (strcmp(token, "disabled") == 0) {
> +			mhdr->nandeccmode = IBR_HDR_ECC_DISABLED;
> +		} else {
> +			printf("Err.. Invalid kwimage data\n");

Ditto.


It seems you have several such places where you try to find a value in
a list of valid tokens. I suggest you use array of strings for that,
for example:

	char *boot_from[] = {
		"spi",
		"nand",
		"sata",
		NULL,
	};

So you can search in a loop instead of a (long) if...else...else list;
put that in a function, and you can use this function fol all such
searches by just passing a different string list as argument.


> +	case CMD_NAND_PAGE_SIZE:
> +		mhdr->nandpagesize =
> +			(uint16_t) strtoul (token, tempptr, 16);
> +			printf("Nand page size = %x\n", mhdr->nandpagesize);
> +		break;
> +	case CMD_SATA_PIO_MODE:
> +		mhdr->satapiomode =
> +			(uint8_t) strtoul (token, tempptr, 16);
> +			printf("Sata PIO mode = %x\n", mhdr->satapiomode);
> +		break;
> +	case CMD_DDR_INIT_DELAY:
> +		mhdr->ddrinitdelay =
> +			(uint16_t) strtoul (token, tempptr, 16);
> +		printf("DDR init delay = %d msec\n",
> +					mhdr->ddrinitdelay);
> +		break;
> +	case CMD_DDR_DATA:
> +		exthdr->rcfg[datacmd_cnt].rdata =
> +			(uint32_t) strtoul (token, tempptr, 16);

Please add error checking for all these strtoul() calls.

Please also use a consistent coding style. Either you consistently use
a space after the function name, i. e. "foo ()", or you do not, i. e.
"foo()" - but please do not mix both styles at random.

> +		if (datacmd_cnt > KWBIMAGE_MAX_CONFIG ) {
> +			printf("Err.. found more than max "
> +				"allowed(%d) configurations\n",
> +				KWBIMAGE_MAX_CONFIG);

Where is this limit coming from? Can the be arbitrarily changed, or is
it some inherent linif of your image format?

Eventually also print line number information, etc.

> +			exit (EXIT_FAILURE);
> +		} else
> +			datacmd_cnt++;
> +		break;
> +	case CMD_INVALID:
> +	default:
> +		printf("Error.. invalid data %s\n",token);

Please print a more helpful error message - see above.

And please try and use a consistent format for error messages, don;t
mix for example "Err..." and "Error...".

My recommendation is to use something like

	"Error: %s[%d] - ...", filename, linenumber, ...

> +void kwdimage_set_ext_header(struct kwb_header *kwbhdr, char* fname) {
> +	bhr_t *mhdr = &kwbhdr->kwb_hdr;
> +	extbhr_t *exthdr = &kwbhdr->kwb_exthdr;
> +	int fd = -1;
> +	int lineno, j;
> +	char *line = NULL;
> +	char * token, *saveptr1, *saveptr2;
> +	size_t len = 0;
> +	enum kwbimage_cmd cmd;
> +
> +	/* set dram register offset */
> +	exthdr->dramregsoffs = (uint32_t)&exthdr->rcfg - (uint32_t)mhdr;
> +
> +	if ((fd = fopen(fname, "r")) == 0) {
> +		if (strlen(fname))
> +			fprintf (stderr, "Err: Can't open %s: %s\n",
> +				fname, strerror(errno));
> +		else
> +			fprintf (stderr, "Err: kwbimage.cfg not specified "
> +				"%s\n"
> +				"add -n <file>/<kwbimage.cfg> to the cmd\n",
> +			strerror(errno));

This makes no sense. If no file name is given you should detect this
based on the argument parsing, not based on the string length. And
this test should be run _before_ you attempot to open the file.

> +	/* Simple kwimage.cfg file parser */
> +	lineno=0;
> +	while ((getline(&line, &len, fd)) != -1) {
> +		lineno++;
> +		token = strtok_r(line, "\r\n", &saveptr1);
> +		/* drop all lines with zero tokens (= empty lines) */
> +		if (token == NULL)
> +			continue;
> +
> +		for (j = 0, cmd = CMD_INVALID, line = token; ; line = NULL) {
> +			token = strtok_r(line, " 	", &saveptr2);

Please write as " \t" so the content of the string constant is clear.

> +			if (token == NULL)
> +			break;

Comment?

> +			/* Drop all text starting with '#' as comments */
> +			if (token[0] == '#')
> +				break;
> +
> +			/* Process rest as valid config command line */
> +			if (j == 0) {
> +				cmd = kwbimage_check_cfgcmd(token, kwbhdr);
> +				if (cmd == CMD_INVALID)
> +					break;
> +			} else if (j == 1) {
> +				kwbimage_check_cfgdata(token, cmd, kwbhdr);
> +			} else {
> +				/*
> +				 * Command format permits only two strings
> +				 * on a line, i.e. command and data
> +				 * if more than two are observed, then error
> +				 * will be reported
> +				 */
> +				printf("Error.. Invalid command line(%d)\n",
> +					lineno);
> +				exit (EXIT_FAILURE);
> +			}

Maybe a "case (...) { ...}" would be easier to read here.

> diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> new file mode 100644
> index 0000000..98ab34f
> --- /dev/null
> +++ b/tools/kwbimage.h
> @@ -0,0 +1,104 @@
> +/*
> + * (C) Copyright 2008
> + * Marvell Semiconductor <www.marvell.com>
> + * iWritten-by: Prafulla Wadaskar <prafulla at marvell.com>

Typo?

> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index 40363a9..ba71874 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -24,6 +24,7 @@
>  
>  #include "mkimage.h"
>  #include <image.h>
> +#include "kwbimage.h"
>  
>  extern int errno;
>  
> @@ -164,6 +165,10 @@ NXTARG:		;
>  		(lflag && (dflag || fflag)))
>  		usage();
>  
> +	if (((argc != 1) && (opt_type == IH_TYPE_KWBIMAGE)) &&
> +		(xflag || lflag))
> +		usage();

Do you promise to add support for listing the image (lflag) later?

>  	if (!eflag) {
>  		ep = addr;
>  		/* If XIP, entry point must be after the U-Boot header */
> @@ -251,7 +256,12 @@ NXTARG:		;
>  	 *
>  	 * write dummy header, to be fixed later
>  	 */
> -	hdr_size = image_get_header_size ();
> +	if (opt_type == IH_TYPE_KWBIMAGE) {
> +		hdr = kwbimage_get_header_ptr();
> +		hdr_size = kwbimage_get_header_size ();
> +	} else
> +		hdr_size = image_get_header_size ();

Maybe we should use a function pointer for image_get_header_size()
which can be set just once, so we don't have to change the code?


>  	memset (hdr, 0, hdr_size);
>  	if (write(ifd, hdr, hdr_size) != hdr_size) {
>  		fprintf (stderr, "%s: Write error on %s: %s\n",
> @@ -339,6 +349,19 @@ NXTARG:		;
>  
>  	hdr = (image_header_t *)ptr;
>  
> +	/* Build new header */
> +	if (opt_type == IH_TYPE_KWBIMAGE) {
> +	checksum = kwbimage_checksum32((uint32_t *)ptr, sbuf.st_size, 0);
> +
> +		if (write(ifd, &checksum, sizeof(uint32_t))
> +					!= sizeof(uint32_t)) {
> +			fprintf (stderr, "%s: Checksum wr err on %s: %s\n",
> +				cmdname, imagefile, strerror(errno));
> +			exit (EXIT_FAILURE);
> +		}
> +		sbuf.st_size += sizeof(uint32_t);
> +		kwbimage_set_header (hdr, &sbuf, addr, ep, name);
> +	} else {
>  	checksum = crc32 (0,

Here indentation becomes wrong.

And I don't like this if()...else... thingy here. I think we should
move the respective code into functions, and just set a pointer to the
function the builds the header.

This prevents an ever growing list of if()...else...else...else...
in case more image formats need to get added.