[U-Boot] [PATCH 2/3] tools: mkimage (type=kwbimage) kirkwood boot image support

Prafulla Wadaskar prafulla at marvell.com
Sun Jul 19 07:27:34 CEST 2009


Dear Wolfgang
Thanks for your quick feedback 

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Sunday, July 19, 2009 3:33 AM
> To: Prafulla Wadaskar
> Cc: u-boot at 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-prafulla at 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 <prafulla at 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: 
> wd at denx.de "Plan to throw one away. You will anyway."
>                               - Fred Brooks, "The Mythical Man Month"
> 


More information about the U-Boot mailing list