From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Tue, 2 Mar 2021 18:06:29 -0500 Subject: [PATCH v3 1/1] env: sf: single function env_sf_save() In-Reply-To: References: <20210202082156.21056-1-Harry.Waschkeit@men.de> <60bb9cc5-1bfb-4957-7c20-7dcf2de28760@men.de> <4857412c-0a63-3c0d-aeda-db3620792c43@gmail.com> Message-ID: <33b60b9f-8f77-6e84-3087-54d9111902f1@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> Reviewed-by: Stefan Roese >>>> --- >>>> 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]; >>>> >>> >>> >> >> > >