[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 at denx.de>
>> ---
>> Cc: Dragan Simic <dsimic at manjaro.org>
>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>> Cc: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> Cc: Quentin Schulz <quentin.schulz at cherry.de>
>> Cc: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Tom Rini <trini at konsulko.com>
>> Cc: u-boot at 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.

[...]