Re: [U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition
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
Re: [U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition
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.k...@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.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;
Re: [U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition
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. +{ + 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
Re: [U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition
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. + access = 0; /* No access to boot partition */ + + /* acknowledge to
[U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition
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) +{ + 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; + } + +