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

Simon Glass sjg at chromium.org
Fri Jan 11 15:31:16 CET 2013


Hi Amarendra,

On Fri, Jan 11, 2013 at 5:50 AM, Amarendra Reddy
<amar.lavanuru at gmail.com> wrote:
> Hi Simon / Jaehoon,
>
> Please find my responses below.
>
> Thanks & Regards
> Amarendra reddy
>
> On 11 January 2013 11:11, Simon Glass <sjg at chromium.org> wrote:
>>
>> 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?
>
>
> 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


More information about the U-Boot mailing list