| Message ID | 20200506084701.8076-3-rasmus.villemoes@prevas.dk |
|---|---|
| State | Accepted |
| Commit | bcb44f62b21e88cc74bc26939eb1dac95d2f430b |
| Headers | show |
| Series | add CONFIG_ENV_SECT_SIZE_AUTO | expand |
On Wed, 2020-05-06 at 10:47 +0200, Rasmus Villemoes wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > This is roughly the U-Boot side equivalent to commit > e282c422e0 (tools: fw_env: use erasesize from MEMGETINFO ioctl). The > motivation is the case where one has a board with several revisions, > where the SPI flashes have different erase sizes. > > In our case, we have an 8K environment, and the flashes have erase > sizes of 4K (newer boards) and 64K (older boards). Currently, we must > set CONFIG_ENV_SECT_SIZE to 64K to make the code work on the older > boards, but for the newer ones, that ends up wasting quite a bit of > time reading/erasing/restoring the last 56K. > > At first, I wanted to allow setting CONFIG_ENV_SECT_SIZE to 0 to mean > "use the erase size the chip reports", but that config > option is used in a number of preprocessor conditionals, and shared > between ENV_IS_IN_FLASH and ENV_IS_IN_SPI_FLASH. I see, It had been good if one could have used 0 > > So instead, introduce a new boolean config option, which for now can > only be used with ENV_IS_IN_SPI_FLASH. If left off, there's no change > in behaviour. Please don't stop at just SPI Flash, we could use this for common NOR flashes too. > > The only slightly annoying detail is that, when selected, the compiler > is apparently not smart enough to see that the the saved_size and > saved_offset variables are only used under the same "if (sect_size > > CONFIG_ENV_SIZE)" condition as where they are computed, so we need to > initialize them to 0 to avoid "may be used uninitialized" warnings. > > On our newer boards with the 4K erase size, saving the environment now > takes 0.080 seconds instead of 0.53 seconds, which directly translates > to that much faster boot time since our logic always causes the > environment to be written during boot. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> > --- > env/Kconfig | 14 ++++++++++++++ > env/sf.c | 10 ++++++++-- > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/env/Kconfig b/env/Kconfig > index 969308fe6c..c90cd04604 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -317,6 +317,20 @@ config ENV_IS_IN_SPI_FLASH > during a "saveenv" operation. CONFIG_ENV_OFFSET_REDUND must be > aligned to an erase sector boundary. > > +config ENV_SECT_SIZE_AUTO > + bool "Use automatically detected sector size" > + depends on ENV_IS_IN_SPI_FLASH > + help > + Some boards exist in multiple variants, with different > + flashes having different sector sizes. In such cases, you > + can select this option to make U-Boot use the actual sector > + size when figuring out how much to erase, which can thus be > + more efficient on the flashes with smaller erase size. Since > + the environment must always be aligned on a sector boundary, > + CONFIG_ENV_OFFSET must be aligned to the largest of the > + different sector sizes, and CONFIG_ENV_SECT_SIZE should be > + set to that value. > + > config USE_ENV_SPI_BUS > bool "SPI flash bus for environment" > depends on ENV_IS_IN_SPI_FLASH > diff --git a/env/sf.c b/env/sf.c > index cd5339578b..644e78fe3d 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -69,7 +69,7 @@ static int env_sf_save(void) > { > env_t env_new; > char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; > - u32 saved_size, saved_offset, sector; > + u32 saved_size = 0, saved_offset = 0, sector; > u32 sect_size = CONFIG_ENV_SECT_SIZE; > int ret; > > @@ -77,6 +77,9 @@ static int env_sf_save(void) > if (ret) > return ret; > > + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) > + sect_size = env_flash->mtd.erasesize; > + > ret = env_export(&env_new); > if (ret) > return -EIO; > @@ -184,7 +187,7 @@ out: > #else > static int env_sf_save(void) > { > - u32 saved_size, saved_offset, sector; > + u32 saved_size = 0, saved_offset = 0, sector; > u32 sect_size = CONFIG_ENV_SECT_SIZE; > char *saved_buffer = NULL; > int ret = 1; > @@ -194,6 +197,9 @@ static int env_sf_save(void) > if (ret) > return ret; > > + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) > + sect_size = env_flash->mtd.erasesize; > + > /* Is the sector larger than the env (i.e. embedded) */ > if (sect_size > CONFIG_ENV_SIZE) { > saved_size = sect_size - CONFIG_ENV_SIZE; > -- > 2.23.0 >
On 06/05/2020 10.59, Joakim Tjernlund wrote: > On Wed, 2020-05-06 at 10:47 +0200, Rasmus Villemoes wrote: >> At first, I wanted to allow setting CONFIG_ENV_SECT_SIZE to 0 to mean >> "use the erase size the chip reports", but that config >> option is used in a number of preprocessor conditionals, and shared >> between ENV_IS_IN_FLASH and ENV_IS_IN_SPI_FLASH. > > I see, It had been good if one could have used 0 > >> >> So instead, introduce a new boolean config option, which for now can >> only be used with ENV_IS_IN_SPI_FLASH. If left off, there's no change >> in behaviour. > > Please don't stop at just SPI Flash, we could use this for common NOR flashes too. Certainly, but I don't have any such hardware, and the code also seems to require a bit more work than the sf.c case due to the preprocessor uses. It's quite possible that the #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE foo; bar; #endif can be replaced by u32 sect_size = CONFIG_ENV_SECT_SIZE; if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) sect_size = [...however we want to get that info....] if (sect_size > CONFIG_ENV_SIZE { foo; bar; } but then there's the uses of CONFIG_ENV_SECT_SIZE to initialize end_addr and end_addr_new. So I think doing the flash.c part requires someone with hardware access. Rasmus
On Wed, 2020-05-06 at 11:11 +0200, Rasmus Villemoes wrote: > > On 06/05/2020 10.59, Joakim Tjernlund wrote: > > On Wed, 2020-05-06 at 10:47 +0200, Rasmus Villemoes wrote: > > > At first, I wanted to allow setting CONFIG_ENV_SECT_SIZE to 0 to mean > > > "use the erase size the chip reports", but that config > > > option is used in a number of preprocessor conditionals, and shared > > > between ENV_IS_IN_FLASH and ENV_IS_IN_SPI_FLASH. > > > > I see, It had been good if one could have used 0 > > > > > So instead, introduce a new boolean config option, which for now can > > > only be used with ENV_IS_IN_SPI_FLASH. If left off, there's no change > > > in behaviour. > > > > Please don't stop at just SPI Flash, we could use this for common NOR flashes too. > > Certainly, but I don't have any such hardware, and the code also seems > to require a bit more work than the sf.c case due to the preprocessor > uses. It's quite possible that the > > #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE > foo; bar; > #endif > > can be replaced by > > u32 sect_size = CONFIG_ENV_SECT_SIZE; > if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) > sect_size = [...however we want to get that info....] > > if (sect_size > CONFIG_ENV_SIZE { > foo; bar; > } > > but then there's the uses of CONFIG_ENV_SECT_SIZE to initialize end_addr > and end_addr_new. So I think doing the flash.c part requires someone > with hardware access. I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport How would fw_env.config be dealt with? Just ignored when auto size? Jocke
On 06/05/2020 11.21, Joakim Tjernlund wrote: > I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport > How would fw_env.config be dealt with? Just ignored when auto size? AFAIK, the U-Boot configuration doesn't affect the userspace fw_env tools, so auto size has to be opt-in separately in fw_env.config, which is where I did use 0 to mean auto (see the referenced commit e282c422e0). You should be able to test that part already with just fw_setenv built from master and an appropriate fw_env.config [or perhaps you need to add another "mtdinfo.type == ..." condition first]. Rasmus
On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On 06/05/2020 11.21, Joakim Tjernlund wrote: > > > I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport > > How would fw_env.config be dealt with? Just ignored when auto size? > > AFAIK, the U-Boot configuration doesn't affect the userspace fw_env > tools, so auto size has to be opt-in separately in fw_env.config, which > is where I did use 0 to mean auto (see the referenced commit > e282c422e0). You should be able to test that part already with just > fw_setenv built from master and an appropriate fw_env.config [or perhaps > you need to add another "mtdinfo.type == ..." condition first]. Oh, I misunderstood then. I was mostly interested in fw_setenv ATM Looking at your fw_setenv commit I have one question: Will this auto mode also work when erase size < env size ? We have the case: # MTD device name Device offset Env. size Flash sector size /dev/mtd1 0x0000 0x2000 0x00001000 Above does not work because EB < Env. size, one have to set the sector size to >= Env. size(0x2000) Jocke
On 06/05/2020 12.00, Joakim Tjernlund wrote: > On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: >> >> On 06/05/2020 11.21, Joakim Tjernlund wrote: >> >>> I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport >>> How would fw_env.config be dealt with? Just ignored when auto size? >> >> AFAIK, the U-Boot configuration doesn't affect the userspace fw_env >> tools, so auto size has to be opt-in separately in fw_env.config, which >> is where I did use 0 to mean auto (see the referenced commit >> e282c422e0). You should be able to test that part already with just >> fw_setenv built from master and an appropriate fw_env.config [or perhaps >> you need to add another "mtdinfo.type == ..." condition first]. > > Oh, I misunderstood then. I was mostly interested in fw_setenv ATM > Looking at your fw_setenv commit I have one question: > Will this auto mode also work when erase size < env size ? > We have the case: > > # MTD device name Device offset Env. size Flash sector size > /dev/mtd1 0x0000 0x2000 0x00001000 > > Above does not work because EB < Env. size, one have to set the sector size > to >= Env. size(0x2000) Yes, it should work just fine. My own board has env size 0x2000, so it spans two 4K sectors (on the boards with the newer flash). As long as one leaves the fifth column (number of sectors) empty or 0, the logic a bit further down will automatically compute how many sectors the environment occupies. Are you saying that the above config is not accepted by fw_setenv or that it causes a run-time error when actually trying to save the env? If so, I'd like to see a strace output. Rasmus
On Wed, 2020-05-06 at 12:00 +0200, Joakim Tjernlund wrote: > On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > On 06/05/2020 11.21, Joakim Tjernlund wrote: > > > > > I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport > > > How would fw_env.config be dealt with? Just ignored when auto size? > > > > AFAIK, the U-Boot configuration doesn't affect the userspace fw_env > > tools, so auto size has to be opt-in separately in fw_env.config, which > > is where I did use 0 to mean auto (see the referenced commit > > e282c422e0). You should be able to test that part already with just > > fw_setenv built from master and an appropriate fw_env.config [or perhaps > > you need to add another "mtdinfo.type == ..." condition first]. > > Oh, I misunderstood then. I was mostly interested in fw_setenv ATM > Looking at your fw_setenv commit I have one question: > Will this auto mode also work when erase size < env size ? > We have the case: > > # MTD device name Device offset Env. size Flash sector size
On Wed, 2020-05-06 at 12:15 +0200, Rasmus Villemoes wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On 06/05/2020 12.00, Joakim Tjernlund wrote: > > On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: > > > On 06/05/2020 11.21, Joakim Tjernlund wrote: > > > > > > > I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport > > > > How would fw_env.config be dealt with? Just ignored when auto size? > > > > > > AFAIK, the U-Boot configuration doesn't affect the userspace fw_env > > > tools, so auto size has to be opt-in separately in fw_env.config, which > > > is where I did use 0 to mean auto (see the referenced commit > > > e282c422e0). You should be able to test that part already with just > > > fw_setenv built from master and an appropriate fw_env.config [or perhaps > > > you need to add another "mtdinfo.type == ..." condition first]. > > > > Oh, I misunderstood then. I was mostly interested in fw_setenv ATM > > Looking at your fw_setenv commit I have one question: > > Will this auto mode also work when erase size < env size ? > > We have the case: > > > > # MTD device name Device offset Env. size Flash sector size > > /dev/mtd1 0x0000 0x2000 0x00001000 > > > > Above does not work because EB < Env. size, one have to set the sector size > > to >= Env. size(0x2000) > > Yes, it should work just fine. My own board has env size 0x2000, so it > spans two 4K sectors (on the boards with the newer flash). As long as > one leaves the fifth column (number of sectors) empty or 0, the logic a > bit further down will automatically compute how many sectors the > environment occupies. Nice, got to try your latest fw_setenv patch then :) > > Are you saying that the above config is not accepted by fw_setenv or > that it causes a run-time error when actually trying to save the env? If > so, I'd like to see a strace output. No, I have yet to try your patch. I am saying that /dev/mtd1 0x0000 0x2000 0x00001000 in my somewhat older fw_setenv, I think it would silently ignore writing the env. in that case(if I recall correctly)
On 06/05/2020 12.18, Joakim Tjernlund wrote: > On Wed, 2020-05-06 at 12:00 +0200, Joakim Tjernlund wrote: >> On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: >>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. >>> >>> >>> On 06/05/2020 11.21, Joakim Tjernlund wrote: >>> >>>> I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport >>>> How would fw_env.config be dealt with? Just ignored when auto size? >>> >>> AFAIK, the U-Boot configuration doesn't affect the userspace fw_env >>> tools, so auto size has to be opt-in separately in fw_env.config, which >>> is where I did use 0 to mean auto (see the referenced commit >>> e282c422e0). You should be able to test that part already with just >>> fw_setenv built from master and an appropriate fw_env.config [or perhaps >>> you need to add another "mtdinfo.type == ..." condition first]. >> >> Oh, I misunderstood then. I was mostly interested in fw_setenv ATM >> Looking at your fw_setenv commit I have one question: >> Will this auto mode also work when erase size < env size ? >> We have the case: >> >> # MTD device name Device offset Env. size Flash sector size > > From my POV, this "Flash sector size" name is misleading. "Size to erase" would be better. > It must be >= Env. size and a multiple of EB size. > auto would mean to select a size >= Env. size, round up to closest multiple of EB size. No, it is indeed supposed to be the flash sector size, and _not_ the size to erase; that's why the code has logic to compute how many sectors to erase (which you can also optionally specify in a fifth column, but as I said, that's done automatically if not specified), and the erase size is then, later yet, of course computed as (erase size)*#sectors. All that has nothing at all to do with my patch, that's how it all used to work. >> /dev/mtd1 0x0000 0x2000 0x00001000 >> >> Above does not work because EB < Env. size, one have to set the sector size >> to >= Env. size(0x2000) Are you sure? As I said, it works just fine on my end. The only thing my auto-patch does is to make /dev/mtd1 0x0000 0x2000 0x00001000 equivalent to /dev/mtd1 0x0000 0x2000 0 if the flash does have 4k erase size, so my patch wouldn't make the latter work if the former didn't already. [But again, the reason I can't use the former is to be compatible with the boards that have 64K erase size]. Rasmus
On Wed, 2020-05-06 at 12:47 +0200, Rasmus Villemoes wrote: > On 06/05/2020 12.18, Joakim Tjernlund wrote: > > On Wed, 2020-05-06 at 12:00 +0200, Joakim Tjernlund wrote: > > > On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > > > > > > > On 06/05/2020 11.21, Joakim Tjernlund wrote: > > > > > > > > > I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport > > > > > How would fw_env.config be dealt with? Just ignored when auto size? > > > > > > > > AFAIK, the U-Boot configuration doesn't affect the userspace fw_env > > > > tools, so auto size has to be opt-in separately in fw_env.config, which > > > > is where I did use 0 to mean auto (see the referenced commit > > > > e282c422e0). You should be able to test that part already with just > > > > fw_setenv built from master and an appropriate fw_env.config [or perhaps > > > > you need to add another "mtdinfo.type == ..." condition first]. > > > > > > Oh, I misunderstood then. I was mostly interested in fw_setenv ATM > > > Looking at your fw_setenv commit I have one question: > > > Will this auto mode also work when erase size < env size ? > > > We have the case: > > > > > > # MTD device name Device offset Env. size Flash sector size > > > > From my POV, this "Flash sector size" name is misleading. "Size to erase" would be better. > > It must be >= Env. size and a multiple of EB size. > > auto would mean to select a size >= Env. size, round up to closest multiple of EB size. > > No, it is indeed supposed to be the flash sector size, and _not_ the > size to erase; that's why the code has logic to compute how many sectors > to erase (which you can also optionally specify in a fifth column, but > as I said, that's done automatically if not specified), and the erase > size is then, later yet, of course computed as (erase size)*#sectors. > > All that has nothing at all to do with my patch, that's how it all used > to work. > > > > /dev/mtd1 0x0000 0x2000 0x00001000 > > > > > > Above does not work because EB < Env. size, one have to set the sector size > > > to >= Env. size(0x2000) > > Are you sure? As I said, it works just fine on my end. The only thing my > auto-patch does is to make Yes, but then my fw_setenv is from 2015.04, not much has happened to fw_setenv since then though I will test you auto fw_setenv soon Jocke > > /dev/mtd1 0x0000 0x2000 0x00001000 > > equivalent to > > /dev/mtd1 0x0000 0x2000 0 > > if the flash does have 4k erase size, so my patch wouldn't make the > latter work if the former didn't already. [But again, the reason I can't > use the former is to be compatible with the boards that have 64K erase > size]. > > Rasmus
On Wed, 2020-05-06 at 13:06 +0200, Joakim Tjernlund wrote: > On Wed, 2020-05-06 at 12:47 +0200, Rasmus Villemoes wrote: > > On 06/05/2020 12.18, Joakim Tjernlund wrote: > > > On Wed, 2020-05-06 at 12:00 +0200, Joakim Tjernlund wrote: > > > > On Wed, 2020-05-06 at 11:37 +0200, Rasmus Villemoes wrote: > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > > > > > > > > > > On 06/05/2020 11.21, Joakim Tjernlund wrote: > > > > > > > > > > > I can test NOR Flash ,I am on an older u-boot ATM but fw_setenv stuff should be simple to backport > > > > > > How would fw_env.config be dealt with? Just ignored when auto size? > > > > > > > > > > AFAIK, the U-Boot configuration doesn't affect the userspace fw_env > > > > > tools, so auto size has to be opt-in separately in fw_env.config, which > > > > > is where I did use 0 to mean auto (see the referenced commit > > > > > e282c422e0). You should be able to test that part already with just > > > > > fw_setenv built from master and an appropriate fw_env.config [or perhaps > > > > > you need to add another "mtdinfo.type == ..." condition first]. > > > > > > > > Oh, I misunderstood then. I was mostly interested in fw_setenv ATM > > > > Looking at your fw_setenv commit I have one question: > > > > Will this auto mode also work when erase size < env size ? > > > > We have the case: > > > > > > > > # MTD device name Device offset Env. size Flash sector size > > > > > > From my POV, this "Flash sector size" name is misleading. "Size to erase" would be better. > > > It must be >= Env. size and a multiple of EB size. > > > auto would mean to select a size >= Env. size, round up to closest multiple of EB size. > > > > No, it is indeed supposed to be the flash sector size, and _not_ the > > size to erase; that's why the code has logic to compute how many sectors > > to erase (which you can also optionally specify in a fifth column, but > > as I said, that's done automatically if not specified), and the erase > > size is then, later yet, of course computed as (erase size)*#sectors. > > > > All that has nothing at all to do with my patch, that's how it all used > > to work. > > > > > > /dev/mtd1 0x0000 0x2000 0x00001000 > > > > > > > > Above does not work because EB < Env. size, one have to set the sector size > > > > to >= Env. size(0x2000) > > > > Are you sure? As I said, it works just fine on my end. The only thing my > > auto-patch does is to make > > Yes, but then my fw_setenv is from 2015.04, not much has happened to fw_setenv since then though > I will test you auto fw_setenv soon Some docs about this new auto mode in fw_env.config would probably be a good idea too.
On 06/05/2020 10.47, Rasmus Villemoes wrote: > This is roughly the U-Boot side equivalent to commit > e282c422e0 (tools: fw_env: use erasesize from MEMGETINFO ioctl). The > motivation is the case where one has a board with several revisions, > where the SPI flashes have different erase sizes.> > In our case, we have an 8K environment, and the flashes have erase > sizes of 4K (newer boards) and 64K (older boards). Currently, we must > set CONFIG_ENV_SECT_SIZE to 64K to make the code work on the older > boards, but for the newer ones, that ends up wasting quite a bit of > time reading/erasing/restoring the last 56K. > > At first, I wanted to allow setting CONFIG_ENV_SECT_SIZE to 0 to mean > "use the erase size the chip reports", but that config > option is used in a number of preprocessor conditionals, and shared > between ENV_IS_IN_FLASH and ENV_IS_IN_SPI_FLASH. > > So instead, introduce a new boolean config option, which for now can > only be used with ENV_IS_IN_SPI_FLASH. If left off, there's no change > in behaviour. > > The only slightly annoying detail is that, when selected, the compiler > is apparently not smart enough to see that the the saved_size and > saved_offset variables are only used under the same "if (sect_size > > CONFIG_ENV_SIZE)" condition as where they are computed, so we need to > initialize them to 0 to avoid "may be used uninitialized" warnings. > > On our newer boards with the 4K erase size, saving the environment now > takes 0.080 seconds instead of 0.53 seconds, which directly translates > to that much faster boot time since our logic always causes the > environment to be written during boot. ping > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > env/Kconfig | 14 ++++++++++++++ > env/sf.c | 10 ++++++++-- > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/env/Kconfig b/env/Kconfig > index 969308fe6c..c90cd04604 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -317,6 +317,20 @@ config ENV_IS_IN_SPI_FLASH > during a "saveenv" operation. CONFIG_ENV_OFFSET_REDUND must be > aligned to an erase sector boundary. > > +config ENV_SECT_SIZE_AUTO > + bool "Use automatically detected sector size" > + depends on ENV_IS_IN_SPI_FLASH > + help > + Some boards exist in multiple variants, with different > + flashes having different sector sizes. In such cases, you > + can select this option to make U-Boot use the actual sector > + size when figuring out how much to erase, which can thus be > + more efficient on the flashes with smaller erase size. Since > + the environment must always be aligned on a sector boundary, > + CONFIG_ENV_OFFSET must be aligned to the largest of the > + different sector sizes, and CONFIG_ENV_SECT_SIZE should be > + set to that value. > + > config USE_ENV_SPI_BUS > bool "SPI flash bus for environment" > depends on ENV_IS_IN_SPI_FLASH > diff --git a/env/sf.c b/env/sf.c > index cd5339578b..644e78fe3d 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -69,7 +69,7 @@ static int env_sf_save(void) > { > env_t env_new; > char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; > - u32 saved_size, saved_offset, sector; > + u32 saved_size = 0, saved_offset = 0, sector; > u32 sect_size = CONFIG_ENV_SECT_SIZE; > int ret; > > @@ -77,6 +77,9 @@ static int env_sf_save(void) > if (ret) > return ret; > > + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) > + sect_size = env_flash->mtd.erasesize; > + > ret = env_export(&env_new); > if (ret) > return -EIO; > @@ -184,7 +187,7 @@ out: > #else > static int env_sf_save(void) > { > - u32 saved_size, saved_offset, sector; > + u32 saved_size = 0, saved_offset = 0, sector; > u32 sect_size = CONFIG_ENV_SECT_SIZE; > char *saved_buffer = NULL; > int ret = 1; > @@ -194,6 +197,9 @@ static int env_sf_save(void) > if (ret) > return ret; > > + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) > + sect_size = env_flash->mtd.erasesize; > + > /* Is the sector larger than the env (i.e. embedded) */ > if (sect_size > CONFIG_ENV_SIZE) { > saved_size = sect_size - CONFIG_ENV_SIZE; > -- Rasmus Villemoes Software Developer Prevas A/S Hedeager 3 DK-8200 Aarhus N +45 51210274 rasmus.villemoes@prevas.dk www.prevas.dk
Dear Rasmus, In message <125344f4-2b69-1c7a-0f5a-6d27ab312487@prevas.dk> you wrote: > > > > On our newer boards with the 4K erase size, saving the environment now > > takes 0.080 seconds instead of 0.53 seconds, which directly translates > > to that much faster boot time since our logic always causes the > > environment to be written during boot. Is this your way of implementing scheduled obsolescence? Automatically writing the environment on every boot is always a really bad idea. Never ever do that. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de When the bosses talk about improving productivity, they are never talking about themselves.
On 24/09/2020 12.54, Wolfgang Denk wrote: > Dear Rasmus, > > In message <125344f4-2b69-1c7a-0f5a-6d27ab312487@prevas.dk> you wrote: >>> >>> On our newer boards with the 4K erase size, saving the environment now >>> takes 0.080 seconds instead of 0.53 seconds, which directly translates >>> to that much faster boot time since our logic always causes the >>> environment to be written during boot. > > Is this your way of implementing scheduled obsolescence? I'll ignore that. > Automatically writing the environment on every boot is always a > really bad idea. Never ever do that. Well, there's really no other place on this piece of hardware to implement a boot counter/slot selection. Sure, we could use another part of the spi flash and define our own storage format etc., but then we might as well use the u-boot environment, which allows us to use existing tools/APIs both on the U-Boot and linux side. And yes, the number of erase cycles compared to how often this device is booted was taken into account when this was decided. Rasmus
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topicproducts.com Please consider the environment before printing this e-mail On 24-09-2020 13:08, Rasmus Villemoes wrote: > On 24/09/2020 12.54, Wolfgang Denk wrote: >> Dear Rasmus, >> >> In message <125344f4-2b69-1c7a-0f5a-6d27ab312487@prevas.dk> you wrote: >>>> >>>> On our newer boards with the 4K erase size, saving the environment now >>>> takes 0.080 seconds instead of 0.53 seconds, which directly translates >>>> to that much faster boot time since our logic always causes the >>>> environment to be written during boot. >> >> Is this your way of implementing scheduled obsolescence? > > I'll ignore that. > >> Automatically writing the environment on every boot is always a >> really bad idea. Never ever do that. > > Well, there's really no other place on this piece of hardware to > implement a boot counter/slot selection. Sure, we could use another part > of the spi flash and define our own storage format etc., but then we > might as well use the u-boot environment, which allows us to use > existing tools/APIs both on the U-Boot and linux side. > > And yes, the number of erase cycles compared to how often this device is > booted was taken into account when this was decided. If it's SPI NOR flash, you can make use of the fact that you can change any 1 into a 0 without erasing the flash sector. So if you use a unary counter (the number of consecutive zero bits), incrementing the counter doesn't require a sector flash. I once used this for a medical device. After an upgade, the flash would boot-count and after 3 failed boots fall back to the previous image. The software would mark the image as "good" once booted okay by clearing the special "image is okay" bit. This way there's only one erase cycle for every software upgrade, even with the use of a bootcounter.
On 24/09/2020 13.35, Mike Looijmans wrote: >>> Automatically writing the environment on every boot is always a >>> really bad idea. Never ever do that. >> >> Well, there's really no other place on this piece of hardware to >> implement a boot counter/slot selection. Sure, we could use another part >> of the spi flash and define our own storage format etc., but then we >> might as well use the u-boot environment, which allows us to use >> existing tools/APIs both on the U-Boot and linux side. >> >> And yes, the number of erase cycles compared to how often this device is >> booted was taken into account when this was decided. > > If it's SPI NOR flash, you can make use of the fact that you can change > any 1 into a 0 without erasing the flash sector. > > So if you use a unary counter (the number of consecutive zero bits), > incrementing the counter doesn't require a sector flash. > > I once used this for a medical device. After an upgade, the flash would > boot-count and after 3 failed boots fall back to the previous image. The > software would mark the image as "good" once booted okay by clearing the > special "image is okay" bit. > > This way there's only one erase cycle for every software upgrade, even > with the use of a bootcounter. Yes, I/we considered some scheme like that (though we store a bit more information, so not exactly what you described), essentially just using a sector as append-only storage for an array if fixed-size small blobs (I think we could fit everything into 16 bytes or so, but if its 4 or 16 or ... doesn't really matter). On reading, we'd read the whole sector and use the last not-all-ones blob, probably with some sanity checking. On writing, we'd only have to do an erase once every 256 or 1024 or whatever. But regardless, the complexity of having to write a small linux tool for that and patching the u-boot side similarly, and maintaining that code forever (because we really _do_ plan to keep updating the software on these devices, including u-boot), and that the spi flash has 100000 expected erase cycles, meant that we went for the much simpler, and easily extendible, solution. In the field, these devices are usually turned on and then run for _months_. But when they are booted, getting up and running quickly is somewhat important. In our test rack, they are exposed to 1000s of power-cut tests among others (so one of those might eventually wear out the spi flash - but actually getting data on when that can be expected is just a bonus), so there saving half a second of boot time means we can do more tests in a given time span. Rasmus
diff --git a/env/Kconfig b/env/Kconfig index 969308fe6c..c90cd04604 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -317,6 +317,20 @@ config ENV_IS_IN_SPI_FLASH during a "saveenv" operation. CONFIG_ENV_OFFSET_REDUND must be aligned to an erase sector boundary. +config ENV_SECT_SIZE_AUTO + bool "Use automatically detected sector size" + depends on ENV_IS_IN_SPI_FLASH + help + Some boards exist in multiple variants, with different + flashes having different sector sizes. In such cases, you + can select this option to make U-Boot use the actual sector + size when figuring out how much to erase, which can thus be + more efficient on the flashes with smaller erase size. Since + the environment must always be aligned on a sector boundary, + CONFIG_ENV_OFFSET must be aligned to the largest of the + different sector sizes, and CONFIG_ENV_SECT_SIZE should be + set to that value. + config USE_ENV_SPI_BUS bool "SPI flash bus for environment" depends on ENV_IS_IN_SPI_FLASH diff --git a/env/sf.c b/env/sf.c index cd5339578b..644e78fe3d 100644 --- a/env/sf.c +++ b/env/sf.c @@ -69,7 +69,7 @@ static int env_sf_save(void) { env_t env_new; char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; - u32 saved_size, saved_offset, sector; + u32 saved_size = 0, saved_offset = 0, sector; u32 sect_size = CONFIG_ENV_SECT_SIZE; int ret; @@ -77,6 +77,9 @@ static int env_sf_save(void) if (ret) return ret; + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) + sect_size = env_flash->mtd.erasesize; + ret = env_export(&env_new); if (ret) return -EIO; @@ -184,7 +187,7 @@ out: #else static int env_sf_save(void) { - u32 saved_size, saved_offset, sector; + u32 saved_size = 0, saved_offset = 0, sector; u32 sect_size = CONFIG_ENV_SECT_SIZE; char *saved_buffer = NULL; int ret = 1; @@ -194,6 +197,9 @@ static int env_sf_save(void) if (ret) return ret; + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) + sect_size = env_flash->mtd.erasesize; + /* Is the sector larger than the env (i.e. embedded) */ if (sect_size > CONFIG_ENV_SIZE) { saved_size = sect_size - CONFIG_ENV_SIZE;
This is roughly the U-Boot side equivalent to commit e282c422e0 (tools: fw_env: use erasesize from MEMGETINFO ioctl). The motivation is the case where one has a board with several revisions, where the SPI flashes have different erase sizes. In our case, we have an 8K environment, and the flashes have erase sizes of 4K (newer boards) and 64K (older boards). Currently, we must set CONFIG_ENV_SECT_SIZE to 64K to make the code work on the older boards, but for the newer ones, that ends up wasting quite a bit of time reading/erasing/restoring the last 56K. At first, I wanted to allow setting CONFIG_ENV_SECT_SIZE to 0 to mean "use the erase size the chip reports", but that config option is used in a number of preprocessor conditionals, and shared between ENV_IS_IN_FLASH and ENV_IS_IN_SPI_FLASH. So instead, introduce a new boolean config option, which for now can only be used with ENV_IS_IN_SPI_FLASH. If left off, there's no change in behaviour. The only slightly annoying detail is that, when selected, the compiler is apparently not smart enough to see that the the saved_size and saved_offset variables are only used under the same "if (sect_size > CONFIG_ENV_SIZE)" condition as where they are computed, so we need to initialize them to 0 to avoid "may be used uninitialized" warnings. On our newer boards with the 4K erase size, saving the environment now takes 0.080 seconds instead of 0.53 seconds, which directly translates to that much faster boot time since our logic always causes the environment to be written during boot. Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> --- env/Kconfig | 14 ++++++++++++++ env/sf.c | 10 ++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-)