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

[U-Boot] [PATCH v3 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 2 years ago [1] but it
was rejected without any feedback. So I send this rebased v2 so that it
finally gets reviewed and merged. This was not successfull either, again
without any given feedback. So this is a 3rd attempt to get some
feedback to get this mailined.

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

Changes in v3:
- Rebased on v2015.10
- Rebased on v2015.10

Changes in v2:
- Rebased on v2014.10

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

 common/cmd_sf.c             | 11 +++++++++++
 common/env_sf.c             | 28 ++++++++++++++--------------
 include/configs/km/km_arm.h |  6 ++++--
 3 files changed, 29 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[U-Boot] [PATCH v3 1/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 v3:
- Rebased on v2015.10

Changes in v2:
- Rebased on v2014.10

 common/cmd_sf.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index ac7f5df..9c51dca 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -152,6 +152,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.
@@ -540,6 +548,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);
@@ -579,5 +589,6 @@ U_BOOT_CMD(
 	"sf update addr offset|partition len	- erase and write `len' bytes from memory\n"
 	"					  at `addr' to flash at `offset'\n"
 	"					  or to start of mtd `partition'\n"
+	"sf release				- release the current flash device\n"
 	SF_TEST_HELP
 );
-- 
1.8.3.1

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Jagan Teki]

On 13 November 2015 at 18:55, Valentin Longchamp
<valentin.longchamp@keymile.com> wrote:
> 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).

So you want to change the state of pin multiplexing on your board with
connected slave devices example: spi nor flash is it? what exactly the
need of releasing? why can't we use pin multiplexing changes like
selecting or deselecting particular lines through driver or from board
files itself.

>
> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
> ---
>
> Changes in v3:
> - Rebased on v2015.10
>
> Changes in v2:
> - Rebased on v2014.10
>
>  common/cmd_sf.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index ac7f5df..9c51dca 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -152,6 +152,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.
> @@ -540,6 +548,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);
> @@ -579,5 +589,6 @@ U_BOOT_CMD(
>         "sf update addr offset|partition len    - erase and write `len' bytes from memory\n"
>         "                                         at `addr' to flash at `offset'\n"
>         "                                         or to start of mtd `partition'\n"
> +       "sf release                             - release the current flash device\n"
>         SF_TEST_HELP
>  );
> --
> 1.8.3.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Jagan | openedev.

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Valentin Longchamp]

On 19/11/2015 17:57, Jagan Teki wrote:
> On 13 November 2015 at 18:55, Valentin Longchamp
> <valentin.longchamp@keymile.com> wrote:
>> 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).
> 
> So you want to change the state of pin multiplexing on your board with
> connected slave devices example: spi nor flash is it? what exactly the
> need of releasing? why can't we use pin multiplexing changes like
> selecting or deselecting particular lines through driver or from board
> files itself.

That's our use case yes. Let me explain you it again in detail. Some of the
signals used to access the NAND Flash and the SPI NOR are shared. At reset, they
are available for the SPI NOR, since u-boot is in there and the CPU then
accesses it.

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 to SPI NOR to perform these
accesses. But we have to make sure that the pins are configured back to NAND by
calling spi_flash_free() after these accesses and that's why I introduced the
release() function.

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.

We are *indeed* using pin multiplexing changes, in our case, they are
implemented in the spi controller driver: drivers/spi/kirkwood_spi.c. To be very
specific, in our case this sf release command allows to explicitely call
spi_flash_free() which calls spi_free_slave(), which in our case
(kirkwood_spi.c) sets the pins back to their previous configuration.

> 
>>
>> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
>> ---
>>
>> Changes in v3:
>> - Rebased on v2015.10
>>
>> Changes in v2:
>> - Rebased on v2014.10
>>
>>  common/cmd_sf.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>> index ac7f5df..9c51dca 100644
>> --- a/common/cmd_sf.c
>> +++ b/common/cmd_sf.c
>> @@ -152,6 +152,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.
>> @@ -540,6 +548,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);
>> @@ -579,5 +589,6 @@ U_BOOT_CMD(
>>         "sf update addr offset|partition len    - erase and write `len' bytes from memory\n"
>>         "                                         at `addr' to flash at `offset'\n"
>>         "                                         or to start of mtd `partition'\n"
>> +       "sf release                             - release the current flash device\n"
>>         SF_TEST_HELP
>>  );
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
> 

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Simon Glass]

Hi,

On 20 November 2015 at 03:13, Valentin Longchamp
<valentin.longchamp@keymile.com> wrote:
> On 19/11/2015 17:57, Jagan Teki wrote:
>> On 13 November 2015 at 18:55, Valentin Longchamp
>> <valentin.longchamp@keymile.com> wrote:
>>> 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).
>>
>> So you want to change the state of pin multiplexing on your board with
>> connected slave devices example: spi nor flash is it? what exactly the
>> need of releasing? why can't we use pin multiplexing changes like
>> selecting or deselecting particular lines through driver or from board
>> files itself.
>
> That's our use case yes. Let me explain you it again in detail. Some of the
> signals used to access the NAND Flash and the SPI NOR are shared. At reset, they
> are available for the SPI NOR, since u-boot is in there and the CPU then
> accesses it.
>
> 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 to SPI NOR to perform these
> accesses. But we have to make sure that the pins are configured back to NAND by
> calling spi_flash_free() after these accesses and that's why I introduced the
> release() function.
>
> 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.
>
> We are *indeed* using pin multiplexing changes, in our case, they are
> implemented in the spi controller driver: drivers/spi/kirkwood_spi.c. To be very
> specific, in our case this sf release command allows to explicitely call
> spi_flash_free() which calls spi_free_slave(), which in our case
> (kirkwood_spi.c) sets the pins back to their previous configuration.

Does your board use driver model from SPI and SPI flash? If not I
think that should be the first step.

Regards,
Simon
[snip]

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Valentin Longchamp]

Hi Simon,

On 20/11/2015 18:19, Simon Glass wrote:
> Hi,
> 
> On 20 November 2015 at 03:13, Valentin Longchamp
> <valentin.longchamp@keymile.com> wrote:
>> On 19/11/2015 17:57, Jagan Teki wrote:
>>> On 13 November 2015 at 18:55, Valentin Longchamp
>>> <valentin.longchamp@keymile.com> wrote:
>>>> 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).
>>>
>>> So you want to change the state of pin multiplexing on your board with
>>> connected slave devices example: spi nor flash is it? what exactly the
>>> need of releasing? why can't we use pin multiplexing changes like
>>> selecting or deselecting particular lines through driver or from board
>>> files itself.
>>
>> That's our use case yes. Let me explain you it again in detail. Some of the
>> signals used to access the NAND Flash and the SPI NOR are shared. At reset, they
>> are available for the SPI NOR, since u-boot is in there and the CPU then
>> accesses it.
>>
>> 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 to SPI NOR to perform these
>> accesses. But we have to make sure that the pins are configured back to NAND by
>> calling spi_flash_free() after these accesses and that's why I introduced the
>> release() function.
>>
>> 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.
>>
>> We are *indeed* using pin multiplexing changes, in our case, they are
>> implemented in the spi controller driver: drivers/spi/kirkwood_spi.c. To be very
>> specific, in our case this sf release command allows to explicitely call
>> spi_flash_free() which calls spi_free_slave(), which in our case
>> (kirkwood_spi.c) sets the pins back to their previous configuration.
> 
> Does your board use driver model from SPI and SPI flash? If not I
> think that should be the first step.
> 

No we don't. Could you please elaborate on how this would cover this use case
and should be the first step ?

I am open to other ways to cover this use case of ours, especially since this
was done more than 2 years ago and u-boot has changed since then. However I
don't see the direct link between the driver model and how it would allow to
make sure spi_flash_free() is called in our u-boot env scripts.

Valentin

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Simon Glass]

Hi Valentine,

On 23 November 2015 at 02:19, Valentin Longchamp
<valentin.longchamp@keymile.com> wrote:
> Hi Simon,
>
> On 20/11/2015 18:19, Simon Glass wrote:
>> Hi,
>>
>> On 20 November 2015 at 03:13, Valentin Longchamp
>> <valentin.longchamp@keymile.com> wrote:
>>> On 19/11/2015 17:57, Jagan Teki wrote:
>>>> On 13 November 2015 at 18:55, Valentin Longchamp
>>>> <valentin.longchamp@keymile.com> wrote:
>>>>> 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).
>>>>
>>>> So you want to change the state of pin multiplexing on your board with
>>>> connected slave devices example: spi nor flash is it? what exactly the
>>>> need of releasing? why can't we use pin multiplexing changes like
>>>> selecting or deselecting particular lines through driver or from board
>>>> files itself.
>>>
>>> That's our use case yes. Let me explain you it again in detail. Some of the
>>> signals used to access the NAND Flash and the SPI NOR are shared. At reset, they
>>> are available for the SPI NOR, since u-boot is in there and the CPU then
>>> accesses it.
>>>
>>> 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 to SPI NOR to perform these
>>> accesses. But we have to make sure that the pins are configured back to NAND by
>>> calling spi_flash_free() after these accesses and that's why I introduced the
>>> release() function.
>>>
>>> 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.
>>>
>>> We are *indeed* using pin multiplexing changes, in our case, they are
>>> implemented in the spi controller driver: drivers/spi/kirkwood_spi.c. To be very
>>> specific, in our case this sf release command allows to explicitely call
>>> spi_flash_free() which calls spi_free_slave(), which in our case
>>> (kirkwood_spi.c) sets the pins back to their previous configuration.
>>
>> Does your board use driver model from SPI and SPI flash? If not I
>> think that should be the first step.
>>
>
> No we don't. Could you please elaborate on how this would cover this use case
> and should be the first step ?
>
> I am open to other ways to cover this use case of ours, especially since this
> was done more than 2 years ago and u-boot has changed since then. However I
> don't see the direct link between the driver model and how it would allow to
> make sure spi_flash_free() is called in our u-boot env scripts.

In driver model you would have a remove() method for your SPI driver
which does the required pinmux changing.

There is a detailed howto in doc/driver-model showing how to port your
driver over. Please let me know if you need any help/ideas.

Regards,
Simon

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Valentin Longchamp]

On 24/11/2015 02:49, Simon Glass wrote:
> Hi Valentine,
> 
> On 23 November 2015 at 02:19, Valentin Longchamp
> <valentin.longchamp@keymile.com> wrote:
>> Hi Simon,
>>
>> On 20/11/2015 18:19, Simon Glass wrote:
>>> Hi,
>>>
>>> On 20 November 2015 at 03:13, Valentin Longchamp
>>> <valentin.longchamp@keymile.com> wrote:
>>>> On 19/11/2015 17:57, Jagan Teki wrote:
>>>>> On 13 November 2015 at 18:55, Valentin Longchamp
>>>>> <valentin.longchamp@keymile.com> wrote:
>>>>>> 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).
>>>>>
>>>>> So you want to change the state of pin multiplexing on your board with
>>>>> connected slave devices example: spi nor flash is it? what exactly the
>>>>> need of releasing? why can't we use pin multiplexing changes like
>>>>> selecting or deselecting particular lines through driver or from board
>>>>> files itself.
>>>>
>>>> That's our use case yes. Let me explain you it again in detail. Some of the
>>>> signals used to access the NAND Flash and the SPI NOR are shared. At reset, they
>>>> are available for the SPI NOR, since u-boot is in there and the CPU then
>>>> accesses it.
>>>>
>>>> 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 to SPI NOR to perform these
>>>> accesses. But we have to make sure that the pins are configured back to NAND by
>>>> calling spi_flash_free() after these accesses and that's why I introduced the
>>>> release() function.
>>>>
>>>> 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.
>>>>
>>>> We are *indeed* using pin multiplexing changes, in our case, they are
>>>> implemented in the spi controller driver: drivers/spi/kirkwood_spi.c. To be very
>>>> specific, in our case this sf release command allows to explicitely call
>>>> spi_flash_free() which calls spi_free_slave(), which in our case
>>>> (kirkwood_spi.c) sets the pins back to their previous configuration.
>>>
>>> Does your board use driver model from SPI and SPI flash? If not I
>>> think that should be the first step.
>>>
>>
>> No we don't. Could you please elaborate on how this would cover this use case
>> and should be the first step ?
>>
>> I am open to other ways to cover this use case of ours, especially since this
>> was done more than 2 years ago and u-boot has changed since then. However I
>> don't see the direct link between the driver model and how it would allow to
>> make sure spi_flash_free() is called in our u-boot env scripts.
> 
> In driver model you would have a remove() method for your SPI driver
> which does the required pinmux changing.

OK, when looking at SPI flash, SPI controller driver and the driver model, I
have found out why we require this "release" command.

So the SPI subsystem and its drivers (with or without DM support) make sure that
spi_claim_bus/release_bus are called before/after the accesses. This should
cover the pinmux configuration stuff.

In our case, it was not sufficient since drivers/spi/kirkwood_spi.c does the
pinmux configuration at 2 places: spi_claim_bus/release_bus for the IO
(MOSI/MISO/CLK) AND spi_setup_slave/free_slave for the chip select.

Since spi_free_slave is not always called after spi_setup_slave (for instance
after a sf probe command on the u-boot prompt), the CS pin was not always given
back to the NAND flash controller that's why there was a need for a sf release
command in our case.

I have now moved all the pinmux configuration for kirkwood_spi into
spi_claim_bus/release_bus and the need for this sf release command is not
necessary anymore.

I am going to test this a bit more and send a new patch series which will not
require this release command.

> 
> There is a detailed howto in doc/driver-model showing how to port your
> driver over. Please let me know if you need any help/ideas.

I have had a look at it and it is a great help on porting a driver to this
model. That's something we plan to do for our boards eventually.

Valentin

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Stefan Roese]

Hi Valentine,

On 24.11.2015 17:02, Valentin Longchamp wrote:

<snip>

>>>> Does your board use driver model from SPI and SPI flash? If not I
>>>> think that should be the first step.
>>>>
>>>
>>> No we don't. Could you please elaborate on how this would cover this use case
>>> and should be the first step ?
>>>
>>> I am open to other ways to cover this use case of ours, especially since this
>>> was done more than 2 years ago and u-boot has changed since then. However I
>>> don't see the direct link between the driver model and how it would allow to
>>> make sure spi_flash_free() is called in our u-boot env scripts.
>>
>> In driver model you would have a remove() method for your SPI driver
>> which does the required pinmux changing.
>
> OK, when looking at SPI flash, SPI controller driver and the driver model, I
> have found out why we require this "release" command.
>
> So the SPI subsystem and its drivers (with or without DM support) make sure that
> spi_claim_bus/release_bus are called before/after the accesses. This should
> cover the pinmux configuration stuff.
>
> In our case, it was not sufficient since drivers/spi/kirkwood_spi.c does the
> pinmux configuration at 2 places: spi_claim_bus/release_bus for the IO
> (MOSI/MISO/CLK) AND spi_setup_slave/free_slave for the chip select.
>
> Since spi_free_slave is not always called after spi_setup_slave (for instance
> after a sf probe command on the u-boot prompt), the CS pin was not always given
> back to the NAND flash controller that's why there was a need for a sf release
> command in our case.
>
> I have now moved all the pinmux configuration for kirkwood_spi into
> spi_claim_bus/release_bus and the need for this sf release command is not
> necessary anymore.
>
> I am going to test this a bit more and send a new patch series which will not
> require this release command.
>
>>
>> There is a detailed howto in doc/driver-model showing how to port your
>> driver over. Please let me know if you need any help/ideas.
>
> I have had a look at it and it is a great help on porting a driver to this
> model. That's something we plan to do for our boards eventually.

So did you already port the kirkwood SPI driver to DM? I'm asking, since
I also do have a patch for this in the queue. I'll hopefully be able
to post it by end of this week.

BTW: I would love to see the Kirkwood platform to be moved over to
arch-mvebu at some time, which already supports DM.

Thanks,
Stefan

[U-Boot] [PATCH v3 1/3] cmd_sf: add 'release' command

[Valentin Longchamp]

On 25/11/2015 04:52, Stefan Roese wrote:
> Hi Valentine,
> 
> On 24.11.2015 17:02, Valentin Longchamp wrote:
> 
> <snip>
> 
>>>>> Does your board use driver model from SPI and SPI flash? If not I
>>>>> think that should be the first step.
>>>>>
>>>>
>>>> No we don't. Could you please elaborate on how this would cover this use case
>>>> and should be the first step ?
>>>>
>>>> I am open to other ways to cover this use case of ours, especially since this
>>>> was done more than 2 years ago and u-boot has changed since then. However I
>>>> don't see the direct link between the driver model and how it would allow to
>>>> make sure spi_flash_free() is called in our u-boot env scripts.
>>>
>>> In driver model you would have a remove() method for your SPI driver
>>> which does the required pinmux changing.
>>
>> OK, when looking at SPI flash, SPI controller driver and the driver model, I
>> have found out why we require this "release" command.
>>
>> So the SPI subsystem and its drivers (with or without DM support) make sure that
>> spi_claim_bus/release_bus are called before/after the accesses. This should
>> cover the pinmux configuration stuff.
>>
>> In our case, it was not sufficient since drivers/spi/kirkwood_spi.c does the
>> pinmux configuration at 2 places: spi_claim_bus/release_bus for the IO
>> (MOSI/MISO/CLK) AND spi_setup_slave/free_slave for the chip select.
>>
>> Since spi_free_slave is not always called after spi_setup_slave (for instance
>> after a sf probe command on the u-boot prompt), the CS pin was not always given
>> back to the NAND flash controller that's why there was a need for a sf release
>> command in our case.
>>
>> I have now moved all the pinmux configuration for kirkwood_spi into
>> spi_claim_bus/release_bus and the need for this sf release command is not
>> necessary anymore.
>>
>> I am going to test this a bit more and send a new patch series which will not
>> require this release command.
>>
>>>
>>> There is a detailed howto in doc/driver-model showing how to port your
>>> driver over. Please let me know if you need any help/ideas.
>>
>> I have had a look at it and it is a great help on porting a driver to this
>> model. That's something we plan to do for our boards eventually.
> 
> So did you already port the kirkwood SPI driver to DM? I'm asking, since
> I also do have a patch for this in the queue. I'll hopefully be able
> to post it by end of this week.

No I didn't. I was just looking at what should be done for that. For my above
problem I want to send a patch to the current driver without porting it at all.

> 
> BTW: I would love to see the Kirkwood platform to be moved over to
> arch-mvebu at some time, which already supports DM.
> 

Tell me if you're doing something in this area, I may give some help or test a
few things, according to the remaining time besides my current projects. The
Kirkwood platform would definitely be better if this was done.

Valentin

[U-Boot] [PATCH v3 2/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 v3:
- Rebased on v2015.10

Changes in v2: None

 common/env_sf.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/common/env_sf.c b/common/env_sf.c
index 9409831..19fcbd3 100644
--- a/common/env_sf.c
+++ b/common/env_sf.c
@@ -52,14 +52,11 @@ int saveenv(void)
 	u32	saved_size, saved_offset, sector = 1;
 	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);
@@ -131,6 +128,9 @@ int saveenv(void)
 	if (saved_buffer)
 		free(saved_buffer);
 
+	spi_flash_free(env_flash);
+	env_flash = NULL;
+
 	return ret;
 }
 
@@ -228,14 +228,11 @@ int saveenv(void)
 	int	ret = 1;
 	env_t	env_new;
 
+	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) */
@@ -288,6 +285,9 @@ int saveenv(void)
 	if (saved_buffer)
 		free(saved_buffer);
 
+	spi_flash_free(env_flash);
+	env_flash = NULL;
+
 	return ret;
 }
 
-- 
1.8.3.1

[U-Boot] [PATCH v3 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 v3: None
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 15fca1a..7a0d3b1 100644
--- a/include/configs/km/km_arm.h
+++ b/include/configs/km/km_arm.h
@@ -271,13 +271,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.3.1