Dear Minkyu,
Thankyou for the review comments.
Please find my responses below.
Thanks & Regards
Amarendra Reddy
On 9 April 2013 16:25, Minkyu Kang <mk7.k...@samsung.com> wrote:
> Dear Amarendra,
>
> On 05/04/13 16:39, Amarendra Reddy wrote:
> > Hi Jaehoon,
> >
> > Please find the responses below
> >
> > Thanks & Regards
> > Amarendra Reddy
> >
> > On 5 April 2013 11:51, Jaehoon Chung <jh80.ch...@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...@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.
>
> Why? I think, It doesn't make sense.
> OK. I will replace the above with 'else if'.
> >
> >
> >>> + 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 ?
>
> Yes, It's a constant.
>
I meant from uboot coding standard point of view.
Ok. I shall use 'boot_part_access(mmc, 1, part_num, access)' in next
patchset.
> >
> > 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@lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> >>
> >
> >
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
> Thanks,
> Minkyu Kang.
>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
