[U-Boot] [PATCH] env: add flash_read function
The flash_read function is a wrapper over spi_flash_read, which enables the env to read multiple flash page size from flash until '\0\0' is read or the end of env partition is reached. Instead of reading the entire env size. When it reads '\0\0', it stops reading further the env and assumes that the rest of env is '\0'.
This is an optimization for large environments that contain few bytes of environment variables. In this case it doesn't need to read the entire environment and only few pages.
Signed-off-by: Horatiu Vultur horatiu.vultur@microchip.com --- env/sf.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 2e3c600..e1e1a94 100644 --- a/env/sf.c +++ b/env/sf.c @@ -81,6 +81,39 @@ static int setup_flash_device(void) return 0; }
+static int is_end(char *addr, int 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 flash_read(struct spi_flash *flash, u32 offset, int size, void *buf) +{ + u32 addr = 0; + u32 page_size = flash->page_size; + + memset(buf, 0x0, size); + for (int i = 0; i < size / 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) @@ -116,8 +149,8 @@ static int env_sf_save(void) ret = -ENOMEM; goto done; } - ret = spi_flash_read(env_flash, saved_offset, - saved_size, saved_buffer); + ret = flash_read(env_flash, saved_offset, + saved_size, saved_buffer); if (ret) goto done; } @@ -183,10 +216,10 @@ static int env_sf_load(void) if (ret) goto out;
- 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); + read1_fail = flash_read(env_flash, CONFIG_ENV_OFFSET, + CONFIG_ENV_SIZE, tmp_env1); + read2_fail = 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); @@ -220,8 +253,8 @@ static int env_sf_save(void) if (!saved_buffer) goto done;
- ret = spi_flash_read(env_flash, saved_offset, - saved_size, saved_buffer); + ret = flash_read(env_flash, saved_offset, + saved_size, saved_buffer); if (ret) goto done; } @@ -277,10 +310,9 @@ static int env_sf_load(void) if (ret) goto out;
- ret = spi_flash_read(env_flash, - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); + ret = flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); if (ret) { - set_default_env("spi_flash_read() failed", 0); + set_default_env("flash_read() failed", 0); goto err_read; }
Dear Horatiu,
In message 1543678222-15837-1-git-send-email-horatiu.vultur@microchip.com you wrote:
The flash_read function is a wrapper over spi_flash_read, which enables the env to read multiple flash page size from flash until '\0\0' is read or the end of env partition is reached. Instead of reading the entire env size. When it reads '\0\0', it stops reading further the env and assumes that the rest of env is '\0'.
This is an optimization for large environments that contain few bytes of environment variables. In this case it doesn't need to read the entire environment and only few pages.
Signed-off-by: Horatiu Vultur horatiu.vultur@microchip.com
env/sf.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 2e3c600..e1e1a94 100644 --- a/env/sf.c +++ b/env/sf.c @@ -81,6 +81,39 @@ static int setup_flash_device(void) return 0; }
+static int is_end(char *addr, int size)
That should be
(const char *addr, size_t size)
instead.
+static int flash_read(struct spi_flash *flash, u32 offset, int size, void *buf)
I think this should be
(const struct spi_flash *flash, u32 offset, size_t len, void *buf)
[Yes, for reasons of consistency with spi_flash_read() I would also rename "size" into "len".]
Also I recommend to chose another name for the function instead of "flash_read". At first glance this looks like a generalization over spi_flash_read() which supports other flash types than SPI flash as well - but it does not. Instead, it is a specialized version of spi_flash_read() tailored for environment reading only.
Maybe spi_flash_read_env() would be a more reasonable name?
+static int flash_read(struct spi_flash *flash, u32 offset, int size, void *buf) +{
- u32 addr = 0;
- u32 page_size = flash->page_size;
- memset(buf, 0x0, size);
- for (int i = 0; i < size / 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;
Is this correct? Is the read() function not supposed to return the number of read bytes?
Best regards,
Wolfgang Denk
Hi Wolfgang,
The 12/03/2018 17:08, Wolfgang Denk wrote:
Dear Horatiu,
In message 1543678222-15837-1-git-send-email-horatiu.vultur@microchip.com you wrote:
The flash_read function is a wrapper over spi_flash_read, which enables the env to read multiple flash page size from flash until '\0\0' is read or the end of env partition is reached. Instead of reading the entire env size. When it reads '\0\0', it stops reading further the env and assumes that the rest of env is '\0'.
This is an optimization for large environments that contain few bytes of environment variables. In this case it doesn't need to read the entire environment and only few pages.
Signed-off-by: Horatiu Vultur horatiu.vultur@microchip.com
env/sf.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 2e3c600..e1e1a94 100644 --- a/env/sf.c +++ b/env/sf.c @@ -81,6 +81,39 @@ static int setup_flash_device(void) return 0; }
+static int is_end(char *addr, int size)
That should be
(const char *addr, size_t size)
instead.
I will change this in the next version.
+static int flash_read(struct spi_flash *flash, u32 offset, int size, void *buf)
I think this should be
(const struct spi_flash *flash, u32 offset, size_t len, void *buf)
[Yes, for reasons of consistency with spi_flash_read() I would also rename "size" into "len".]
The same here.
Also I recommend to chose another name for the function instead of "flash_read". At first glance this looks like a generalization over spi_flash_read() which supports other flash types than SPI flash as well - but it does not. Instead, it is a specialized version of spi_flash_read() tailored for environment reading only.
Maybe spi_flash_read_env() would be a more reasonable name?
Yes spi_flash_read_env is a better name. I will update it in the next version.
+static int flash_read(struct spi_flash *flash, u32 offset, int size, void *buf) +{
- u32 addr = 0;
- u32 page_size = flash->page_size;
- memset(buf, 0x0, size);
- for (int i = 0; i < size / 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;
Is this correct? Is the read() function not supposed to return the number of read bytes?
It is true that C read function returns the number of bytes read, but the intention of the function flash_read was to mimic the function spi_flash_read, which returns 0 if OK otherwise an error code. And that's the reason why the function flash_read returns 0 and not number of bytes read.
It would not be a problem for me to change the function flash_read to return the number of bytes read.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de The optimum committee has no members. - Norman Augustine
participants (2)
-
Horatiu Vultur -
Wolfgang Denk