From mboxrd@z Thu Jan 1 00:00:00 1970 From: Harry Waschkeit Date: Tue, 2 Feb 2021 18:09:16 +0100 Subject: [PATCH v3 1/1] env: sf: single function env_sf_save() In-Reply-To: References: <20210202082156.21056-1-Harry.Waschkeit@men.de> Message-ID: <103f99af-d166-e400-457b-32b0fd776ce9@men.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> Reviewed-by: Stefan Roese >>>> --- >>>> 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