Dear Prabhakar Lad, In message <1340708826-26707-1-git-send-email-prabhakar....@ti.com> you wrote: > From: Alagu Sankar <alagusan...@embwise.com> > > This is a Linux command line tool specific to TI's Davinci platforms, for > flashing UBL (User Boot Loader), u-boot and u-boot Environment in the MMC/SD > card. This MMC/SD card can be used for booting Davinci platforms that supports > MMC/SD boot option.
Do we also build UBL as part of the U-Boot source tree? If not, then why is this tool supposed to be part of the U-Boot tree? How does this work with a SPL? > --- a/Makefile > +++ b/Makefile > @@ -726,7 +726,7 @@ clean: > @rm -f $(obj)examples/api/demo{,.bin} > @rm -f $(obj)tools/bmp_logo $(obj)tools/easylogo/easylogo \ > $(obj)tools/env/{fw_printenv,fw_setenv} \ > - $(obj)tools/envcrc \ > + $(obj)tools/envcrc $(obj)tools/uflash/uflash \ > $(obj)tools/gdb/{astest,gdbcont,gdbsend} \ > $(obj)tools/gen_eth_addr $(obj)tools/img2srec \ > $(obj)tools/mk{env,}image $(obj)tools/mpc86x_clk \ Please keep list sorted. > +e. Using the 'uflash' utility, place the UBL and u-uoot binaries on the MMC > + card. Copy the u-boot.bin to tools/uflash directory Why is this copy operation needed? And where is the UBL binary coming from? > diff --git a/tools/uflash/config.txt b/tools/uflash/config.txt > new file mode 100644 > index 0000000..f6acb22 > --- /dev/null > +++ b/tools/uflash/config.txt > @@ -0,0 +1,11 @@ > +bootargs=console=ttyS0,115200n8 root=/dev/mmcblk0p1 rootwait rootfstype=ext3 > rw > +bootcmd=ext2load mmc 0 0x80700000 boot/uImage; bootm 0x80700000 > +bootdelay=1 > +baudrate=115200 > +bootfile="uImage" > +stdin=serial > +stdout=serial > +stderr=serial > +ethact=dm9000 > +videostd=ntsc This looks like U-Boot environment settings? Why are these in the tools/uflash/ directory? I would expect these are board specific? For example, what in case a board uses a different baud rate? Is this really supposed to be board independent? It doesn't look so... > + And please, no trailing empty lines! ... > + if (!strcmp(platform, "DM3XX")) { > + if (!uboot_load_address) > + uboot_load_address = DM3XX_UBOOT_LOAD_ADDRESS; > + if (!uboot_entry_point) > + uboot_entry_point = DM3XX_UBOOT_LOAD_ADDRESS; > + } > + > + if (!strcmp(platform, "OMAPL138")) { > + if (!uboot_load_address) > + uboot_load_address = DA850_UBOOT_LOAD_ADDRESS; > + if (!uboot_entry_point) > + uboot_entry_point = DA850_UBOOT_LOAD_ADDRESS; > + } So this is actually all hardwired for a few very specific board configurations, right? . > +static int get_file_size(char *fname) > +{ > + FILE *fp; > + int size; > + > + fp = fopen(fname, "rb"); > + if (fp == NULL) { > + fprintf(stdout, "File %s Open Error : %s\n", > + fname, strerror(errno)); > + return -1; > + } > + > + fseek(fp, 0, SEEK_END); > + size = ftell(fp); > + fclose(fp); Why not simply using stat() ? > +static int write_file(int devfd, char *fname) > +{ > + FILE *fp; > + int readlen, writelen; > + > + fp = fopen(fname, "rb"); > + if (fp == NULL) { > + fprintf(stderr, "File %s Open Error: %s", > + fname, strerror(errno)); > + return -1; > + } > + > + while ((readlen = fread(readbuf, 1, BLOCK_SIZE, fp)) > 0) { > + if (readlen < BLOCK_SIZE) > + memset(&readbuf[readlen], 0, BLOCK_SIZE-readlen); > + > + writelen = write(devfd, readbuf, BLOCK_SIZE); > + if (writelen < BLOCK_SIZE) { > + close(devfd); > + return -1; > + } > + } > + > + fclose(fp); You don't even print a warning or error message in case of read errors? Ouch... 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 C makes it easy for you to shoot yourself in the foot. C++ makes that harder, but when you do, it blows away your whole leg. -- Bjarne Stroustrup _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot