[U-Boot] [PATCH v2 0/3] Serial Flash: call spi_flash_free more coherently

[U-Boot] [PATCH v2 0/3] Serial Flash: call spi_flash_free more coherently

[Valentin Longchamp]

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

[U-Boot] [PATCH v2 1/3] env_sf: generalize call to spi_flash_free after accesses

[Valentin Longchamp]

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.

Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>

---

Changes in v2:
- Rebased on v2014.10

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

diff --git a/common/env_sf.c b/common/env_sf.c
index 37ab13a..823f4ad 100644
--- a/common/env_sf.c
+++ b/common/env_sf.c
@@ -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

[U-Boot] [PATCH v2 2/3] cmd_sf: add 'release' command

[Valentin Longchamp]

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).

Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
---

Changes in v2: None

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

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index c60e8d1..453b466 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -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

[U-Boot] [PATCH v2 3/3] km_arm: call 'sf release' in the newenv and update scripts

[Valentin Longchamp]

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.

Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
---

Changes in v2: None

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

diff --git a/include/configs/km/km_arm.h b/include/configs/km/km_arm.h
index d31e674..d8f86b4 100644
--- a/include/configs/km/km_arm.h
+++ b/include/configs/km/km_arm.h
@@ -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

[U-Boot] [PATCH v2 0/3] Serial Flash: call spi_flash_free more coherently

[Jagan Teki]

On 3 November 2014 at 19:31, Valentin Longchamp
<valentin.longchamp@keymile.com> wrote:
> 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.

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 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

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.

>   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
>

thanks!
-- 
Jagan.

[U-Boot] [PATCH v2 0/3] Serial Flash: call spi_flash_free more coherently

[Valentin Longchamp]

On 12/18/2014 03:03 PM, Jagan Teki wrote:
> On 3 November 2014 at 19:31, Valentin Longchamp
> <valentin.longchamp@keymile.com> wrote:
>> 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.
> 
> So, for each probe calls you must need to free the flash even-though
> the flash is-in-use, looks some different behavior.

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

> 
> Can you pleas elaborate little more.

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/

> 
>>
>> 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
> 
> 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.

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 ?

> 
>>   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
>>
> 
> thanks!
> 

Valentin