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

login
register
mail settings
Submitter Amar
Date Jan. 4, 2013, 9:34 a.m.
Message ID <1357292050-12137-10-git-send-email-amarendra.xt@samsung.com>
Download mbox | patch
Permalink /patch/209411/
State Superseded
Delegated to: Minkyu Kang
Headers show

Comments

Amar - Jan. 4, 2013, 9:34 a.m.
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(-)
Simon Glass - Jan. 10, 2013, 4:46 p.m.
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'?

Regards,
Simon

> --
> 1.8.0
>
Jaehoon Chung - Jan. 11, 2013, 3:54 a.m.
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
>>
>
Simon Glass - Jan. 11, 2013, 5:41 a.m.
HI Jaehoon,

On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung@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@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>"

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
>>>
>>
>
Amarendra Reddy - Jan. 11, 2013, 1:50 p.m.
Hi Simon / Jaehoon,

Please find my responses below.

Thanks & Regards
Amarendra reddy

On 11 January 2013 11:11, Simon Glass <sjg@chromium.org> wrote:

> HI Jaehoon,
>
> On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung@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@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?
>

Ok. Shall put the entire block in a new 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?
>
Ok.

>  >>
> >>> +                               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
> >>
>
Ok.

>  >>> +                       }
> >>> +               }
> >>> +
> >>> +               if (strcmp(argv[1], "close") == 0) {
> >>> +                       if (!(mmc_boot_close(mmc))) {
> >>> +                               printf("EMMC CLOSE Success.\n");
> >>
> >> Shouldn't print a message on success
> >>
>
Ok. shall remove the print message in success case.

>>> +                               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?
>
>
The functions "mmc_boot_open()" and "mmc_boot_close()" have lot of
commom code. So Jaehoon suggested to
combine them into single generic function as below
1) So a single generic function "mmc_boot_part_access(struct mmc *mmc, int
ack, int part_num, int access)" to be used
instead of two functions open() and close().
2) By doing so user can specify which boot partition to be accessed (opened
/ closed).

The parameters *ack, part_num, access,* represent the values of bits in the
PARTITION_CONFIG field
of the Extended CSD register in order to address one of the partitions.
PARTITION_CONFIG - [179]:
-------------------------------------------
Bit 6: BOOT_ACK (R/W/E)
0x0 : No boot acknowledge sent (default)
0x1 : Boot acknowledge sent during boot operation
Bit[5:3] : BOOT_PARTITION_ENABLE (R/W/E)
User selects boot data that will be sent to master
0x0 : Device not boot enabled (default)
0x1 : Boot partition 1 enabled for boot
0x2 : Boot partition 2 enabled for boot
Bit[2:0] : PARTITION_ACCESS (before BOOT_PARTITION_ACCESS, R/W/E_P)
User selects partitions to access
0x0 : No access to boot partition (default)
0x1 : R/W boot partition 1
0x2 : R/W boot partition 2
0x3 : R/W Replay Protected Memory Block (RPMB)

Please comment on the above.

> > Also i think that we can reduce the code line.
>
> OK good.
>
> Regards,
> Simon
>
> >
> > Best Regards,
> > Jaehoon Chung
> >>
> >> Regards,
> >> Simon
> >>
> >>> --
> >>> 1.8.0
> >>>
> >>
> >
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Simon Glass - Jan. 11, 2013, 2:31 p.m.
Hi Amarendra,

On Fri, Jan 11, 2013 at 5:50 AM, Amarendra Reddy
<amar.lavanuru@gmail.com> wrote:
> Hi Simon / Jaehoon,
>
> Please find my responses below.
>
> Thanks & Regards
> Amarendra reddy
>
> On 11 January 2013 11:11, Simon Glass <sjg@chromium.org> wrote:
>>
>> HI Jaehoon,
>>
>> On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.chung@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@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?
>
>
> Ok. Shall put the entire block in a new 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?
>
> Ok.
>>
>> >>
>> >>> +                               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
>> >>
>
> Ok.
>>
>> >>> +                       }
>> >>> +               }
>> >>> +
>> >>> +               if (strcmp(argv[1], "close") == 0) {
>> >>> +                       if (!(mmc_boot_close(mmc))) {
>> >>> +                               printf("EMMC CLOSE Success.\n");
>> >>
>> >> Shouldn't print a message on success
>> >>
>
> Ok. shall remove the print message in success case.
>
>>>> +                               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?
>>
>
> The functions "mmc_boot_open()" and "mmc_boot_close()" have lot of commom
> code. So Jaehoon suggested to
> combine them into single generic function as below
> 1) So a single generic function "mmc_boot_part_access(struct mmc *mmc, int
> ack, int part_num, int access)" to be used
> instead of two functions open() and close().
> 2) By doing so user can specify which boot partition to be accessed (opened
> / closed).
>
> The parameters ack, part_num, access, represent the values of bits in the
> PARTITION_CONFIG field
> of the Extended CSD register in order to address one of the partitions.
> PARTITION_CONFIG - [179]:
> -------------------------------------------
> Bit 6: BOOT_ACK (R/W/E)
> 0x0 : No boot acknowledge sent (default)
> 0x1 : Boot acknowledge sent during boot operation
> Bit[5:3] : BOOT_PARTITION_ENABLE (R/W/E)
> User selects boot data that will be sent to master
> 0x0 : Device not boot enabled (default)
> 0x1 : Boot partition 1 enabled for boot
> 0x2 : Boot partition 2 enabled for boot
> Bit[2:0] : PARTITION_ACCESS (before BOOT_PARTITION_ACCESS, R/W/E_P)
> User selects partitions to access
> 0x0 : No access to boot partition (default)
> 0x1 : R/W boot partition 1
> 0x2 : R/W boot partition 2
> 0x3 : R/W Replay Protected Memory Block (RPMB)
>
> Please comment on the above.

Yes sounds good.

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

[snip]

Regards,
Simon

Patch

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)) {
+		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");
+				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;
+			}
+		}
+
+		if (strcmp(argv[1], "close") == 0) {
+			if (!(mmc_boot_close(mmc))) {
+				printf("EMMC CLOSE Success.\n");
+				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