Skip to contentSkip to conversation

env_sf: minor refactorings

PATCH New Andreas Fenkart 3 participants 9 messages Apr 8, 2017 – May 3, 2017

lore
Share & tools
lore

Andreas Fenkart

author
#0 lore raw

v2 cover env_sf: minor refactorings

rebased patches on master
fixed compiled error in 1st patch (missing variable declaration) compile tested all patches with travis

Andreas Fenkart (4):
env_sf: factor out prepare_flash_device
enf_sf: reuse setup_flash_device instead of open coding it
env_sf: re-order error handling in single-buffer env_relocate_spec
env_sf: use DIV_ROUND_UP to calculate number of sectors to erase

 common/env_sf.c | 92 ++++++++++++++++++++++-----------------------------------
 1 file changed, 36 insertions(+), 56 deletions(-)
Signature · 1 line
2.11.0
author
#1 lore raw

PATCH v2 1/4 env_sf: factor out prepare_flash_device

copy&paste code found in single/double buffered code path

Signed-off-by Andreas Fenkart <[email protected]>
Reviewed-by Simon Glass <[email protected]>
---
 common/env_sf.c | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)
common/env_sf.c +19 -29
index 27b4d1226a..8af590a3d9 100644
@@ -45,15 +45,11 @@ char *env_name_spec = "SPI Flash";
 
 static struct spi_flash *env_flash;
 
-#if defined(CONFIG_ENV_OFFSET_REDUND)
-int saveenv(void)
+static int setup_flash_device(void)
 {
-	env_t	env_new;
-	char	*saved_buffer = NULL, flag = OBSOLETE_FLAG;
-	u32	saved_size, saved_offset, sector = 1;
-	int	ret;
 #ifdef CONFIG_DM_SPI_FLASH
 	struct udevice *new;
+	int	ret;
 
 	/* speed and mode will be read from DT */
 	ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,

@@ -76,6 +72,20 @@ int saveenv(void)
 		}
 	}
 #endif
+	return 0;
+}
+
+#if defined(CONFIG_ENV_OFFSET_REDUND)
+int saveenv(void)
+{
+	env_t	env_new;
+	char	*saved_buffer = NULL, flag = OBSOLETE_FLAG;
+	u32	saved_size, saved_offset, sector = 1;
+	int	ret;
+
+	ret = setup_flash_device();
+	if (ret)
+		return ret;
 
 	ret = env_export(&env_new);
 	if (ret)

@@ -242,30 +252,10 @@ int saveenv(void)
 	char	*saved_buffer = NULL;
 	int	ret = 1;
 	env_t	env_new;
-#ifdef CONFIG_DM_SPI_FLASH
-	struct udevice *new;
 
-	/* speed and mode will be read from DT */
-	ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
-				     0, 0, &new);
-	if (ret) {
-		set_default_env("!spi_flash_probe_bus_cs() failed");
-		return 1;
-	}
-
-	env_flash = dev_get_uclass_priv(new);
-#else
-
-	if (!env_flash) {
-		env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
-			CONFIG_ENV_SPI_CS,
-			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
-		if (!env_flash) {
-			set_default_env("!spi_flash_probe() failed");
-			return 1;
-		}
-	}
-#endif
+	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) {
Signature · 1 line
2.11.0
author
#2 lore raw

PATCH v2 2/4 enf_sf: reuse setup_flash_device instead of open coding it

In reply to Andreas Fenkart

setup_flash_device selects one of two code paths depending on the driver model being used (=CONFIG_DM_SPI_FLASH). env_relocate_spec only used the non driver-model code path. I'm unsure why, either none of the platforms that need relocation use the driver model, or - worse - the driver model is not yet usable when relocating.

Signed-off-by Andreas Fenkart <[email protected]>
---
 common/env_sf.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
common/env_sf.c +5 -9
index 8af590a3d9..a52fb734c8 100644
@@ -176,12 +176,9 @@ void env_relocate_spec(void)
 		goto out;
 	}
 
-	env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
-			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
-	if (!env_flash) {
-		set_default_env("!spi_flash_probe() failed");
+	ret = setup_flash_device();
+	if (ret)
 		goto out;
-	}
 
 	ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
 				CONFIG_ENV_SIZE, tmp_env1);

@@ -316,10 +313,9 @@ void env_relocate_spec(void)
 	char *buf = NULL;
 
 	buf = (char *)memalign(ARCH_DMA_MINALIGN, CONFIG_ENV_SIZE);
-	env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
-			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
-	if (!env_flash) {
-		set_default_env("!spi_flash_probe() failed");
+
+	ret = setup_flash_device();
+	if (ret) {
 		if (buf)
 			free(buf);
 		return;
Signature · 1 line
2.11.0
#3 lore raw

PATCH v2 2/4 enf_sf: reuse setup_flash_device instead of open coding it

Quoted 10 lines · Andreas Fenkart

setup_flash_device selects one of two code paths depending on the driver model being used (=CONFIG_DM_SPI_FLASH). env_relocate_spec only used the non driver-model code path. I'm unsure why, either none of the platforms that need relocation use the driver model, or - worse - the driver model is not yet usable when relocating.

Signed-off-by Andreas Fenkart <[email protected]>
---
 common/env_sf.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
Reviewed-by Simon Glass <[email protected]>
author
#4 lore raw

PATCH v2 3/4 env_sf: re-order error handling in single-buffer env_relocate_spec

In reply to Andreas Fenkart

this makes it easier comparable to the double-buffered version

Signed-off-by Andreas Fenkart <[email protected]>
Reviewed-by Simon Glass <[email protected]>
---
 common/env_sf.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
common/env_sf.c +11 -9
index a52fb734c8..6a1583ebec 100644
@@ -313,29 +313,31 @@ void env_relocate_spec(void)
 	char *buf = NULL;
 
 	buf = (char *)memalign(ARCH_DMA_MINALIGN, CONFIG_ENV_SIZE);
-
-	ret = setup_flash_device();
-	if (ret) {
-		if (buf)
-			free(buf);
+	if (!buf) {
+		set_default_env("!malloc() failed");
 		return;
 	}
 
+	ret = setup_flash_device();
+	if (ret)
+		goto out;
+
 	ret = spi_flash_read(env_flash,
 		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
 	if (ret) {
 		set_default_env("!spi_flash_read() failed");
-		goto out;
+		goto err_read;
 	}
 
 	ret = env_import(buf, 1);
 	if (ret)
 		gd->env_valid = 1;
-out:
+
+err_read:
 	spi_flash_free(env_flash);
-	if (buf)
-		free(buf);
 	env_flash = NULL;
+out:
+	free(buf);
 }
 #endif
Signature · 1 line
2.11.0
author
#6 lore raw

PATCH v2 4/4 env_sf: use DIV_ROUND_UP to calculate number of sectors to erase

In reply to Andreas Fenkart

simpler to read

Signed-off-by Andreas Fenkart <[email protected]>
---
 common/env_sf.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)
common/env_sf.c +5 -13
index 6a1583ebec..9944602367 100644
@@ -80,7 +80,7 @@ int saveenv(void)
 {
 	env_t	env_new;
 	char	*saved_buffer = NULL, flag = OBSOLETE_FLAG;
-	u32	saved_size, saved_offset, sector = 1;
+	u32	saved_size, saved_offset, sector;
 	int	ret;
 
 	ret = setup_flash_device();

@@ -115,11 +115,7 @@ int saveenv(void)
 			goto done;
 	}
 
-	if (CONFIG_ENV_SIZE > CONFIG_ENV_SECT_SIZE) {
-		sector = CONFIG_ENV_SIZE / CONFIG_ENV_SECT_SIZE;
-		if (CONFIG_ENV_SIZE % CONFIG_ENV_SECT_SIZE)
-			sector++;
-	}
+	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
 
 	puts("Erasing SPI flash...");
 	ret = spi_flash_erase(env_flash, env_new_offset,

@@ -245,7 +241,7 @@ out:
 #else
 int saveenv(void)
 {
-	u32	saved_size, saved_offset, sector = 1;
+	u32	saved_size, saved_offset, sector;
 	char	*saved_buffer = NULL;
 	int	ret = 1;
 	env_t	env_new;

@@ -268,16 +264,12 @@ int saveenv(void)
 			goto done;
 	}
 
-	if (CONFIG_ENV_SIZE > CONFIG_ENV_SECT_SIZE) {
-		sector = CONFIG_ENV_SIZE / CONFIG_ENV_SECT_SIZE;
-		if (CONFIG_ENV_SIZE % CONFIG_ENV_SECT_SIZE)
-			sector++;
-	}
-
 	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);
Signature · 1 line
2.11.0
#8 lore raw

v2 cover env_sf: minor refactorings

In reply to Andreas Fenkart
Quoted 9 lines · Andreas Fenkart

rebased patches on master
fixed compiled error in 1st patch (missing variable declaration) compile tested all patches with travis

Andreas Fenkart (4):
env_sf: factor out prepare_flash_device
enf_sf: reuse setup_flash_device instead of open coding it
env_sf: re-order error handling in single-buffer env_relocate_spec
env_sf: use DIV_ROUND_UP to calculate number of sectors to erase

Reviewed-by Jagan Teki <[email protected]>
Tested-by Jagan Teki <[email protected]>

Applied to u-boot-spi/next

thanks!

Signature · 4 lines
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.