Dear Prafulla Wadaskar,
In message <1248804270-13715-7-git-send-email-prafu...@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 <prafu...@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: w...@denx.de
When choosing between two evils, I always like to take the one I've
never tried before. -- Mae West, "Klondike Annie"
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
