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);elsereturn 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 changeFailed.\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 sizeMB>\n"
" - change sizes of boot and RPMB partions of specifieddevice\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
- 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