[U-Boot] [PATCH V4 9/9] COMMON: MMC: Command to support EMMC booting and to
Amarendra Reddy
amar.lavanuru at gmail.com
Fri Jan 11 14:50:33 CET 2013
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.
> > 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
More information about the U-Boot
mailing list