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

[Simon Glass]

HI Jaehoon,

On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung at samsung.com> wrote:
> On 01/11/2013 01:46 AM, Simon Glass wrote:
>> Hi Amar,
>>
>> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt at 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 at 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>"

Maybe - what do these parameters mean?

> Also i think that we can reduce the code line.

OK good.

Regards,
Simon

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