Serial Flash: call spi_flash_free more coherently

Some board require spi_flash_free to be called after all the accesses, in order, for instance, to restore the pin multiplexing configuration in the case where the SPI pins are multiplexed.

This patch series tries to enhance this. Patch 1 adds spi_flash_free calls to env_sf so that the SPI interface is always "cleaned up" after the env read/writes. Patch 2 adds a 'sf release' command that implicitly calls spi_flash_free and is thus the pendant of 'sf probe'. Patch 3 uses the 'sf command' for the km_arm board scripts.

The whole series had already been sent more than a year ago [1] but it was rejected without any feedback. So I send this rebased v2 so that it finally gets reviewed and merged.

[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/169723

Changes in v2:
- Rebased on v2014.10

Valentin Longchamp (3):
env_sf: generalize call to spi_flash_free after accesses
cmd_sf: add 'release' command
km_arm: call 'sf release' in the newenv and update scripts

 common/cmd_sf.c             | 13 ++++++++++++-
 common/env_sf.c             | 34 ++++++++++++++++------------------
 include/configs/km/km_arm.h |  6 ++++--
 3 files changed, 32 insertions(+), 21 deletions(-)

1.8.0.1

Some board require spi_flash_free to be called after all the accesses, in order, for instance, to restore the pin multiplexing configuration in the case where the SPI pins are multiplexed.

This was done only in env_relocate_spec and not in saveenv. saveenv is thus changed in order to have the same behavior as env_relocate_spec.

Since the static env_flash variable will be NULL after every function call, it is thus removed and its functionality is replaced by a systematic call to spi_flash_probe at the start of both env_relocate_spec and saveenv.

---

Changes in v2:
- Rebased on v2014.10

 common/env_sf.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

index 37ab13a..823f4ad 100644
@@ -41,24 +41,20 @@ DECLARE_GLOBAL_DATA_PTR;
 
 char *env_name_spec = "SPI Flash";
 
-static struct spi_flash *env_flash;
-
 #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;
+	struct spi_flash *env_flash = NULL;
 	int	ret;
 
+	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) {
-		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;
-		}
+		set_default_env("!spi_flash_probe() failed");
+		return 1;
 	}
 
 	ret = env_export(&env_new);

@@ -130,6 +126,8 @@ int saveenv(void)
 	if (saved_buffer)
 		free(saved_buffer);
 
+	spi_flash_free(env_flash);
+
 	return ret;
 }
 

@@ -140,6 +138,7 @@ void env_relocate_spec(void)
 	env_t *tmp_env1 = NULL;
 	env_t *tmp_env2 = NULL;
 	env_t *ep = NULL;
+	struct spi_flash *env_flash = NULL;
 
 	tmp_env1 = (env_t *)malloc(CONFIG_ENV_SIZE);
 	tmp_env2 = (env_t *)malloc(CONFIG_ENV_SIZE);

@@ -211,7 +210,6 @@ void env_relocate_spec(void)
 
 err_read:
 	spi_flash_free(env_flash);
-	env_flash = NULL;
 out:
 	free(tmp_env1);
 	free(tmp_env2);

@@ -223,15 +221,13 @@ int saveenv(void)
 	char	*saved_buffer = NULL;
 	int	ret = 1;
 	env_t	env_new;
+	struct  spi_flash *env_flash = NULL;
 
+	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) {
-		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;
-		}
+		set_default_env("!spi_flash_probe() failed");
+		return 1;
 	}
 
 	/* Is the sector larger than the env (i.e. embedded) */

@@ -284,11 +280,14 @@ int saveenv(void)
 	if (saved_buffer)
 		free(saved_buffer);
 
+	spi_flash_free(env_flash);
+
 	return ret;
 }
 
 void env_relocate_spec(void)
 {
+	struct  spi_flash *env_flash = NULL;
 	int ret;
 	char *buf = NULL;
 

@@ -316,7 +315,6 @@ out:
 	spi_flash_free(env_flash);
 	if (buf)
 		free(buf);
-	env_flash = NULL;
 }
 #endif
 

1.8.0.1

The release command is the pendant of the probe command. This command allows to call spi_flash_free from the command line. This may be necessary for some boards where sf probe does change the state of the hardware (like with some pin multiplexing changes for instance).

---

Changes in v2: None

 common/cmd_sf.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

index c60e8d1..453b466 100644
@@ -121,6 +121,14 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 	return 0;
 }
 
+static int do_spi_flash_release(int argc, char * const argv[])
+{
+	if (flash)
+		spi_flash_free(flash);
+	flash = NULL;
+
+	return 0;
+}
 /**
  * Write a block of data to SPI flash, first checking if it is different from
  * what is already there.

@@ -504,6 +512,8 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc,
 		ret = do_spi_flash_read_write(argc, argv);
 	else if (strcmp(cmd, "erase") == 0)
 		ret = do_spi_flash_erase(argc, argv);
+	else if (strcmp(cmd, "release") == 0)
+		ret = do_spi_flash_release(argc, argv);
 #ifdef CONFIG_CMD_SF_TEST
 	else if (!strcmp(cmd, "test"))
 		ret = do_spi_flash_test(argc, argv);

@@ -538,6 +548,7 @@ U_BOOT_CMD(
 	"sf erase offset [+]len		- erase `len' bytes from `offset'\n"
 	"				  `+len' round up `len' to block size\n"
 	"sf update addr offset len	- erase and write `len' bytes from memory\n"
-	"				  at `addr' to flash at `offset'"
+	"				  at `addr' to flash at `offset'\n"
+	"sf release			- release the current flash device"
 	SF_TEST_HELP
 );

1.8.0.1

This is necessary to make sure that all the pins used for SPI access, especially the CS, are configured back to the NAND Flash interface. Otherwise, if either newenv or update are called, u-boot cannot access the NAND Flash anymore.

---

Changes in v2: None

 include/configs/km/km_arm.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index d31e674..d8f86b4 100644
@@ -273,13 +273,15 @@ int get_scl(void);
 #define	CONFIG_KM_UPDATE_UBOOT						\
 	"update="							\
 		"sf probe 0;sf erase 0 +${filesize};"			\
-		"sf write ${load_addr_r} 0 ${filesize};\0"
+		"sf write ${load_addr_r} 0 ${filesize};"		\
+		"sf release\0"
 
 #if defined CONFIG_KM_ENV_IS_IN_SPI_NOR
 #define CONFIG_KM_NEW_ENV						\
 	"newenv=sf probe 0;"						\
 		"sf erase " __stringify(CONFIG_ENV_OFFSET) " "		\
-		__stringify(CONFIG_ENV_TOTAL_SIZE)"\0"
+		__stringify(CONFIG_ENV_TOTAL_SIZE)";"			\
+		"sf release\0"
 #else
 #define CONFIG_KM_NEW_ENV						\
 	"newenv=setenv addr 0x100000 && "				\

1.8.0.1

So, for each probe calls you must need to free the flash even-though the flash is-in-use, looks some different behavior.

Can you pleas elaborate little more.

This may not be an optimistic solutions where we couldn't add command to have generic behavior which does at-least on probe time - IMHO.

thanks!

Jagan.

To be precise, AFTER the flash has been used is a better description. The flash is not in use anymore.

Let me explain you our use case more in details. In our case the pins used for accessing the SPI NOR flash (contains u-boot and the environment) are multiplexed with the pins used for accessing the NAND Flash.

In an usual boot sequence, the SPI NOR is accessed first (copying u-boot to the RAM, reading out the environment) and so the pins are configured in hardware at boot time for accessing the SPI NOR. After that, they are configured to access the NAND where the kernel and filesystem are stored to boot Linux thanks to env_relocate_spec() calling spi_flash_free() on exit in conjunction with [1]

Now in the case where the boot sequence is interrupted and some accesses are done to the SPI NOR, the pins are changed again temporarily to NOR to do these accesses. But we have to make sure that the pins are configured back to NAND by calling spi_flash_free() after these accesses. In our case, there are 2 types of such accesses:
- environment variables write: this is the first patch of the series. It simply adds calls to spi_flash_free() at function exit no only in env_relocate_spec() but also in saveenv() so that the behavior here is coherent for the whole env_sf file (spi_flash_probe() at function start, spi_flash_free() at function exit).
- updating u-boot: this is solved for us with the last 2 patches of the series. The first one just adds a sf release command that does the opposite/cleanup to sf probe and the second patch just calls this command in our scripts where u-boot is updated/the SPI NOR is written.

[1] http://patchwork.ozlabs.org/patch/162307/

Do you mean optimal ? Anyway I don't understand your sentence/what you mean here. Could you rephrase it, please ?

But if we have a sf probe command, why can't we have a sf release command ?

Valentin