[U-Boot] [PATCH V8 9/9] COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition

Amarendra Reddy amar.lavanuru at gmail.com
Fri Apr 5 09:39:50 CEST 2013


Hi Jaehoon,

Please find the responses below

Thanks & Regards
Amarendra Reddy

On 5 April 2013 11:51, Jaehoon Chung <jh80.chung at samsung.com> wrote:

> Hi Amar,
>
> On 04/03/2013 11:08 PM, Amar wrote:
> > This patch adds commands to access(open/close) and resize boot
> partitions on EMMC.
> >
> > Signed-off-by: Amar <amarendra.xt at samsung.com>
> > ---
> > Changes since V1:
> >       1)Combined the common piece of code between 'open' and 'close'
> >       operations.
> >
> > Changes since V2:
> >       1)Updation of commit message and resubmition of proper patch set.
> >
> > Changes since V3:
> >       No change.
> >
> > Changes since V4:
> >       1)Added a new funtion boot_part_access() to combine the common
> parts of
> >       'mmc open' and 'mmc close' functionalities.
> >       2)Used the generic function "mmc_boot_part_access()" instead of
> >       two functions "mmc_boot_open()" and "mmc_boot_close()". By doing
> so user
> >       can specify which boot partition to be accessed (opened / closed).
> >
> > Changes since V5:
> >       1)Updated minor nits in response to review comments.
> >
> > Changes since V6:
> >       No change.
> >
> > Changes since V7:
> >       1)The piece of code involved in open/close and resize of EMMC boot
> >       partition has been made conditional and is enabled only if the
> macro
> >       CONFIG_SUPPORT_EMMC_BOOT is defined.
> >
> >  common/cmd_mmc.c | 110
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 108 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
> > index 8c53a10..c5f60a2 100644
> > --- a/common/cmd_mmc.c
> > +++ b/common/cmd_mmc.c
> > @@ -147,6 +147,36 @@ U_BOOT_CMD(
> >       "- display info of the current MMC device"
> >  );
> >
> > +#ifdef CONFIG_SUPPORT_EMMC_BOOT
> > +static int boot_part_access(struct mmc *mmc, u32 ack, u32 part_num, u32
> access)
> Why do you use u32? I know that just used 8bit.
>

 Yes, just 8bit would suffice. I will update it.

> +{
> > +     int err;
> > +     err = mmc_boot_part_access(mmc, ack, part_num, access);
> > +
> > +     if ((err == 0) && (access != 0)) {
> > +             printf("\t\t\t!!!Notice!!!\n");
> > +
> > +             printf("!You must close EMMC boot Partition");
> > +             printf("after all images are written\n");
> > +
> > +             printf("!EMMC boot partition has continuity");
> > +             printf("at image writing time.\n");
> > +
> > +             printf("!So, do not close the boot partition");
> > +             printf("before all images are written.\n");
> > +             return 0;
> > +     } else if ((err == 0) && (access == 0))
> > +             return 0;
> > +     else if ((err != 0) && (access != 0)) {
> > +             printf("EMMC boot partition-%d OPEN Failed.\n", part_num);
> > +             return 1;
> > +     } else {
> > +             printf("EMMC boot partition-%d CLOSE Failed.\n", part_num);
> > +             return 1;
> > +     }
> > +}
> > +#endif
> > +
> >  static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> >  {
> >       enum mmc_state state;
> > @@ -248,8 +278,75 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag,
> int argc, char * const argv[])
> >                               curr_device, mmc->part_num);
> >
> >               return 0;
> > -     }
> > +#ifdef CONFIG_SUPPORT_EMMC_BOOT
> > +     } else if ((strcmp(argv[1], "open") == 0) ||
> > +                     (strcmp(argv[1], "close") == 0)) {
> > +             int dev;
> > +             struct mmc *mmc;
> > +             u32 ack, part_num, access = 0;
> > +
> > +             if (argc == 4) {
> > +                     dev = simple_strtoul(argv[2], NULL, 10);
> > +                     part_num = simple_strtoul(argv[3], 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 ((part_num <= 0) || (part_num >
> MMC_NUM_BOOT_PARTITION)) {
> > +                     printf("Invalid boot partition number:\n");
> > +                     printf("Boot partition number cannot be <= 0\n");
> > +                     printf("EMMC44 supports only 2 boot partitions\n");
> What means? EMMC44 supports only 2 boot partitions?
>

As per the eMMC 4.4 specification, eMMC 4.4 has 1 RPMB partition, 2 Boot
partitions and 4 General Purpose partitions.
As there are only 2 Boot partitions, we restrict the user not to use an
invalid partition number.

> +                     return 1;
> > +             }
> > +
> > +             if (strcmp(argv[1], "open") == 0)
> > +                     access = part_num; /* enable R/W access to boot
> part*/
> > +             if (strcmp(argv[1], "close") == 0)
> else if?
>

Even 'else if' is fine, but I wanted to explicitly check the string in both
cases.


> > +                     access = 0; /* No access to boot partition */
> > +
> > +             /* acknowledge to be sent during boot operation */
> > +             ack = 1;
> ack is always set to 1? then Can't initialize ack value as 1.
> Otherwise, boot_part_access(mmc, 1, part_num, access)?
>
> As per JEDEC Standard 'ack' can be 0 / 1.
Right now we are always using ack=1.
Is it OK to use "boot_part_access(mmc, 1, part_num, access)" with a
hardcoded value '1' in the function call ?

Best Regards,
> Jaehoon Chung
> > +             return boot_part_access(mmc, ack, part_num, access);
> > +
> > +     } 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;
> > +             }
> > +#endif /* CONFIG_SUPPORT_EMMC_BOOT */
> > +     }
> >       state = MMC_INVALID;
> >       if (argc == 5 && strcmp(argv[1], "read") == 0)
> >               state = MMC_READ;
> > @@ -324,5 +421,14 @@ 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"
> > +#ifdef CONFIG_SUPPORT_EMMC_BOOT
> > +     "mmc open <dev> <boot_partition>\n"
> > +     " - Enable boot_part for booting and enable R/W access of
> boot_part\n"
> > +     "mmc close <dev> <boot_partition>\n"
> > +     " - Enable boot_part for booting and disable access to boot_part\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
> > +     );
> > +#endif /* !CONFIG_GENERIC_MMC */
> >
>
> _______________________________________________
> 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