[PATCH] env: mmc: statically set the environment partition name

[Patrick DELAUNAY]

Hi,


On 6/13/23 13:05, Emmanuel Di Fede wrote:
> The new opt-out setting, CONFIG_ENV_MMC_PARTITION, statically sets
> the MMC environment partition name. Prior to this patch, the only way
> to declare this partition name was by creating a
> 'u-boot,mmc-env-partition' parameter in the device-tree's /config node.
>
> This setting provides additional flexibility, particularly in cases
> where accessing the device-tree is not straightforward (e.g. QEMU).
>
> If undeclared, the device-tree's setting will be used.
>
> Signed-off-by: Emmanuel Di Fede <emmanuel.difede at cysec.com>
> Cc: Joe Hershberger <joe.hershberger at ni.com>
> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> ---
>   doc/device-tree-bindings/config.txt | 2 ++
>   env/Kconfig                         | 8 ++++++++
>   env/mmc.c                           | 9 +++++++--
>   3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/doc/device-tree-bindings/config.txt b/doc/device-tree-bindings/config.txt
> index 3151778b2c..f50c68bbdc 100644
> --- a/doc/device-tree-bindings/config.txt
> +++ b/doc/device-tree-bindings/config.txt
> @@ -76,6 +76,8 @@ u-boot,mmc-env-partition (int)
>   	precedence. In that case, only if the partition is not found,
>   	mmc-env-offset* will be tried.
>   
> +	Note that CONFIG_ENV_MMC_PARTITION overrides this device-tree setting.
> +
>   u-boot,no-apm-finalize (bool)
>   	For x86 devices running on coreboot, this tells U-Boot not to lock
>   	down the Intel Management Engine (ME) registers. This allows U-Boot to
> diff --git a/env/Kconfig b/env/Kconfig
> index 2bbe4c466a..de38b00bd1 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -659,6 +659,14 @@ config SYS_MMC_ENV_PART
>   	  partition 0 or the first boot partition, which is 1 or some other defined
>   	  partition.
>   
> +config ENV_MMC_PARTITION
> +	string "mmc environment partition name"
> +	depends on ENV_IS_IN_MMC
> +	help
> +	  MMC partition name used to save environment variables.
> +	  If this variable is unset, u-boot will try to get the env partition name
> +	  from the DTB's /config node.
> +


as it is sting this define is always activated => with default ""

if you want to test it with IS_ENABLED you need to add a other bolean config

I encounter the same issue several time

example: CONFIG_USE_DEFAULT_ENV_FILE/CONFIG_DEFAULT_ENV_FILE
                CONFIG_USE_BOOTCOMMAND / CONFIG_BOOTCOMMAND
                CONFIG_USE_BOOTARGS / CONFIG_BOOTARGS

sometthing like:

+config USE_ENV_MMC_PARTITION
+	boolean "use the mmc environment partition name"
+	depends on ENV_IS_IN_MMC

+config ENV_MMC_PARTITION
+	string "mmc environment partition name"
+	depends on USE_ENV_MMC_PARTITION
+	help
+	  MMC partition name used to save environment variables.
+	  If this variable is unset, u-boot will try to get the env partition name
+	  from the DTB's /config node.



>   config ENV_MMC_USE_DT
>   	bool "Read partition name and offset in DT"
>   	depends on ENV_IS_IN_MMC && OF_CONTROL
> diff --git a/env/mmc.c b/env/mmc.c
> index 7a5836ad66..887df61fbe 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -114,8 +114,13 @@ static inline s64 mmc_offset(struct mmc *mmc, int copy)
>   	if (IS_ENABLED(CONFIG_SYS_MMC_ENV_PART))
>   		hwpart = mmc_get_env_part(mmc);
>   
> -	/* look for the partition in mmc CONFIG_SYS_MMC_ENV_DEV */
> -	str = ofnode_conf_read_str(dt_prop.partition);
> +	if (IS_ENABLED(CONFIG_ENV_MMC_PARTITION)) {

see previous comment =>

if (IS_ENABLED(CONFIG_USE_ENV_MMC_PARTITION)) {


> +		str = CONFIG_ENV_MMC_PARTITION;
> +	} else {
> +		/* look for the partition in mmc CONFIG_SYS_MMC_ENV_DEV */
> +		str = ofnode_conf_read_str(dt_prop.partition);
> +	}


I think it is can be simplified by using the macro with parameters


str = IS_ENABLED(CONFIG_USE_ENV_MMC_PARTITION,

                              CONFIG_ENV_MMC_PARTITION,

ofnode_conf_read_str(dt_prop.partition));

and that corrects a compilation issue when
CONFIG_USE_ENV_MMC_PARTITION /  CONFIG_ENV_MMC_PARTITION
are not defined



> +
>   	if (str) {
>   		/* try to place the environment at end of the partition */
>   		err = mmc_offset_try_partition(str, copy, &val);


Patrick