Dear Wolfgang
Thanks for your quick feedback
> -----Original Message-----
> From: Wolfgang Denk [mailto:w...@denx.de]
> Sent: Sunday, July 19, 2009 3:33 AM
> To: Prafulla Wadaskar
> Cc: u-boot@lists.denx.de; Manas Saksena; Ronen Shitrit;
> Nicolas Pitre; Ashish Karkare; Prabhanjan Sarnaik; Lennert Buijtenhek
> Subject: Re: [U-Boot] [PATCH 2/3] tools: mkimage
> (type=kwbimage) kirkwood boot image support
>
> Dear Prafulla Wadaskar,
>
> In message
> <1247967958-4446-2-git-send-email-prafu...@marvell.com> you wrote:
> > For more details refer docs/README.kwbimage
>
> ...
> > diff --git a/doc/README.kwbimage b/doc/README.kwbimage new
> file mode
> > 100644 index 0000000..b00a053
> > --- /dev/null
> > +++ b/doc/README.kwbimage
> > @@ -0,0 +1,77 @@
> > +---------------------------------------------
> > +Kirkwood Boot Image generation using mkimage
> > +---------------------------------------------
> > +
> > +This document describes the U-Boot feature as it is
> implemented for
> > +the Kirkwood family of SoCs.
> > +
> > +The Kirkwood SoC's can boot directly from NAND FLASH, SPI
> FLASH, SATA
> > +etc. using its internal bootRom support.
> > +
> > +for more details refer section 24.2 of Kirkwood functional
> specifications.
> > +ref: www.marvell.com/products/embedded.../kirkwood/index.jsp
> > +
> > +Commad syntax:
> > +./tools/mkimage -n <board specific configuration file> \
> > + -T kwbimage -a <start address> -e
> <execution address>
> > +-d <input_raw_binary> <output_kwboot_file>
> > +
> > +for ex.
> > +./tools/mkimage -n ./board/Marvell/openrd_base/kwbimage.cfg \
> > + -T kwbimage -a 0x00600000 -e 0x00600000 -d
> u-boot.bin
> > +u-boot.kwb
> > +
> > +kwimage support available with mkimage utility will
> generate kirkwood
> > +boot image that can be flashed on the board nand/spi flash
>
>
> Maximum line length exceeded. Please fix globally.
Okay I will correct them
>
>
> ...
> > +Author: Prafulla Wadaskar <prafu...@marvell.com>
> > +------------------------------------------------------
> > +
>
> Please do not add trailing empty lines.
Okay
>
> > diff --git a/include/image.h b/include/image.h index
> f183757..ed94dda
> > 100644
> > --- a/include/image.h
> > +++ b/include/image.h
> > @@ -158,6 +158,7 @@
> > #define IH_TYPE_SCRIPT 6 /* Script file
> */
> > #define IH_TYPE_FILESYSTEM 7 /* Filesystem Image
> (any type) */
> > #define IH_TYPE_FLATDT 8 /* Binary Flat
> Device Tree Blob */
> > +#define IH_TYPE_KWBIMAGE 9 /* Boot Rom
> Image */
>
> Please s/Boot Rom Image/Kirkwood Boot Rom Image/ or similar.
Okay
>
> > diff --git a/tools/kwbimage.c b/tools/kwbimage.c new file
> mode 100644
> > index 0000000..06f256e
> > --- /dev/null
> > +++ b/tools/kwbimage.c
> > ...
> > +/*
> > + * Generates 32 bit checksum
> > + */
> > +u32 kwbimage_checksum32(void *start, u32 len, u32 csum) {
> > + register u32 sum = csum;
> > + volatile u32 *startp = (volatile u32 *)start;
> > +
> > + do {
> > + sum += *startp;
> > + startp++;
> > + len -= sizeof(u32);
> > + } while (len > 0);
> > + return (sum);
> > +}
>
> This will fail if start is not 32bit aligned; if you can
> guarantee that it is always aligned, then please pass a u32 pointer.
Yes the pointer is char *, I will add check for 32bit alligned ptr and use of
temp buffer to correct this
>
> > +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, i;
> > + char *line = NULL;
> > + char cmdstr[MAX_KWBCMD_LEN], datastr[MAX_KWBDATA_LEN];
> > + size_t len = 0;
> > +
> > + exthdr->dramregsoffs = (u32)&exthdr->rcfg - (u32)mhdr;
> > +
> > + if ((fd = fopen(fname, "r")) == 0) {
> > + fprintf (stderr, "Err: Can't open %s: %s\n",
> > + fname, strerror(errno));
> > + exit (EXIT_FAILURE);
> > + }
> > +
> > + i = lineno = 0;
> > + while ((getline(&line, &len, fd)) != -1) {
> > + /* skip empty lines and lines staring with # */
>
> s/staring/starting/
Okay
>
> > + lineno++;
> > + if (!(line[0] != '#' && strlen(line) != 1))
> > + continue;
>
> This is a bit simple-minded. This will for example fail on
> DOS-formatted files, and for lines that contain only white
> space (which still look "empty" to most users and are thus
> hard to spot).
To take care of Dos formatted file I should use "strlen(line) <= 1" right
As explained in doc.README.kwimage,
any other line apart from above will be considered as valid configuration line.
This is bare minimal parsing provided here which is sufficient
>
> > + if (strlen(line) > (MAX_KWBCMD_LEN + MAX_KWBDATA_LEN)) {
> > + printf("Err.. %s(line no %d) too long\n",
>
> "Error: %s[%d] Line too long\n" ?
Good one.. I will update this
>
> > diff --git a/tools/kwbimage.h b/tools/kwbimage.h new file
> mode 100644
> > index 0000000..c54b701
> > --- /dev/null
> > +++ b/tools/kwbimage.h
> > ...
> > +/* typedefs */
> > +typedef char s8;
> > +typedef unsigned char u8;
> > +
> > +typedef int s32;
> > +typedef unsigned int u32;
> > +
> > +typedef short s16;
> > +typedef unsigned short u16;
> > +
> > +typedef long s64;
> > +typedef unsigned long u64;
>
> Please get rid of these.
Okay
>
> > diff --git a/tools/mkimage.c b/tools/mkimage.c index
> c5b593c..9a11071
> > 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;
> >
> > @@ -251,7 +252,13 @@ NXTARG: ;
> > * write dummy header, to be fixed later
> > */
> > int hdr_size;
> > - 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 ();
> > +
> > memset (hdr, 0, hdr_size);
> > if (write(ifd, hdr, hdr_size) != hdr_size) {
> > fprintf (stderr, "%s: Write error on %s: %s\n",
> > @@ -339,6 +346,19 @@ NXTARG: ;
> >
> > hdr = (image_header_t *)ptr;
> >
> > + /* Build new header */
> > + if (opt_type == IH_TYPE_KWBIMAGE) {
> > + checksum = kwbimage_checksum32((void *)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,
> > (const char *)(ptr + hdr_size),
> > sbuf.st_size - hdr_size);
> > @@ -361,6 +381,7 @@ NXTARG: ;
> >
> > image_print_contents (hdr);
> >
> > + }
> > (void) munmap((void *)ptr, sbuf.st_size);
> >
> > /* We're a bit of paranoid */
>
> Hmm... it seems you add only image creation code. But "mkimage -l"
> should work on such an image, too. And "imls" in U-Boot
> should be working, too.
Well I will disable other generic mkimage options including -l for kwbimage ;-)
Can we add this in second part which is not required too?
For me great thing is that we can support kwimage generation through mkimage.
Regards..
Prafulla . .
>
> 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 "Plan to throw one away. You will anyway."
> - Fred Brooks, "The Mythical Man Month"
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
