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