Hi Wolfgang,
On Wed, Jun 27, 2012 at 03:12:07, Wolfgang Denk wrote:
> 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?
>
No, we do not build UBL as part of U-Boot.
> If not, then why is this tool supposed to be part of the U-Boot tree?
>
> How does this work with a SPL?
>
This command has options to flash u-boot images to MMC/SD card. When SPL
is supported, this command can be used to flash the single SPL image to
MMC/SD card.
> > --- 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.
>
Ok.
>
> > +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?
>
This copy is not needed as long as the path to u-boot.bin is specified
Correctly in command line.
> And where is the UBL binary coming from?
>
UBL binary is optional. We can flash only u-boot.bin.
>
>
> > 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...
>
I agree with this. Can you think of any other scenario?
> > +
>
> And please, no trailing empty lines!
>
Ok.
> ...
> > + 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?
>
Yes.
> .
> > +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() ?
>
Yes that makes sense.
> > +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...
>
Ok , I'll fix it in V2 version.
Thx,
--Prabhakar Lad
>
> 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
