[U-Boot] [PATCH V4 9/9] COMMON: MMC: Command to support EMMC booting and to

Jaehoon Chung
2013-01-11T03:54:50Z

On 01/11/2013 01:46 AM, Simon Glass wrote:
> Hi Amar,
>
> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
>> This patch adds commands to open, close and resize boot partitions on EMMC.
>>
>> Changes from V1:
>> 1)Combined the common piece of code between 'open' and 'close'
>> operations.
>>
>> Changes from V2:
>> 1)Updation of commit message and resubmition of proper patch set.
>>
>> Changes from V3:
>> No change.
>>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> ---
>> common/cmd_mmc.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
>> index 7dacd51..1dabb5b 100644
>> --- a/common/cmd_mmc.c
>> +++ b/common/cmd_mmc.c
>> @@ -248,6 +248,84 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> curr_device, mmc->part_num);
>>
>> return 0;
>> + } else if ((strcmp(argv[1], "open") == 0) ||
>> + (strcmp(argv[1], "close") == 0)) {
>
> How about putting this block in its own function?
>
>> + int dev;
>> + struct mmc *mmc;
>> +
>> + if (argc == 2)
>> + dev = curr_device;
>> + else if (argc == 3)
>> + dev = simple_strtoul(argv[2], 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 (strcmp(argv[1], "open") == 0) {
>> + if (!(mmc_boot_open(mmc))) {
>> + printf("EMMC OPEN Success.\n");
>> + printf("\t\t\t!!!Notice!!!\n");
>> + printf("!You must close EMMC"
>> + " boot Partition after all"
>> + " images are written\n");
>
> Do you need to split these strings so much? Perhaps when it is in a
> function the indenting will be less?
>
>> + printf("!EMMC boot partition"
>> + " has continuity at"
>> + " image writing time.\n");
>> + printf("!So, Do not close boot"
>> + " partition, Before, all"
>> + " images are written.\n");
>> + return 0;
>> + } else {
>> + printf("EMMC OPEN Failed.\n");
>> + return 1;
>
> You could put this above the other block and reduce indenting:
>
> if (mmc_boot_open(mmc)) {
> printf("EMMC OPEN Failed.\n");
> return 1;
> }
> ...code continues
>
>> + }
>> + }
>> +
>> + if (strcmp(argv[1], "close") == 0) {
>> + if (!(mmc_boot_close(mmc))) {
>> + printf("EMMC CLOSE Success.\n");
>
> Shouldn't print a message on success
>
>> + return 0;
>> + } else {
>> + printf("EMMC CLOSE Failed.\n");
>> + return 1;
>> + }
>> + }
>> + } 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;
>> + }
>> }
>>
>> state = MMC_INVALID;
>> @@ -317,5 +395,9 @@ 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"
>> + "mmc open <device num> - opens the specified device\n"
>> + "mmc close <device num> - closes the specified device\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
>
> Also did you see Wolfgang's suggestion that we put the partition stuff
> in the 'part' command (at least that's what I think he said). You
> could have 'part open', 'part close' and maybe 'part resize'?
How about using "mmc bootpart <device_num> <ack> <enable> <access>"
Also i think that we can reduce the code line.

Best Regards,
Jaehoon Chung
>
> Regards,
> Simon
>
>> --
>> 1.8.0
>>
>