* [PATCH v3 1/1] env: sf: single function env_sf_save()
@ 2021-02-02 8:21 Harry Waschkeit
2021-02-02 9:30 ` Stefan Roese
2021-03-01 16:39 ` Harry Waschkeit
0 siblings, 2 replies; 11+ messages in thread
From: Harry Waschkeit @ 2021-02-02 8:21 UTC (permalink / raw)
To: u-boot
?Instead of implementing redundant environments in two very similar
functions env_sf_save(), handle redundancy in one function, placing the
few differences in appropriate pre-compiler sections depending on config
option CONFIG_ENV_OFFSET_REDUND.
Additionally, several checkpatch complaints were addressed.
This patch is in preparation for adding support for env erase.
Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
Reviewed-by: Stefan Roese <sr@denx.de>
---
Change in v3:
- no change in patch, only added "reviewed-by" to commit log
Change in v2:
- remove one more #ifdef, instead take advantage of compiler attribute
__maybe_unused for one variable used only in case of redundant
environments
env/sf.c | 130 ++++++++++++++++++-------------------------------------
1 file changed, 41 insertions(+), 89 deletions(-)
diff --git a/env/sf.c b/env/sf.c
index 937778aa37..199114fd3b 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -66,13 +66,14 @@ static int setup_flash_device(void)
return 0;
}
-#if defined(CONFIG_ENV_OFFSET_REDUND)
static int env_sf_save(void)
{
env_t env_new;
- char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
+ char *saved_buffer = NULL;
u32 saved_size, saved_offset, sector;
+ ulong offset;
int ret;
+ __maybe_unused char flag = ENV_REDUND_OBSOLETE;
ret = setup_flash_device();
if (ret)
@@ -81,27 +82,33 @@ static int env_sf_save(void)
ret = env_export(&env_new);
if (ret)
return -EIO;
- env_new.flags = ENV_REDUND_ACTIVE;
- if (gd->env_valid == ENV_VALID) {
- env_new_offset = CONFIG_ENV_OFFSET_REDUND;
- env_offset = CONFIG_ENV_OFFSET;
- } else {
- env_new_offset = CONFIG_ENV_OFFSET;
- env_offset = CONFIG_ENV_OFFSET_REDUND;
- }
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+ env_new.flags = ENV_REDUND_ACTIVE;
+
+ if (gd->env_valid == ENV_VALID) {
+ env_new_offset = CONFIG_ENV_OFFSET_REDUND;
+ env_offset = CONFIG_ENV_OFFSET;
+ } else {
+ env_new_offset = CONFIG_ENV_OFFSET;
+ env_offset = CONFIG_ENV_OFFSET_REDUND;
+ }
+ offset = env_new_offset;
+#else
+ offset = CONFIG_ENV_OFFSET;
+#endif
/* Is the sector larger than the env (i.e. embedded) */
if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
- saved_offset = env_new_offset + CONFIG_ENV_SIZE;
+ saved_offset = offset + CONFIG_ENV_SIZE;
saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
if (!saved_buffer) {
ret = -ENOMEM;
goto done;
}
- ret = spi_flash_read(env_flash, saved_offset,
- saved_size, saved_buffer);
+ ret = spi_flash_read(env_flash, saved_offset, saved_size,
+ saved_buffer);
if (ret)
goto done;
}
@@ -109,35 +116,39 @@ static int env_sf_save(void)
sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
puts("Erasing SPI flash...");
- ret = spi_flash_erase(env_flash, env_new_offset,
- sector * CONFIG_ENV_SECT_SIZE);
+ ret = spi_flash_erase(env_flash, offset,
+ sector * CONFIG_ENV_SECT_SIZE);
if (ret)
goto done;
puts("Writing to SPI flash...");
- ret = spi_flash_write(env_flash, env_new_offset,
- CONFIG_ENV_SIZE, &env_new);
+ ret = spi_flash_write(env_flash, offset,
+ CONFIG_ENV_SIZE, &env_new);
if (ret)
goto done;
if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
- ret = spi_flash_write(env_flash, saved_offset,
- saved_size, saved_buffer);
+ ret = spi_flash_write(env_flash, saved_offset, saved_size,
+ saved_buffer);
if (ret)
goto done;
}
- ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
- sizeof(env_new.flags), &flag);
- if (ret)
- goto done;
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+ ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
+ sizeof(env_new.flags), &flag);
+ if (ret)
+ goto done;
- puts("done\n");
+ puts("done\n");
- gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+ gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
- printf("Valid environment: %d\n", (int)gd->env_valid);
+ printf("Valid environment: %d\n", (int)gd->env_valid);
+#else
+ puts("done\n");
+#endif
done:
if (saved_buffer)
@@ -146,6 +157,7 @@ static int env_sf_save(void)
return ret;
}
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
static int env_sf_load(void)
{
int ret;
@@ -183,66 +195,6 @@ out:
return ret;
}
#else
-static int env_sf_save(void)
-{
- u32 saved_size, saved_offset, sector;
- char *saved_buffer = NULL;
- int ret = 1;
- env_t env_new;
-
- ret = setup_flash_device();
- if (ret)
- return ret;
-
- /* Is the sector larger than the env (i.e. embedded) */
- if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
- saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
- saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
- saved_buffer = malloc(saved_size);
- if (!saved_buffer)
- goto done;
-
- ret = spi_flash_read(env_flash, saved_offset,
- saved_size, saved_buffer);
- if (ret)
- goto done;
- }
-
- ret = env_export(&env_new);
- if (ret)
- goto done;
-
- sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
-
- puts("Erasing SPI flash...");
- ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
- sector * CONFIG_ENV_SECT_SIZE);
- if (ret)
- goto done;
-
- puts("Writing to SPI flash...");
- ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
- CONFIG_ENV_SIZE, &env_new);
- if (ret)
- goto done;
-
- if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
- ret = spi_flash_write(env_flash, saved_offset,
- saved_size, saved_buffer);
- if (ret)
- goto done;
- }
-
- ret = 0;
- puts("done\n");
-
- done:
- if (saved_buffer)
- free(saved_buffer);
-
- return ret;
-}
-
static int env_sf_load(void)
{
int ret;
@@ -258,8 +210,8 @@ static int env_sf_load(void)
if (ret)
goto out;
- ret = spi_flash_read(env_flash,
- CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
+ ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
+ buf);
if (ret) {
env_set_default("spi_flash_read() failed", 0);
goto err_read;
@@ -292,7 +244,7 @@ static int env_sf_init(void)
env_t *env_ptr = (env_t *)env_sf_get_env_addr();
if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
- gd->env_addr = (ulong)&(env_ptr->data);
+ gd->env_addr = (ulong)&env_ptr->data;
gd->env_valid = 1;
} else {
gd->env_addr = (ulong)&default_environment[0];
--
2.29.0
duagon Germany GmbH
Neuwieder Stra?e 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-02-02 8:21 [PATCH v3 1/1] env: sf: single function env_sf_save() Harry Waschkeit
@ 2021-02-02 9:30 ` Stefan Roese
2021-02-02 14:43 ` Harry Waschkeit
2021-03-01 16:39 ` Harry Waschkeit
1 sibling, 1 reply; 11+ messages in thread
From: Stefan Roese @ 2021-02-02 9:30 UTC (permalink / raw)
To: u-boot
On 02.02.21 09:21, Harry Waschkeit wrote:
> ?Instead of implementing redundant environments in two very similar
> functions env_sf_save(), handle redundancy in one function, placing the
> few differences in appropriate pre-compiler sections depending on config
> option CONFIG_ENV_OFFSET_REDUND.
>
> Additionally, several checkpatch complaints were addressed.
>
> This patch is in preparation for adding support for env erase.
>
> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> ---
> Change in v3:
> - no change in patch, only added "reviewed-by" to commit log
JFYI:
No need to re-send this patch with this added RB tag, as I already did
send a new RB to the last mail as reply. Patchwork collects these tags
when sent to the list. So you only need to include them, if you send
a new patch version.
Thanks,
Stefan
> Change in v2:
> - remove one more #ifdef, instead take advantage of compiler attribute
> __maybe_unused for one variable used only in case of redundant
> environments
>
> env/sf.c | 130 ++++++++++++++++++-------------------------------------
> 1 file changed, 41 insertions(+), 89 deletions(-)
>
> diff --git a/env/sf.c b/env/sf.c
> index 937778aa37..199114fd3b 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
> return 0;
> }
>
> -#if defined(CONFIG_ENV_OFFSET_REDUND)
> static int env_sf_save(void)
> {
> env_t env_new;
> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
> + char *saved_buffer = NULL;
> u32 saved_size, saved_offset, sector;
> + ulong offset;
> int ret;
> + __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>
> ret = setup_flash_device();
> if (ret)
> @@ -81,27 +82,33 @@ static int env_sf_save(void)
> ret = env_export(&env_new);
> if (ret)
> return -EIO;
> - env_new.flags = ENV_REDUND_ACTIVE;
>
> - if (gd->env_valid == ENV_VALID) {
> - env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> - env_offset = CONFIG_ENV_OFFSET;
> - } else {
> - env_new_offset = CONFIG_ENV_OFFSET;
> - env_offset = CONFIG_ENV_OFFSET_REDUND;
> - }
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> + env_new.flags = ENV_REDUND_ACTIVE;
> +
> + if (gd->env_valid == ENV_VALID) {
> + env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> + env_offset = CONFIG_ENV_OFFSET;
> + } else {
> + env_new_offset = CONFIG_ENV_OFFSET;
> + env_offset = CONFIG_ENV_OFFSET_REDUND;
> + }
> + offset = env_new_offset;
> +#else
> + offset = CONFIG_ENV_OFFSET;
> +#endif
>
> /* Is the sector larger than the env (i.e. embedded) */
> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> - saved_offset = env_new_offset + CONFIG_ENV_SIZE;
> + saved_offset = offset + CONFIG_ENV_SIZE;
> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
> if (!saved_buffer) {
> ret = -ENOMEM;
> goto done;
> }
> - ret = spi_flash_read(env_flash, saved_offset,
> - saved_size, saved_buffer);
> + ret = spi_flash_read(env_flash, saved_offset, saved_size,
> + saved_buffer);
> if (ret)
> goto done;
> }
> @@ -109,35 +116,39 @@ static int env_sf_save(void)
> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>
> puts("Erasing SPI flash...");
> - ret = spi_flash_erase(env_flash, env_new_offset,
> - sector * CONFIG_ENV_SECT_SIZE);
> + ret = spi_flash_erase(env_flash, offset,
> + sector * CONFIG_ENV_SECT_SIZE);
> if (ret)
> goto done;
>
> puts("Writing to SPI flash...");
>
> - ret = spi_flash_write(env_flash, env_new_offset,
> - CONFIG_ENV_SIZE, &env_new);
> + ret = spi_flash_write(env_flash, offset,
> + CONFIG_ENV_SIZE, &env_new);
> if (ret)
> goto done;
>
> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> - ret = spi_flash_write(env_flash, saved_offset,
> - saved_size, saved_buffer);
> + ret = spi_flash_write(env_flash, saved_offset, saved_size,
> + saved_buffer);
> if (ret)
> goto done;
> }
>
> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> - sizeof(env_new.flags), &flag);
> - if (ret)
> - goto done;
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> + sizeof(env_new.flags), &flag);
> + if (ret)
> + goto done;
>
> - puts("done\n");
> + puts("done\n");
>
> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>
> - printf("Valid environment: %d\n", (int)gd->env_valid);
> + printf("Valid environment: %d\n", (int)gd->env_valid);
> +#else
> + puts("done\n");
> +#endif
>
> done:
> if (saved_buffer)
> @@ -146,6 +157,7 @@ static int env_sf_save(void)
> return ret;
> }
>
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> static int env_sf_load(void)
> {
> int ret;
> @@ -183,66 +195,6 @@ out:
> return ret;
> }
> #else
> -static int env_sf_save(void)
> -{
> - u32 saved_size, saved_offset, sector;
> - char *saved_buffer = NULL;
> - int ret = 1;
> - env_t env_new;
> -
> - ret = setup_flash_device();
> - if (ret)
> - return ret;
> -
> - /* Is the sector larger than the env (i.e. embedded) */
> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
> - saved_buffer = malloc(saved_size);
> - if (!saved_buffer)
> - goto done;
> -
> - ret = spi_flash_read(env_flash, saved_offset,
> - saved_size, saved_buffer);
> - if (ret)
> - goto done;
> - }
> -
> - ret = env_export(&env_new);
> - if (ret)
> - goto done;
> -
> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
> -
> - puts("Erasing SPI flash...");
> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
> - sector * CONFIG_ENV_SECT_SIZE);
> - if (ret)
> - goto done;
> -
> - puts("Writing to SPI flash...");
> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
> - CONFIG_ENV_SIZE, &env_new);
> - if (ret)
> - goto done;
> -
> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> - ret = spi_flash_write(env_flash, saved_offset,
> - saved_size, saved_buffer);
> - if (ret)
> - goto done;
> - }
> -
> - ret = 0;
> - puts("done\n");
> -
> - done:
> - if (saved_buffer)
> - free(saved_buffer);
> -
> - return ret;
> -}
> -
> static int env_sf_load(void)
> {
> int ret;
> @@ -258,8 +210,8 @@ static int env_sf_load(void)
> if (ret)
> goto out;
>
> - ret = spi_flash_read(env_flash,
> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> + buf);
> if (ret) {
> env_set_default("spi_flash_read() failed", 0);
> goto err_read;
> @@ -292,7 +244,7 @@ static int env_sf_init(void)
> env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>
> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
> - gd->env_addr = (ulong)&(env_ptr->data);
> + gd->env_addr = (ulong)&env_ptr->data;
> gd->env_valid = 1;
> } else {
> gd->env_addr = (ulong)&default_environment[0];
>
Viele Gr??e,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-02-02 9:30 ` Stefan Roese
@ 2021-02-02 14:43 ` Harry Waschkeit
2021-02-02 14:54 ` Stefan Roese
0 siblings, 1 reply; 11+ messages in thread
From: Harry Waschkeit @ 2021-02-02 14:43 UTC (permalink / raw)
To: u-boot
?
On 02.02.21 10:30, Stefan Roese wrote:
> On 02.02.21 09:21, Harry Waschkeit wrote:
>> ?Instead of implementing redundant environments in two very similar
>> functions env_sf_save(), handle redundancy in one function, placing the
>> few differences in appropriate pre-compiler sections depending on config
>> option CONFIG_ENV_OFFSET_REDUND.
>>
>> Additionally, several checkpatch complaints were addressed.
>>
>> This patch is in preparation for adding support for env erase.
>>
>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>> ---
>> Change in v3:
>> ? - no change in patch, only added "reviewed-by" to commit log
>
> JFYI:
> No need to re-send this patch with this added RB tag, as I already did
> send a new RB to the last mail as reply. Patchwork collects these tags
> when sent to the list. So you only need to include them, if you send
> a new patch version.
thanks for this hint, obviously I step into it all the time ...
Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
Back on topic: I guess that was all I needed to do so that the patch will get merged when its time comes.
If not, please let me know.
Thanks,
Harry
> Thanks,
> Stefan
>
>> Change in v2:
>> ? - remove one more #ifdef, instead take advantage of compiler attribute
>> ??? __maybe_unused for one variable used only in case of redundant
>> ??? environments
>>
>> ? env/sf.c | 130 ++++++++++++++++++-------------------------------------
>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>
>> diff --git a/env/sf.c b/env/sf.c
>> index 937778aa37..199114fd3b 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>> ????? return 0;
>> ? }
>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>> ? static int env_sf_save(void)
>> ? {
>> ????? env_t??? env_new;
>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>> +??? char??? *saved_buffer = NULL;
>> ????? u32??? saved_size, saved_offset, sector;
>> +??? ulong??? offset;
>> ????? int??? ret;
>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>> ????? ret = setup_flash_device();
>> ????? if (ret)
>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>> ????? ret = env_export(&env_new);
>> ????? if (ret)
>> ????????? return -EIO;
>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>> -??? if (gd->env_valid == ENV_VALID) {
>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> -??????? env_offset = CONFIG_ENV_OFFSET;
>> -??? } else {
>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>> -??? }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>> +
>> +??????? if (gd->env_valid == ENV_VALID) {
>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>> +??????? } else {
>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>> +??????? }
>> +??????? offset = env_new_offset;
>> +#else
>> +??????? offset = CONFIG_ENV_OFFSET;
>> +#endif
>> ????? /* Is the sector larger than the env (i.e. embedded) */
>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>> ????????? if (!saved_buffer) {
>> ????????????? ret = -ENOMEM;
>> ????????????? goto done;
>> ????????? }
>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>> -??????????????????? saved_size, saved_buffer);
>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>> +???????????????????? saved_buffer);
>> ????????? if (ret)
>> ????????????? goto done;
>> ????? }
>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> ????? puts("Erasing SPI flash...");
>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>> +??? ret = spi_flash_erase(env_flash, offset,
>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>> ????? if (ret)
>> ????????? goto done;
>> ????? puts("Writing to SPI flash...");
>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>> -??????? CONFIG_ENV_SIZE, &env_new);
>> +??? ret = spi_flash_write(env_flash, offset,
>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>> ????? if (ret)
>> ????????? goto done;
>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>> -??????????????????? saved_size, saved_buffer);
>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>> +????????????????????? saved_buffer);
>> ????????? if (ret)
>> ????????????? goto done;
>> ????? }
>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> -??????????????? sizeof(env_new.flags), &flag);
>> -??? if (ret)
>> -??????? goto done;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> +????????????????????? sizeof(env_new.flags), &flag);
>> +??????? if (ret)
>> +??????????? goto done;
>> -??? puts("done\n");
>> +??????? puts("done\n");
>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>> +#else
>> +??????? puts("done\n");
>> +#endif
>> ?? done:
>> ????? if (saved_buffer)
>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>> ????? return ret;
>> ? }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> ? static int env_sf_load(void)
>> ? {
>> ????? int ret;
>> @@ -183,66 +195,6 @@ out:
>> ????? return ret;
>> ? }
>> ? #else
>> -static int env_sf_save(void)
>> -{
>> -??? u32??? saved_size, saved_offset, sector;
>> -??? char??? *saved_buffer = NULL;
>> -??? int??? ret = 1;
>> -??? env_t??? env_new;
>> -
>> -??? ret = setup_flash_device();
>> -??? if (ret)
>> -??????? return ret;
>> -
>> -??? /* Is the sector larger than the env (i.e. embedded) */
>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>> -??????? saved_buffer = malloc(saved_size);
>> -??????? if (!saved_buffer)
>> -??????????? goto done;
>> -
>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>> -??????????? saved_size, saved_buffer);
>> -??????? if (ret)
>> -??????????? goto done;
>> -??? }
>> -
>> -??? ret = env_export(&env_new);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> -
>> -??? puts("Erasing SPI flash...");
>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? puts("Writing to SPI flash...");
>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>> -??????? CONFIG_ENV_SIZE, &env_new);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>> -??????????? saved_size, saved_buffer);
>> -??????? if (ret)
>> -??????????? goto done;
>> -??? }
>> -
>> -??? ret = 0;
>> -??? puts("done\n");
>> -
>> - done:
>> -??? if (saved_buffer)
>> -??????? free(saved_buffer);
>> -
>> -??? return ret;
>> -}
>> -
>> ? static int env_sf_load(void)
>> ? {
>> ????? int ret;
>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>> ????? if (ret)
>> ????????? goto out;
>> -??? ret = spi_flash_read(env_flash,
>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>> +???????????????? buf);
>> ????? if (ret) {
>> ????????? env_set_default("spi_flash_read() failed", 0);
>> ????????? goto err_read;
>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>> ????????? gd->env_valid??? = 1;
>> ????? } else {
>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>
>
>
> Viele Gr??e,
> Stefan
>
--
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Stra?e 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-02-02 14:43 ` Harry Waschkeit
@ 2021-02-02 14:54 ` Stefan Roese
2021-02-02 17:09 ` Harry Waschkeit
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Roese @ 2021-02-02 14:54 UTC (permalink / raw)
To: u-boot
On 02.02.21 15:43, Harry Waschkeit wrote:
> ?
> On 02.02.21 10:30, Stefan Roese wrote:
>> On 02.02.21 09:21, Harry Waschkeit wrote:
>>> ?Instead of implementing redundant environments in two very similar
>>> functions env_sf_save(), handle redundancy in one function, placing the
>>> few differences in appropriate pre-compiler sections depending on config
>>> option CONFIG_ENV_OFFSET_REDUND.
>>>
>>> Additionally, several checkpatch complaints were addressed.
>>>
>>> This patch is in preparation for adding support for env erase.
>>>
>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>> ---
>>> Change in v3:
>>> ? - no change in patch, only added "reviewed-by" to commit log
>>
>> JFYI:
>> No need to re-send this patch with this added RB tag, as I already did
>> send a new RB to the last mail as reply. Patchwork collects these tags
>> when sent to the list. So you only need to include them, if you send
>> a new patch version.
>
> thanks for this hint, obviously I step into it all the time ...
>
> Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
>
> Back on topic: I guess that was all I needed to do so that the patch
> will get merged when its time comes.
Yes, now we (you) need a bit of patience, so that other people might
review this patch as well. And usually it will get handled after some
time (depending on the development stage of U-Boot, merge window open
or shortly before release etc).
It does not hurt of course to check this from time to time and
"trigger" the maintainer of the subsystem or the custodian this
patch is delegated to nothing is happening here for a too long
time (like more than 1 month).
Thanks,
Stefan
> If not, please let me know.
>
> Thanks,
> Harry
>
>> Thanks,
>> Stefan
>>
>>> Change in v2:
>>> ? - remove one more #ifdef, instead take advantage of compiler attribute
>>> ??? __maybe_unused for one variable used only in case of redundant
>>> ??? environments
>>>
>>> ? env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 937778aa37..199114fd3b 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>>> ????? return 0;
>>> ? }
>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>> ? static int env_sf_save(void)
>>> ? {
>>> ????? env_t??? env_new;
>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>> +??? char??? *saved_buffer = NULL;
>>> ????? u32??? saved_size, saved_offset, sector;
>>> +??? ulong??? offset;
>>> ????? int??? ret;
>>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>> ????? ret = setup_flash_device();
>>> ????? if (ret)
>>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>>> ????? ret = env_export(&env_new);
>>> ????? if (ret)
>>> ????????? return -EIO;
>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>> -??? if (gd->env_valid == ENV_VALID) {
>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>> -??? } else {
>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> -??? }
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>> +
>>> +??????? if (gd->env_valid == ENV_VALID) {
>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>> +??????? } else {
>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +??????? }
>>> +??????? offset = env_new_offset;
>>> +#else
>>> +??????? offset = CONFIG_ENV_OFFSET;
>>> +#endif
>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>> ????????? if (!saved_buffer) {
>>> ????????????? ret = -ENOMEM;
>>> ????????????? goto done;
>>> ????????? }
>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>> -??????????????????? saved_size, saved_buffer);
>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>> +???????????????????? saved_buffer);
>>> ????????? if (ret)
>>> ????????????? goto done;
>>> ????? }
>>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> ????? puts("Erasing SPI flash...");
>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>> +??? ret = spi_flash_erase(env_flash, offset,
>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>> ????? if (ret)
>>> ????????? goto done;
>>> ????? puts("Writing to SPI flash...");
>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>> +??? ret = spi_flash_write(env_flash, offset,
>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>> ????? if (ret)
>>> ????????? goto done;
>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>> -??????????????????? saved_size, saved_buffer);
>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>> +????????????????????? saved_buffer);
>>> ????????? if (ret)
>>> ????????????? goto done;
>>> ????? }
>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t,
>>> flags),
>>> -??????????????? sizeof(env_new.flags), &flag);
>>> -??? if (ret)
>>> -??????? goto done;
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??????? ret = spi_flash_write(env_flash, env_offset +
>>> offsetof(env_t, flags),
>>> +????????????????????? sizeof(env_new.flags), &flag);
>>> +??????? if (ret)
>>> +??????????? goto done;
>>> -??? puts("done\n");
>>> +??????? puts("done\n");
>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID :
>>> ENV_REDUND;
>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID :
>>> ENV_REDUND;
>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>> +#else
>>> +??????? puts("done\n");
>>> +#endif
>>> ?? done:
>>> ????? if (saved_buffer)
>>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>>> ????? return ret;
>>> ? }
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> ? static int env_sf_load(void)
>>> ? {
>>> ????? int ret;
>>> @@ -183,66 +195,6 @@ out:
>>> ????? return ret;
>>> ? }
>>> ? #else
>>> -static int env_sf_save(void)
>>> -{
>>> -??? u32??? saved_size, saved_offset, sector;
>>> -??? char??? *saved_buffer = NULL;
>>> -??? int??? ret = 1;
>>> -??? env_t??? env_new;
>>> -
>>> -??? ret = setup_flash_device();
>>> -??? if (ret)
>>> -??????? return ret;
>>> -
>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>> -??????? saved_buffer = malloc(saved_size);
>>> -??????? if (!saved_buffer)
>>> -??????????? goto done;
>>> -
>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>> -??????????? saved_size, saved_buffer);
>>> -??????? if (ret)
>>> -??????????? goto done;
>>> -??? }
>>> -
>>> -??? ret = env_export(&env_new);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> -
>>> -??? puts("Erasing SPI flash...");
>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? puts("Writing to SPI flash...");
>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>> -??????????? saved_size, saved_buffer);
>>> -??????? if (ret)
>>> -??????????? goto done;
>>> -??? }
>>> -
>>> -??? ret = 0;
>>> -??? puts("done\n");
>>> -
>>> - done:
>>> -??? if (saved_buffer)
>>> -??????? free(saved_buffer);
>>> -
>>> -??? return ret;
>>> -}
>>> -
>>> ? static int env_sf_load(void)
>>> ? {
>>> ????? int ret;
>>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>>> ????? if (ret)
>>> ????????? goto out;
>>> -??? ret = spi_flash_read(env_flash,
>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>> +???????????????? buf);
>>> ????? if (ret) {
>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>> ????????? goto err_read;
>>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>> ????????? gd->env_valid??? = 1;
>>> ????? } else {
>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>
>>
>>
>> Viele Gr??e,
>> Stefan
>>
>
>
Viele Gr??e,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-02-02 14:54 ` Stefan Roese
@ 2021-02-02 17:09 ` Harry Waschkeit
2021-02-03 6:10 ` Stefan Roese
0 siblings, 1 reply; 11+ messages in thread
From: Harry Waschkeit @ 2021-02-02 17:09 UTC (permalink / raw)
To: u-boot
On 02.02.21 15:54, Stefan Roese wrote:
> On 02.02.21 15:43, Harry Waschkeit wrote:
>> ?
>> On 02.02.21 10:30, Stefan Roese wrote:
>>> On 02.02.21 09:21, Harry Waschkeit wrote:
>>>> ?Instead of implementing redundant environments in two very similar
>>>> functions env_sf_save(), handle redundancy in one function, placing the
>>>> few differences in appropriate pre-compiler sections depending on config
>>>> option CONFIG_ENV_OFFSET_REDUND.
>>>>
>>>> Additionally, several checkpatch complaints were addressed.
>>>>
>>>> This patch is in preparation for adding support for env erase.
>>>>
>>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>> ---
>>>> Change in v3:
>>>> ? - no change in patch, only added "reviewed-by" to commit log
>>>
>>> JFYI:
>>> No need to re-send this patch with this added RB tag, as I already did
>>> send a new RB to the last mail as reply. Patchwork collects these tags
>>> when sent to the list. So you only need to include them, if you send
>>> a new patch version.
>>
>> thanks for this hint, obviously I step into it all the time ...
>>
>> Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
>>
>> Back on topic: I guess that was all I needed to do so that the patch will get merged when its time comes.
>
> Yes, now we (you) need a bit of patience, so that other people might
> review this patch as well. And usually it will get handled after some
> time (depending on the development stage of U-Boot, merge window open
> or shortly before release etc).
Ok, no problem with that.
That also means that I should wait with submission of the other patch (adding "env erase" support) until this one is merged.
Otherwise the patch would need to be significantly different ...
Hmm, I guess it would have been better to not send that patch standalone but instead as a first one in a two-patch series where the second one adds the new functionality on top of the clean-up.
Anyway, something to keep in mind for next time :-)
> It does not hurt of course to check this from time to time and
> "trigger" the maintainer of the subsystem or the custodian this
> patch is delegated to nothing is happening here for a too long
> time (like more than 1 month).
Alright, good to know :-)
Thanks,
Harry
> Thanks,
> Stefan
>
>> If not, please let me know.
>>
>> Thanks,
>> Harry
>>
>>> Thanks,
>>> Stefan
>>>
>>>> Change in v2:
>>>> ? - remove one more #ifdef, instead take advantage of compiler attribute
>>>> ??? __maybe_unused for one variable used only in case of redundant
>>>> ??? environments
>>>>
>>>> ? env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/env/sf.c b/env/sf.c
>>>> index 937778aa37..199114fd3b 100644
>>>> --- a/env/sf.c
>>>> +++ b/env/sf.c
>>>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>>>> ????? return 0;
>>>> ? }
>>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>>> ? static int env_sf_save(void)
>>>> ? {
>>>> ????? env_t??? env_new;
>>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>>> +??? char??? *saved_buffer = NULL;
>>>> ????? u32??? saved_size, saved_offset, sector;
>>>> +??? ulong??? offset;
>>>> ????? int??? ret;
>>>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>>> ????? ret = setup_flash_device();
>>>> ????? if (ret)
>>>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>>>> ????? ret = env_export(&env_new);
>>>> ????? if (ret)
>>>> ????????? return -EIO;
>>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>> -??? if (gd->env_valid == ENV_VALID) {
>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>>> -??? } else {
>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> -??? }
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>> +
>>>> +??????? if (gd->env_valid == ENV_VALID) {
>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>>> +??????? } else {
>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> +??????? }
>>>> +??????? offset = env_new_offset;
>>>> +#else
>>>> +??????? offset = CONFIG_ENV_OFFSET;
>>>> +#endif
>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>> ????????? if (!saved_buffer) {
>>>> ????????????? ret = -ENOMEM;
>>>> ????????????? goto done;
>>>> ????????? }
>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>> -??????????????????? saved_size, saved_buffer);
>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>> +???????????????????? saved_buffer);
>>>> ????????? if (ret)
>>>> ????????????? goto done;
>>>> ????? }
>>>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> ????? puts("Erasing SPI flash...");
>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>> ????? if (ret)
>>>> ????????? goto done;
>>>> ????? puts("Writing to SPI flash...");
>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>> ????? if (ret)
>>>> ????????? goto done;
>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>> -??????????????????? saved_size, saved_buffer);
>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>> +????????????????????? saved_buffer);
>>>> ????????? if (ret)
>>>> ????????????? goto done;
>>>> ????? }
>>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>> +??????? if (ret)
>>>> +??????????? goto done;
>>>> -??? puts("done\n");
>>>> +??????? puts("done\n");
>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> +#else
>>>> +??????? puts("done\n");
>>>> +#endif
>>>> ?? done:
>>>> ????? if (saved_buffer)
>>>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>>>> ????? return ret;
>>>> ? }
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> ? static int env_sf_load(void)
>>>> ? {
>>>> ????? int ret;
>>>> @@ -183,66 +195,6 @@ out:
>>>> ????? return ret;
>>>> ? }
>>>> ? #else
>>>> -static int env_sf_save(void)
>>>> -{
>>>> -??? u32??? saved_size, saved_offset, sector;
>>>> -??? char??? *saved_buffer = NULL;
>>>> -??? int??? ret = 1;
>>>> -??? env_t??? env_new;
>>>> -
>>>> -??? ret = setup_flash_device();
>>>> -??? if (ret)
>>>> -??????? return ret;
>>>> -
>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>> -??????? saved_buffer = malloc(saved_size);
>>>> -??????? if (!saved_buffer)
>>>> -??????????? goto done;
>>>> -
>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>> -??????????? saved_size, saved_buffer);
>>>> -??????? if (ret)
>>>> -??????????? goto done;
>>>> -??? }
>>>> -
>>>> -??? ret = env_export(&env_new);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> -
>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> -
>>>> -??? puts("Erasing SPI flash...");
>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> -
>>>> -??? puts("Writing to SPI flash...");
>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> -
>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>> -??????????? saved_size, saved_buffer);
>>>> -??????? if (ret)
>>>> -??????????? goto done;
>>>> -??? }
>>>> -
>>>> -??? ret = 0;
>>>> -??? puts("done\n");
>>>> -
>>>> - done:
>>>> -??? if (saved_buffer)
>>>> -??????? free(saved_buffer);
>>>> -
>>>> -??? return ret;
>>>> -}
>>>> -
>>>> ? static int env_sf_load(void)
>>>> ? {
>>>> ????? int ret;
>>>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>>>> ????? if (ret)
>>>> ????????? goto out;
>>>> -??? ret = spi_flash_read(env_flash,
>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>>> +???????????????? buf);
>>>> ????? if (ret) {
>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>> ????????? goto err_read;
>>>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>> ????????? gd->env_valid??? = 1;
>>>> ????? } else {
>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>
>>>
>>>
>>> Viele Gr??e,
>>> Stefan
>>>
>>
>>
>
>
> Viele Gr??e,
> Stefan
>
--
Harry Waschkeit - Software Engineer
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-02-02 17:09 ` Harry Waschkeit
@ 2021-02-03 6:10 ` Stefan Roese
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Roese @ 2021-02-03 6:10 UTC (permalink / raw)
To: u-boot
On 02.02.21 18:09, Harry Waschkeit wrote:
> On 02.02.21 15:54, Stefan Roese wrote:
>> On 02.02.21 15:43, Harry Waschkeit wrote:
>>> ?
>>> On 02.02.21 10:30, Stefan Roese wrote:
>>>> On 02.02.21 09:21, Harry Waschkeit wrote:
>>>>> ?Instead of implementing redundant environments in two very similar
>>>>> functions env_sf_save(), handle redundancy in one function, placing
>>>>> the
>>>>> few differences in appropriate pre-compiler sections depending on
>>>>> config
>>>>> option CONFIG_ENV_OFFSET_REDUND.
>>>>>
>>>>> Additionally, several checkpatch complaints were addressed.
>>>>>
>>>>> This patch is in preparation for adding support for env erase.
>>>>>
>>>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>>> ---
>>>>> Change in v3:
>>>>> ? - no change in patch, only added "reviewed-by" to commit log
>>>>
>>>> JFYI:
>>>> No need to re-send this patch with this added RB tag, as I already did
>>>> send a new RB to the last mail as reply. Patchwork collects these tags
>>>> when sent to the list. So you only need to include them, if you send
>>>> a new patch version.
>>>
>>> thanks for this hint, obviously I step into it all the time ...
>>>
>>> Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
>>>
>>> Back on topic: I guess that was all I needed to do so that the patch
>>> will get merged when its time comes.
>>
>> Yes, now we (you) need a bit of patience, so that other people might
>> review this patch as well. And usually it will get handled after some
>> time (depending on the development stage of U-Boot, merge window open
>> or shortly before release etc).
>
> Ok, no problem with that.
>
> That also means that I should wait with submission of the other patch
> (adding "env erase" support) until this one is merged.
> Otherwise the patch would need to be significantly different ...
No, you don't have to wait. This would be counter-productive. Please
just add the dependancy to this patch in the commit text of the new
one to make this clear. Best would be below the "---" line so that
it will not be included in the git history.
> Hmm, I guess it would have been better to not send that patch standalone
> but instead as a first one in a two-patch series where the second one
> adds the new functionality on top of the clean-up.
> Anyway, something to keep in mind for next time :-)
Yep, that would have been even better. ;)
Thanks,
Stefan
>> It does not hurt of course to check this from time to time and
>> "trigger" the maintainer of the subsystem or the custodian this
>> patch is delegated to nothing is happening here for a too long
>> time (like more than 1 month).
>
> Alright, good to know :-)
>
> Thanks,
> Harry
>
>> Thanks,
>> Stefan
>>
>>> If not, please let me know.
>>>
>>> Thanks,
>>> Harry
>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>> Change in v2:
>>>>> ? - remove one more #ifdef, instead take advantage of compiler
>>>>> attribute
>>>>> ??? __maybe_unused for one variable used only in case of redundant
>>>>> ??? environments
>>>>>
>>>>> ? env/sf.c | 130
>>>>> ++++++++++++++++++-------------------------------------
>>>>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>>>>
>>>>> diff --git a/env/sf.c b/env/sf.c
>>>>> index 937778aa37..199114fd3b 100644
>>>>> --- a/env/sf.c
>>>>> +++ b/env/sf.c
>>>>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>>>>> ????? return 0;
>>>>> ? }
>>>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>>>> ? static int env_sf_save(void)
>>>>> ? {
>>>>> ????? env_t??? env_new;
>>>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>>>> +??? char??? *saved_buffer = NULL;
>>>>> ????? u32??? saved_size, saved_offset, sector;
>>>>> +??? ulong??? offset;
>>>>> ????? int??? ret;
>>>>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>>>> ????? ret = setup_flash_device();
>>>>> ????? if (ret)
>>>>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>>>>> ????? ret = env_export(&env_new);
>>>>> ????? if (ret)
>>>>> ????????? return -EIO;
>>>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>>> -??? if (gd->env_valid == ENV_VALID) {
>>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>>>> -??? } else {
>>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> -??? }
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>>> +
>>>>> +??????? if (gd->env_valid == ENV_VALID) {
>>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>>>> +??????? } else {
>>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> +??????? }
>>>>> +??????? offset = env_new_offset;
>>>>> +#else
>>>>> +??????? offset = CONFIG_ENV_OFFSET;
>>>>> +#endif
>>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>>> ????????? if (!saved_buffer) {
>>>>> ????????????? ret = -ENOMEM;
>>>>> ????????????? goto done;
>>>>> ????????? }
>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>> -??????????????????? saved_size, saved_buffer);
>>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>>> +???????????????????? saved_buffer);
>>>>> ????????? if (ret)
>>>>> ????????????? goto done;
>>>>> ????? }
>>>>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>> ????? puts("Erasing SPI flash...");
>>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> ????? if (ret)
>>>>> ????????? goto done;
>>>>> ????? puts("Writing to SPI flash...");
>>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>>> ????? if (ret)
>>>>> ????????? goto done;
>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>> -??????????????????? saved_size, saved_buffer);
>>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>>> +????????????????????? saved_buffer);
>>>>> ????????? if (ret)
>>>>> ????????????? goto done;
>>>>> ????? }
>>>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t,
>>>>> flags),
>>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> +??????? ret = spi_flash_write(env_flash, env_offset +
>>>>> offsetof(env_t, flags),
>>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>>> +??????? if (ret)
>>>>> +??????????? goto done;
>>>>> -??? puts("done\n");
>>>>> +??????? puts("done\n");
>>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID :
>>>>> ENV_REDUND;
>>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID :
>>>>> ENV_REDUND;
>>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>> +#else
>>>>> +??????? puts("done\n");
>>>>> +#endif
>>>>> ?? done:
>>>>> ????? if (saved_buffer)
>>>>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>>>>> ????? return ret;
>>>>> ? }
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> ? static int env_sf_load(void)
>>>>> ? {
>>>>> ????? int ret;
>>>>> @@ -183,66 +195,6 @@ out:
>>>>> ????? return ret;
>>>>> ? }
>>>>> ? #else
>>>>> -static int env_sf_save(void)
>>>>> -{
>>>>> -??? u32??? saved_size, saved_offset, sector;
>>>>> -??? char??? *saved_buffer = NULL;
>>>>> -??? int??? ret = 1;
>>>>> -??? env_t??? env_new;
>>>>> -
>>>>> -??? ret = setup_flash_device();
>>>>> -??? if (ret)
>>>>> -??????? return ret;
>>>>> -
>>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>>> -??????? saved_buffer = malloc(saved_size);
>>>>> -??????? if (!saved_buffer)
>>>>> -??????????? goto done;
>>>>> -
>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>> -??????????? saved_size, saved_buffer);
>>>>> -??????? if (ret)
>>>>> -??????????? goto done;
>>>>> -??? }
>>>>> -
>>>>> -??? ret = env_export(&env_new);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>> -
>>>>> -??? puts("Erasing SPI flash...");
>>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? puts("Writing to SPI flash...");
>>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>> -??????????? saved_size, saved_buffer);
>>>>> -??????? if (ret)
>>>>> -??????????? goto done;
>>>>> -??? }
>>>>> -
>>>>> -??? ret = 0;
>>>>> -??? puts("done\n");
>>>>> -
>>>>> - done:
>>>>> -??? if (saved_buffer)
>>>>> -??????? free(saved_buffer);
>>>>> -
>>>>> -??? return ret;
>>>>> -}
>>>>> -
>>>>> ? static int env_sf_load(void)
>>>>> ? {
>>>>> ????? int ret;
>>>>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>>>>> ????? if (ret)
>>>>> ????????? goto out;
>>>>> -??? ret = spi_flash_read(env_flash,
>>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
>>>>> CONFIG_ENV_SIZE,
>>>>> +???????????????? buf);
>>>>> ????? if (ret) {
>>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>>> ????????? goto err_read;
>>>>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>>> ????????? gd->env_valid??? = 1;
>>>>> ????? } else {
>>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>>
>>>>
>>>>
>>>> Viele Gr??e,
>>>> Stefan
>>>>
>>>
>>>
>>
>>
>> Viele Gr??e,
>> Stefan
>>
>
>
Viele Gr??e,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-02-02 8:21 [PATCH v3 1/1] env: sf: single function env_sf_save() Harry Waschkeit
2021-02-02 9:30 ` Stefan Roese
@ 2021-03-01 16:39 ` Harry Waschkeit
2021-03-01 23:10 ` Sean Anderson
1 sibling, 1 reply; 11+ messages in thread
From: Harry Waschkeit @ 2021-03-01 16:39 UTC (permalink / raw)
To: u-boot
?Hi again,
gentle ping for that patch, also in view of subsequently sent patch ...
https://lists.denx.de/pipermail/u-boot/2021-February/439797.html
... which saw a competing patch by Patrick Delaunay a week later:
https://lists.denx.de/pipermail/u-boot/2021-February/440769.html
However, the latter doesn't seem to take embedded environments into account.
Best regards,
Harry
On 02.02.21 09:21, Harry Waschkeit wrote:
> Instead of implementing redundant environments in two very similar
> functions env_sf_save(), handle redundancy in one function, placing the
> few differences in appropriate pre-compiler sections depending on config
> option CONFIG_ENV_OFFSET_REDUND.
>
> Additionally, several checkpatch complaints were addressed.
>
> This patch is in preparation for adding support for env erase.
>
> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> ---
> Change in v3:
> - no change in patch, only added "reviewed-by" to commit log
>
> Change in v2:
> - remove one more #ifdef, instead take advantage of compiler attribute
> __maybe_unused for one variable used only in case of redundant
> environments
>
> env/sf.c | 130 ++++++++++++++++++-------------------------------------
> 1 file changed, 41 insertions(+), 89 deletions(-)
>
> diff --git a/env/sf.c b/env/sf.c
> index 937778aa37..199114fd3b 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
> return 0;
> }
>
> -#if defined(CONFIG_ENV_OFFSET_REDUND)
> static int env_sf_save(void)
> {
> env_t env_new;
> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
> + char *saved_buffer = NULL;
> u32 saved_size, saved_offset, sector;
> + ulong offset;
> int ret;
> + __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>
> ret = setup_flash_device();
> if (ret)
> @@ -81,27 +82,33 @@ static int env_sf_save(void)
> ret = env_export(&env_new);
> if (ret)
> return -EIO;
> - env_new.flags = ENV_REDUND_ACTIVE;
>
> - if (gd->env_valid == ENV_VALID) {
> - env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> - env_offset = CONFIG_ENV_OFFSET;
> - } else {
> - env_new_offset = CONFIG_ENV_OFFSET;
> - env_offset = CONFIG_ENV_OFFSET_REDUND;
> - }
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> + env_new.flags = ENV_REDUND_ACTIVE;
> +
> + if (gd->env_valid == ENV_VALID) {
> + env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> + env_offset = CONFIG_ENV_OFFSET;
> + } else {
> + env_new_offset = CONFIG_ENV_OFFSET;
> + env_offset = CONFIG_ENV_OFFSET_REDUND;
> + }
> + offset = env_new_offset;
> +#else
> + offset = CONFIG_ENV_OFFSET;
> +#endif
>
> /* Is the sector larger than the env (i.e. embedded) */
> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> - saved_offset = env_new_offset + CONFIG_ENV_SIZE;
> + saved_offset = offset + CONFIG_ENV_SIZE;
> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
> if (!saved_buffer) {
> ret = -ENOMEM;
> goto done;
> }
> - ret = spi_flash_read(env_flash, saved_offset,
> - saved_size, saved_buffer);
> + ret = spi_flash_read(env_flash, saved_offset, saved_size,
> + saved_buffer);
> if (ret)
> goto done;
> }
> @@ -109,35 +116,39 @@ static int env_sf_save(void)
> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>
> puts("Erasing SPI flash...");
> - ret = spi_flash_erase(env_flash, env_new_offset,
> - sector * CONFIG_ENV_SECT_SIZE);
> + ret = spi_flash_erase(env_flash, offset,
> + sector * CONFIG_ENV_SECT_SIZE);
> if (ret)
> goto done;
>
> puts("Writing to SPI flash...");
>
> - ret = spi_flash_write(env_flash, env_new_offset,
> - CONFIG_ENV_SIZE, &env_new);
> + ret = spi_flash_write(env_flash, offset,
> + CONFIG_ENV_SIZE, &env_new);
> if (ret)
> goto done;
>
> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> - ret = spi_flash_write(env_flash, saved_offset,
> - saved_size, saved_buffer);
> + ret = spi_flash_write(env_flash, saved_offset, saved_size,
> + saved_buffer);
> if (ret)
> goto done;
> }
>
> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> - sizeof(env_new.flags), &flag);
> - if (ret)
> - goto done;
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> + sizeof(env_new.flags), &flag);
> + if (ret)
> + goto done;
>
> - puts("done\n");
> + puts("done\n");
>
> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>
> - printf("Valid environment: %d\n", (int)gd->env_valid);
> + printf("Valid environment: %d\n", (int)gd->env_valid);
> +#else
> + puts("done\n");
> +#endif
>
> done:
> if (saved_buffer)
> @@ -146,6 +157,7 @@ static int env_sf_save(void)
> return ret;
> }
>
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> static int env_sf_load(void)
> {
> int ret;
> @@ -183,66 +195,6 @@ out:
> return ret;
> }
> #else
> -static int env_sf_save(void)
> -{
> - u32 saved_size, saved_offset, sector;
> - char *saved_buffer = NULL;
> - int ret = 1;
> - env_t env_new;
> -
> - ret = setup_flash_device();
> - if (ret)
> - return ret;
> -
> - /* Is the sector larger than the env (i.e. embedded) */
> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
> - saved_buffer = malloc(saved_size);
> - if (!saved_buffer)
> - goto done;
> -
> - ret = spi_flash_read(env_flash, saved_offset,
> - saved_size, saved_buffer);
> - if (ret)
> - goto done;
> - }
> -
> - ret = env_export(&env_new);
> - if (ret)
> - goto done;
> -
> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
> -
> - puts("Erasing SPI flash...");
> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
> - sector * CONFIG_ENV_SECT_SIZE);
> - if (ret)
> - goto done;
> -
> - puts("Writing to SPI flash...");
> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
> - CONFIG_ENV_SIZE, &env_new);
> - if (ret)
> - goto done;
> -
> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> - ret = spi_flash_write(env_flash, saved_offset,
> - saved_size, saved_buffer);
> - if (ret)
> - goto done;
> - }
> -
> - ret = 0;
> - puts("done\n");
> -
> - done:
> - if (saved_buffer)
> - free(saved_buffer);
> -
> - return ret;
> -}
> -
> static int env_sf_load(void)
> {
> int ret;
> @@ -258,8 +210,8 @@ static int env_sf_load(void)
> if (ret)
> goto out;
>
> - ret = spi_flash_read(env_flash,
> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> + buf);
> if (ret) {
> env_set_default("spi_flash_read() failed", 0);
> goto err_read;
> @@ -292,7 +244,7 @@ static int env_sf_init(void)
> env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>
> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
> - gd->env_addr = (ulong)&(env_ptr->data);
> + gd->env_addr = (ulong)&env_ptr->data;
> gd->env_valid = 1;
> } else {
> gd->env_addr = (ulong)&default_environment[0];
>
--
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Stra?e 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-03-01 16:39 ` Harry Waschkeit
@ 2021-03-01 23:10 ` Sean Anderson
2021-03-02 18:09 ` Harry Waschkeit
0 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2021-03-01 23:10 UTC (permalink / raw)
To: u-boot
On 3/1/21 11:39 AM, Harry Waschkeit wrote:
> ?Hi again,
>
> gentle ping for that patch, also in view of subsequently sent patch ...
>
> https://lists.denx.de/pipermail/u-boot/2021-February/439797.html
>
> ... which saw a competing patch by Patrick Delaunay a week later:
>
> https://lists.denx.de/pipermail/u-boot/2021-February/440769.html
>
> However, the latter doesn't seem to take embedded environments into account.
Can you give an example of where your patch would work while Patrick's wouldn't?
Thanks.
--Sean
>
> Best regards,
> Harry
>
>
> On 02.02.21 09:21, Harry Waschkeit wrote:
>> Instead of implementing redundant environments in two very similar
>> functions env_sf_save(), handle redundancy in one function, placing the
>> few differences in appropriate pre-compiler sections depending on config
>> option CONFIG_ENV_OFFSET_REDUND.
>>
>> Additionally, several checkpatch complaints were addressed.
>>
>> This patch is in preparation for adding support for env erase.
>>
>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>> ---
>> Change in v3:
>> - no change in patch, only added "reviewed-by" to commit log
>>
>> Change in v2:
>> - remove one more #ifdef, instead take advantage of compiler attribute
>> __maybe_unused for one variable used only in case of redundant
>> environments
>>
>> env/sf.c | 130 ++++++++++++++++++-------------------------------------
>> 1 file changed, 41 insertions(+), 89 deletions(-)
>>
>> diff --git a/env/sf.c b/env/sf.c
>> index 937778aa37..199114fd3b 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>> return 0;
>> }
>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>> static int env_sf_save(void)
>> {
>> env_t env_new;
>> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>> + char *saved_buffer = NULL;
>> u32 saved_size, saved_offset, sector;
>> + ulong offset;
>> int ret;
>> + __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>> ret = setup_flash_device();
>> if (ret)
>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>> ret = env_export(&env_new);
>> if (ret)
>> return -EIO;
>> - env_new.flags = ENV_REDUND_ACTIVE;
>> - if (gd->env_valid == ENV_VALID) {
>> - env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> - env_offset = CONFIG_ENV_OFFSET;
>> - } else {
>> - env_new_offset = CONFIG_ENV_OFFSET;
>> - env_offset = CONFIG_ENV_OFFSET_REDUND;
>> - }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> + env_new.flags = ENV_REDUND_ACTIVE;
>> +
>> + if (gd->env_valid == ENV_VALID) {
>> + env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> + env_offset = CONFIG_ENV_OFFSET;
>> + } else {
>> + env_new_offset = CONFIG_ENV_OFFSET;
>> + env_offset = CONFIG_ENV_OFFSET_REDUND;
>> + }
>> + offset = env_new_offset;
>> +#else
>> + offset = CONFIG_ENV_OFFSET;
>> +#endif
>> /* Is the sector larger than the env (i.e. embedded) */
>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>> + saved_offset = offset + CONFIG_ENV_SIZE;
>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>> if (!saved_buffer) {
>> ret = -ENOMEM;
>> goto done;
>> }
>> - ret = spi_flash_read(env_flash, saved_offset,
>> - saved_size, saved_buffer);
>> + ret = spi_flash_read(env_flash, saved_offset, saved_size,
>> + saved_buffer);
>> if (ret)
>> goto done;
>> }
>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> puts("Erasing SPI flash...");
>> - ret = spi_flash_erase(env_flash, env_new_offset,
>> - sector * CONFIG_ENV_SECT_SIZE);
>> + ret = spi_flash_erase(env_flash, offset,
>> + sector * CONFIG_ENV_SECT_SIZE);
>> if (ret)
>> goto done;
>> puts("Writing to SPI flash...");
>> - ret = spi_flash_write(env_flash, env_new_offset,
>> - CONFIG_ENV_SIZE, &env_new);
>> + ret = spi_flash_write(env_flash, offset,
>> + CONFIG_ENV_SIZE, &env_new);
>> if (ret)
>> goto done;
>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> - ret = spi_flash_write(env_flash, saved_offset,
>> - saved_size, saved_buffer);
>> + ret = spi_flash_write(env_flash, saved_offset, saved_size,
>> + saved_buffer);
>> if (ret)
>> goto done;
>> }
>> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> - sizeof(env_new.flags), &flag);
>> - if (ret)
>> - goto done;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> + sizeof(env_new.flags), &flag);
>> + if (ret)
>> + goto done;
>> - puts("done\n");
>> + puts("done\n");
>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> - printf("Valid environment: %d\n", (int)gd->env_valid);
>> + printf("Valid environment: %d\n", (int)gd->env_valid);
>> +#else
>> + puts("done\n");
>> +#endif
>> done:
>> if (saved_buffer)
>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>> return ret;
>> }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> static int env_sf_load(void)
>> {
>> int ret;
>> @@ -183,66 +195,6 @@ out:
>> return ret;
>> }
>> #else
>> -static int env_sf_save(void)
>> -{
>> - u32 saved_size, saved_offset, sector;
>> - char *saved_buffer = NULL;
>> - int ret = 1;
>> - env_t env_new;
>> -
>> - ret = setup_flash_device();
>> - if (ret)
>> - return ret;
>> -
>> - /* Is the sector larger than the env (i.e. embedded) */
>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>> - saved_buffer = malloc(saved_size);
>> - if (!saved_buffer)
>> - goto done;
>> -
>> - ret = spi_flash_read(env_flash, saved_offset,
>> - saved_size, saved_buffer);
>> - if (ret)
>> - goto done;
>> - }
>> -
>> - ret = env_export(&env_new);
>> - if (ret)
>> - goto done;
>> -
>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> -
>> - puts("Erasing SPI flash...");
>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>> - sector * CONFIG_ENV_SECT_SIZE);
>> - if (ret)
>> - goto done;
>> -
>> - puts("Writing to SPI flash...");
>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>> - CONFIG_ENV_SIZE, &env_new);
>> - if (ret)
>> - goto done;
>> -
>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> - ret = spi_flash_write(env_flash, saved_offset,
>> - saved_size, saved_buffer);
>> - if (ret)
>> - goto done;
>> - }
>> -
>> - ret = 0;
>> - puts("done\n");
>> -
>> - done:
>> - if (saved_buffer)
>> - free(saved_buffer);
>> -
>> - return ret;
>> -}
>> -
>> static int env_sf_load(void)
>> {
>> int ret;
>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>> if (ret)
>> goto out;
>> - ret = spi_flash_read(env_flash,
>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>> + buf);
>> if (ret) {
>> env_set_default("spi_flash_read() failed", 0);
>> goto err_read;
>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>> env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>> - gd->env_addr = (ulong)&(env_ptr->data);
>> + gd->env_addr = (ulong)&env_ptr->data;
>> gd->env_valid = 1;
>> } else {
>> gd->env_addr = (ulong)&default_environment[0];
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-03-01 23:10 ` Sean Anderson
@ 2021-03-02 18:09 ` Harry Waschkeit
2021-03-02 23:06 ` Sean Anderson
0 siblings, 1 reply; 11+ messages in thread
From: Harry Waschkeit @ 2021-03-02 18:09 UTC (permalink / raw)
To: u-boot
Hello Sean,
On 02.03.21 00:10, Sean Anderson wrote:
> On 3/1/21 11:39 AM, Harry Waschkeit wrote:
>> ?Hi again,
>>
>> gentle ping for that patch, also in view of subsequently sent patch ...
>>
>> ? https://lists.denx.de/pipermail/u-boot/2021-February/439797.html
>>
>> ... which saw a competing patch by Patrick Delaunay a week later:
>>
>> ? https://lists.denx.de/pipermail/u-boot/2021-February/440769.html
>>
>> However, the latter doesn't seem to take embedded environments into account.
>
> Can you give an example of where your patch would work while Patrick's wouldn't?
I didn't dig too deep into Patrick's patch and maybe I simply miss something important, but I don't even see an spi_flash_erase() in his code, so I'm wondering how it could work at all.
What I do see in his code is spi_flash_write() with a CONFIG_ENV_SIZE worth of zeroes to the environment environment offset, but this can only work for an erased flash as far as I know.
And erasing the flash part where the environment is stored should take an environment sizes below sector size into account; the rest of the environment sector could be used otherwise, i.e. CONFIG_ENV_SIZE < CONFIG_ENV_SECT_SIZE.
Function env_sf_save() takes care of all of that, so does the env_sf_erase() in my patch, Patrick's version of env_sf_erase() does not.
(side note: using more than a flash sector for environment and sharing the last one with different data isn't handled at all@the moment, probably because it was never needed)
Best regards,
Harry
>
> Thanks.
>
> --Sean
>
>>
>> Best regards,
>> Harry
>>
>>
>> On 02.02.21 09:21, Harry Waschkeit wrote:
>>> Instead of implementing redundant environments in two very similar
>>> functions env_sf_save(), handle redundancy in one function, placing the
>>> few differences in appropriate pre-compiler sections depending on config
>>> option CONFIG_ENV_OFFSET_REDUND.
>>>
>>> Additionally, several checkpatch complaints were addressed.
>>>
>>> This patch is in preparation for adding support for env erase.
>>>
>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>> ---
>>> Change in v3:
>>> ? - no change in patch, only added "reviewed-by" to commit log
>>>
>>> Change in v2:
>>> ? - remove one more #ifdef, instead take advantage of compiler attribute
>>> ??? __maybe_unused for one variable used only in case of redundant
>>> ??? environments
>>>
>>> ? env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 937778aa37..199114fd3b 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>>> ????? return 0;
>>> ? }
>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>> ? static int env_sf_save(void)
>>> ? {
>>> ????? env_t??? env_new;
>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>> +??? char??? *saved_buffer = NULL;
>>> ????? u32??? saved_size, saved_offset, sector;
>>> +??? ulong??? offset;
>>> ????? int??? ret;
>>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>> ????? ret = setup_flash_device();
>>> ????? if (ret)
>>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>>> ????? ret = env_export(&env_new);
>>> ????? if (ret)
>>> ????????? return -EIO;
>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>> -??? if (gd->env_valid == ENV_VALID) {
>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>> -??? } else {
>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> -??? }
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>> +
>>> +??????? if (gd->env_valid == ENV_VALID) {
>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>> +??????? } else {
>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +??????? }
>>> +??????? offset = env_new_offset;
>>> +#else
>>> +??????? offset = CONFIG_ENV_OFFSET;
>>> +#endif
>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>> ????????? if (!saved_buffer) {
>>> ????????????? ret = -ENOMEM;
>>> ????????????? goto done;
>>> ????????? }
>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>> -??????????????????? saved_size, saved_buffer);
>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>> +???????????????????? saved_buffer);
>>> ????????? if (ret)
>>> ????????????? goto done;
>>> ????? }
>>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> ????? puts("Erasing SPI flash...");
>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>> +??? ret = spi_flash_erase(env_flash, offset,
>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>> ????? if (ret)
>>> ????????? goto done;
>>> ????? puts("Writing to SPI flash...");
>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>> +??? ret = spi_flash_write(env_flash, offset,
>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>> ????? if (ret)
>>> ????????? goto done;
>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>> -??????????????????? saved_size, saved_buffer);
>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>> +????????????????????? saved_buffer);
>>> ????????? if (ret)
>>> ????????????? goto done;
>>> ????? }
>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>> -??????????????? sizeof(env_new.flags), &flag);
>>> -??? if (ret)
>>> -??????? goto done;
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>> +????????????????????? sizeof(env_new.flags), &flag);
>>> +??????? if (ret)
>>> +??????????? goto done;
>>> -??? puts("done\n");
>>> +??????? puts("done\n");
>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>> +#else
>>> +??????? puts("done\n");
>>> +#endif
>>> ?? done:
>>> ????? if (saved_buffer)
>>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>>> ????? return ret;
>>> ? }
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> ? static int env_sf_load(void)
>>> ? {
>>> ????? int ret;
>>> @@ -183,66 +195,6 @@ out:
>>> ????? return ret;
>>> ? }
>>> ? #else
>>> -static int env_sf_save(void)
>>> -{
>>> -??? u32??? saved_size, saved_offset, sector;
>>> -??? char??? *saved_buffer = NULL;
>>> -??? int??? ret = 1;
>>> -??? env_t??? env_new;
>>> -
>>> -??? ret = setup_flash_device();
>>> -??? if (ret)
>>> -??????? return ret;
>>> -
>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>> -??????? saved_buffer = malloc(saved_size);
>>> -??????? if (!saved_buffer)
>>> -??????????? goto done;
>>> -
>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>> -??????????? saved_size, saved_buffer);
>>> -??????? if (ret)
>>> -??????????? goto done;
>>> -??? }
>>> -
>>> -??? ret = env_export(&env_new);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> -
>>> -??? puts("Erasing SPI flash...");
>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? puts("Writing to SPI flash...");
>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>> -??????????? saved_size, saved_buffer);
>>> -??????? if (ret)
>>> -??????????? goto done;
>>> -??? }
>>> -
>>> -??? ret = 0;
>>> -??? puts("done\n");
>>> -
>>> - done:
>>> -??? if (saved_buffer)
>>> -??????? free(saved_buffer);
>>> -
>>> -??? return ret;
>>> -}
>>> -
>>> ? static int env_sf_load(void)
>>> ? {
>>> ????? int ret;
>>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>>> ????? if (ret)
>>> ????????? goto out;
>>> -??? ret = spi_flash_read(env_flash,
>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>> +???????????????? buf);
>>> ????? if (ret) {
>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>> ????????? goto err_read;
>>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>> ????????? gd->env_valid??? = 1;
>>> ????? } else {
>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>
>>
>>
>
>
--
Harry Waschkeit - Software Engineer
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-03-02 18:09 ` Harry Waschkeit
@ 2021-03-02 23:06 ` Sean Anderson
2021-03-03 17:52 ` Harry Waschkeit
0 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2021-03-02 23:06 UTC (permalink / raw)
To: u-boot
On 3/2/21 1:09 PM, Harry Waschkeit wrote:
> Hello Sean,
>
> On 02.03.21 00:10, Sean Anderson wrote:
>> On 3/1/21 11:39 AM, Harry Waschkeit wrote:
>>> ?Hi again,
>>>
>>> gentle ping for that patch, also in view of subsequently sent patch ...
>>>
>>> https://lists.denx.de/pipermail/u-boot/2021-February/439797.html
>>>
>>> ... which saw a competing patch by Patrick Delaunay a week later:
>>>
>>> https://lists.denx.de/pipermail/u-boot/2021-February/440769.html
>>>
>>> However, the latter doesn't seem to take embedded environments into account.
>>
>> Can you give an example of where your patch would work while Patrick's wouldn't?
>
> I didn't dig too deep into Patrick's patch and maybe I simply miss
> something important, but I don't even see an spi_flash_erase() in his
> code, so I'm wondering how it could work at all.
Writing to SPI flash can only set bits to 0. To set bits to 1 you must
erase them. Conceptually, write(buf) does
flash[i] &= buf[i];
And erase() does
flash[i] = -1;
So writing 0s always succeeds, and we are certain to invalidate the
environment (as long as our hash has a non-zero value for an all-zero
input).
> What I do see in his code is spi_flash_write() with a CONFIG_ENV_SIZE
> worth of zeroes to the environment environment offset, but this can
> only work for an erased flash as far as I know.
env_sf_save always erases the environment before flashing. So your patch
effectively erases env twice before writing to it.
> And erasing the flash part where the environment is stored should take
> an environment sizes below sector size into account; the rest of the
> environment sector could be used otherwise, i.e. CONFIG_ENV_SIZE <
> CONFIG_ENV_SECT_SIZE.
Right, but typically we have a write granularity of one byte for SPI
flashes. So you can clear only the environment, and let enf_sf_save deal
with other data in the sector.
--Sean
> Function env_sf_save() takes care of all of that, so does the
> env_sf_erase() in my patch, Patrick's version of env_sf_erase() does
> not.
>
> (side note: using more than a flash sector for environment and sharing
> the last one with different data isn't handled at all at the moment,
> probably because it was never needed)
>
> Best regards,
> Harry
>
>>
>> Thanks.
>>
>> --Sean
>>
>>>
>>> Best regards,
>>> Harry
>>>
>>>
>>> On 02.02.21 09:21, Harry Waschkeit wrote:
>>>> Instead of implementing redundant environments in two very similar
>>>> functions env_sf_save(), handle redundancy in one function, placing the
>>>> few differences in appropriate pre-compiler sections depending on config
>>>> option CONFIG_ENV_OFFSET_REDUND.
>>>>
>>>> Additionally, several checkpatch complaints were addressed.
>>>>
>>>> This patch is in preparation for adding support for env erase.
>>>>
>>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>> ---
>>>> Change in v3:
>>>> - no change in patch, only added "reviewed-by" to commit log
>>>>
>>>> Change in v2:
>>>> - remove one more #ifdef, instead take advantage of compiler attribute
>>>> __maybe_unused for one variable used only in case of redundant
>>>> environments
>>>>
>>>> env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>>> 1 file changed, 41 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/env/sf.c b/env/sf.c
>>>> index 937778aa37..199114fd3b 100644
>>>> --- a/env/sf.c
>>>> +++ b/env/sf.c
>>>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>>>> return 0;
>>>> }
>>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>>> static int env_sf_save(void)
>>>> {
>>>> env_t env_new;
>>>> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>>> + char *saved_buffer = NULL;
>>>> u32 saved_size, saved_offset, sector;
>>>> + ulong offset;
>>>> int ret;
>>>> + __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>>> ret = setup_flash_device();
>>>> if (ret)
>>>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>>>> ret = env_export(&env_new);
>>>> if (ret)
>>>> return -EIO;
>>>> - env_new.flags = ENV_REDUND_ACTIVE;
>>>> - if (gd->env_valid == ENV_VALID) {
>>>> - env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> - env_offset = CONFIG_ENV_OFFSET;
>>>> - } else {
>>>> - env_new_offset = CONFIG_ENV_OFFSET;
>>>> - env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> - }
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> + env_new.flags = ENV_REDUND_ACTIVE;
>>>> +
>>>> + if (gd->env_valid == ENV_VALID) {
>>>> + env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> + env_offset = CONFIG_ENV_OFFSET;
>>>> + } else {
>>>> + env_new_offset = CONFIG_ENV_OFFSET;
>>>> + env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> + }
>>>> + offset = env_new_offset;
>>>> +#else
>>>> + offset = CONFIG_ENV_OFFSET;
>>>> +#endif
>>>> /* Is the sector larger than the env (i.e. embedded) */
>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>> + saved_offset = offset + CONFIG_ENV_SIZE;
>>>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>> if (!saved_buffer) {
>>>> ret = -ENOMEM;
>>>> goto done;
>>>> }
>>>> - ret = spi_flash_read(env_flash, saved_offset,
>>>> - saved_size, saved_buffer);
>>>> + ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>> + saved_buffer);
>>>> if (ret)
>>>> goto done;
>>>> }
>>>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>>>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> puts("Erasing SPI flash...");
>>>> - ret = spi_flash_erase(env_flash, env_new_offset,
>>>> - sector * CONFIG_ENV_SECT_SIZE);
>>>> + ret = spi_flash_erase(env_flash, offset,
>>>> + sector * CONFIG_ENV_SECT_SIZE);
>>>> if (ret)
>>>> goto done;
>>>> puts("Writing to SPI flash...");
>>>> - ret = spi_flash_write(env_flash, env_new_offset,
>>>> - CONFIG_ENV_SIZE, &env_new);
>>>> + ret = spi_flash_write(env_flash, offset,
>>>> + CONFIG_ENV_SIZE, &env_new);
>>>> if (ret)
>>>> goto done;
>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> - ret = spi_flash_write(env_flash, saved_offset,
>>>> - saved_size, saved_buffer);
>>>> + ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>> + saved_buffer);
>>>> if (ret)
>>>> goto done;
>>>> }
>>>> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>> - sizeof(env_new.flags), &flag);
>>>> - if (ret)
>>>> - goto done;
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>> + sizeof(env_new.flags), &flag);
>>>> + if (ret)
>>>> + goto done;
>>>> - puts("done\n");
>>>> + puts("done\n");
>>>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> - printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> + printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> +#else
>>>> + puts("done\n");
>>>> +#endif
>>>> done:
>>>> if (saved_buffer)
>>>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>>>> return ret;
>>>> }
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> static int env_sf_load(void)
>>>> {
>>>> int ret;
>>>> @@ -183,66 +195,6 @@ out:
>>>> return ret;
>>>> }
>>>> #else
>>>> -static int env_sf_save(void)
>>>> -{
>>>> - u32 saved_size, saved_offset, sector;
>>>> - char *saved_buffer = NULL;
>>>> - int ret = 1;
>>>> - env_t env_new;
>>>> -
>>>> - ret = setup_flash_device();
>>>> - if (ret)
>>>> - return ret;
>>>> -
>>>> - /* Is the sector larger than the env (i.e. embedded) */
>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>> - saved_buffer = malloc(saved_size);
>>>> - if (!saved_buffer)
>>>> - goto done;
>>>> -
>>>> - ret = spi_flash_read(env_flash, saved_offset,
>>>> - saved_size, saved_buffer);
>>>> - if (ret)
>>>> - goto done;
>>>> - }
>>>> -
>>>> - ret = env_export(&env_new);
>>>> - if (ret)
>>>> - goto done;
>>>> -
>>>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> -
>>>> - puts("Erasing SPI flash...");
>>>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>> - sector * CONFIG_ENV_SECT_SIZE);
>>>> - if (ret)
>>>> - goto done;
>>>> -
>>>> - puts("Writing to SPI flash...");
>>>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>> - CONFIG_ENV_SIZE, &env_new);
>>>> - if (ret)
>>>> - goto done;
>>>> -
>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> - ret = spi_flash_write(env_flash, saved_offset,
>>>> - saved_size, saved_buffer);
>>>> - if (ret)
>>>> - goto done;
>>>> - }
>>>> -
>>>> - ret = 0;
>>>> - puts("done\n");
>>>> -
>>>> - done:
>>>> - if (saved_buffer)
>>>> - free(saved_buffer);
>>>> -
>>>> - return ret;
>>>> -}
>>>> -
>>>> static int env_sf_load(void)
>>>> {
>>>> int ret;
>>>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>>>> if (ret)
>>>> goto out;
>>>> - ret = spi_flash_read(env_flash,
>>>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>>> + buf);
>>>> if (ret) {
>>>> env_set_default("spi_flash_read() failed", 0);
>>>> goto err_read;
>>>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>>>> env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>> - gd->env_addr = (ulong)&(env_ptr->data);
>>>> + gd->env_addr = (ulong)&env_ptr->data;
>>>> gd->env_valid = 1;
>>>> } else {
>>>> gd->env_addr = (ulong)&default_environment[0];
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/1] env: sf: single function env_sf_save()
2021-03-02 23:06 ` Sean Anderson
@ 2021-03-03 17:52 ` Harry Waschkeit
0 siblings, 0 replies; 11+ messages in thread
From: Harry Waschkeit @ 2021-03-03 17:52 UTC (permalink / raw)
To: u-boot
On 03.03.21 00:06, Sean Anderson wrote:
> On 3/2/21 1:09 PM, Harry Waschkeit wrote:
>> Hello Sean,
>>
>> On 02.03.21 00:10, Sean Anderson wrote:
>>> On 3/1/21 11:39 AM, Harry Waschkeit wrote:
>>>> ?Hi again,
>>>>
>>>> gentle ping for that patch, also in view of subsequently sent patch ...
>>>>
>>>> ? https://lists.denx.de/pipermail/u-boot/2021-February/439797.html
>>>>
>>>> ... which saw a competing patch by Patrick Delaunay a week later:
>>>>
>>>> ? https://lists.denx.de/pipermail/u-boot/2021-February/440769.html
>>>>
>>>> However, the latter doesn't seem to take embedded environments into account.
>>>
>>> Can you give an example of where your patch would work while Patrick's wouldn't?
>>
>> I didn't dig too deep into Patrick's patch and maybe I simply miss
>> something important, but I don't even see an spi_flash_erase() in his
>> code, so I'm wondering how it could work at all.
>
> Writing to SPI flash can only set bits to 0. To set bits to 1 you must
> erase them. Conceptually, write(buf) does
>
> ????flash[i] &= buf[i];
>
> And erase() does
>
> ????flash[i] = -1;
>
> So writing 0s always succeeds, and we are certain to invalidate the
> environment (as long as our hash has a non-zero value for an all-zero
> input).
Sure.
So it's only the name that is confusing because there's no erase happening at all in env_sf_erase(), a real flash erase is necessary afterwards if you want to write data to environment space.
I think it would be worth a comment in the function then, maybe also others get confused what a function called something with erase actually does with flash data ;-)
>> What I do see in his code is spi_flash_write() with a CONFIG_ENV_SIZE
>> worth of zeroes to the environment environment offset, but this can
>> only work for an erased flash as far as I know.
>
> env_sf_save always erases the environment before flashing. So your patch
> effectively erases env twice before writing to it.
Yeah, you're right, it was made under the false assumption that the erase function actually should erase something ... ;-/
>> And erasing the flash part where the environment is stored should take
>> an environment sizes below sector size into account; the rest of the
>> environment sector could be used otherwise, i.e. CONFIG_ENV_SIZE <
>> CONFIG_ENV_SECT_SIZE.
>
> Right, but typically we have a write granularity of one byte for SPI
> flashes. So you can clear only the environment, and let enf_sf_save deal
> with other data in the sector.
Correct.
So my patches are actually worthless ... interesting that Patrick came up with his patches only right after my attempts to add that functionality, but not discussing my approach.
I'd loved to make a small contribution to U-Boot, maybe I've more luck next time ... ;-)
Best regards,
Harry
> --Sean
>
>> Function env_sf_save() takes care of all of that, so does the
>> env_sf_erase() in my patch, Patrick's version of env_sf_erase() does
>> not.
>>
>> (side note: using more than a flash sector for environment and sharing
>> the last one with different data isn't handled at all at the moment,
>> probably because it was never needed)
>>
>> Best regards,
>> Harry
>>
>>>
>>> Thanks.
>>>
>>> --Sean
>>>
>>>>
>>>> Best regards,
>>>> Harry
>>>>
>>>>
>>>> On 02.02.21 09:21, Harry Waschkeit wrote:
>>>>> Instead of implementing redundant environments in two very similar
>>>>> functions env_sf_save(), handle redundancy in one function, placing the
>>>>> few differences in appropriate pre-compiler sections depending on config
>>>>> option CONFIG_ENV_OFFSET_REDUND.
>>>>>
>>>>> Additionally, several checkpatch complaints were addressed.
>>>>>
>>>>> This patch is in preparation for adding support for env erase.
>>>>>
>>>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>>> ---
>>>>> Change in v3:
>>>>> ? - no change in patch, only added "reviewed-by" to commit log
>>>>>
>>>>> Change in v2:
>>>>> ? - remove one more #ifdef, instead take advantage of compiler attribute
>>>>> ??? __maybe_unused for one variable used only in case of redundant
>>>>> ??? environments
>>>>>
>>>>> ? env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>>>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>>>>
>>>>> diff --git a/env/sf.c b/env/sf.c
>>>>> index 937778aa37..199114fd3b 100644
>>>>> --- a/env/sf.c
>>>>> +++ b/env/sf.c
>>>>> @@ -66,13 +66,14 @@ static int setup_flash_device(void)
>>>>> ????? return 0;
>>>>> ? }
>>>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>>>> ? static int env_sf_save(void)
>>>>> ? {
>>>>> ????? env_t??? env_new;
>>>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>>>> +??? char??? *saved_buffer = NULL;
>>>>> ????? u32??? saved_size, saved_offset, sector;
>>>>> +??? ulong??? offset;
>>>>> ????? int??? ret;
>>>>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>>>> ????? ret = setup_flash_device();
>>>>> ????? if (ret)
>>>>> @@ -81,27 +82,33 @@ static int env_sf_save(void)
>>>>> ????? ret = env_export(&env_new);
>>>>> ????? if (ret)
>>>>> ????????? return -EIO;
>>>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>>> -??? if (gd->env_valid == ENV_VALID) {
>>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>>>> -??? } else {
>>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> -??? }
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>>> +
>>>>> +??????? if (gd->env_valid == ENV_VALID) {
>>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>>>> +??????? } else {
>>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>> +??????? }
>>>>> +??????? offset = env_new_offset;
>>>>> +#else
>>>>> +??????? offset = CONFIG_ENV_OFFSET;
>>>>> +#endif
>>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> ????????? saved_size =CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>>> +??????? saved_offset = offset +CONFIG_ENV_SIZE;
>>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>>> ????????? if (!saved_buffer) {
>>>>> ????????????? ret = -ENOMEM;
>>>>> ????????????? goto done;
>>>>> ????????? }
>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>> -??????????????????? saved_size, saved_buffer);
>>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>>> +???????????????????? saved_buffer);
>>>>> ????????? if (ret)
>>>>> ????????????? goto done;
>>>>> ????? }
>>>>> @@ -109,35 +116,39 @@ static int env_sf_save(void)
>>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>> ????? puts("Erasing SPI flash...");
>>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> ????? if (ret)
>>>>> ????????? goto done;
>>>>> ????? puts("Writing to SPI flash...");
>>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>>> ????? if (ret)
>>>>> ????????? goto done;
>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>> -??????????????????? saved_size, saved_buffer);
>>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>>> +????????????????????? saved_buffer);
>>>>> ????????? if (ret)
>>>>> ????????????? goto done;
>>>>> ????? }
>>>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>>> +??????? if (ret)
>>>>> +??????????? goto done;
>>>>> -??? puts("done\n");
>>>>> +??????? puts("done\n");
>>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND? ENV_VALID : ENV_REDUND;
>>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>> +#else
>>>>> +??????? puts("done\n");
>>>>> +#endif
>>>>> ?? done:
>>>>> ????? if (saved_buffer)
>>>>> @@ -146,6 +157,7 @@ static int env_sf_save(void)
>>>>> ????? return ret;
>>>>> ? }
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> ? static int env_sf_load(void)
>>>>> ? {
>>>>> ????? int ret;
>>>>> @@ -183,66 +195,6 @@ out:
>>>>> ????? return ret;
>>>>> ? }
>>>>> ? #else
>>>>> -static int env_sf_save(void)
>>>>> -{
>>>>> -??? u32??? saved_size, saved_offset, sector;
>>>>> -??? char??? *saved_buffer = NULL;
>>>>> -??? int??? ret = 1;
>>>>> -??? env_t??? env_new;
>>>>> -
>>>>> -??? ret = setup_flash_device();
>>>>> -??? if (ret)
>>>>> -??????? return ret;
>>>>> -
>>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>>> -??????? saved_buffer = malloc(saved_size);
>>>>> -??????? if (!saved_buffer)
>>>>> -??????????? goto done;
>>>>> -
>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>> -??????????? saved_size, saved_buffer);
>>>>> -??????? if (ret)
>>>>> -??????????? goto done;
>>>>> -??? }
>>>>> -
>>>>> -??? ret = env_export(&env_new);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>> -
>>>>> -??? puts("Erasing SPI flash...");
>>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? puts("Writing to SPI flash...");
>>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>> -??????????? saved_size, saved_buffer);
>>>>> -??????? if (ret)
>>>>> -??????????? goto done;
>>>>> -??? }
>>>>> -
>>>>> -??? ret = 0;
>>>>> -??? puts("done\n");
>>>>> -
>>>>> - done:
>>>>> -??? if (saved_buffer)
>>>>> -??????? free(saved_buffer);
>>>>> -
>>>>> -??? return ret;
>>>>> -}
>>>>> -
>>>>> ? static int env_sf_load(void)
>>>>> ? {
>>>>> ????? int ret;
>>>>> @@ -258,8 +210,8 @@ static int env_sf_load(void)
>>>>> ????? if (ret)
>>>>> ????????? goto out;
>>>>> -??? ret = spi_flash_read(env_flash,
>>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>>>> +???????????????? buf);
>>>>> ????? if (ret) {
>>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>>> ????????? goto err_read;
>>>>> @@ -292,7 +244,7 @@ static int env_sf_init(void)
>>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>>> ????????? gd->env_valid??? = 1;
>>>>> ????? } else {
>>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Harry Waschkeit - Software Engineer
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-03 17:52 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 8:21 [PATCH v3 1/1] env: sf: single function env_sf_save() Harry Waschkeit
2021-02-02 9:30 ` Stefan Roese
2021-02-02 14:43 ` Harry Waschkeit
2021-02-02 14:54 ` Stefan Roese
2021-02-02 17:09 ` Harry Waschkeit
2021-02-03 6:10 ` Stefan Roese
2021-03-01 16:39 ` Harry Waschkeit
2021-03-01 23:10 ` Sean Anderson
2021-03-02 18:09 ` Harry Waschkeit
2021-03-02 23:06 ` Sean Anderson
2021-03-03 17:52 ` Harry Waschkeit
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.