diff mbox series

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

Message ID 20220722160655.3904213-8-foss+uboot@0leil.net
State Superseded
Delegated to: Kever Yang
Headers show
Series Puma RK3399 migration to TPL and numerous fixes | expand

Commit Message

Quentin Schulz July 22, 2022, 4:06 p.m. UTC 
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://lore.kernel.org/u-boot/20220715151552.953654-1-foss+uboot@0leil.net/
https://lore.kernel.org/u-boot/20220715151552.953654-2-foss+uboot@0leil.net/

 .../puma_rk3399/puma-rk3399.c                 | 37 +++++++++++++++++++
 configs/puma-rk3399_defconfig                 |  1 +
 2 files changed, 38 insertions(+)

Comments

Kever Yang Sept. 1, 2022, 1:03 p.m. UTC | #1 
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://lore.kernel.org/u-boot/20220715151552.953654-1-foss+uboot@0leil.net/
> https://lore.kernel.org/u-boot/20220715151552.953654-2-foss+uboot@0leil.net/
>
>   .../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.


Thanks,

- Kever

>   CONFIG_ENV_IS_IN_MMC=y
>   CONFIG_ENV_IS_IN_SPI_FLASH=y
>   CONFIG_ENV_SPI_MAX_HZ=50000000
Quentin Schulz Sept. 1, 2022, 1:13 p.m. UTC | #2 
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.org_u-2Dboot_20220715151552.953654-2D1-2Dfoss-2Buboot-400leil.net_&d=DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWHIRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=wgEMbr3EjeCtvcWU_UoXqNOwQulaVN-0Qb2yL2ysaOs&e= https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220715151552.953654-2D2-2Dfoss-2Buboot-400leil.net_&d=DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWHIRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=PKwYBMB7r8ekIPV1ZG7xkj7vF60YNFlYXQRrvaVgJR8&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
Kever Yang Sept. 4, 2022, 11:49 a.m. UTC | #3 
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.

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.


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.org_u-2Dboot_20220715151552.953654-2D1-2Dfoss-2Buboot-400leil.net_&d=DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWHIRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=wgEMbr3EjeCtvcWU_UoXqNOwQulaVN-0Qb2yL2ysaOs&e= 
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220715151552.953654-2D2-2Dfoss-2Buboot-400leil.net_&d=DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWHIRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=PKwYBMB7r8ekIPV1ZG7xkj7vF60YNFlYXQRrvaVgJR8&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
Quentin Schulz Sept. 6, 2022, 9:22 a.m. UTC | #4 
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.

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.org_u-2Dboot_20220715151552.953654-2D1-2Dfoss-2Buboot-400leil.net_&d=DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWHIRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=wgEMbr3EjeCtvcWU_UoXqNOwQulaVN-0Qb2yL2ysaOs&e= https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220715151552.953654-2D2-2Dfoss-2Buboot-400leil.net_&d=DwIDaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=TZndtGz1ePTd2Il6YcEjqzo9oXv73RCWHIRVSiFVsnp2OzyCJEDzZ2KPz56AcWdn&s=PKwYBMB7r8ekIPV1ZG7xkj7vF60YNFlYXQRrvaVgJR8&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
Patrick DELAUNAY Sept. 14, 2022, 9:17 a.m. UTC | #5 
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
Kever Yang Sept. 20, 2022, 12:28 p.m. UTC | #6 
Hi Patrick, Quentin,


Here is the definition about the ENV_IS_NOWHERE:

config ENV_IS_NOWHERE
         bool "Environment is not stored"
help
           Define this if you don't want to or can't have an environment 
stored
           on a storage medium. In this case the environemnt will still 
exist
           while U-Boot is running, but once U-Boot exits it will not be
           stored. U-Boot will therefore always start up with a default
           environment.


Which means ENV_IS_NOWHERE is ALWAYS use default environment,

but not stored on a storage medium.

I think what you want is a new ENV_IS_ANYWHERE which not able to

decide when the firmware is build but must be some where when the boot

device is decided.


Thanks,
- Kever
On 2022/9/14 17:17, patrick.delaunay@foss.st.com wrote:
> 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
Quentin Schulz Sept. 20, 2022, 2:02 p.m. UTC | #7 
Hi Kever,

On 9/20/22 14:28, Kever Yang wrote:
> Hi Patrick, Quentin,
> 
> 
> Here is the definition about the ENV_IS_NOWHERE:
> 
> config ENV_IS_NOWHERE
>          bool "Environment is not stored"
> help
>            Define this if you don't want to or can't have an environment 
> stored
>            on a storage medium. In this case the environemnt will still 
> exist
>            while U-Boot is running, but once U-Boot exits it will not be
>            stored. U-Boot will therefore always start up with a default
>            environment.
> 
> 
> Which means ENV_IS_NOWHERE is ALWAYS use default environment,
> 
> but not stored on a storage medium.
> 
> I think what you want is a new ENV_IS_ANYWHERE which not able to
> 
> decide when the firmware is build but must be some where when the boot
> 
> device is decided.
> 

I do not share the same understanding. For me, ENV_IS_NOWHERE means the 
environment is stored in RAM, once you exit U-Boot or reset the board, 
it's gone. That's my understanding of the code, I can concede that the 
help message of the Kconfig option is confusing. It's just another 
"kind" of environment to me.

If the point was to ALWAYS use the default environment, one wouldn't be 
able to enable the option while other ENV_IS_IN_* are enabled. It is 
however possible.

ENV_IS_ANYWHERE is not a correct name for what I want, because I 
specifically do NOT want to load from anywhere. I want to load from a 
specific medium, and if not possible have a fallback to avoid U-Boot 
cryptically crashing.

Maybe we should rename ENV_IS_NOWHERE to ENV_IS_IN_RAM, maybe we could 
also stop crashing if there's no medium to load the environment from 
that is available, maybe we could rephrase the help text of the Kconfig 
option, but this is unrelated to this patch series.

Finally, STM32, some i.MX and a couple of Xilinx based boards actually 
have ENV_IS_NOWHERE enabled at the same time as other ENV_IS_IN_* 
options, so I'm clearly not the first one to use it this way. Also, see 
arch/arm/mach-imx/imx8m/soc.c for an implementation of 
arch_env_get_location that requires ENV_IS_NOWHERE to work and has 
almost the same logic as I'm trying to implement.

I'm trying to fix a non-booting board. This patch series is also only 
impacting the board I'm maintaining and nothing else. If merging the v2 
of this patch series is really asking you something unimaginable, just 
drop this patch from the series, merge the rest and we'll continue 
arguing on a resend.

Quentin
diff mbox series

Patch

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
 CONFIG_ENV_IS_IN_MMC=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_ENV_SPI_MAX_HZ=50000000