[PATCH] cmd: mmc: add mmc partboot

[gr embeter]

On Tue, 16 Feb 2021 at 10:00 Jaehoon Chung <jh80.chung at samsung.com> wrote:

> On 2/16/21 5:18 PM, gr embeter wrote:
> > Hi Jaehoon,
> > thanks for your comments.
> >
> > On Tue, 16 Feb 2021 at 00:30 Jaehoon Chung <jh80.chung at samsung.com>
> wrote:
> >
> >> Hi Grygorii,
> >>
> >> On 2/15/21 10:04 PM, gr embeter wrote:
> >>> Hello Jaehoon,
> >>>
> >>> On Sun, 14 Feb 2021 at 23:08 Jaehoon Chung <jh80.chung at samsung.com>
> >> wrote:
> >>>
> >>>> Dear Grygorii,
> >>>>
> >>>> On 2/12/21 7:32 PM, grygorii tertychnyi wrote:
> >>>>> This patch allows to determine active boot partition in boot script:
> >>>>>
> >>>>> if mmc partboot ${mmcdev} 2; then
> >>>>>     echo "booted from eMMC boot1 partition"
> >>>>> fi
> >>>>
> >>>> I don't know what purpose this patch has.
> >>>
> >>>
> >>> With an eMMC as a boot source I have two boot partitions, i.e. “boot1”
> >> and
> >>> “boot2”, with two different versions of U-Boot on each partition.
> >>>
> >>> I also have two different kernels located on eMMC “user” partition,
> let’s
> >>> say “image1” and “image2”.
> >>>
> >>> So, I want to boot “image1” kernel if it is U-boot from “boot1”
> partition
> >>> is booted now,
> >>> and to boot “image2” kernel if it is U-boot from “boot2” partition is
> >>> booted.
> >>>
> >>> It is easy to do it manually. I just added possibility to do it with
> >> U-boot
> >>> script now,
> >>> because “mmc partconf ...” does not let you check the current boot
> >>> partition with hush.
> >>>
> >>> Hope, I explained the purpose.
> >>
> >> I remembered. Because i feel to see similar patch to use bootpart.
> >> At that time, i also asked same question. Sorry for not remembered
> yours.
> >>
> >>
> >>
> https://protect2.fireeye.com/v1/url?k=dac52f49-855e1643-dac4a406-0cc47a31384a-4f6e07c1e040623a&q=1&e=3829977a-6bc3-49e3-868a-0dcba5fab9e4&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20201212074633.891704-1-grembeter%40outlook.com%2F
> >>
> >
> > Indeed. I should’ve mentioned this link. Sorry. I’ll extend commit
> message.
> >
> >
> >>>
> >>>
> >>>
> >>>>
> >>>> Best Regards,
> >>>> Jaehoon Chung
> >>>>
> >>>>>
> >>>>> Signed-off-by: Grygorii Tertychnyi <
> >>>> grygorii.tertychnyi at leica-geosystems.com>
> >>>>> ---
> >>>>>  cmd/mmc.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>>>>  1 file changed, 39 insertions(+)
> >>>>>
> >>>>> diff --git a/cmd/mmc.c b/cmd/mmc.c
> >>>>> index 1529a3e05ddd..010d6ab9aa19 100644
> >>>>> --- a/cmd/mmc.c
> >>>>> +++ b/cmd/mmc.c
> >>>>> @@ -771,6 +771,41 @@ static int do_mmc_boot_resize(struct cmd_tbl
> >>>> *cmdtp, int flag,
> >>>>>       return CMD_RET_SUCCESS;
> >>>>>  }
> >>>>>
> >>>>> +static int do_mmc_partboot(struct cmd_tbl *cmdtp, int flag,
> >>>>> +                        int argc, char *const argv[])
> >>>>> +{
> >>>>> +     int dev;
> >>>>> +     struct mmc *mmc;
> >>>>> +     u8 part_args, part_emmc;
> >>>>> +
> >>>>> +     if (argc != 3)
> >>>>> +             return CMD_RET_USAGE;
> >>>>> +
> >>>>> +     dev = simple_strtoul(argv[1], NULL, 10);
> >>>>> +
> >>>>> +     mmc = init_mmc_device(dev, false);
> >>>>> +     if (!mmc)
> >>>>> +             return CMD_RET_FAILURE;
> >>>>> +
> >>>>> +     if (IS_SD(mmc)) {
> >>>>> +             puts("PARTITION_CONFIG only exists on eMMC\n");
> >>>>> +             return CMD_RET_FAILURE;
> >>>>> +     }
> >>>>> +
> >>>>> +     if (mmc->part_config == MMCPART_NOAVAILABLE) {
> >>>>> +             printf("No part_config info for ver. 0x%x\n",
> >>>> mmc->version);
> >>>>> +             return CMD_RET_FAILURE;
> >>>>> +     }
> >>>>> +
> >>>>> +     part_emmc = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> >>>>> +     part_args = simple_strtoul(argv[2], NULL, 10);
> >>>>> +
> >>>>> +     if (part_emmc == part_args)
> >>>>> +             return CMD_RET_SUCCESS;
> >>>>> +     else
> >>>>> +             return CMD_RET_FAILURE;
> >>>>> +}
> >>>>> +
> >>>>>  static int mmc_partconf_print(struct mmc *mmc)
> >>>>>  {
> >>>>>       u8 ack, access, part;
> >>>>> @@ -953,6 +988,7 @@ static struct cmd_tbl cmd_mmc[] = {
> >>>>>  #ifdef CONFIG_SUPPORT_EMMC_BOOT
> >>>>>       U_BOOT_CMD_MKENT(bootbus, 5, 0, do_mmc_bootbus, "", ""),
> >>>>>       U_BOOT_CMD_MKENT(bootpart-resize, 4, 0, do_mmc_boot_resize, "",
> >>>> ""),
> >>>>> +     U_BOOT_CMD_MKENT(partboot, 3, 0, do_mmc_partboot, "", ""),
> >>
> >> partboot can be confused. how about changing more clear name, like
> >> mmc_bootpart_check?
> >
> >
> > Agree. We have “bootpart-resize”, and “bootpart-check” looks very
> natural.
> >
> > I’ll prepare V2 for
> > mmc bootpart-check <dev> <boot_partition>
> >
> >
> >>
> >>>>>       U_BOOT_CMD_MKENT(partconf, 5, 0, do_mmc_partconf, "", ""),
> >>>>>       U_BOOT_CMD_MKENT(rst-function, 3, 0, do_mmc_rst_func, "", ""),
> >>>>>  #endif
> >>>>> @@ -1021,6 +1057,9 @@ U_BOOT_CMD(
> >>>>>       " - Set the BOOT_BUS_WIDTH field of the specified device\n"
> >>>>>       "mmc bootpart-resize <dev> <boot part size MB> <RPMB part size
> >>>> MB>\n"
> >>>>>       " - Change sizes of boot and RPMB partitions of specified
> >> device\n"
> >>>>> +     "mmc partboot dev boot_partition\n"
> >>
> >> why it needs to pass "boot_partition"?
> >
> >
> > We need to return true or false to be able to use “if” in boot script
> > (hush).
> > Comparing the real value with the given one is the only way to return
> true
> > or false. So, this argument is mandatory.
>
> How about setting environment variable in checking command? With optional
> arguments.
> e,g) mmc partconf-check <dev> [varname] ?
>
> - mmc partconf-check 0
> Then just returned BOOT_PARTITON_ENABLE[5:3] value.
> - mmc partconf-check 0 boot_partition
> Then it will be set to boot_partition as PARTITION_ENABLE[5:3] value.
>
> boot_partition=0x2
>
> I think that you can use %boot_partition variable in your boot script.
>
> Refer to cmd/part.c.
>
> How about this? It's just my opinion.


it looks like an overkill for this purpose.
it is additional “if”: 1st to check the return value of bootpart-check (to
verify the value is set), 2nd to check the value of variable.
And if there is “save env” call in the same boot script, we would need to
unset this new variable.


>
> Best Regards,
> Jaehoon Chung
>
> >
> >
> >> And use more clear usage..with optional or mandatory.
> >> <> - mandatory
> >> [] - optional
> >
> >
> > OK.
> >
> >
> >>
> >> mmc partboot <dev>
> >
> >
> >>
> >>>>> +     " - Return success if the given boot_partition value matches
> >>>> BOOT_PARTITION_ENABLE\n"
> >>>>> +     "   bit field of the specified device\n"
> >>>>>       "mmc partconf dev [boot_ack boot_partition partition_access]\n"
> >>>>>       " - Show or change the bits of the PARTITION_CONFIG field of
> the
> >>>> specified device\n"
> >>>>>       "mmc rst-function dev value\n"
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >
>
>