[PATCH resend 0/2] add CONFIG_ENV_SECT_SIZE_AUTO

[PATCH resend 0/2] add CONFIG_ENV_SECT_SIZE_AUTO

[Rasmus Villemoes]

This is a resend, just rebased to current master, of patches I sent
way back in May.

This is roughly the U-Boot side equivalent to commit e282c422e0
(tools: fw_env: use erasesize from MEMGETINFO ioctl), at least for
SPI_FLASH backend.

When CONFIG_ENV_SECT_SIZE_AUTO is not selected (and it is of course
default n), there's no functional change, and the compiler even seems
to generate identical binary code.

The motivation is to cut about half a second off boottime on our newer
revisions, while still having the same U-Boot binary work on both.

Rasmus Villemoes (2):
  env/sf.c: use a variable to hold the sector size
  env: add CONFIG_ENV_SECT_SIZE_AUTO

 env/Kconfig | 14 ++++++++++++++
 env/sf.c    | 32 ++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.29.2

[PATCH resend 1/2] env/sf.c: use a variable to hold the sector size

[Rasmus Villemoes]

As preparation for the next patch, use a local variable to represent
the sector size. No functional change.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/sf.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 88ec1108b6..d9ed08a78e 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -73,6 +73,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	sect_size = CONFIG_ENV_SECT_SIZE;
 	int	ret;
 
 	ret = setup_flash_device();
@@ -93,8 +94,8 @@ static int env_sf_save(void)
 	}
 
 	/* 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;
+	if (sect_size > CONFIG_ENV_SIZE) {
+		saved_size = sect_size - CONFIG_ENV_SIZE;
 		saved_offset = env_new_offset + CONFIG_ENV_SIZE;
 		saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
 		if (!saved_buffer) {
@@ -107,11 +108,11 @@ static int env_sf_save(void)
 			goto done;
 	}
 
-	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
+	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, sect_size);
 
 	puts("Erasing SPI flash...");
 	ret = spi_flash_erase(env_flash, env_new_offset,
-				sector * CONFIG_ENV_SECT_SIZE);
+				sector * sect_size);
 	if (ret)
 		goto done;
 
@@ -122,7 +123,7 @@ static int env_sf_save(void)
 	if (ret)
 		goto done;
 
-	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
+	if (sect_size > CONFIG_ENV_SIZE) {
 		ret = spi_flash_write(env_flash, saved_offset,
 					saved_size, saved_buffer);
 		if (ret)
@@ -187,6 +188,7 @@ out:
 static int env_sf_save(void)
 {
 	u32	saved_size, saved_offset, sector;
+	u32	sect_size = CONFIG_ENV_SECT_SIZE;
 	char	*saved_buffer = NULL;
 	int	ret = 1;
 	env_t	env_new;
@@ -196,8 +198,8 @@ static int env_sf_save(void)
 		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;
+	if (sect_size > CONFIG_ENV_SIZE) {
+		saved_size = sect_size - CONFIG_ENV_SIZE;
 		saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
 		saved_buffer = malloc(saved_size);
 		if (!saved_buffer)
@@ -213,11 +215,11 @@ static int env_sf_save(void)
 	if (ret)
 		goto done;
 
-	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
+	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, sect_size);
 
 	puts("Erasing SPI flash...");
 	ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
-		sector * CONFIG_ENV_SECT_SIZE);
+		sector * sect_size);
 	if (ret)
 		goto done;
 
@@ -227,7 +229,7 @@ static int env_sf_save(void)
 	if (ret)
 		goto done;
 
-	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
+	if (sect_size > CONFIG_ENV_SIZE) {
 		ret = spi_flash_write(env_flash, saved_offset,
 			saved_size, saved_buffer);
 		if (ret)
-- 
2.29.2

[PATCH resend 2/2] env: add CONFIG_ENV_SECT_SIZE_AUTO

[Rasmus Villemoes]

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@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 b473d7cfe1..844c312870 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -324,6 +324,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 d9ed08a78e..06cc62e005 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -72,7 +72,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;
 
@@ -80,6 +80,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;
@@ -187,7 +190,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;
@@ -197,6 +200,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.29.2

[PATCH resend 1/2] env/sf.c: use a variable to hold the sector size

[Tom Rini]

On Wed, Apr 14, 2021 at 08:51:42PM +0200, Rasmus Villemoes wrote:

> As preparation for the next patch, use a local variable to represent
> the sector size. No functional change.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210418/056fc007/attachment.sig>

[PATCH resend 2/2] env: add CONFIG_ENV_SECT_SIZE_AUTO

[Tom Rini]

On Wed, Apr 14, 2021 at 08:51:43PM +0200, 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210418/85813240/attachment.sig>