Hi Simon / Jaehoon, Please find my responses below.
Thanks & Regards Amarendra reddy On 11 January 2013 11:11, Simon Glass <s...@chromium.org> wrote: > HI Jaehoon, > > On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.ch...@samsung.com> > wrote: > > 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? > Ok. Shall put the entire block in a new 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? > Ok. > >> > >>> + 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 > >> > Ok. > >>> + } > >>> + } > >>> + > >>> + if (strcmp(argv[1], "close") == 0) { > >>> + if (!(mmc_boot_close(mmc))) { > >>> + printf("EMMC CLOSE Success.\n"); > >> > >> Shouldn't print a message on success > >> > Ok. shall remove the print message in success case. >>> + 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>" Maybe - what do these parameters mean? > > The functions "mmc_boot_open()" and "mmc_boot_close()" have lot of commom code. So Jaehoon suggested to combine them into single generic function as below 1) So a single generic function "mmc_boot_part_access(struct mmc *mmc, int ack, int part_num, int access)" to be used instead of two functions open() and close(). 2) By doing so user can specify which boot partition to be accessed (opened / closed). The parameters *ack, part_num, access,* represent the values of bits in the PARTITION_CONFIG field of the Extended CSD register in order to address one of the partitions. PARTITION_CONFIG - [179]: ------------------------------------------- Bit 6: BOOT_ACK (R/W/E) 0x0 : No boot acknowledge sent (default) 0x1 : Boot acknowledge sent during boot operation Bit[5:3] : BOOT_PARTITION_ENABLE (R/W/E) User selects boot data that will be sent to master 0x0 : Device not boot enabled (default) 0x1 : Boot partition 1 enabled for boot 0x2 : Boot partition 2 enabled for boot Bit[2:0] : PARTITION_ACCESS (before BOOT_PARTITION_ACCESS, R/W/E_P) User selects partitions to access 0x0 : No access to boot partition (default) 0x1 : R/W boot partition 1 0x2 : R/W boot partition 2 0x3 : R/W Replay Protected Memory Block (RPMB) Please comment on the above. > > Also i think that we can reduce the code line. > > OK good. > > Regards, > Simon > > > > > 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 >
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot