diff mbox series

[2/2] env: add CONFIG_ENV_SECT_SIZE_AUTO

Message ID 20200506084701.8076-3-rasmus.villemoes@prevas.dk
State Accepted
Commit bcb44f62b21e88cc74bc26939eb1dac95d2f430b
Headers show
Series add CONFIG_ENV_SECT_SIZE_AUTO | expand

Commit Message

Rasmus Villemoes May 6, 2020, 8:47 a.m. UTC
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(-)

Comments

Joakim Tjernlund May 6, 2020, 8:59 a.m. UTC | #1
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
>
Rasmus Villemoes May 6, 2020, 9:11 a.m. UTC | #2
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
Joakim Tjernlund May 6, 2020, 9:21 a.m. UTC | #3
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
Rasmus Villemoes May 6, 2020, 9:37 a.m. UTC | #4
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
Joakim Tjernlund May 6, 2020, 10 a.m. UTC | #5
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
Rasmus Villemoes May 6, 2020, 10:15 a.m. UTC | #6
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
Joakim Tjernlund May 6, 2020, 10:18 a.m. UTC | #7
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
Joakim Tjernlund May 6, 2020, 10:22 a.m. UTC | #8
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)
Rasmus Villemoes May 6, 2020, 10:47 a.m. UTC | #9
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
Joakim Tjernlund May 6, 2020, 11:06 a.m. UTC | #10
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
Joakim Tjernlund May 6, 2020, 11:08 a.m. UTC | #11
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.
Rasmus Villemoes Sept. 18, 2020, 8:17 a.m. UTC | #12
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
Wolfgang Denk Sept. 24, 2020, 10:54 a.m. UTC | #13
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.
Rasmus Villemoes Sept. 24, 2020, 11:08 a.m. UTC | #14
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
Mike Looijmans Sept. 24, 2020, 11:35 a.m. UTC | #15
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.
Rasmus Villemoes Sept. 24, 2020, 11:58 a.m. UTC | #16
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 mbox series

Patch

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;