All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function"
@ 2019-03-13 11:15 Heiko Schocher
  2019-03-13 12:21 ` Stefano Babic
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Heiko Schocher @ 2019-03-13 11:15 UTC (permalink / raw)
  To: u-boot

This reverts commit 9a9d66f5eff0f443de4c2c6ca3e27771ed14b1b4.

because it breaks fw_setenv and U-Boot interworking, if
U-Boot environment is stored in a SPI-NOR.

Reproduce it with:
boot linux with empty Environment and store a variable
with fw_setenv into it, the Environment is now filled
with 0xff:

root at ckey5e:10:8e:~# hexdump -C /dev/mtd4
00000000  e9 e8 07 fa 01 62 6f 6f  74 63 6d 64 3d 72 75 6e  |.....bootcmd=run|
[...]
00000f30  7d 00 75 62 69 62 6f 6f  74 76 6f 6c 3d 32 00 00  |}.ubibootvol=2..|
00000f40  00 ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|

Boot now U-Boot prints:

Loading Environment from SPI Flash... SF: Detected s25fl128l with page size 256 Bytes, erase size 4 KiB, total 16 MiB
*** Warning - bad CRC, using default environment

Reason is the above commit, as it only reads until \0\0
is found, and assumes the rest of the Environment
space is filled with 0x00, which is not the case when
saving an Environment under linux with fw_setenv.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
I already posted a fix:
https://lists.denx.de/pipermail/u-boot/2019-January/355148.html
which I withdrawed:
https://lists.denx.de/pipermail/u-boot/2019-January/355529.html

because I did not recognised the correlation between fw_setenv
in linux context and saveenv in U-Boot context.

I think now, best is to revert this commit.

Comments?

This patch drops checkpatch warnings:
env/sf.c:120: check: Alignment should match open parenthesis
env/sf.c:224: check: Alignment should match open parenthesis
env/sf.c:281: check: Alignment should match open parenthesis

but as it is a revert I did not correct them.

 env/sf.c | 56 +++++++++++---------------------------------------------
 1 file changed, 11 insertions(+), 45 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index b3dec82c35..23cbad5d88 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -81,40 +81,6 @@ static int setup_flash_device(void)
 	return 0;
 }
 
-static int is_end(const char *addr, size_t size)
-{
-	/* The end of env variables is marked by '\0\0' */
-	int i = 0;
-
-	for (i = 0; i < size - 1; ++i)
-		if (addr[i] == 0x0 && addr[i + 1] == 0x0)
-			return 1;
-	return 0;
-}
-
-static int spi_flash_read_env(struct spi_flash *flash, u32 offset, size_t len,
-			      void *buf)
-{
-	u32 addr = 0;
-	u32 page_size = flash->page_size;
-
-	memset(buf, 0x0, len);
-	for (int i = 0; i < len / page_size; ++i) {
-		int ret = spi_flash_read(flash, offset, page_size,
-					 &((char *)buf)[addr]);
-
-		if (ret < 0)
-			return ret;
-
-		if (is_end(&((char *)buf)[addr], page_size))
-			return 0;
-
-		addr += page_size;
-		offset += page_size;
-	}
-	return 0;
-}
-
 #if defined(CONFIG_ENV_OFFSET_REDUND)
 #ifdef CMD_SAVEENV
 static int env_sf_save(void)
@@ -150,8 +116,8 @@ static int env_sf_save(void)
 			ret = -ENOMEM;
 			goto done;
 		}
-		ret = spi_flash_read_env(env_flash, saved_offset,
-					 saved_size, saved_buffer);
+		ret = spi_flash_read(env_flash, saved_offset,
+					saved_size, saved_buffer);
 		if (ret)
 			goto done;
 	}
@@ -217,10 +183,10 @@ static int env_sf_load(void)
 	if (ret)
 		goto out;
 
-	read1_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET,
-					CONFIG_ENV_SIZE, tmp_env1);
-	read2_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET_REDUND,
-					CONFIG_ENV_SIZE, tmp_env2);
+	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
+				    CONFIG_ENV_SIZE, tmp_env1);
+	read2_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND,
+				    CONFIG_ENV_SIZE, tmp_env2);
 
 	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
 				read2_fail);
@@ -254,8 +220,8 @@ static int env_sf_save(void)
 		if (!saved_buffer)
 			goto done;
 
-		ret = spi_flash_read_env(env_flash, saved_offset,
-					 saved_size, saved_buffer);
+		ret = spi_flash_read(env_flash, saved_offset,
+			saved_size, saved_buffer);
 		if (ret)
 			goto done;
 	}
@@ -311,10 +277,10 @@ static int env_sf_load(void)
 	if (ret)
 		goto out;
 
-	ret = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
-				 buf);
+	ret = spi_flash_read(env_flash,
+		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
 	if (ret) {
-		set_default_env("spi_flash_read_env() failed", 0);
+		set_default_env("spi_flash_read() failed", 0);
 		goto err_read;
 	}
 
-- 
2.17.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function"
  2019-03-13 11:15 [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function" Heiko Schocher
@ 2019-03-13 12:21 ` Stefano Babic
  2019-03-14 12:53 ` Horatiu Vultur
  2019-03-14 15:36 ` [U-Boot] " Tom Rini
  2 siblings, 0 replies; 6+ messages in thread
From: Stefano Babic @ 2019-03-13 12:21 UTC (permalink / raw)
  To: u-boot

On 13/03/19 12:15, Heiko Schocher wrote:
> This reverts commit 9a9d66f5eff0f443de4c2c6ca3e27771ed14b1b4.
> 
> because it breaks fw_setenv and U-Boot interworking, if
> U-Boot environment is stored in a SPI-NOR.
> 
> Reproduce it with:
> boot linux with empty Environment and store a variable
> with fw_setenv into it, the Environment is now filled
> with 0xff:
> 
> root at ckey5e:10:8e:~# hexdump -C /dev/mtd4
> 00000000  e9 e8 07 fa 01 62 6f 6f  74 63 6d 64 3d 72 75 6e  |.....bootcmd=run|
> [...]
> 00000f30  7d 00 75 62 69 62 6f 6f  74 76 6f 6c 3d 32 00 00  |}.ubibootvol=2..|
> 00000f40  00 ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> 
> Boot now U-Boot prints:
> 
> Loading Environment from SPI Flash... SF: Detected s25fl128l with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> *** Warning - bad CRC, using default environment
> 
> Reason is the above commit, as it only reads until \0\0
> is found, and assumes the rest of the Environment
> space is filled with 0x00, which is not the case when
> saving an Environment under linux with fw_setenv.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---

Acked-by: Stefano Babic <sbabic@denx.de>

In fact, the commit breaks the API between U-Boot and Linux and
optimizes a specific use case in U-Boot only. The API foresees that
there is an environment of a specific size without bothering (in linux)
where it is stored. It is strictly required that the environment has the
same format for all underlying storage - it must be possible to simply
copy the environment. Commit breaks tools like mkenvimage, too.

Best regards,
Stefano Babic

> I already posted a fix:
> https://lists.denx.de/pipermail/u-boot/2019-January/355148.html
> which I withdrawed:
> https://lists.denx.de/pipermail/u-boot/2019-January/355529.html
> 
> because I did not recognised the correlation between fw_setenv
> in linux context and saveenv in U-Boot context.
> 
> I think now, best is to revert this commit.
> 
> Comments?
> 
> This patch drops checkpatch warnings:
> env/sf.c:120: check: Alignment should match open parenthesis
> env/sf.c:224: check: Alignment should match open parenthesis
> env/sf.c:281: check: Alignment should match open parenthesis
> 
> but as it is a revert I did not correct them.
> 
>  env/sf.c | 56 +++++++++++---------------------------------------------
>  1 file changed, 11 insertions(+), 45 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index b3dec82c35..23cbad5d88 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -81,40 +81,6 @@ static int setup_flash_device(void)
>  	return 0;
>  }
>  
> -static int is_end(const char *addr, size_t size)
> -{
> -	/* The end of env variables is marked by '\0\0' */
> -	int i = 0;
> -
> -	for (i = 0; i < size - 1; ++i)
> -		if (addr[i] == 0x0 && addr[i + 1] == 0x0)
> -			return 1;
> -	return 0;
> -}
> -
> -static int spi_flash_read_env(struct spi_flash *flash, u32 offset, size_t len,
> -			      void *buf)
> -{
> -	u32 addr = 0;
> -	u32 page_size = flash->page_size;
> -
> -	memset(buf, 0x0, len);
> -	for (int i = 0; i < len / page_size; ++i) {
> -		int ret = spi_flash_read(flash, offset, page_size,
> -					 &((char *)buf)[addr]);
> -
> -		if (ret < 0)
> -			return ret;
> -
> -		if (is_end(&((char *)buf)[addr], page_size))
> -			return 0;
> -
> -		addr += page_size;
> -		offset += page_size;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_ENV_OFFSET_REDUND)
>  #ifdef CMD_SAVEENV
>  static int env_sf_save(void)
> @@ -150,8 +116,8 @@ static int env_sf_save(void)
>  			ret = -ENOMEM;
>  			goto done;
>  		}
> -		ret = spi_flash_read_env(env_flash, saved_offset,
> -					 saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset,
> +					saved_size, saved_buffer);
>  		if (ret)
>  			goto done;
>  	}
> @@ -217,10 +183,10 @@ static int env_sf_load(void)
>  	if (ret)
>  		goto out;
>  
> -	read1_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET,
> -					CONFIG_ENV_SIZE, tmp_env1);
> -	read2_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET_REDUND,
> -					CONFIG_ENV_SIZE, tmp_env2);
> +	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
> +				    CONFIG_ENV_SIZE, tmp_env1);
> +	read2_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND,
> +				    CONFIG_ENV_SIZE, tmp_env2);
>  
>  	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
>  				read2_fail);
> @@ -254,8 +220,8 @@ static int env_sf_save(void)
>  		if (!saved_buffer)
>  			goto done;
>  
> -		ret = spi_flash_read_env(env_flash, saved_offset,
> -					 saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset,
> +			saved_size, saved_buffer);
>  		if (ret)
>  			goto done;
>  	}
> @@ -311,10 +277,10 @@ static int env_sf_load(void)
>  	if (ret)
>  		goto out;
>  
> -	ret = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> -				 buf);
> +	ret = spi_flash_read(env_flash,
> +		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>  	if (ret) {
> -		set_default_env("spi_flash_read_env() failed", 0);
> +		set_default_env("spi_flash_read() failed", 0);
>  		goto err_read;
>  	}
>  
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function"
  2019-03-13 11:15 [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function" Heiko Schocher
  2019-03-13 12:21 ` Stefano Babic
@ 2019-03-14 12:53 ` Horatiu Vultur
  2019-03-15  4:34   ` Heiko Schocher
  2019-03-14 15:36 ` [U-Boot] " Tom Rini
  2 siblings, 1 reply; 6+ messages in thread
From: Horatiu Vultur @ 2019-03-14 12:53 UTC (permalink / raw)
  To: u-boot

Hi Heiko,

I managed to reproduce the issue that you described.

Don't you think it is a little bit too harsh to remove the commit
completely?

I am not sure but how many cases are where UBoot doesn't store anything
in flash? And then lets the linux to update the flash.

Or is wouldn't better to update fw_setenv to set the entire flash to
0x0 when it detects that the CRC error? The same way how the saveenv it
is doing in UBoot?

/Horatiu

The 03/13/2019 12:15, Heiko Schocher wrote:
> This reverts commit 9a9d66f5eff0f443de4c2c6ca3e27771ed14b1b4.
> 
> because it breaks fw_setenv and U-Boot interworking, if
> U-Boot environment is stored in a SPI-NOR.
> 
> Reproduce it with:
> boot linux with empty Environment and store a variable
> with fw_setenv into it, the Environment is now filled
> with 0xff:
> 
> root at ckey5e:10:8e:~# hexdump -C /dev/mtd4
> 00000000  e9 e8 07 fa 01 62 6f 6f  74 63 6d 64 3d 72 75 6e  |.....bootcmd=run|
> [...]
> 00000f30  7d 00 75 62 69 62 6f 6f  74 76 6f 6c 3d 32 00 00  |}.ubibootvol=2..|
> 00000f40  00 ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> 
> Boot now U-Boot prints:
> 
> Loading Environment from SPI Flash... SF: Detected s25fl128l with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> *** Warning - bad CRC, using default environment
> 
> Reason is the above commit, as it only reads until \0\0
> is found, and assumes the rest of the Environment
> space is filled with 0x00, which is not the case when
> saving an Environment under linux with fw_setenv.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> I already posted a fix:
> https://lists.denx.de/pipermail/u-boot/2019-January/355148.html
> which I withdrawed:
> https://lists.denx.de/pipermail/u-boot/2019-January/355529.html
> 
> because I did not recognised the correlation between fw_setenv
> in linux context and saveenv in U-Boot context.
> 
> I think now, best is to revert this commit.
> 
> Comments?
> 
> This patch drops checkpatch warnings:
> env/sf.c:120: check: Alignment should match open parenthesis
> env/sf.c:224: check: Alignment should match open parenthesis
> env/sf.c:281: check: Alignment should match open parenthesis
> 
> but as it is a revert I did not correct them.
> 
>  env/sf.c | 56 +++++++++++---------------------------------------------
>  1 file changed, 11 insertions(+), 45 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index b3dec82c35..23cbad5d88 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -81,40 +81,6 @@ static int setup_flash_device(void)
>  	return 0;
>  }
>  
> -static int is_end(const char *addr, size_t size)
> -{
> -	/* The end of env variables is marked by '\0\0' */
> -	int i = 0;
> -
> -	for (i = 0; i < size - 1; ++i)
> -		if (addr[i] == 0x0 && addr[i + 1] == 0x0)
> -			return 1;
> -	return 0;
> -}
> -
> -static int spi_flash_read_env(struct spi_flash *flash, u32 offset, size_t len,
> -			      void *buf)
> -{
> -	u32 addr = 0;
> -	u32 page_size = flash->page_size;
> -
> -	memset(buf, 0x0, len);
> -	for (int i = 0; i < len / page_size; ++i) {
> -		int ret = spi_flash_read(flash, offset, page_size,
> -					 &((char *)buf)[addr]);
> -
> -		if (ret < 0)
> -			return ret;
> -
> -		if (is_end(&((char *)buf)[addr], page_size))
> -			return 0;
> -
> -		addr += page_size;
> -		offset += page_size;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_ENV_OFFSET_REDUND)
>  #ifdef CMD_SAVEENV
>  static int env_sf_save(void)
> @@ -150,8 +116,8 @@ static int env_sf_save(void)
>  			ret = -ENOMEM;
>  			goto done;
>  		}
> -		ret = spi_flash_read_env(env_flash, saved_offset,
> -					 saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset,
> +					saved_size, saved_buffer);
>  		if (ret)
>  			goto done;
>  	}
> @@ -217,10 +183,10 @@ static int env_sf_load(void)
>  	if (ret)
>  		goto out;
>  
> -	read1_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET,
> -					CONFIG_ENV_SIZE, tmp_env1);
> -	read2_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET_REDUND,
> -					CONFIG_ENV_SIZE, tmp_env2);
> +	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
> +				    CONFIG_ENV_SIZE, tmp_env1);
> +	read2_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND,
> +				    CONFIG_ENV_SIZE, tmp_env2);
>  
>  	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
>  				read2_fail);
> @@ -254,8 +220,8 @@ static int env_sf_save(void)
>  		if (!saved_buffer)
>  			goto done;
>  
> -		ret = spi_flash_read_env(env_flash, saved_offset,
> -					 saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset,
> +			saved_size, saved_buffer);
>  		if (ret)
>  			goto done;
>  	}
> @@ -311,10 +277,10 @@ static int env_sf_load(void)
>  	if (ret)
>  		goto out;
>  
> -	ret = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> -				 buf);
> +	ret = spi_flash_read(env_flash,
> +		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>  	if (ret) {
> -		set_default_env("spi_flash_read_env() failed", 0);
> +		set_default_env("spi_flash_read() failed", 0);
>  		goto err_read;
>  	}
>  
> -- 
> 2.17.2
> 

-- 
/Horatiu

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] Revert "env: add spi_flash_read_env function"
  2019-03-13 11:15 [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function" Heiko Schocher
  2019-03-13 12:21 ` Stefano Babic
  2019-03-14 12:53 ` Horatiu Vultur
@ 2019-03-14 15:36 ` Tom Rini
  2 siblings, 0 replies; 6+ messages in thread
From: Tom Rini @ 2019-03-14 15:36 UTC (permalink / raw)
  To: u-boot

On Wed, Mar 13, 2019 at 12:15:45PM +0100, Heiko Schocher wrote:

> This reverts commit 9a9d66f5eff0f443de4c2c6ca3e27771ed14b1b4.
> 
> because it breaks fw_setenv and U-Boot interworking, if
> U-Boot environment is stored in a SPI-NOR.
> 
> Reproduce it with:
> boot linux with empty Environment and store a variable
> with fw_setenv into it, the Environment is now filled
> with 0xff:
> 
> root at ckey5e:10:8e:~# hexdump -C /dev/mtd4
> 00000000  e9 e8 07 fa 01 62 6f 6f  74 63 6d 64 3d 72 75 6e  |.....bootcmd=run|
> [...]
> 00000f30  7d 00 75 62 69 62 6f 6f  74 76 6f 6c 3d 32 00 00  |}.ubibootvol=2..|
> 00000f40  00 ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> 
> Boot now U-Boot prints:
> 
> Loading Environment from SPI Flash... SF: Detected s25fl128l with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> *** Warning - bad CRC, using default environment
> 
> Reason is the above commit, as it only reads until \0\0
> is found, and assumes the rest of the Environment
> space is filled with 0x00, which is not the case when
> saving an Environment under linux with fw_setenv.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Acked-by: Stefano Babic <sbabic@denx.de>

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/20190314/017913bb/attachment.sig>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function"
  2019-03-14 12:53 ` Horatiu Vultur
@ 2019-03-15  4:34   ` Heiko Schocher
  2019-03-26 14:19     ` Horatiu Vultur
  0 siblings, 1 reply; 6+ messages in thread
From: Heiko Schocher @ 2019-03-15  4:34 UTC (permalink / raw)
  To: u-boot

Hello Horatiu,

Am 14.03.2019 um 13:53 schrieb Horatiu Vultur:
> Hi Heiko,
> 
> I managed to reproduce the issue that you described.

Fine, thanks for testing!

> Don't you think it is a little bit too harsh to remove the commit
> completely?

I am unsure here ...

> I am not sure but how many cases are where UBoot doesn't store anything
> in flash? And then lets the linux to update the flash.

I think that can happen very often. In my case for example we boot
SPL/U-Boot with usb loader, than an initsystem with kernel/dtb/ramdisk
and start swupdate for installing all sources (including setup
the Environment with fw_setenv)...

> Or is wouldn't better to update fw_setenv to set the entire flash to
> 0x0 when it detects that the CRC error? The same way how the saveenv it
> is doing in UBoot?

Indeed, just reproduced this on my board. Saving the Environment
with saveenv fills with 0x0 and saving with fw_setenv fills with
0xff.

This is no problem, until your optimization, and I think, we cannot
change this here without breaking a lot of boards, running fine.

I am here also on Stefanos side always to read the hole Environment
space, because we cannot be sure, with what the empty space is filled
up.

Back to your intention for this patch, stated in the commit message:
"""
     This is an optimization for large environments that contain few bytes
     environment variables. In this case it doesn't need to read the entire
     environment and only few pages.
"""

Hmm... why you use a big Environment, if you only have "few bytes" of
variables ?

Simply set CONFIG_ENV_SIZE to a smaller value than your CONFIG_ENV_SECT_SIZE

Is this not possible for you ?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function"
  2019-03-15  4:34   ` Heiko Schocher
@ 2019-03-26 14:19     ` Horatiu Vultur
  0 siblings, 0 replies; 6+ messages in thread
From: Horatiu Vultur @ 2019-03-26 14:19 UTC (permalink / raw)
  To: u-boot

Hi Heiko,

Sorry, for late reply. I have seen that this patch was accepted, so
I have only few things bellow.

The 03/15/2019 05:34, Heiko Schocher wrote:
> External E-Mail
> 
> 
> Hello Horatiu,
> 
> Am 14.03.2019 um 13:53 schrieb Horatiu Vultur:
> > Hi Heiko,
> > 
> > I managed to reproduce the issue that you described.
> 
> Fine, thanks for testing!
> 
> > Don't you think it is a little bit too harsh to remove the commit
> > completely?
> 
> I am unsure here ...
> 
> > I am not sure but how many cases are where UBoot doesn't store anything
> > in flash? And then lets the linux to update the flash.
> 
> I think that can happen very often. In my case for example we boot
> SPL/U-Boot with usb loader, than an initsystem with kernel/dtb/ramdisk
> and start swupdate for installing all sources (including setup
> the Environment with fw_setenv)...
> 
> > Or is wouldn't better to update fw_setenv to set the entire flash to
> > 0x0 when it detects that the CRC error? The same way how the saveenv it
> > is doing in UBoot?
> 
> Indeed, just reproduced this on my board. Saving the Environment
> with saveenv fills with 0x0 and saving with fw_setenv fills with
> 0xff.
> 
> This is no problem, until your optimization, and I think, we cannot
> change this here without breaking a lot of boards, running fine.
> 
> I am here also on Stefanos side always to read the hole Environment
> space, because we cannot be sure, with what the empty space is filled
> up.
> 
> Back to your intention for this patch, stated in the commit message:
> """
>     This is an optimization for large environments that contain few bytes
>     environment variables. In this case it doesn't need to read the entire
>     environment and only few pages.
> """
> 
> Hmm... why you use a big Environment, if you only have "few bytes" of
> variables ?

We choose a big Environment because different boards can have different
flashes which can have different page size. Therefore we chose for the
environmnet size to be the least common multiple.

> 
> Simply set CONFIG_ENV_SIZE to a smaller value than your CONFIG_ENV_SECT_SIZE
> 
> Is this not possible for you ?

Thanks for the suggestion. I have done few tests with this option and
improves the speed setting the CONFIG_ENV_SIZE to something smaller than
CONFIG_ENV_SECT_SIZE.

> 
> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

-- 
/Horatiu

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-03-26 14:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13 11:15 [U-Boot] [PATCH] Revert "env: add spi_flash_read_env function" Heiko Schocher
2019-03-13 12:21 ` Stefano Babic
2019-03-14 12:53 ` Horatiu Vultur
2019-03-15  4:34   ` Heiko Schocher
2019-03-26 14:19     ` Horatiu Vultur
2019-03-14 15:36 ` [U-Boot] " Tom Rini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.