[U-Boot] [PATCH 9/9] COMMON: MMC: Command to support eMMC booting

[Amarendra Reddy]

Hi SImon,

Thanks for the review comments.
Please find below the responses for your comments.


Thanks & Regards
Amarendra



On 20 December 2012 08:10, Simon Glass <sjg at chromium.org> wrote:

> Hi Amar,
>
> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt at samsung.com> wrote:
>
> > This patch adds commands to open, close and create partitions on eMMC
> >
> > Signed-off-by: Amar <amarendra.xt at samsung.com>
> > ---
> >  common/cmd_mmc.c |  101
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 100 insertions(+), 1 deletions(-)
> >
> > diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
> > index 62a1c22..1fd6b94 100644
> > --- a/common/cmd_mmc.c
> > +++ b/common/cmd_mmc.c
> > @@ -248,6 +248,102 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag,
> int
> > argc, char * const argv[])
> >                                 curr_device, mmc->part_num);
> >
> >                 return 0;
> > +       } else if (strcmp(argv[1], "open") == 0) {
> > +               int dev;
> > +               struct mmc *mmc;
> > +
> > +               if (argc == 2)
> > +                       dev = curr_device;
> > +               else if (argc == 3)
> > +                       dev = simple_strtoul(argv[2], NULL, 10);
> > +               else if (argc == 4)
> > +                       return 1;
> >
>
> Should this be  CMD_RET_USAGE? What is returning 1 for?
>  Yes. Shall correct it.
> > +
> > +               else
> > +                       return CMD_RET_USAGE;
> > +
> > +               mmc = find_mmc_device(dev);
> > +               if (!mmc) {
> > +                       printf("no mmc device at slot %x\n", dev);
> > +                       return 1;
> > +               }
> > +
> > +               if (IS_SD(mmc)) {
> > +                       printf("SD device cannot be opened/closed\n");
> > +                       return 1;
> > +               }
> > +
> > +               if (!(mmc_boot_open(mmc))) {
> > +                       printf("eMMC OPEN Success.!!\n");
> > +                       printf("\t\t\t!!!Notice!!!\n");
> > +                       printf("!You must close eMMC boot Partition"
> > +                                               "after all image
> > writing!\n");
> > +                       printf("!eMMC boot partition has continuity"
> > +                                               "at image writing
> > time.!\n");
> > +                       printf("!So, Do not close boot partition,
> Before,"
> > +                                               "all images is
> > written.!\n");
> >
>
> Maybe:
>
>  So, do not close boot partition before all images are written
> OK.. will change the wordings.

+               } else {
> > +                       printf("eMMC OPEN Failed.!!\n");
> >
>
> Is it eMMC or MMC? Lower case or upper case? Your messages should be
> consistent. And you don't need the !!! I think.
>
> OK. Will maintain EMMC consistently every where. Will remove "!!!".
>
 > +               }
> > +               return 0;
> > +
> > +       } else if (strcmp(argv[1], "close") == 0) {
> > +               int dev;
> > +               struct mmc *mmc;
> > +
> > +               if (argc == 2)
> > +                       dev = curr_device;
> > +               else if (argc == 3)
> > +                       dev = simple_strtoul(argv[2], NULL, 10);
> > +               else if (argc == 4)
> > +                       return 1;
> >
>
> Same Q here as above.
>
> Ok, will be addressed.
>

> > +               else
> > +                       return CMD_RET_USAGE;
> > +
> > +               mmc = find_mmc_device(dev);
> > +               if (!mmc) {
> > +                       printf("no mmc device at slot %x\n", dev);
> > +                       return 1;
> > +               }
> > +
> > +               if (IS_SD(mmc)) {
> > +                       printf("SD device cannot be opened/closed\n");
> > +                       return 1;
> > +               }
> > +
> >
>
> Seems the open/close code is almost the same. Can you combine it?
>
 Ok. Will combine open/close.
>
> > +               if (!(mmc_boot_close(mmc)))
> > +                       printf("eMMC CLOSE Success.!!\n");
> > +               else
> > +                       printf("eMMC CLOSE Failed.!!\n");
> > +
> > +               return 0;
> > +
> > +       } else if (strcmp(argv[1], "bootpart") == 0) {
> > +               int dev;
> > +               dev = simple_strtoul(argv[2], NULL, 10);
> > +
> > +               struct mmc *mmc = find_mmc_device(dev);
> > +               u32 bootsize = simple_strtoul(argv[3], NULL, 10);
> > +               u32 rpmbsize = simple_strtoul(argv[4], NULL, 10);
> > +
> > +               if (!mmc) {
> > +                       printf("no mmc device at slot %x\n", dev);
> > +                       return 1;
> > +               }
> > +
> > +               if (IS_SD(mmc)) {
> > +                       printf("It is not a eMMC device\n");
> > +                       return 1;
> > +               }
> > +
> > +               if (0 == mmc_boot_partition_size_change(mmc,
> > +                                               bootsize, rpmbsize)) {
> > +                       printf("eMMC boot partition Size %d MB!!\n",
> > bootsize);
> > +                       printf("eMMC RPMB partition Size %d MB!!\n",
> > rpmbsize);
> > +               } else {
> > +                       printf("eMMC boot partition Size change
> > Failed.!!\n");
> >
>
> return 1 here I think
>
 Yes. WIll be corrected.
>
> > +               }
> > +               return 0;
> >         }
> >
> >         if (strcmp(argv[1], "read") == 0)
> > @@ -318,5 +414,8 @@ U_BOOT_CMD(
> >         "mmc rescan\n"
> >         "mmc part - lists available partition on current mmc device\n"
> >         "mmc dev [dev] [part] - show or set current mmc device
> > [partition]\n"
> > -       "mmc list - lists available devices");
> > +       "mmc list - lists available devices\n"
> > +       "mmc open <device num> - opens the specified device\n"
> > +       "mmc close <device num> - closes the specified device\n"
> > +       "mmc bootpart <device num> <boot part size MB> <RPMB part size
> > MB>\n");
> >
>
> Can you add another line of help here explaining what this does?
> OK. I will add another line of help for mmc bootpart.
>
> >  #endif
> > --
> > 1.7.0.4
> >
> > Regards,
> Simon
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>