[U-Boot] Fwd: Re: [PATCH 2/3] env_mmc: support env partition setup in runtime

[Igor Grinberg]

ping.

On 07/03/14 11:36, Igor Grinberg wrote:
> Hi Pantelis, Tom,
> 
> Apparently, Dmitry has sent the message in html format...
> 
> Resending now...
> Sorry for that...
> 
> 
> -------- Original Message --------
> Subject: 	Re: [U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime
> Date: 	Wed, 25 Jun 2014 10:42:11 +0300
> From: 	Dmitry Lifshitz <lifshitz at compulab.co.il>
> To: 	Pantelis Antoniou <pantelis.antoniou at gmail.com>
> CC: 	u-boot at lists.denx.de, Tom Rini <trini at ti.com>, Igor Grinberg <grinberg at compulab.co.il>
> 
> 
> 
> Hi Pantelis,
> 
> On 06/12/2014 06:10 PM, Pantelis Antoniou wrote:
>> Hi Dmitry,
>>
>> I took a good look at the patch and there's a problem.
>>
>> On Apr 27, 2014, at 1:18 PM, Dmitry Lifshitz wrote:
>>
>>> Add callback with __weak annotation to allow setup of environment
>>> partition number in runtime from a board file.
>>>
>>> Signed-off-by: Dmitry Lifshitz <lifshitz at compulab.co.il>
>>> Signed-off-by: Igor Grinberg <grinberg at compulab.co.il>
>>> ---
>>> common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
>>> 1 files changed, 26 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/common/env_mmc.c b/common/env_mmc.c
>>> index 045428c..5d4b5f4 100644
>>> --- a/common/env_mmc.c
>>> +++ b/common/env_mmc.c
>>> @@ -62,6 +62,30 @@ int env_init(void)
>>> 	return 0;
>>> }
>>>
>>> +
>>> +#ifdef CONFIG_SYS_MMC_ENV_PART
>>> +__weak uint mmc_get_env_part(struct mmc *mmc)
>>> +{
>>> +	return CONFIG_SYS_MMC_ENV_PART;
>>> +}
>>> +
>>> +static int mmc_set_env_part(struct mmc *mmc)
>>> +{
>>> +	uint part = mmc_get_env_part(mmc);
>>> +
>>> +	if (part != mmc->part_num) {
>>> +		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV, part)) {
>>> +			puts("MMC partition switch failed\n");
>>> +			return -1;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +#else
>>> +static inline int mmc_set_env_part(struct mmc *mmc) {return 0; };
>>> +#endif
>>> +
>>> static int init_mmc_for_env(struct mmc *mmc)
>>> {
>>> 	if (!mmc) {
>>> @@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
>>> 		return -1;
>>> 	}
>>>
>> Just before this hunk, we have this:
>>
>>> #ifdef CONFIG_SYS_MMC_ENV_PART
>>>         int dev = CONFIG_SYS_MMC_ENV_DEV;
>>>
>>> #ifdef CONFIG_SPL_BUILD
>>>         dev = 0;
>>> #endif
>>> #endif
>>>
>> This appears to be broken for SPL in case that CONFIG_SYS_MMC_ENV_DEV is not 0.
> 
> Exactly as it was broken before the patch, right?
> 
>>
>> SPL hardcoded dev to 0, while mmc_switch_part is implicitly operating on
>> CONFIG_SYS_MMC_ENV_DEV.
>>
>> Please rework it so that the SPL case is unaffected.
> 
> This patch does not change the behavior and its purpose
> is not fixing the current  code.
> 
> The bug describe can be fixed by later patch and possibly
> will require additional review and testing.
> 
> Given that this patch does not change the behavior, can it be accepted please ?
> 
> Regards,
> 
> Dmitry
> 
>>> -#ifdef CONFIG_SYS_MMC_ENV_PART
>>> -	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {
>>> -		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
>>> -				    CONFIG_SYS_MMC_ENV_PART)) {
>>> -			puts("MMC partition switch failed\n");
>>> -			return -1;
>>> -		}
>>> -	}
>>> -#endif
>>> +	if (mmc_set_env_part(mmc))
>>> +		return -1;
>>>
>>> 	return 0;
>>> }
>>> -- 
>>> 1.7.5.4
>>
>>
> 
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

-- 
Regards,
Igor.