[U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition
Minkyu Kang
mk7.kang at samsung.com
Tue Apr 9 12:55:01 CEST 2013
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 at 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 at 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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Thanks,
Minkyu Kang.
More information about the U-Boot
mailing list