Patchwork [U-Boot,V8,9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition

login
register
mail settings
Submitter Amar
Date April 3, 2013, 2:08 p.m.
Message ID <1364998113-13428-10-git-send-email-amarendra.xt@samsung.com>
Download mbox | patch
Permalink /patch/233482/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Amar - April 3, 2013, 2:08 p.m.
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(-)
Jaehoon Chung - April 5, 2013, 6:21 a.m.
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 */
>
Amarendra Reddy - April 5, 2013, 7:39 a.m.
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
>
Minkyu Kang - April 9, 2013, 10:55 a.m.
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.
Amarendra Reddy - April 9, 2013, 11:27 a.m.
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.
>
>

Patch

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 */