[U-Boot] [U-Boot, PATCH v3 2/2] fastboot: Flash command support

[Dileep Katta]

Thanks for the review comments. Working on the fix.

Regards,
Dileep

On 11 September 2014 13:42, Wolfgang Denk <wd at denx.de> wrote:
>
> Dear Dileep Katta,
>
> In message <1410417650-16522-2-git-send-email-dileep.katta at linaro.org> you wrote:
> > Flash command internally uses DFU, and Fastboot command initialization is modified to add DFU and partition initialization
> > Added oem format functionality for GPT table creation
> > partitioning code is added as disk/part_fastboot.c for better usability
> >
> > Fastboot flash command code is enabled and being tested on BeagleBone Black.
> > OMAP3 Beagle configuration is modified to fix the build errors, but this configuration needs to be updated as per the flash functionality.
>
> Please fix the line length in the commit message.
>
> Also, checkpatch says:
>
>         WARNING: do not add new typedefs
>         #1073: FILE: include/usb/fastboot.h:98:
>         +typedef struct fastboot_ptentry fastboot_ptentry;
>
> > +int handle_flash(char *part_name, char *response, char *transfer_buffer)
> > +{
> > +     int status = 0;
> > +     printf("download_bytes: 0x%x\n", download_bytes);
> > +     if (download_bytes) {
> > +             struct fastboot_ptentry *ptn;
> > +
> > +             /* Next is the partition name */
> > +             ptn = fastboot_flash_find_ptn(part_name);
> > +
> > +             if (ptn == 0) {
> > +                     printf("Partition:[%s] does not exist\n", part_name);
> > +                     sprintf(response, "FAILpartition does not exist");
> > +             } else if ((download_bytes > ptn->length) &&
> > +                     !(ptn->flags & FASTBOOT_PTENTRY_FLAGS_WRITE_ENV)) {
> > +                     printf("Image too large for the partition\n");
> > +                     sprintf(response, "FAILimage too large for partition");
> > +             } else if (ptn->flags & FASTBOOT_PTENTRY_FLAGS_WRITE_ENV) {
> > +                     /* Check if this is not really a flash write,
> > +                      * but instead a saveenv
> > +                      */
> > +                     unsigned int i = 0;
> > +                     /* Env file is expected with a NULL delimeter between
> > +                      * env variables So replace New line Feeds(0x0a) with
> > +                      * NULL (0x00)
> > +                      */
>
> Incorrect multiline comment style.  Please fix globally.
>
> Also this sequence of comment - declaration - comment is highly
> confusing.  What is the first comment ("Check if..") actually related
> to?
>
> > +                     for (i = 0; i < download_bytes; i++) {
> > +                             if (transfer_buffer[i] == 0x0a)
> > +                                     transfer_buffer[i] = 0x00;
> > +                     }
> > +                     memset(env_ptr->data, 0, ENV_SIZE);
> > +                     memcpy(env_ptr->data, transfer_buffer, download_bytes);
> > +                     do_env_save(NULL, 0, 1, NULL);
> > +                     printf("saveenv to '%s' DONE!\n", ptn->name);
>
> I think it is a bad idea to split formatting / reformatting
> envrionment data alll over the place.  This should be done only once,
> in the environment handling code.  Also, I do not like you using
> do_env_save() here - this should remain a static function.  Why do't
> you simply use saveenv() here?
>
> Finally, the "saveenv to '%s' DONE!\n" is IMO too verbose.  No news is
> Good News - we usually do not report abot the success of operations,
> as this is what we expect.  We should only print error messages when
> such occur - which points out another problem of that code: there is
> no error checking nor handling there...
>
>
> > +static void end_ptbl(struct ptable *ptbl)
> > +{
> > +     struct efi_header *hdr = &ptbl->header;
> > +     u32 n;
> > +
> > +     n = crc32(0, 0, 0);
>
> What exactly is this good for?
>
> > +     n = crc32(n, (void *)ptbl->entry, sizeof(ptbl->entry));
> > +     hdr->entries_crc32 = n;
> > +
> > +     n = crc32(0, 0, 0);
>
> What exactly is this good for?
>
> > +     n = crc32(0, (void *)&ptbl->header, sizeof(ptbl->header));
> > +     hdr->crc32 = n;
> > +}
>
>
> > +     for (n = 0; n < (128/4); n++) {
> > +             /* read partition */
> > +             source[0] = '\0';
> > +             dest[0] = '\0';
> > +             length[0] = '\0';
>
> You overwrite source, dest, and length by the sprintf()s just a few
> lines below.  So why are you doing this here?
>
> > +             mmc_read[2] = source;
> > +             mmc_read[3] = dest;
> > +             mmc_read[4] = length;
> > +
> > +             sprintf(source, "%p", entry);
> > +             sprintf(dest, "0x%x", 0x1+n);
> > +             sprintf(length, "0x%x", 1);
>
>
> > +int board_mmc_fbtptn_init(void)
> > +{
> > +     char *mmc_init[2] = {"mmc", "rescan",};
> > +     char dev[2];
> > +     char *mmc_dev[3] = {"mmc", "dev", NULL};
> > +
> > +     mmc_dev[2] = dev;
>
> Why do you initialize mmc_dev[2] with another value just a line
> above?  Why don't you use the correct value right from the beginning?
>
> > +     if (do_mmcops(NULL, 0, 3, mmc_dev)) {
> > +             printf("MMC DEV: %d selection FAILED!\n", CONFIG_FB_MMCDEV);
> > +             return -1;
> > +     }
> > +
> > +     if (do_mmcops(NULL, 0, 2, mmc_init)) {
> > +                     printf("FAIL:Init of MMC card\n");
> > +                     return 1;
> > +     }
>
> What exactly are your return codes?  Here you return -1 in case of
> error, there +1 - what's the logic behind that?
>
> > +int do_format(void)
> > +{
> > +     struct ptable *ptbl = &the_ptable;
> > +     /* unsigned sector_sz; */
>
> Please remove dead code.
>
> > +     printf("\ndo_format ..!!");
>
> Is this not overly verbose?
>
> > +     /* get mmc info */
> > +     struct mmc *mmc = find_mmc_device(CONFIG_FB_MMCDEV);
>
> Please do not mix code and declarations.  Also, always add a blank
> line after declarations.
>
> > +     if (mmc_init(mmc)) {
> > +             printf("\n mmc init FAILED");
> > +             return -1;
> > +     } else{
>
> After the return, no else is needed here.
>
> > +             printf("\nmmc capacity is: %llu", mmc->capacity);
> > +             printf("\nmmc: number of blocks:0x%lx", mmc->block_dev.lba);
> > +             printf("\nmmc: block size:0x%lx", mmc->block_dev.blksz);
> > +     }
>
> > +     /* 10/11:modified as per PSP release support */
> > +     char *mmc_write[5]  = {"mmc", "write", NULL, NULL, NULL};
> > +     char source[32], dest[32], length[32];
> > +
> > +     char dev[2];
> > +     char *mmc_dev[3] = {"mmc", "dev", NULL};
> > +     mmc_dev[2] = dev;
>
> See above - please do not mix code and declarations, please use the
> right initialization right away.
>
> > +     } else {
> > +             printf("Writing mbr is DONE!\n");
>
> Less verbose, please.
>
>
> 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
> At the source of every error which is blamed on the computer you will
> find at least two human errors, including the error of blaming it  on
> the computer.