[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 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.
> 

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 at cherry.de>

Thanks!
Quentin