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