diff mbox

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

Message ID 1398593928-13860-3-git-send-email-lifshitz@compulab.co.il
State Superseded
Delegated to: Pantelis Antoniou
Headers show

Commit Message

Dmitry Lifshitz April 27, 2014, 10:18 a.m. UTC 
Add callback with __weak annotation to allow setup of environment
partition number in runtime from a board file.

Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

Comments

Igor Grinberg May 25, 2014, 7:23 a.m. UTC | #1 
ping..

On 04/27/14 13:18, 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@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@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;
>  	}
>  
> -#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;
>  }
>
Tom Rini May 30, 2014, 6:56 p.m. UTC | #2 
On Sun, May 25, 2014 at 10:23:46AM +0300, Igor Grinberg wrote:

> ping..
> 
> On 04/27/14 13:18, 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@compulab.co.il>
> > Signed-off-by: Igor Grinberg <grinberg@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;
> >  	}
> >  
> > -#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;
> >  }
> > 

Pantelis, weren't you going to pick this up?  Thanks!
Igor Grinberg June 11, 2014, 6:51 a.m. UTC | #3 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Tom,

rc3 is out...

On 05/30/14 21:56, Tom Rini wrote:
> On Sun, May 25, 2014 at 10:23:46AM +0300, Igor Grinberg wrote:
> 
>> ping..
>>
>> On 04/27/14 13:18, 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@compulab.co.il>
>>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>

[...]

> 
> Pantelis, weren't you going to pick this up?  Thanks!

Can we please have this in for 2014.07?

Thanks!

- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJTl/xzAAoJEBDE8YO64EfayCkQAJ0UFo/F6DebROHb3fU7u5/J
iW1zRT7i1quZU5vnSa5E1I8vJ7O7IdWErrvgD6wweFjy03ueaE/m3uHXhQnNW4NC
Kgmfr28wCnlHVEJX0RA7Ei0tTbziZTCgZ5+D7xyya/CH+wpDBXw57609XG/cVwH+
JXhKlDPLOyp74kLxwPWT4jJ8ZA/GEZWL97VGWA62OQI5KEh2lASNCjHm9U5bqLPf
6Jl6F57VEsEWyCzz0DrrNZFHeyxIM45o6aUnwz4BrZAZZ+UwXnRkuVPSwKVGlGYc
+TaMjsu3XNV14MtPh5M6tbSs6mpOGHAW20ik0SwT72v4hh7SV4RyWdCY9JbKdKRG
2mEocMs7EFucrNjp0G8YD6U93b9BtTPSpiSnzaBnpECk3GTZQymsSVBn8J4qqoqK
Ucp59mrZc5rFJXU+hCWFcGAdGqfUOuIhX6xPLd0AQlfHgswrEqJ7GBEvZEDLOnnk
wU68HpNs0tW863BYfdabZnsj5BK51NlqIOAI86zCfywDmdIgtZtse7AtlJD5J9Lt
uWLGfxScppTErM8vXRBN/p2YaoCbaLiV/ur/JO4gQNrw5PSPNUUKiooABfHn9HCg
2aABhEiuloslrkgKQKx5TEKT7skVqkbRnzkBD81E/1kasx0o/dEGiADxWiIXdTZw
G1PXeefGklZlCjp8/h2I
=YCwX
-----END PGP SIGNATURE-----
Pantelis Antoniou June 12, 2014, 3:10 p.m. UTC | #4 
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@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@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.

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.

> -#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

Regards

-- Pantelis
Dmitry Lifshitz June 25, 2014, 8:27 a.m. UTC | #5 
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@compulab.co.il>
>> Signed-off-by: Igor Grinberg<grinberg@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
>
diff mbox

Patch

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;
 	}
 
-#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;
 }