| Message ID | 1364998113-13428-10-git-send-email-amarendra.xt@samsung.com |
|---|---|
| State | New |
| Headers | show |
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.xt@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. > +{ > + 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? > + 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? > + 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)? 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 */ >
Hi Jaehoon, Please find the responses below Thanks & Regards Amarendra Reddy On 5 April 2013 11:51, Jaehoon Chung <jh80.chung@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.xt@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. > > + 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 ? 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 >
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.chung@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.xt@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.
Dear Minkyu, Thankyou for the review comments. Please find my responses below. Thanks & Regards Amarendra Reddy On 9 April 2013 16:25, Minkyu Kang <mk7.kang@samsung.com> wrote: > 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.chung@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.xt@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. > OK. I will replace the above with 'else if'. > > > > > >>> + 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. > I meant from uboot coding standard point of view. Ok. I shall use 'boot_part_access(mmc, 1, part_num, access)' in next patchset. > > > > 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. > >
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) +{ + 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"); + return 1; + } + + if (strcmp(argv[1], "open") == 0) + access = part_num; /* enable R/W access to boot part*/ + if (strcmp(argv[1], "close") == 0) + access = 0; /* No access to boot partition */ + + /* acknowledge to be sent during boot operation */ + ack = 1; + 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 */
This patch adds commands to access(open/close) and resize boot partitions on EMMC. Signed-off-by: Amar <amarendra.xt@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(-)