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

Reply via email to