Dear Amarendra, On 05/04/13 16:39, Amarendra Reddy wrote: > Hi Jaehoon, > > Please find the responses below > > Thanks & Regards > Amarendra Reddy > > On 5 April 2013 11:51, Jaehoon Chung <jh80.ch...@samsung.com> wrote: > >> Hi Amar, >> >> On 04/03/2013 11:08 PM, Amar wrote: >>> This patch adds commands to access(open/close) and resize boot >> partitions on EMMC. >>> >>> Signed-off-by: Amar <amarendra...@samsung.com> >>> --- >>> Changes since V1: >>> 1)Combined the common piece of code between 'open' and 'close' >>> operations. >>> >>> Changes since V2: >>> 1)Updation of commit message and resubmition of proper patch set. >>> >>> Changes since V3: >>> No change. >>> >>> Changes since V4: >>> 1)Added a new funtion boot_part_access() to combine the common >> parts of >>> 'mmc open' and 'mmc close' functionalities. >>> 2)Used the generic function "mmc_boot_part_access()" instead of >>> two functions "mmc_boot_open()" and "mmc_boot_close()". By doing >> so user >>> can specify which boot partition to be accessed (opened / closed). >>> >>> Changes since V5: >>> 1)Updated minor nits in response to review comments. >>> >>> Changes since V6: >>> No change. >>> >>> Changes since V7: >>> 1)The piece of code involved in open/close and resize of EMMC boot >>> partition has been made conditional and is enabled only if the >> macro >>> CONFIG_SUPPORT_EMMC_BOOT is defined. >>> >>> common/cmd_mmc.c | 110 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 108 insertions(+), 2 deletions(-) >>> >>> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c >>> index 8c53a10..c5f60a2 100644 >>> --- a/common/cmd_mmc.c >>> +++ b/common/cmd_mmc.c >>> @@ -147,6 +147,36 @@ U_BOOT_CMD( >>> "- display info of the current MMC device" >>> ); >>> >>> +#ifdef CONFIG_SUPPORT_EMMC_BOOT >>> +static int boot_part_access(struct mmc *mmc, u32 ack, u32 part_num, u32 >> access) >> Why do you use u32? I know that just used 8bit. >> > > Yes, just 8bit would suffice. I will update it. > >> +{ >>> + int err; >>> + err = mmc_boot_part_access(mmc, ack, part_num, access); >>> + >>> + if ((err == 0) && (access != 0)) { >>> + printf("\t\t\t!!!Notice!!!\n"); >>> + >>> + printf("!You must close EMMC boot Partition"); >>> + printf("after all images are written\n"); >>> + >>> + printf("!EMMC boot partition has continuity"); >>> + printf("at image writing time.\n"); >>> + >>> + printf("!So, do not close the boot partition"); >>> + printf("before all images are written.\n"); >>> + return 0; >>> + } else if ((err == 0) && (access == 0)) >>> + return 0; >>> + else if ((err != 0) && (access != 0)) { >>> + printf("EMMC boot partition-%d OPEN Failed.\n", part_num); >>> + return 1; >>> + } else { >>> + printf("EMMC boot partition-%d CLOSE Failed.\n", part_num); >>> + return 1; >>> + } >>> +} >>> +#endif >>> + >>> static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const >> argv[]) >>> { >>> enum mmc_state state; >>> @@ -248,8 +278,75 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag, >> int argc, char * const argv[]) >>> curr_device, mmc->part_num); >>> >>> return 0; >>> - } >>> +#ifdef CONFIG_SUPPORT_EMMC_BOOT >>> + } else if ((strcmp(argv[1], "open") == 0) || >>> + (strcmp(argv[1], "close") == 0)) { >>> + int dev; >>> + struct mmc *mmc; >>> + u32 ack, part_num, access = 0; >>> + >>> + if (argc == 4) { >>> + dev = simple_strtoul(argv[2], NULL, 10); >>> + part_num = simple_strtoul(argv[3], 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 ((part_num <= 0) || (part_num > >> MMC_NUM_BOOT_PARTITION)) { >>> + printf("Invalid boot partition number:\n"); >>> + printf("Boot partition number cannot be <= 0\n"); >>> + printf("EMMC44 supports only 2 boot partitions\n"); >> What means? EMMC44 supports only 2 boot partitions? >> > > As per the eMMC 4.4 specification, eMMC 4.4 has 1 RPMB partition, 2 Boot > partitions and 4 General Purpose partitions. > As there are only 2 Boot partitions, we restrict the user not to use an > invalid partition number. > >> + return 1; >>> + } >>> + >>> + if (strcmp(argv[1], "open") == 0) >>> + access = part_num; /* enable R/W access to boot >> part*/ >>> + if (strcmp(argv[1], "close") == 0) >> else if? >> > > Even 'else if' is fine, but I wanted to explicitly check the string in both > cases.
Why? I think, It doesn't make sense. > > >>> + access = 0; /* No access to boot partition */ >>> + >>> + /* acknowledge to be sent during boot operation */ >>> + ack = 1; >> ack is always set to 1? then Can't initialize ack value as 1. >> Otherwise, boot_part_access(mmc, 1, part_num, access)? >> >> As per JEDEC Standard 'ack' can be 0 / 1. > Right now we are always using ack=1. > Is it OK to use "boot_part_access(mmc, 1, part_num, access)" with a > hardcoded value '1' in the function call ? Yes, It's a constant. > > Best Regards, >> Jaehoon Chung >>> + return boot_part_access(mmc, ack, part_num, access); >>> + >>> + } 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; >>> + } >>> +#endif /* CONFIG_SUPPORT_EMMC_BOOT */ >>> + } >>> state = MMC_INVALID; >>> if (argc == 5 && strcmp(argv[1], "read") == 0) >>> state = MMC_READ; >>> @@ -324,5 +421,14 @@ 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" >>> +#ifdef CONFIG_SUPPORT_EMMC_BOOT >>> + "mmc open <dev> <boot_partition>\n" >>> + " - Enable boot_part for booting and enable R/W access of >> boot_part\n" >>> + "mmc close <dev> <boot_partition>\n" >>> + " - Enable boot_part for booting and disable access to boot_part\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 >>> + ); >>> +#endif /* !CONFIG_GENERIC_MMC */ >>> >> >> _______________________________________________ >> 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 > Thanks, Minkyu Kang. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot