On 01/11/2013 01:46 AM, Simon Glass wrote: > Hi Amar, > > On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra...@samsung.com> wrote: >> This patch adds commands to open, close and resize boot partitions on EMMC. >> >> Changes from V1: >> 1)Combined the common piece of code between 'open' and 'close' >> operations. >> >> Changes from V2: >> 1)Updation of commit message and resubmition of proper patch set. >> >> Changes from V3: >> No change. >> >> Signed-off-by: Amar <amarendra...@samsung.com> >> --- >> common/cmd_mmc.c | 84 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 83 insertions(+), 1 deletion(-) >> >> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c >> index 7dacd51..1dabb5b 100644 >> --- a/common/cmd_mmc.c >> +++ b/common/cmd_mmc.c >> @@ -248,6 +248,84 @@ 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) || >> + (strcmp(argv[1], "close") == 0)) { > > How about putting this block in its own function? > >> + int dev; >> + struct mmc *mmc; >> + >> + if (argc == 2) >> + dev = curr_device; >> + else if (argc == 3) >> + dev = simple_strtoul(argv[2], NULL, 10); >> + 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 (strcmp(argv[1], "open") == 0) { >> + 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" >> + " images are written\n"); > > Do you need to split these strings so much? Perhaps when it is in a > function the indenting will be less? > >> + printf("!EMMC boot partition" >> + " has continuity at" >> + " image writing time.\n"); >> + printf("!So, Do not close boot" >> + " partition, Before, all" >> + " images are written.\n"); >> + return 0; >> + } else { >> + printf("EMMC OPEN Failed.\n"); >> + return 1; > > You could put this above the other block and reduce indenting: > > if (mmc_boot_open(mmc)) { > printf("EMMC OPEN Failed.\n"); > return 1; > } > ...code continues > >> + } >> + } >> + >> + if (strcmp(argv[1], "close") == 0) { >> + if (!(mmc_boot_close(mmc))) { >> + printf("EMMC CLOSE Success.\n"); > > Shouldn't print a message on success > >> + return 0; >> + } else { >> + printf("EMMC CLOSE Failed.\n"); >> + return 1; >> + } >> + } >> + } else if (strcmp(argv[1], "bootpart") == 0) { >> + int dev; >> + dev = simple_strtoul(argv[2], NULL, 10); >> + >> + u32 bootsize = simple_strtoul(argv[3], NULL, 10); >> + u32 rpmbsize = simple_strtoul(argv[4], NULL, 10); >> + struct mmc *mmc = find_mmc_device(dev); >> + 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); >> + return 0; >> + } else { >> + printf("EMMC boot partition Size change Failed.\n"); >> + return 1; >> + } >> } >> >> state = MMC_INVALID; >> @@ -317,5 +395,9 @@ 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" >> + " - change sizes of boot and RPMB partions of specified device\n"); >> #endif > > Also did you see Wolfgang's suggestion that we put the partition stuff > in the 'part' command (at least that's what I think he said). You > could have 'part open', 'part close' and maybe 'part resize'? How about using "mmc bootpart <device_num> <ack> <enable> <access>" Also i think that we can reduce the code line.
Best Regards, Jaehoon Chung > > Regards, > Simon > >> -- >> 1.8.0 >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot