RE: [PATCH 07/12] rockchip: puma-rk3399: load environment from same medium as one used to load U-Boot proper

[<patrick.delaunay@foss.st.com>]

Hi,


ST Restricted

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Quentin Schulz
> Sent: mardi 6 septembre 2022 11:23
> To: Kever Yang <kever.yang@rock-chips.com>; Quentin Schulz
> <foss+uboot@0leil.net>
> Cc: sjg@chromium.org; philipp.tomsich@vrull.eu; klaus.goger@theobroma-
> systems.com; knaerzche@gmail.com; u-boot@lists.denx.de
> Subject: Re: [PATCH 07/12] rockchip: puma-rk3399: load environment from same
> medium as one used to load U-Boot proper
> 
> Hi Kever,
> 
> On 9/4/22 13:49, Kever Yang wrote:
> > Hi Quentin,
> >
> > The Kconfig from env/Kconfig
> >
> > config ENV_IS_NOWHERE
> >          bool "Environment is not stored"
> >          default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \
> >                       !ENV_IS_IN_FAT && !ENV_IS_IN_FLASH && \
> >                       !ENV_IS_IN_MMC && !ENV_IS_IN_NAND && \
> >                       !ENV_IS_IN_NVRAM && !ENV_IS_IN_ONENAND && \
> >                       !ENV_IS_IN_REMOTE && !ENV_IS_IN_SPI_FLASH && \
> >                       !ENV_IS_IN_UBI
> >
> > I think the logic is the env parameter is stored on some kind of
> > storage, or NOWHERE.
> >
> 
> It's *default* to yes if none is defined. It's not a requirement.
> 
> > And what you want to do is to load from the same medium as SPL boot
> > device(location of U-Boot proper),
> >
> > this could not be NOWHERE.
> >
> 
> This can be nowhere in case the proper config option is not selected.
> E.g. we're booting from SPI-NOR but the ENV_IS_IN_SPI_FLASH is not set.
> How does one handle this scenario? Since we don't want fallbacks, it needs to be
> "nowhere". What you're suggesting is to let the user run a broken build and have it
> cryptically crash with the following error:
> 
> initcall sequence 00000000002866c0 failed at call 0000000000256b34 (err=-19)
> 
> Instead, I suggest to more gracefully error out at runtime by letting the user know
> that the option is not compiled in but U-Boot is still usable.
> 
> If you have another way to forbid fallbacks but not crash U-Boot in case the option
> is not selected, let me know.
>

For information : It is exactly that I do to stm32mp15 platform

See board/st/stm32mp1/stm32mp1.c::env_get_location()

ENV_IS_NOWHERE = use the default environment, embedded in U-Boot and env

It is the fallback if environment not activated ENV_IS_IN_*  for the boot device, even it is not the case by default in stm32mp15_defconfig.

And it is why , I change Kconfig to allow activation of any ENV_IS_IN_* and ENV_IS_NOWHERE

Patrick
 
> Cheers,
> Quentin
> >
> > Thanks,
> >
> > - Kever
> >
> > On 2022/9/1 21:13, Quentin Schulz wrote:
> >> Hi Kever
> >>
> >> On 9/1/22 15:03, Kever Yang wrote:
> >>> Hi Quentin,
> >>>
> >>> On 2022/7/23 00:06, Quentin Schulz wrote:
> >>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>>>
> >>>> Chances are when one boots U-Boot proper from a given storage
> >>>> medium, they want the same medium to be used to load and store the
> environment.
> >>>>
> >>>> This basically allows to have completely separate U-Boot
> >>>> (TPL/SPL/U-Boot
> >>>> proper/environment) per storage medium which is convenient when
> >>>> working with recovery from SD-Card as one would just need to insert
> >>>> a properly configured SD-Card into the device to have access to
> >>>> their whole debug setup.
> >>>>
> >>>> No fallback mechanism is provided as to not dirty other storage
> >>>> medium environment by mistake. However, since
> >>>> arch_env_get_location() is called by env_init() which is part of
> >>>> the pre-relocation process, a valid, non-ENVL_UNKNOWN, value shall
> >>>> be returned otherwise the relocation fails with the following
> >>>> message:
> >>>> initcall sequence 00000000002866c0 failed at call 0000000000256b34
> >>>> (err=-19)
> >>>>
> >>>> This valid, non-ENVL_UNKNOWN, value is ENVL_NOWHERE which
> requires
> >>>> to always select CONFIG_ENV_IS_NOWHERE otherwise this work-around
> >>>> does not work.
> >>>>
> >>>> Cc: Quentin Schulz <foss+uboot@0leil.net>
> >>>> Signed-off-by: Quentin Schulz
> >>>> <quentin.schulz@theobroma-systems.com>
> >>>> ---
> >>>>
> >>>> Depends on
> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.or
> >>>> g_u-2Dboot_20220715151552.953654-2D1-2Dfoss-2Buboot-400leil.net_&d=
> >>>>
> DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSq
> HGq8
> >>>> yBP6m6qZZ4njZguQhZhkI_-
> 172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWH
> >>>>
> IRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=wgEMbr3EjeCtvcWU_UoXqNOwQula
> VN-0Q
> >>>> b2yL2ysaOs&e=
> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.or
> >>>> g_u-2Dboot_20220715151552.953654-2D2-2Dfoss-2Buboot-400leil.net_&d=
> >>>>
> DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSq
> HGq8
> >>>> yBP6m6qZZ4njZguQhZhkI_-
> 172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWH
> >>>>
> IRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=PKwYBMB7r8ekIPV1ZG7xkj7vF60YN
> FlYX
> >>>> QRrvaVgJR8&e=
> >>>>   .../puma_rk3399/puma-rk3399.c                 | 37
> >>>> +++++++++++++++++++
> >>>>   configs/puma-rk3399_defconfig                 |  1 +
> >>>>   2 files changed, 38 insertions(+)
> >>>>
> >>>> diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> >>>> b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> >>>> index 5e5e58c88e..7ef4bac24b 100644
> >>>> --- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> >>>> +++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> >>>> @@ -6,6 +6,7 @@
> >>>>   #include <common.h>
> >>>>   #include <dm.h>
> >>>>   #include <env.h>
> >>>> +#include <env_internal.h>
> >>>>   #include <init.h>
> >>>>   #include <log.h>
> >>>>   #include <misc.h>
> >>>> @@ -135,6 +136,42 @@ int mmc_get_env_dev(void)
> >>>>       return CONFIG_SYS_MMC_ENV_DEV;
> >>>>   }
> >>>> +#if !IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> >>>> +#error Please enable CONFIG_ENV_IS_NOWHERE #endif
> >>>> +
> >>>> +enum env_location arch_env_get_location(enum env_operation op, int
> >>>> prio)
> >>>> +{
> >>>> +    const char *boot_device =
> >>>> +        ofnode_read_chosen_string("u-boot,spl-boot-device");
> >>>> +
> >>>> +    if (prio > 0)
> >>>> +        return ENVL_UNKNOWN;
> >>>> +
> >>>> +    if (!boot_device) {
> >>>> +        debug("%s: /chosen/u-boot,spl-boot-device not set\n",
> >>>> +              __func__);
> >>>> +        return ENVL_NOWHERE;
> >>>> +    }
> >>>> +
> >>>> +    debug("%s: booted from %s\n", __func__, boot_device);
> >>>> +
> >>>> +    if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) &&
> >>>> +        !strcmp(boot_device, "/spi@ff1d0000/flash@0"))
> >>>> +        return ENVL_SPI_FLASH;
> >>>> +
> >>>> +    if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC) &&
> >>>> +        (!strcmp(boot_device, "/mmc@fe320000") ||
> >>>> +         !strcmp(boot_device, "/mmc@fe330000")))
> >>>> +        return ENVL_MMC;
> >>>> +
> >>>> +    printf("%s: No environment available: booted from %s but U-Boot "
> >>>> +           "config does not allow loading environment from it.",
> >>>> +           __func__, boot_device);
> >>>> +
> >>>> +    return ENVL_NOWHERE;
> >>>> +}
> >>>> +
> >>>>   int misc_init_r(void)
> >>>>   {
> >>>>       const u32 cpuid_offset = 0x7; diff --git
> >>>> a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
> >>>> index 87d7e4f57c..e218532d70 100644
> >>>> --- a/configs/puma-rk3399_defconfig
> >>>> +++ b/configs/puma-rk3399_defconfig
> >>>> @@ -44,6 +44,7 @@ CONFIG_SPL_OF_CONTROL=y
> >>>>   CONFIG_OF_LIVE=y
> >>>>   CONFIG_OF_SPL_REMOVE_PROPS="interrupt-parent assigned-clocks
> >>>> assigned-clock-rates assigned-clock-parents"
> >>>>   CONFIG_ENV_OVERWRITE=y
> >>>> +CONFIG_ENV_IS_NOWHERE=y
> >>>
> >>> This option is conflict with CONFIG_ENV_IS_IN_MMC,  please check
> >>> again where should be this board get the env.
> >>>
> >>
> >> I created the defconfig with make savedefconfig, so if you're talking
> >> about KConfig conflict, that is incorrect, there is no conflict.
> >>
> >> If you're talking about something else, please clarify because I
> >> don't see the issue right now.
> >>
> >> Cheers,
> >> Quentin