[PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[Marek Vasut]

Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).

The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
in the last conditional, which does not do plain macro compare
(CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
in use, this gets optimized back to original macro compare, but
if OF_CONTROL is in use, this also takes into account the DT
properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Dragan Simic <dsimic@manjaro.org>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Quentin Schulz <quentin.schulz@cherry.de>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
V2: - Rename mmc_env_hwpart_redund() to mmc_env_is_redundant_in_both_boot_hwparts()
    - Return bool
V3: - Hide the mmc_env_is_redundant_in_both_boot_hwparts symbol behind __maybe_unused
      as this symbol is called from code gated behind ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
      in env_mmc_load()
---
 env/mmc.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/env/mmc.c b/env/mmc.c
index 379f5ec9be7..353a7ce72fb 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -40,18 +40,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/*
- * In case the environment is redundant, stored in eMMC hardware boot
- * partition and the environment and redundant environment offsets are
- * identical, store the environment and redundant environment in both
- * eMMC boot partitions, one copy in each.
- * */
-#if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
-     (CONFIG_SYS_MMC_ENV_PART == 1) && \
-     (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
-#define ENV_MMC_HWPART_REDUND	1
-#endif
-
 #if CONFIG_IS_ENABLED(OF_CONTROL)
 
 static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
@@ -217,6 +205,23 @@ static inline s64 mmc_offset(struct mmc *mmc, int copy)
 }
 #endif
 
+static bool __maybe_unused mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
+{
+	/*
+	 * In case the environment is redundant, stored in eMMC hardware boot
+	 * partition and the environment and redundant environment offsets are
+	 * identical, store the environment and redundant environment in both
+	 * eMMC boot partitions, one copy in each.
+	 */
+	if (!IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+		return false;
+
+	if (CONFIG_SYS_MMC_ENV_PART != 1)
+		return false;
+
+	return mmc_offset(mmc, 0) == mmc_offset(mmc, 1);
+}
+
 __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
 {
 	s64 offset = mmc_offset(mmc, copy);
@@ -336,7 +341,7 @@ static int env_mmc_save(void)
 		if (gd->env_valid == ENV_VALID)
 			copy = 1;
 
-		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+		if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
 			ret = mmc_set_env_part(mmc, copy + 1);
 			if (ret)
 				goto fini;
@@ -409,7 +414,7 @@ static int env_mmc_erase(void)
 	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
 		copy = 1;
 
-		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+		if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
 			ret = mmc_set_env_part(mmc, copy + 1);
 			if (ret)
 				goto fini;
@@ -477,7 +482,7 @@ static int env_mmc_load(void)
 		goto fini;
 	}
 
-	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+	if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
 		ret = mmc_set_env_part(mmc, 1);
 		if (ret)
 			goto fini;
@@ -485,7 +490,7 @@ static int env_mmc_load(void)
 
 	read1_fail = read_env(mmc, CONFIG_ENV_SIZE, offset1, tmp_env1);
 
-	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+	if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
 		ret = mmc_set_env_part(mmc, 2);
 		if (ret)
 			goto fini;
-- 
2.47.2

[PATCH v3 2/2] env: mmc: Clean up env_mmc_load() ifdeffery

[Marek Vasut]

Rename the variants of env_mmc_load() for redundant and non-redundant
environment to env_mmc_load_redundant() and env_mmc_load_singular()
respectively and convert the env_mmc_load() implementation to use of
if (IS_ENABLED(...)). As a result, drop __maybe_unused from
mmc_env_is_redundant_in_both_boot_hwparts().

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Dragan Simic <dsimic@manjaro.org>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Quentin Schulz <quentin.schulz@cherry.de>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
V3: New patch
---
 env/mmc.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/env/mmc.c b/env/mmc.c
index 353a7ce72fb..2ef15fb72e7 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -205,7 +205,7 @@ static inline s64 mmc_offset(struct mmc *mmc, int copy)
 }
 #endif
 
-static bool __maybe_unused mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
+static bool mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
 {
 	/*
 	 * In case the environment is redundant, stored in eMMC hardware boot
@@ -448,13 +448,7 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-#if defined(ENV_IS_EMBEDDED)
-static int env_mmc_load(void)
-{
-	return 0;
-}
-#elif defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
-static int env_mmc_load(void)
+static int env_mmc_load_redundant(void)
 {
 	struct mmc *mmc;
 	u32 offset1, offset2;
@@ -510,8 +504,8 @@ err:
 
 	return ret;
 }
-#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
-static int env_mmc_load(void)
+
+static int env_mmc_load_singular(void)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 	struct mmc *mmc;
@@ -556,7 +550,16 @@ err:
 
 	return ret;
 }
-#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
+
+static int env_mmc_load(void)
+{
+	if (IS_ENABLED(CONFIG_ENV_IS_EMBEDDED))
+		return 0;
+	else if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+		return env_mmc_load_redundant();
+	else
+		return env_mmc_load_singular();
+}
 
 U_BOOT_ENV_LOCATION(mmc) = {
 	.location	= ENVL_MMC,
-- 
2.47.2

Re: [PATCH v3 2/2] env: mmc: Clean up env_mmc_load() ifdeffery

[Quentin Schulz]

Hi Marek,

On 2/21/25 7:47 PM, Marek Vasut wrote:
> Rename the variants of env_mmc_load() for redundant and non-redundant
> environment to env_mmc_load_redundant() and env_mmc_load_singular()
> respectively and convert the env_mmc_load() implementation to use of
> if (IS_ENABLED(...)). As a result, drop __maybe_unused from
> mmc_env_is_redundant_in_both_boot_hwparts().
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Dragan Simic <dsimic@manjaro.org>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
> V3: New patch
> ---
>   env/mmc.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 353a7ce72fb..2ef15fb72e7 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -205,7 +205,7 @@ static inline s64 mmc_offset(struct mmc *mmc, int copy)
>   }
>   #endif
>   
> -static bool __maybe_unused mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
> +static bool mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
>   {
>   	/*
>   	 * In case the environment is redundant, stored in eMMC hardware boot
> @@ -448,13 +448,7 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
>   	return (n == blk_cnt) ? 0 : -1;
>   }
>   
> -#if defined(ENV_IS_EMBEDDED)
> -static int env_mmc_load(void)
> -{
> -	return 0;
> -}
> -#elif defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
> -static int env_mmc_load(void)
> +static int env_mmc_load_redundant(void)
>   {
>   	struct mmc *mmc;
>   	u32 offset1, offset2;
> @@ -510,8 +504,8 @@ err:
>   
>   	return ret;
>   }
> -#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
> -static int env_mmc_load(void)
> +
> +static int env_mmc_load_singular(void)
>   {
>   	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
>   	struct mmc *mmc;
> @@ -556,7 +550,16 @@ err:
>   
>   	return ret;
>   }
> -#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
> +
> +static int env_mmc_load(void)
> +{
> +	if (IS_ENABLED(CONFIG_ENV_IS_EMBEDDED))
> +		return 0;
> +	else if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> +		return env_mmc_load_redundant();
> +	else
> +		return env_mmc_load_singular();

The else (not its content!) could be removed since all conditions above 
return.

Isn't this going to be increasing the size of the binary with code that 
won't be run or is the compiler smart enough to remove that based on the 
IS_ENABLED() that are constant false at build time?

The change itself looks fine, therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[Quentin Schulz]

Hi Marek,

On 2/21/25 7:47 PM, Marek Vasut wrote:
> Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
> which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
> almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
> did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).
> 
> The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
> in the last conditional, which does not do plain macro compare
> (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
> mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
> in use, this gets optimized back to original macro compare, but
> if OF_CONTROL is in use, this also takes into account the DT
> properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Dragan Simic <dsimic@manjaro.org>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
> V2: - Rename mmc_env_hwpart_redund() to mmc_env_is_redundant_in_both_boot_hwparts()
>      - Return bool
> V3: - Hide the mmc_env_is_redundant_in_both_boot_hwparts symbol behind __maybe_unused
>        as this symbol is called from code gated behind ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
>        in env_mmc_load()
> ---
>   env/mmc.c | 37 +++++++++++++++++++++----------------
>   1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 379f5ec9be7..353a7ce72fb 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -40,18 +40,6 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> -/*
> - * In case the environment is redundant, stored in eMMC hardware boot
> - * partition and the environment and redundant environment offsets are
> - * identical, store the environment and redundant environment in both
> - * eMMC boot partitions, one copy in each.
> - * */
> -#if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
> -     (CONFIG_SYS_MMC_ENV_PART == 1) && \
> -     (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
> -#define ENV_MMC_HWPART_REDUND	1
> -#endif
> -
>   #if CONFIG_IS_ENABLED(OF_CONTROL)
>   
>   static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
> @@ -217,6 +205,23 @@ static inline s64 mmc_offset(struct mmc *mmc, int copy)
>   }
>   #endif
>   
> +static bool __maybe_unused mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
> +{
> +	/*
> +	 * In case the environment is redundant, stored in eMMC hardware boot
> +	 * partition and the environment and redundant environment offsets are
> +	 * identical, store the environment and redundant environment in both
> +	 * eMMC boot partitions, one copy in each.
> +	 */
> +	if (!IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> +		return false;
> +
> +	if (CONFIG_SYS_MMC_ENV_PART != 1)
> +		return false;
> +
> +	return mmc_offset(mmc, 0) == mmc_offset(mmc, 1);

This is not always equivalent to the current test of

CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND

Indeed, it only is for when OF_CONTROL isn't set.

I would recommend to keep this check in patch 1, then add another patch 
that swaps CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND for 
mmc_offset(mmc, 0) == mmc_offset(mmc, 1)

This will allow to use DT properties like 
u-boot,mmc-env-offset-redundant and u-boot,mmc-env-offset, which isn't 
supported today.

What do you think?

The rest of the diff looks good to me.

Cheers,
Quentin

Re: [PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[Marek Vasut]

On 2/24/25 11:01 AM, Quentin Schulz wrote:
> Hi Marek,

Hi,

> On 2/21/25 7:47 PM, Marek Vasut wrote:
>> Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
>> which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
>> almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
>> did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).
>>
>> The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
>> in the last conditional, which does not do plain macro compare
>> (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
>> mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
>> in use, this gets optimized back to original macro compare, but
>> if OF_CONTROL is in use, this also takes into account the DT
>> properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Dragan Simic <dsimic@manjaro.org>
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Cc: Quentin Schulz <quentin.schulz@cherry.de>
>> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: u-boot@lists.denx.de
>> ---
>> V2: - Rename mmc_env_hwpart_redund() to 
>> mmc_env_is_redundant_in_both_boot_hwparts()
>>      - Return bool
>> V3: - Hide the mmc_env_is_redundant_in_both_boot_hwparts symbol behind 
>> __maybe_unused
>>        as this symbol is called from code gated behind ifdef 
>> CONFIG_SYS_REDUNDAND_ENVIRONMENT
>>        in env_mmc_load()
>> ---
>>   env/mmc.c | 37 +++++++++++++++++++++----------------
>>   1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/env/mmc.c b/env/mmc.c
>> index 379f5ec9be7..353a7ce72fb 100644
>> --- a/env/mmc.c
>> +++ b/env/mmc.c
>> @@ -40,18 +40,6 @@
>>   DECLARE_GLOBAL_DATA_PTR;
>> -/*
>> - * In case the environment is redundant, stored in eMMC hardware boot
>> - * partition and the environment and redundant environment offsets are
>> - * identical, store the environment and redundant environment in both
>> - * eMMC boot partitions, one copy in each.
>> - * */
>> -#if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
>> -     (CONFIG_SYS_MMC_ENV_PART == 1) && \
>> -     (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
>> -#define ENV_MMC_HWPART_REDUND    1
>> -#endif
>> -
>>   #if CONFIG_IS_ENABLED(OF_CONTROL)
>>   static int mmc_env_partition_by_name(struct blk_desc *desc, const 
>> char *str,
>> @@ -217,6 +205,23 @@ static inline s64 mmc_offset(struct mmc *mmc, int 
>> copy)
>>   }
>>   #endif
>> +static bool __maybe_unused 
>> mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
>> +{
>> +    /*
>> +     * In case the environment is redundant, stored in eMMC hardware 
>> boot
>> +     * partition and the environment and redundant environment 
>> offsets are
>> +     * identical, store the environment and redundant environment in 
>> both
>> +     * eMMC boot partitions, one copy in each.
>> +     */
>> +    if (!IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
>> +        return false;
>> +
>> +    if (CONFIG_SYS_MMC_ENV_PART != 1)
>> +        return false;
>> +
>> +    return mmc_offset(mmc, 0) == mmc_offset(mmc, 1);
> 
> This is not always equivalent to the current test of
> 
> CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND
> 
> Indeed, it only is for when OF_CONTROL isn't set.

This is what the patch fixes and this is what the commit message states.

> I would recommend to keep this check in patch 1, then add another patch 
> that swaps CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND for 
> mmc_offset(mmc, 0) == mmc_offset(mmc, 1)

What benefit would that bring ?

> This will allow to use DT properties like u-boot,mmc-env-offset- 
> redundant and u-boot,mmc-env-offset, which isn't supported today.

This is what the patch fixes and this is what the commit message states.

[...]

Re: [PATCH v3 2/2] env: mmc: Clean up env_mmc_load() ifdeffery

[Marek Vasut]

On 2/24/25 10:48 AM, Quentin Schulz wrote:
> Hi Marek,

Hi,

> On 2/21/25 7:47 PM, Marek Vasut wrote:
>> Rename the variants of env_mmc_load() for redundant and non-redundant
>> environment to env_mmc_load_redundant() and env_mmc_load_singular()
>> respectively and convert the env_mmc_load() implementation to use of
>> if (IS_ENABLED(...)). As a result, drop __maybe_unused from
>> mmc_env_is_redundant_in_both_boot_hwparts().
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Dragan Simic <dsimic@manjaro.org>
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Cc: Quentin Schulz <quentin.schulz@cherry.de>
>> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: u-boot@lists.denx.de
>> ---
>> V3: New patch
>> ---
>>   env/mmc.c | 25 ++++++++++++++-----------
>>   1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/env/mmc.c b/env/mmc.c
>> index 353a7ce72fb..2ef15fb72e7 100644
>> --- a/env/mmc.c
>> +++ b/env/mmc.c
>> @@ -205,7 +205,7 @@ static inline s64 mmc_offset(struct mmc *mmc, int 
>> copy)
>>   }
>>   #endif
>> -static bool __maybe_unused 
>> mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
>> +static bool mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
>>   {
>>       /*
>>        * In case the environment is redundant, stored in eMMC hardware 
>> boot
>> @@ -448,13 +448,7 @@ static inline int read_env(struct mmc *mmc, 
>> unsigned long size,
>>       return (n == blk_cnt) ? 0 : -1;
>>   }
>> -#if defined(ENV_IS_EMBEDDED)
>> -static int env_mmc_load(void)
>> -{
>> -    return 0;
>> -}
>> -#elif defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
>> -static int env_mmc_load(void)
>> +static int env_mmc_load_redundant(void)
>>   {
>>       struct mmc *mmc;
>>       u32 offset1, offset2;
>> @@ -510,8 +504,8 @@ err:
>>       return ret;
>>   }
>> -#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
>> -static int env_mmc_load(void)
>> +
>> +static int env_mmc_load_singular(void)
>>   {
>>       ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
>>       struct mmc *mmc;
>> @@ -556,7 +550,16 @@ err:
>>       return ret;
>>   }
>> -#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
>> +
>> +static int env_mmc_load(void)
>> +{
>> +    if (IS_ENABLED(CONFIG_ENV_IS_EMBEDDED))
>> +        return 0;
>> +    else if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
>> +        return env_mmc_load_redundant();
>> +    else
>> +        return env_mmc_load_singular();
> 
> The else (not its content!) could be removed since all conditions above 
> return.
> 
> Isn't this going to be increasing the size of the binary with code that 
> won't be run or is the compiler smart enough to remove that based on the 
> IS_ENABLED() that are constant false at build time?
If that was to be the case, then IS_ENABLED() would be entirely 
pointless in the first place, right ?

Re: [PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[Quentin Schulz]

Hi Marek,

On 2/24/25 6:49 PM, Marek Vasut wrote:
> On 2/24/25 11:01 AM, Quentin Schulz wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On 2/21/25 7:47 PM, Marek Vasut wrote:
>>> Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
>>> which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
>>> almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
>>> did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).
>>>
>>> The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
>>> in the last conditional, which does not do plain macro compare
>>> (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
>>> mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
>>> in use, this gets optimized back to original macro compare, but
>>> if OF_CONTROL is in use, this also takes into account the DT
>>> properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Dragan Simic <dsimic@manjaro.org>
>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>> Cc: Quentin Schulz <quentin.schulz@cherry.de>
>>> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> Cc: u-boot@lists.denx.de
>>> ---
>>> V2: - Rename mmc_env_hwpart_redund() to 
>>> mmc_env_is_redundant_in_both_boot_hwparts()
>>>      - Return bool
>>> V3: - Hide the mmc_env_is_redundant_in_both_boot_hwparts symbol 
>>> behind __maybe_unused
>>>        as this symbol is called from code gated behind ifdef 
>>> CONFIG_SYS_REDUNDAND_ENVIRONMENT
>>>        in env_mmc_load()
>>> ---
>>>   env/mmc.c | 37 +++++++++++++++++++++----------------
>>>   1 file changed, 21 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/env/mmc.c b/env/mmc.c
>>> index 379f5ec9be7..353a7ce72fb 100644
>>> --- a/env/mmc.c
>>> +++ b/env/mmc.c
>>> @@ -40,18 +40,6 @@
>>>   DECLARE_GLOBAL_DATA_PTR;
>>> -/*
>>> - * In case the environment is redundant, stored in eMMC hardware boot
>>> - * partition and the environment and redundant environment offsets are
>>> - * identical, store the environment and redundant environment in both
>>> - * eMMC boot partitions, one copy in each.
>>> - * */
>>> -#if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
>>> -     (CONFIG_SYS_MMC_ENV_PART == 1) && \
>>> -     (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
>>> -#define ENV_MMC_HWPART_REDUND    1
>>> -#endif
>>> -
>>>   #if CONFIG_IS_ENABLED(OF_CONTROL)
>>>   static int mmc_env_partition_by_name(struct blk_desc *desc, const 
>>> char *str,
>>> @@ -217,6 +205,23 @@ static inline s64 mmc_offset(struct mmc *mmc, 
>>> int copy)
>>>   }
>>>   #endif
>>> +static bool __maybe_unused 
>>> mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
>>> +{
>>> +    /*
>>> +     * In case the environment is redundant, stored in eMMC hardware 
>>> boot
>>> +     * partition and the environment and redundant environment 
>>> offsets are
>>> +     * identical, store the environment and redundant environment in 
>>> both
>>> +     * eMMC boot partitions, one copy in each.
>>> +     */
>>> +    if (!IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
>>> +        return false;
>>> +
>>> +    if (CONFIG_SYS_MMC_ENV_PART != 1)
>>> +        return false;
>>> +
>>> +    return mmc_offset(mmc, 0) == mmc_offset(mmc, 1);
>>
>> This is not always equivalent to the current test of
>>
>> CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND
>>
>> Indeed, it only is for when OF_CONTROL isn't set.
> 
> This is what the patch fixes and this is what the commit message states.
> 

Sigh... Should have been as careful as I was reviewing the code and read 
the commit log once again. Sorry for the noise.

>> I would recommend to keep this check in patch 1, then add another 
>> patch that swaps CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND for 
>> mmc_offset(mmc, 0) == mmc_offset(mmc, 1)
> 
> What benefit would that bring ?
> 

1) rework to not use ifdefery,
2) add support for OF properties,

If 2) becomes an issue, easy to revert and keep the move out of ifdefery in.

Considering the change is documented in the commit log,

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On ven., févr. 21, 2025 at 19:47, Marek Vasut <marex@denx.de> wrote:

> Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
> which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
> almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
> did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).
>
> The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
> in the last conditional, which does not do plain macro compare
> (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
> mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
> in use, this gets optimized back to original macro compare, but
> if OF_CONTROL is in use, this also takes into account the DT
> properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.
>
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---

Re: [PATCH v3 2/2] env: mmc: Clean up env_mmc_load() ifdeffery

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On ven., févr. 21, 2025 at 19:47, Marek Vasut <marex@denx.de> wrote:

> Rename the variants of env_mmc_load() for redundant and non-redundant
> environment to env_mmc_load_redundant() and env_mmc_load_singular()
> respectively and convert the env_mmc_load() implementation to use of
> if (IS_ENABLED(...)). As a result, drop __maybe_unused from
> mmc_env_is_redundant_in_both_boot_hwparts().
>
> Signed-off-by: Marek Vasut <marex@denx.de>

Nice cleanup!

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---

Re: [PATCH v3 1/2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties

[Tom Rini]

On Fri, 21 Feb 2025 19:47:23 +0100, Marek Vasut wrote:

> Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
> which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
> almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
> did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).
> 
> The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
> in the last conditional, which does not do plain macro compare
> (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
> mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
> in use, this gets optimized back to original macro compare, but
> if OF_CONTROL is in use, this also takes into account the DT
> properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom