[U-Boot] [PATCH 1/2] env: Fix env_load_location

[U-Boot] [PATCH 1/2] env: Fix env_load_location

[York Sun]

Commit 7d714a24d725 ("env: Support multiple environments") added
static variable env_load_location. When saving environmental
variables, this variable is presumed to have the value set before.
In case the value was set before relocation and U-Boot runs from a
NOR flash, this variable wasn't writable. This causes failure when
saving the environment. To save this location, global data must be
used instead.

Signed-off-by: York Sun <york.sun@nxp.com>
CC: Maxime Ripard <maxime.ripard@free-electrons.com>
---
Limited test on LS1043ARDB.

 env/env.c                         | 8 +++-----
 include/asm-generic/global_data.h | 1 +
 include/environment.h             | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/env/env.c b/env/env.c
index 9a89832..edfb575 100644
--- a/env/env.c
+++ b/env/env.c
@@ -62,8 +62,6 @@ static enum env_location env_locations[] = {
 #endif
 };
 
-static enum env_location env_load_location = ENVL_UNKNOWN;
-
 static bool env_has_inited(enum env_location location)
 {
 	return gd->env_has_init & BIT(location);
@@ -108,11 +106,11 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
 		if (prio >= ARRAY_SIZE(env_locations))
 			return ENVL_UNKNOWN;
 
-		env_load_location = env_locations[prio];
-		return env_load_location;
+		gd->env_load_location = env_locations[prio];
+		return gd->env_load_location;
 
 	case ENVOP_SAVE:
-		return env_load_location;
+		return gd->env_load_location;
 	}
 
 	return ENVL_UNKNOWN;
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index fd8cd45..10f1441 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -51,6 +51,7 @@ typedef struct global_data {
 	unsigned long env_addr;		/* Address  of Environment struct */
 	unsigned long env_valid;	/* Environment valid? enum env_valid */
 	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
+	int env_load_location;
 
 	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
 	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
diff --git a/include/environment.h b/include/environment.h
index a406050..0f339da 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -188,6 +188,7 @@ enum env_valid {
 };
 
 enum env_location {
+	ENVL_UNKNOWN,
 	ENVL_EEPROM,
 	ENVL_EXT4,
 	ENVL_FAT,
@@ -202,7 +203,6 @@ enum env_location {
 	ENVL_NOWHERE,
 
 	ENVL_COUNT,
-	ENVL_UNKNOWN,
 };
 
 /* value for the various operations we want to perform on the env */
-- 
2.7.4

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[York Sun]

Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
lookup function") dropped the default action if driver doesn't have
get_char() defined. This causes failure to get environmental
variables from NOR flash. Add back this default action for now.

Signed-off-by: York Sun <york.sun@nxp.com>
CC: Maxime Ripard <maxime.ripard@free-electrons.com>
---
Limited test on LS1043ARDB.

 env/env.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c
index edfb575..210bae2 100644
--- a/env/env.c
+++ b/env/env.c
@@ -159,7 +159,7 @@ int env_get_char(int index)
 		int ret;
 
 		if (!drv->get_char)
-			continue;
+			return *(uchar *)(gd->env_addr + index);
 
 		if (!env_has_inited(drv->location))
 			continue;
-- 
2.7.4

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Maxime Ripard]

On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:
> Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
> lookup function") dropped the default action if driver doesn't have
> get_char() defined. This causes failure to get environmental
> variables from NOR flash. Add back this default action for now.
> 
> Signed-off-by: York Sun <york.sun@nxp.com>
> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> Limited test on LS1043ARDB.
> 
>  env/env.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/env/env.c b/env/env.c
> index edfb575..210bae2 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -159,7 +159,7 @@ int env_get_char(int index)
>  		int ret;
>  
>  		if (!drv->get_char)
> -			continue;
> +			return *(uchar *)(gd->env_addr + index);

Thinking more about this, I think this would break the case where the
first environment in your list has no get_char method, but the second
might.

Would something like that work?

------------ 8< --------
diff --git a/env/env.c b/env/env.c
index 9a89832c1aaf..391c9c0df2f0 100644
--- a/env/env.c
+++ b/env/env.c
@@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
 int env_get_char(int index)
 {
        struct env_driver *drv;
+       bool has_get_char = false;
        int prio;
 
        if (gd->env_valid == ENV_INVALID)
@@ -166,6 +167,7 @@ int env_get_char(int index)
                if (!env_has_inited(drv->location))
                        continue;
 
+               has_get_char = true;
                ret = drv->get_char(index);
                if (!ret)
                        return 0;
@@ -174,6 +176,9 @@ int env_get_char(int index)
                      drv->name, ret);
        }
 
+       if (!has_get_char)
+               return *(uchar *)(gd->env_addr + index);
+
        return -ENODEV;
 }
------ 8< -------

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/3a4fb80f/attachment.sig>

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Simon Goldschmidt]

[e-news abonnieren]<http://www.pepperl-fuchs.de/e-news> [YouTube: Pepperl+Fuchs auf YouTube] <http://www.youtube.com/user/PepperlFuchsGmbH>     [@PepperlFuchsDE folgen] <https://twitter.com/PepperlFuchsDE>   [Pepperl+Fuchs auf XING] <https://www.xing.com/companies/pepperl%252bfuchsgmbh>

________________________________
[Pepperl+Fuchs GmbH]<http://www.pepperl-fuchs.com>

Denken Sie bitte an die Umwelt und prüfen Sie, ob diese E-Mail wirklich ausgedruckt werden muss.

SocialMedia2014

On 08.02.2018 09:47, Maxime Ripard wrote:

On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:


Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
lookup function") dropped the default action if driver doesn't have
get_char() defined. This causes failure to get environmental
variables from NOR flash. Add back this default action for now.

Signed-off-by: York Sun <york.sun@nxp.com><mailto:york.sun@nxp.com>
CC: Maxime Ripard <maxime.ripard@free-electrons.com><mailto:maxime.ripard@free-electrons.com>
---
Limited test on LS1043ARDB.

 env/env.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c
index edfb575..210bae2 100644
--- a/env/env.c
+++ b/env/env.c
@@ -159,7 +159,7 @@ int env_get_char(int index)
                int ret;

                if (!drv->get_char)
-                       continue;
+                       return *(uchar *)(gd->env_addr + index);



Thinking more about this, I think this would break the case where the
first environment in your list has no get_char method, but the second
might.

How can we decide which way is wanted? With your patch below, we might end up loading chars from a low-prio environment (which is not CRC checked) in the early boot stage. Later, we load the environment from another env driver with higher priority.

That's why I suggested removing the 'get_char' callback from the env drivers :-)
Early boot stage environment lookups would still work the old way reading from 'gd->env_addr + index'.

Simon


Would something like that work?

------------ 8< --------
diff --git a/env/env.c b/env/env.c
index 9a89832c1aaf..391c9c0df2f0 100644
--- a/env/env.c
+++ b/env/env.c
@@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
 int env_get_char(int index)
 {
        struct env_driver *drv;
+       bool has_get_char = false;
        int prio;

        if (gd->env_valid == ENV_INVALID)
@@ -166,6 +167,7 @@ int env_get_char(int index)
                if (!env_has_inited(drv->location))
                        continue;

+               has_get_char = true;
                ret = drv->get_char(index);
                if (!ret)
                        return 0;
@@ -174,6 +176,9 @@ int env_get_char(int index)
                      drv->name, ret);
        }

+       if (!has_get_char)
+               return *(uchar *)(gd->env_addr + index);
+
        return -ENODEV;
 }
------ 8< -------

Thanks!
Maxime





_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de<mailto:U-Boot@lists.denx.de>
https://lists.denx.de/listinfo/u-boot


-------------- next part --------------
A non-text attachment was scrubbed...
Name: image53adc6.BMP
Type: image/bmp
Size: 1782 bytes
Desc: image53adc6.BMP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/9a583ce5/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image90c3c9.BMP
Type: image/bmp
Size: 1782 bytes
Desc: image90c3c9.BMP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/9a583ce5/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image12d4d9.BMP
Type: image/bmp
Size: 1782 bytes
Desc: image12d4d9.BMP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/9a583ce5/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image518fd1.BMP
Type: image/bmp
Size: 1782 bytes
Desc: image518fd1.BMP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/9a583ce5/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: imagedae50d.BMP
Type: image/bmp
Size: 55350 bytes
Desc: imagedae50d.BMP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/9a583ce5/attachment-0009.bin>

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Simon Goldschmidt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8-sig", Size: 1479 bytes --]

Sorry for the html mess, I'll resend in a minute.

Simon



Pepperl+Fuchs GmbH, Mannheim
Geschaeftsfuehrer/Managing Directors: Dr.-Ing. Gunther Kegel (Vors./CEO), Werner Guthier, Mehmet Hatiboglu
Vorsitzender des Aufsichtsrats/Chairman of the supervisory board: Claus Michael
Registergericht/Register Court: AG Mannheim HRB 4713
On 08.02.2018 10:42, Simon Goldschmidt wrote:
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot


Wichtiger Hinweis:
Diese E-Mail einschliesslich ihrer Anhaenge enthaelt vertrauliche und rechtlich geschuetzte Informationen, die nur fuer den Adressaten bestimmt sind. 
Sollten Sie nicht der bezeichnete Adressat sein, so teilen Sie dies bitte dem Absender umgehend mit und loeschen Sie diese Nachricht und ihre Anhaenge. Die unbefugte Weitergabe, das Anfertigen von Kopien und jede Veraenderung der E-Mail ist untersagt. Der Absender haftet nicht fuer Inhalte von veraenderten E-Mails.


Important Information:
This e-mail message including its attachments contains confidential and legally protected information solely intended for the addressee. If you are not the intended addressee of this message, please contact the addresser immediately and delete this message including its attachments. The unauthorized dissemination, copying and change of this e-mail are strictly forbidden. The addresser shall not be liable for the content of such changed e-mails.

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Simon Goldschmidt]

On 08.02.2018 09:47, Maxime Ripard wrote:
> On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:
>> Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
>> lookup function") dropped the default action if driver doesn't have
>> get_char() defined. This causes failure to get environmental
>> variables from NOR flash. Add back this default action for now.
>>
>> Signed-off-by: York Sun <york.sun@nxp.com>
>> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>> Limited test on LS1043ARDB.
>>
>>   env/env.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index edfb575..210bae2 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>>   		int ret;
>>   
>>   		if (!drv->get_char)
>> -			continue;
>> +			return *(uchar *)(gd->env_addr + index);
> Thinking more about this, I think this would break the case where the
> first environment in your list has no get_char method, but the second
> might.


How can we decide which way is wanted? With your patch below, we might 
end up loading chars from a low-prio environment (which is not CRC 
checked) in the early boot stage. Later, we load the environment from 
another env driver with higher priority.

That's why I suggested removing the 'get_char' callback from the env 
drivers :-) Early boot stage environment lookups would still work the 
old way reading from 'gd->env_addr + index'.

Simon


> Would something like that work?
>
> ------------ 8< --------
> diff --git a/env/env.c b/env/env.c
> index 9a89832c1aaf..391c9c0df2f0 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
>   int env_get_char(int index)
>   {
>          struct env_driver *drv;
> +       bool has_get_char = false;
>          int prio;
>   
>          if (gd->env_valid == ENV_INVALID)
> @@ -166,6 +167,7 @@ int env_get_char(int index)
>                  if (!env_has_inited(drv->location))
>                          continue;
>   
> +               has_get_char = true;
>                  ret = drv->get_char(index);
>                  if (!ret)
>                          return 0;
> @@ -174,6 +176,9 @@ int env_get_char(int index)
>                        drv->name, ret);
>          }
>   
> +       if (!has_get_char)
> +               return *(uchar *)(gd->env_addr + index);
> +
>          return -ENODEV;
>   }
> ------ 8< -------
>
> Thanks!
> Maxime
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Maxime Ripard]

On Thu, Feb 08, 2018 at 10:52:20AM +0100, Simon Goldschmidt wrote:
> On 08.02.2018 09:47, Maxime Ripard wrote:
> > On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:
> > > Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
> > > lookup function") dropped the default action if driver doesn't have
> > > get_char() defined. This causes failure to get environmental
> > > variables from NOR flash. Add back this default action for now.
> > > 
> > > Signed-off-by: York Sun <york.sun@nxp.com>
> > > CC: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > > Limited test on LS1043ARDB.
> > > 
> > >   env/env.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/env/env.c b/env/env.c
> > > index edfb575..210bae2 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -159,7 +159,7 @@ int env_get_char(int index)
> > >   		int ret;
> > >   		if (!drv->get_char)
> > > -			continue;
> > > +			return *(uchar *)(gd->env_addr + index);
> > Thinking more about this, I think this would break the case where the
> > first environment in your list has no get_char method, but the second
> > might.
> 
> 
> How can we decide which way is wanted? With your patch below, we might end
> up loading chars from a low-prio environment (which is not CRC checked) in
> the early boot stage. Later, we load the environment from another env driver
> with higher priority.

Ah, right.

> That's why I suggested removing the 'get_char' callback from the env drivers
> :-) Early boot stage environment lookups would still work the old way
> reading from 'gd->env_addr + index'.

If that works on York's board, I'm all in.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Goldschmidt Simon]

Maxime, York,

I've just sent a patch that removes 'get_char' callback from the env 
drivers. This should restore the old behaviour we had before supporting 
multiple environment drivers (for all but eeprom, of course).


Simon


On 08.02.2018 23:10, Maxime Ripard wrote:
> On Thu, Feb 08, 2018 at 10:52:20AM +0100, Simon Goldschmidt wrote:
>> On 08.02.2018 09:47, Maxime Ripard wrote:
>>> On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:
>>>> Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
>>>> lookup function") dropped the default action if driver doesn't have
>>>> get_char() defined. This causes failure to get environmental
>>>> variables from NOR flash. Add back this default action for now.
>>>>
>>>> Signed-off-by: York Sun <york.sun@nxp.com>
>>>> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>> ---
>>>> Limited test on LS1043ARDB.
>>>>
>>>>    env/env.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/env/env.c b/env/env.c
>>>> index edfb575..210bae2 100644
>>>> --- a/env/env.c
>>>> +++ b/env/env.c
>>>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>>>>    		int ret;
>>>>    		if (!drv->get_char)
>>>> -			continue;
>>>> +			return *(uchar *)(gd->env_addr + index);
>>> Thinking more about this, I think this would break the case where the
>>> first environment in your list has no get_char method, but the second
>>> might.
>>
>> How can we decide which way is wanted? With your patch below, we might end
>> up loading chars from a low-prio environment (which is not CRC checked) in
>> the early boot stage. Later, we load the environment from another env driver
>> with higher priority.
> Ah, right.
>
>> That's why I suggested removing the 'get_char' callback from the env drivers
>> :-) Early boot stage environment lookups would still work the old way
>> reading from 'gd->env_addr + index'.
> If that works on York's board, I'm all in.
>
> Maxime
>

[U-Boot] [PATCH 1/2] env: Fix env_load_location

[Maxime Ripard]

Hi,

Thanks for your patch

On Wed, Feb 07, 2018 at 02:17:11PM -0800, York Sun wrote:
> Commit 7d714a24d725 ("env: Support multiple environments") added
> static variable env_load_location. When saving environmental
> variables, this variable is presumed to have the value set before.
> In case the value was set before relocation and U-Boot runs from a
> NOR flash, this variable wasn't writable. This causes failure when
> saving the environment. To save this location, global data must be
> used instead.
> 
> Signed-off-by: York Sun <york.sun@nxp.com>
> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> Limited test on LS1043ARDB.
> 
>  env/env.c                         | 8 +++-----
>  include/asm-generic/global_data.h | 1 +
>  include/environment.h             | 2 +-
>  3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 9a89832..edfb575 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -62,8 +62,6 @@ static enum env_location env_locations[] = {
>  #endif
>  };
>  
> -static enum env_location env_load_location = ENVL_UNKNOWN;
> -
>  static bool env_has_inited(enum env_location location)
>  {
>  	return gd->env_has_init & BIT(location);
> @@ -108,11 +106,11 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
>  		if (prio >= ARRAY_SIZE(env_locations))
>  			return ENVL_UNKNOWN;
>  
> -		env_load_location = env_locations[prio];
> -		return env_load_location;
> +		gd->env_load_location = env_locations[prio];
> +		return gd->env_load_location;
>  
>  	case ENVOP_SAVE:
> -		return env_load_location;
> +		return gd->env_load_location;
>  	}
>  
>  	return ENVL_UNKNOWN;
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index fd8cd45..10f1441 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -51,6 +51,7 @@ typedef struct global_data {
>  	unsigned long env_addr;		/* Address  of Environment struct */
>  	unsigned long env_valid;	/* Environment valid? enum env_valid */
>  	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
> +	int env_load_location;
>  
>  	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
>  	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
> diff --git a/include/environment.h b/include/environment.h
> index a406050..0f339da 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -188,6 +188,7 @@ enum env_valid {
>  };
>  
>  enum env_location {
> +	ENVL_UNKNOWN,
>  	ENVL_EEPROM,
>  	ENVL_EXT4,
>  	ENVL_FAT,
> @@ -202,7 +203,6 @@ enum env_location {
>  	ENVL_NOWHERE,
>  
>  	ENVL_COUNT,
> -	ENVL_UNKNOWN,

Why did you need to change this? This looks a bit odd.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180208/8f54ce1f/attachment.sig>

[U-Boot] [PATCH 1/2] env: Fix env_load_location

[Simon Goldschmidt]

On 08.02.2018 09:38, Maxime Ripard wrote:
> Hi,
>
> Thanks for your patch
>
> On Wed, Feb 07, 2018 at 02:17:11PM -0800, York Sun wrote:
>> Commit 7d714a24d725 ("env: Support multiple environments") added
>> static variable env_load_location. When saving environmental
>> variables, this variable is presumed to have the value set before.
>> In case the value was set before relocation and U-Boot runs from a
>> NOR flash, this variable wasn't writable. This causes failure when
>> saving the environment. To save this location, global data must be
>> used instead.

Out of curiosity: which linker sections are affected of this issue? I 
assume only initialized data ('.data') is affected as it is left in 
place. Also, I assume that uninitialized data ('.bss') is not affected 
(as your linker can place this into ram)?
If so, this issue could be fixed by changing ENVL_UNKNOWN to zero (which 
you already do in your patch) and leaving 'env_load_location' 
uninitialized, in which case it is initialized to zero (== ENVL_UNKNOWN) 
as defined by the C standard.

Another possibility to fix this would be to have your linker script put 
'data' into ram but initialize it from rom. Something like this:

.data :
{
   <your section contents here>
} >ram AT>rom

To me, both versions would be better than to add members to struct 
global_dat.

>> Signed-off-by: York Sun <york.sun@nxp.com>
>> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>> Limited test on LS1043ARDB.
>>
>>   env/env.c                         | 8 +++-----
>>   include/asm-generic/global_data.h | 1 +
>>   include/environment.h             | 2 +-
>>   3 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index 9a89832..edfb575 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -62,8 +62,6 @@ static enum env_location env_locations[] = {
>>   #endif
>>   };
>>   
>> -static enum env_location env_load_location = ENVL_UNKNOWN;
>> -
>>   static bool env_has_inited(enum env_location location)
>>   {
>>   	return gd->env_has_init & BIT(location);
>> @@ -108,11 +106,11 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
>>   		if (prio >= ARRAY_SIZE(env_locations))
>>   			return ENVL_UNKNOWN;
>>   
>> -		env_load_location = env_locations[prio];
>> -		return env_load_location;
>> +		gd->env_load_location = env_locations[prio];
>> +		return gd->env_load_location;
>>   
>>   	case ENVOP_SAVE:
>> -		return env_load_location;
>> +		return gd->env_load_location;
>>   	}
>>   
>>   	return ENVL_UNKNOWN;
>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>> index fd8cd45..10f1441 100644
>> --- a/include/asm-generic/global_data.h
>> +++ b/include/asm-generic/global_data.h
>> @@ -51,6 +51,7 @@ typedef struct global_data {
>>   	unsigned long env_addr;		/* Address  of Environment struct */
>>   	unsigned long env_valid;	/* Environment valid? enum env_valid */
>>   	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
>> +	int env_load_location;
>>   
>>   	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
>>   	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
>> diff --git a/include/environment.h b/include/environment.h
>> index a406050..0f339da 100644
>> --- a/include/environment.h
>> +++ b/include/environment.h
>> @@ -188,6 +188,7 @@ enum env_valid {
>>   };
>>   
>>   enum env_location {
>> +	ENVL_UNKNOWN,
>>   	ENVL_EEPROM,
>>   	ENVL_EXT4,
>>   	ENVL_FAT,
>> @@ -202,7 +203,6 @@ enum env_location {
>>   	ENVL_NOWHERE,
>>   
>>   	ENVL_COUNT,
>> -	ENVL_UNKNOWN,
> Why did you need to change this? This looks a bit odd.

The global data struct is initialized to zero. I guess this change is 
meant to ensure 'gd->env_load_location' is initialized to ENV_UNKOWN 
(see my comments above).

Simon

>
> Thanks!
> Maxime
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 1/2] env: Fix env_load_location

[York Sun]

On 02/08/2018 02:05 AM, Simon Goldschmidt wrote:
> On 08.02.2018 09:38, Maxime Ripard wrote:
>> Hi,
>>
>> Thanks for your patch
>>
>> On Wed, Feb 07, 2018 at 02:17:11PM -0800, York Sun wrote:
>>> Commit 7d714a24d725 ("env: Support multiple environments") added
>>> static variable env_load_location. When saving environmental
>>> variables, this variable is presumed to have the value set before.
>>> In case the value was set before relocation and U-Boot runs from a
>>> NOR flash, this variable wasn't writable. This causes failure when
>>> saving the environment. To save this location, global data must be
>>> used instead.
> 
> Out of curiosity: which linker sections are affected of this issue? I 
> assume only initialized data ('.data') is affected as it is left in 
> place. Also, I assume that uninitialized data ('.bss') is not affected 
> (as your linker can place this into ram)?
> If so, this issue could be fixed by changing ENVL_UNKNOWN to zero (which 
> you already do in your patch) and leaving 'env_load_location' 
> uninitialized, in which case it is initialized to zero (== ENVL_UNKNOWN) 
> as defined by the C standard.

Leaving it as ENVL_UNKNOWN doesn't fix the problem. The
env_load_location needs to be valid after relocation. Leaving it as
unknown doesn't let you write env.

> 
> Another possibility to fix this would be to have your linker script put 
> 'data' into ram but initialize it from rom. Something like this:
> 
> .data :
> {
>    <your section contents here>
> } >ram AT>rom
> 
> To me, both versions would be better than to add members to struct 
> global_dat.

Not everything in initial RAM is copied to final RAM. After relocation,
we shouldn't be using initial RAM at all. Actually the initial RAM may
be used for other purpose.

> 
>>> Signed-off-by: York Sun <york.sun@nxp.com>
>>> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>> Limited test on LS1043ARDB.
>>>
>>>   env/env.c                         | 8 +++-----
>>>   include/asm-generic/global_data.h | 1 +
>>>   include/environment.h             | 2 +-
>>>   3 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/env/env.c b/env/env.c
>>> index 9a89832..edfb575 100644
>>> --- a/env/env.c
>>> +++ b/env/env.c
>>> @@ -62,8 +62,6 @@ static enum env_location env_locations[] = {
>>>   #endif
>>>   };
>>>   
>>> -static enum env_location env_load_location = ENVL_UNKNOWN;
>>> -
>>>   static bool env_has_inited(enum env_location location)
>>>   {
>>>   	return gd->env_has_init & BIT(location);
>>> @@ -108,11 +106,11 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
>>>   		if (prio >= ARRAY_SIZE(env_locations))
>>>   			return ENVL_UNKNOWN;
>>>   
>>> -		env_load_location = env_locations[prio];
>>> -		return env_load_location;
>>> +		gd->env_load_location = env_locations[prio];
>>> +		return gd->env_load_location;
>>>   
>>>   	case ENVOP_SAVE:
>>> -		return env_load_location;
>>> +		return gd->env_load_location;
>>>   	}
>>>   
>>>   	return ENVL_UNKNOWN;
>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>> index fd8cd45..10f1441 100644
>>> --- a/include/asm-generic/global_data.h
>>> +++ b/include/asm-generic/global_data.h
>>> @@ -51,6 +51,7 @@ typedef struct global_data {
>>>   	unsigned long env_addr;		/* Address  of Environment struct */
>>>   	unsigned long env_valid;	/* Environment valid? enum env_valid */
>>>   	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
>>> +	int env_load_location;
>>>   
>>>   	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
>>>   	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
>>> diff --git a/include/environment.h b/include/environment.h
>>> index a406050..0f339da 100644
>>> --- a/include/environment.h
>>> +++ b/include/environment.h
>>> @@ -188,6 +188,7 @@ enum env_valid {
>>>   };
>>>   
>>>   enum env_location {
>>> +	ENVL_UNKNOWN,
>>>   	ENVL_EEPROM,
>>>   	ENVL_EXT4,
>>>   	ENVL_FAT,
>>> @@ -202,7 +203,6 @@ enum env_location {
>>>   	ENVL_NOWHERE,
>>>   
>>>   	ENVL_COUNT,
>>> -	ENVL_UNKNOWN,
>> Why did you need to change this? This looks a bit odd.
> 
> The global data struct is initialized to zero. I guess this change is 
> meant to ensure 'gd->env_load_location' is initialized to ENV_UNKOWN 
> (see my comments above).

Yes. That was my intention.

York

[U-Boot] [U-Boot,1/2] env: Fix env_load_location

[Tom Rini]

On Wed, Feb 07, 2018 at 02:17:11PM -0800, York Sun wrote:

> Commit 7d714a24d725 ("env: Support multiple environments") added
> static variable env_load_location. When saving environmental
> variables, this variable is presumed to have the value set before.
> In case the value was set before relocation and U-Boot runs from a
> NOR flash, this variable wasn't writable. This causes failure when
> saving the environment. To save this location, global data must be
> used instead.
> 
> Signed-off-by: York Sun <york.sun@nxp.com>
> CC: Maxime Ripard <maxime.ripard@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180217/e7265311/attachment.sig>