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

login
register
mail settings
Submitter Amar
Date Dec. 17, 2012, 11:19 a.m.
Message ID <1355743176-12305-10-git-send-email-amarendra.xt@samsung.com>
Download mbox | patch
Permalink /patch/206829/
State Superseded
Delegated to: Minkyu Kang
Headers show

Comments

Amar - Dec. 17, 2012, 11:19 a.m.
This patch adds commands to open, close and create partitions on eMMC

Signed-off-by: Amar <amarendra.xt@samsung.com>
---
 common/cmd_mmc.c |  101 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 100 insertions(+), 1 deletions(-)
Simon Glass - Dec. 20, 2012, 2:40 a.m.
Hi Amar,

On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt@samsung.com> wrote:

> This patch adds commands to open, close and create partitions on eMMC
>
> Signed-off-by: Amar <amarendra.xt@samsung.com>
> ---
>  common/cmd_mmc.c |  101
> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 100 insertions(+), 1 deletions(-)
>
> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
> index 62a1c22..1fd6b94 100644
> --- a/common/cmd_mmc.c
> +++ b/common/cmd_mmc.c
> @@ -248,6 +248,102 @@ 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) {
> +               int dev;
> +               struct mmc *mmc;
> +
> +               if (argc == 2)
> +                       dev = curr_device;
> +               else if (argc == 3)
> +                       dev = simple_strtoul(argv[2], NULL, 10);
> +               else if (argc == 4)
> +                       return 1;
>

Should this be  CMD_RET_USAGE? What is returning 1 for?

> +
> +               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 (!(mmc_boot_open(mmc))) {
> +                       printf("eMMC OPEN Success.!!\n");
> +                       printf("\t\t\t!!!Notice!!!\n");
> +                       printf("!You must close eMMC boot Partition"
> +                                               "after all image
> writing!\n");
> +                       printf("!eMMC boot partition has continuity"
> +                                               "at image writing
> time.!\n");
> +                       printf("!So, Do not close boot partition, Before,"
> +                                               "all images is
> written.!\n");
>

Maybe:

 So, do not close boot partition before all images are written

+               } else {
> +                       printf("eMMC OPEN Failed.!!\n");
>

Is it eMMC or MMC? Lower case or upper case? Your messages should be
consistent. And you don't need the !!! I think.


> +               }
> +               return 0;
> +
> +       } else if (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 if (argc == 4)
> +                       return 1;
>

Same Q here as above.


> +               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;
> +               }
> +
>

Seems the open/close code is almost the same. Can you combine it?


> +               if (!(mmc_boot_close(mmc)))
> +                       printf("eMMC CLOSE Success.!!\n");
> +               else
> +                       printf("eMMC CLOSE Failed.!!\n");
> +
> +               return 0;
> +
> +       } else if (strcmp(argv[1], "bootpart") == 0) {
> +               int dev;
> +               dev = simple_strtoul(argv[2], NULL, 10);
> +
> +               struct mmc *mmc = find_mmc_device(dev);
> +               u32 bootsize = simple_strtoul(argv[3], NULL, 10);
> +               u32 rpmbsize = simple_strtoul(argv[4], NULL, 10);
> +
> +               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);
> +               } else {
> +                       printf("eMMC boot partition Size change
> Failed.!!\n");
>

return 1 here I think


> +               }
> +               return 0;
>         }
>
>         if (strcmp(argv[1], "read") == 0)
> @@ -318,5 +414,8 @@ 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");
>

Can you add another line of help here explaining what this does?


>  #endif
> --
> 1.7.0.4
>
> Regards,
Simon
Amarendra Reddy - Dec. 20, 2012, 1:45 p.m.
Hi SImon,

Thanks for the review comments.
Please find below the responses for your comments.


Thanks & Regards
Amarendra



On 20 December 2012 08:10, Simon Glass <sjg@chromium.org> wrote:

> Hi Amar,
>
> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt@samsung.com> wrote:
>
> > This patch adds commands to open, close and create partitions on eMMC
> >
> > Signed-off-by: Amar <amarendra.xt@samsung.com>
> > ---
> >  common/cmd_mmc.c |  101
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 100 insertions(+), 1 deletions(-)
> >
> > diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
> > index 62a1c22..1fd6b94 100644
> > --- a/common/cmd_mmc.c
> > +++ b/common/cmd_mmc.c
> > @@ -248,6 +248,102 @@ 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) {
> > +               int dev;
> > +               struct mmc *mmc;
> > +
> > +               if (argc == 2)
> > +                       dev = curr_device;
> > +               else if (argc == 3)
> > +                       dev = simple_strtoul(argv[2], NULL, 10);
> > +               else if (argc == 4)
> > +                       return 1;
> >
>
> Should this be  CMD_RET_USAGE? What is returning 1 for?
>  Yes. Shall correct it.
> > +
> > +               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 (!(mmc_boot_open(mmc))) {
> > +                       printf("eMMC OPEN Success.!!\n");
> > +                       printf("\t\t\t!!!Notice!!!\n");
> > +                       printf("!You must close eMMC boot Partition"
> > +                                               "after all image
> > writing!\n");
> > +                       printf("!eMMC boot partition has continuity"
> > +                                               "at image writing
> > time.!\n");
> > +                       printf("!So, Do not close boot partition,
> Before,"
> > +                                               "all images is
> > written.!\n");
> >
>
> Maybe:
>
>  So, do not close boot partition before all images are written
> OK.. will change the wordings.

+               } else {
> > +                       printf("eMMC OPEN Failed.!!\n");
> >
>
> Is it eMMC or MMC? Lower case or upper case? Your messages should be
> consistent. And you don't need the !!! I think.
>
> OK. Will maintain EMMC consistently every where. Will remove "!!!".
>
 > +               }
> > +               return 0;
> > +
> > +       } else if (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 if (argc == 4)
> > +                       return 1;
> >
>
> Same Q here as above.
>
> Ok, will be addressed.
>

> > +               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;
> > +               }
> > +
> >
>
> Seems the open/close code is almost the same. Can you combine it?
>
 Ok. Will combine open/close.
>
> > +               if (!(mmc_boot_close(mmc)))
> > +                       printf("eMMC CLOSE Success.!!\n");
> > +               else
> > +                       printf("eMMC CLOSE Failed.!!\n");
> > +
> > +               return 0;
> > +
> > +       } else if (strcmp(argv[1], "bootpart") == 0) {
> > +               int dev;
> > +               dev = simple_strtoul(argv[2], NULL, 10);
> > +
> > +               struct mmc *mmc = find_mmc_device(dev);
> > +               u32 bootsize = simple_strtoul(argv[3], NULL, 10);
> > +               u32 rpmbsize = simple_strtoul(argv[4], NULL, 10);
> > +
> > +               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);
> > +               } else {
> > +                       printf("eMMC boot partition Size change
> > Failed.!!\n");
> >
>
> return 1 here I think
>
 Yes. WIll be corrected.
>
> > +               }
> > +               return 0;
> >         }
> >
> >         if (strcmp(argv[1], "read") == 0)
> > @@ -318,5 +414,8 @@ 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");
> >
>
> Can you add another line of help here explaining what this does?
> OK. I will add another line of help for mmc bootpart.
>
> >  #endif
> > --
> > 1.7.0.4
> >
> > Regards,
> Simon
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>

Patch

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index 62a1c22..1fd6b94 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -248,6 +248,102 @@  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) {
+		int dev;
+		struct mmc *mmc;
+
+		if (argc == 2)
+			dev = curr_device;
+		else if (argc == 3)
+			dev = simple_strtoul(argv[2], NULL, 10);
+		else if (argc == 4)
+			return 1;
+
+		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 (!(mmc_boot_open(mmc))) {
+			printf("eMMC OPEN Success.!!\n");
+			printf("\t\t\t!!!Notice!!!\n");
+			printf("!You must close eMMC boot Partition"
+						"after all image writing!\n");
+			printf("!eMMC boot partition has continuity"
+						"at image writing time.!\n");
+			printf("!So, Do not close boot partition, Before,"
+						"all images is written.!\n");
+		} else {
+			printf("eMMC OPEN Failed.!!\n");
+		}
+		return 0;
+
+	} else if (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 if (argc == 4)
+			return 1;
+		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 (!(mmc_boot_close(mmc)))
+			printf("eMMC CLOSE Success.!!\n");
+		else
+			printf("eMMC CLOSE Failed.!!\n");
+
+		return 0;
+
+	} else if (strcmp(argv[1], "bootpart") == 0) {
+		int dev;
+		dev = simple_strtoul(argv[2], NULL, 10);
+
+		struct mmc *mmc = find_mmc_device(dev);
+		u32 bootsize = simple_strtoul(argv[3], NULL, 10);
+		u32 rpmbsize = simple_strtoul(argv[4], NULL, 10);
+
+		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);
+		} else {
+			printf("eMMC boot partition Size change Failed.!!\n");
+		}
+		return 0;
 	}
 
 	if (strcmp(argv[1], "read") == 0)
@@ -318,5 +414,8 @@  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");
 #endif