| Message ID | 1357292050-12137-10-git-send-email-amarendra.xt@samsung.com |
|---|---|
| State | New |
| Headers | show |
Hi Amar, On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@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.xt@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'? Regards, Simon > -- > 1.8.0 >
On 01/11/2013 01:46 AM, Simon Glass wrote: > Hi Amar, > > On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@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.xt@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 >> >
HI Jaehoon, On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung@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.xt@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.xt@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>" Maybe - what do these parameters mean? > Also i think that we can reduce the code line. OK good. Regards, Simon > > Best Regards, > Jaehoon Chung >> >> Regards, >> Simon >> >>> -- >>> 1.8.0 >>> >> >
Hi Simon / Jaehoon, Please find my responses below. Thanks & Regards Amarendra reddy On 11 January 2013 11:11, Simon Glass <sjg@chromium.org> wrote: > HI Jaehoon, > > On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung@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.xt@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.xt@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 >
Hi Amarendra, On Fri, Jan 11, 2013 at 5:50 AM, Amarendra Reddy <amar.lavanuru@gmail.com> wrote: > Hi Simon / Jaehoon, > > Please find my responses below. > > Thanks & Regards > Amarendra reddy > > On 11 January 2013 11:11, Simon Glass <sjg@chromium.org> wrote: >> >> HI Jaehoon, >> >> On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung@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.xt@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.xt@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. Yes sounds good. >> >> > Also i think that we can reduce the code line. >> >> OK good. >> [snip] Regards, Simon
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)) { + 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"); + 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; + } + } + + if (strcmp(argv[1], "close") == 0) { + if (!(mmc_boot_close(mmc))) { + printf("EMMC CLOSE Success.\n"); + 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
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.xt@samsung.com> --- common/cmd_mmc.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-)