[PATCH v4 0/5] sf: Add documentation and an 'sf mmap' command

[PATCH v4 0/5] sf: Add documentation and an 'sf mmap' command

[Simon Glass]

This little series adds documentation and a few other tidy-ups to the
'sf' command.

It also provides a way to access memory-mapped SPI via the command line.

Changes in v4:
- Split out the 'const' change into a separate patch
- Show the 'sf probe' output in the examples

Changes in v3:
- Add configuration and return value also

Changes in v2:
- Explain why 'usage' cannot be const
- Many fixes as suggested by Heinrich

Simon Glass (5):
  command: Use a constant pointer for the help
  sf: Use const for the stage name
  sf: Tidy up code to avoid #ifdef
  sf: doc: Add documentation for the 'sf' command
  sf: Provide a command to access memory-mapped SPI

 arch/Kconfig        |   2 +
 cmd/Kconfig         |   8 ++
 cmd/sf.c            |  67 +++++++---
 doc/usage/index.rst |   1 +
 doc/usage/sf.rst    | 308 ++++++++++++++++++++++++++++++++++++++++++++
 include/command.h   |   2 +-
 6 files changed, 369 insertions(+), 19 deletions(-)
 create mode 100644 doc/usage/sf.rst

-- 
2.33.0.464.g1972c5931b-goog

[PATCH v4 1/5] command: Use a constant pointer for the help

[Simon Glass]

This text should never change during execution, so it makes sense to
use a const char * so that it can be declared as const in the code.
Update struct cmd_tbl with a const char * pointer for 'help'.

We cannot make usage const because of the bmode command, used on mx53ppd
for example.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Explain why 'usage' cannot be const

 include/command.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/command.h b/include/command.h
index 137cfbc3231..f8e07a591c6 100644
--- a/include/command.h
+++ b/include/command.h
@@ -45,7 +45,7 @@ struct cmd_tbl {
 			       char *const argv[]);
 	char		*usage;		/* Usage message	(short)	*/
 #ifdef	CONFIG_SYS_LONGHELP
-	char		*help;		/* Help  message	(long)	*/
+	const char	*help;		/* Help  message	(long)	*/
 #endif
 #ifdef CONFIG_AUTO_COMPLETE
 	/* do auto completion on the arguments */
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v4 1/5] command: Use a constant pointer for the help

[Jagan Teki]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> This text should never change during execution, so it makes sense to
> use a const char * so that it can be declared as const in the code.
> Update struct cmd_tbl with a const char * pointer for 'help'.
>
> We cannot make usage const because of the bmode command, used on mx53ppd
> for example.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Explain why 'usage' cannot be const

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v4 1/5] command: Use a constant pointer for the help

[Heinrich Schuchardt]

On 9/19/21 23:49, Simon Glass wrote:
> This text should never change during execution, so it makes sense to
> use a const char * so that it can be declared as const in the code.
> Update struct cmd_tbl with a const char * pointer for 'help'.
>
> We cannot make usage const because of the bmode command, used on mx53ppd
> for example.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Explain why 'usage' cannot be const
>
>   include/command.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/command.h b/include/command.h
> index 137cfbc3231..f8e07a591c6 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -45,7 +45,7 @@ struct cmd_tbl {
>   			       char *const argv[]);
>   	char		*usage;		/* Usage message	(short)	*/
>   #ifdef	CONFIG_SYS_LONGHELP
> -	char		*help;		/* Help  message	(long)	*/
> +	const char	*help;		/* Help  message	(long)	*/
>   #endif
>   #ifdef CONFIG_AUTO_COMPLETE
>   	/* do auto completion on the arguments */
>

Re: [PATCH v4 1/5] command: Use a constant pointer for the help

[Simon Glass]

On 9/19/21 23:49, Simon Glass wrote:
> This text should never change during execution, so it makes sense to
> use a const char * so that it can be declared as const in the code.
> Update struct cmd_tbl with a const char * pointer for 'help'.
>
> We cannot make usage const because of the bmode command, used on mx53ppd
> for example.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Explain why 'usage' cannot be const
>
>   include/command.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to u-boot-dm, thanks!

Re: [PATCH v4 1/5] command: Use a constant pointer for the help

[Simon Glass]

On 9/19/21 23:49, Simon Glass wrote:
> This text should never change during execution, so it makes sense to
> use a const char * so that it can be declared as const in the code.
> Update struct cmd_tbl with a const char * pointer for 'help'.
>
> We cannot make usage const because of the bmode command, used on mx53ppd
> for example.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Explain why 'usage' cannot be const
>
>   include/command.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to u-boot-dm, thanks!
Applied to u-boot-dm/next, thanks!

[PATCH v4 2/5] sf: Use const for the stage name

[Simon Glass]

This is not updated at runtime so should be marked const. Update the code
accordingly.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 cmd/sf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index eac27ed2d77..15361a4bddf 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -394,7 +394,7 @@ enum {
 	STAGE_COUNT,
 };
 
-static char *stage_name[STAGE_COUNT] = {
+static const char *stage_name[STAGE_COUNT] = {
 	"erase",
 	"check",
 	"write",
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v4 2/5] sf: Use const for the stage name

[Jagan Teki]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> This is not updated at runtime so should be marked const. Update the code
> accordingly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v4 2/5] sf: Use const for the stage name

[Heinrich Schuchardt]

On 9/19/21 23:49, Simon Glass wrote:
> This is not updated at runtime so should be marked const. Update the code
> accordingly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v1)
>
>   cmd/sf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmd/sf.c b/cmd/sf.c
> index eac27ed2d77..15361a4bddf 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -394,7 +394,7 @@ enum {
>   	STAGE_COUNT,
>   };
>
> -static char *stage_name[STAGE_COUNT] = {
> +static const char *stage_name[STAGE_COUNT] = {
>   	"erase",
>   	"check",
>   	"write",
>

Re: [PATCH v4 2/5] sf: Use const for the stage name

[Simon Glass]

On 9/19/21 23:49, Simon Glass wrote:
> This is not updated at runtime so should be marked const. Update the code
> accordingly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v1)
>
>   cmd/sf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to u-boot-dm, thanks!

Re: [PATCH v4 2/5] sf: Use const for the stage name

[Simon Glass]

On 9/19/21 23:49, Simon Glass wrote:
> This is not updated at runtime so should be marked const. Update the code
> accordingly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v1)
>
>   cmd/sf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to u-boot-dm, thanks!
Applied to u-boot-dm/next, thanks!

[PATCH v4 3/5] sf: Tidy up code to avoid #ifdef

[Simon Glass]

Update this code to use IS_ENABLED() instead.

Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---

(no changes since v1)

 cmd/sf.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index 15361a4bddf..72246d912fe 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -384,7 +384,6 @@ static int do_spi_protect(int argc, char *const argv[])
 	return ret == 0 ? 0 : 1;
 }
 
-#ifdef CONFIG_CMD_SF_TEST
 enum {
 	STAGE_ERASE,
 	STAGE_CHECK,
@@ -548,7 +547,6 @@ static int do_spi_flash_test(int argc, char *const argv[])
 
 	return 0;
 }
-#endif /* CONFIG_CMD_SF_TEST */
 
 static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
 			char *const argv[])
@@ -582,10 +580,8 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
 		ret = do_spi_flash_erase(argc, argv);
 	else if (strcmp(cmd, "protect") == 0)
 		ret = do_spi_protect(argc, argv);
-#ifdef CONFIG_CMD_SF_TEST
-	else if (!strcmp(cmd, "test"))
+	else if (IS_ENABLED(CONFIG_CMD_SF_TEST) && !strcmp(cmd, "test"))
 		ret = do_spi_flash_test(argc, argv);
-#endif
 	else
 		ret = -1;
 
@@ -597,16 +593,8 @@ usage:
 	return CMD_RET_USAGE;
 }
 
-#ifdef CONFIG_CMD_SF_TEST
-#define SF_TEST_HELP "\nsf test offset len		" \
-		"- run a very basic destructive test"
-#else
-#define SF_TEST_HELP
-#endif
-
-U_BOOT_CMD(
-	sf,	5,	1,	do_spi_flash,
-	"SPI flash sub-system",
+#ifdef CONFIG_SYS_LONGHELP
+static const char long_help[] =
 	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
 	"				  and chip select\n"
 	"sf read addr offset|partition len	- read `len' bytes starting at\n"
@@ -622,6 +610,14 @@ U_BOOT_CMD(
 	"					  at `addr' to flash at `offset'\n"
 	"					  or to start of mtd `partition'\n"
 	"sf protect lock/unlock sector len	- protect/unprotect 'len' bytes starting\n"
-	"					  at address 'sector'\n"
-	SF_TEST_HELP
+	"					  at address 'sector'"
+#ifdef CONFIG_CMD_SF_TEST
+	"\nsf test offset len		- run a very basic destructive test"
+#endif
+#endif /* CONFIG_SYS_LONGHELP */
+	;
+
+U_BOOT_CMD(
+	sf,	5,	1,	do_spi_flash,
+	"SPI flash sub-system", long_help
 );
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v4 3/5] sf: Tidy up code to avoid #ifdef

[Pratyush Yadav]

Hi Simon,

On 19/09/21 03:49PM, Simon Glass wrote:
> Update this code to use IS_ENABLED() instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Nitpick: Trailers shouldn't have a blank line between them. I see it for 
this patch and 4/5 as well. It probably doesn't matter, but I wonder if 
it will trip up some tools that work on commit trailers like 
git-interpret-trailers. Something you might want to fix in your 
workflow...

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v4 3/5] sf: Tidy up code to avoid #ifdef

[Simon Glass]

Hi Pratyush,

On Mon, 20 Sept 2021 at 05:08, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Simon,
>
> On 19/09/21 03:49PM, Simon Glass wrote:
> > Update this code to use IS_ENABLED() instead.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>
> Nitpick: Trailers shouldn't have a blank line between them. I see it for
> this patch and 4/5 as well. It probably doesn't matter, but I wonder if
> it will trip up some tools that work on commit trailers like
> git-interpret-trailers. Something you might want to fix in your
> workflow...

The fix to 'patman status -d <new_branch>' is in mainline but was not
in the tree where I ran this tool, unfortunately.

So hopefully this won't happen again.

Regards,
Simon

Re: [PATCH v4 3/5] sf: Tidy up code to avoid #ifdef

[Jagan Teki]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> Update this code to use IS_ENABLED() instead.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v4 3/5] sf: Tidy up code to avoid #ifdef

[Simon Glass]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> Update this code to use IS_ENABLED() instead.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Applied to u-boot-dm, thanks!

Re: [PATCH v4 3/5] sf: Tidy up code to avoid #ifdef

[Simon Glass]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> Update this code to use IS_ENABLED() instead.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Applied to u-boot-dm, thanks!
Applied to u-boot-dm/next, thanks!

[PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Simon Glass]

This command is fairly complicated so documentation is useful.
Unfortunately I an not sure how the MTD side of things works and cannot
find information about that.

Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Pratyush Yadav <p.yadav@ti.com>
---

Changes in v4:
- Split out the 'const' change into a separate patch
- Show the 'sf probe' output in the examples

Changes in v2:
- Many fixes as suggested by Heinrich

 doc/usage/index.rst |   1 +
 doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 246 insertions(+)
 create mode 100644 doc/usage/sf.rst

diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 356f2a56181..9a7b12b7c54 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -43,6 +43,7 @@ Shell commands
    qfw
    reset
    sbi
+   sf
    scp03
    setexpr
    size
diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
new file mode 100644
index 00000000000..71bd1be5175
--- /dev/null
+++ b/doc/usage/sf.rst
@@ -0,0 +1,245 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+sf command
+==========
+
+Synopis
+-------
+
+::
+
+    sf probe [[[<bus>:]<cs>] [<hz> [<mode>]]]
+    sf read <addr> <offset>|<partition> <len>
+    sf write <addr> <offset>|<partition> <len>
+    sf erase <offset>|<partition> <len>
+    sf update <addr> <offset>|<partition> <len>
+    sf protect lock|unlock <sector> <len>
+    sf test <offset>|<partition> <len>
+
+Description
+-----------
+
+The *sf* command is used to access SPI flash, supporting read/write/erase and
+a few other functions.
+
+Probe
+-----
+
+The flash must first be probed with *sf probe* before any of the other
+subcommands can be used. All of the parameters are optional:
+
+bus
+	SPI bus number containing the SPI-flash chip, e.g. 0. If you don't know
+	the number, you can use 'dm uclass' to see all the spi devices,
+	and check the value for 'seq' for each one (here 0 and 2)::
+
+	   uclass 89: spi
+	   0     spi@0 @ 05484960, seq 0
+	   1     spi@1 @ 05484b40, seq 2
+
+cs
+	SPI chip-select to use for the chip. This is often 0 and can be omitted,
+	but in some cases multiple slaves are attached to a SPI controller,
+	selected by a chip-select line for each one.
+
+hz
+	Speed of the SPI bus in hertz. This normally defaults to 100000, i.e.
+	100KHz, which is very slow. Note that if the device exists in the
+	device tree, there might be a speed provided there, in which case this
+	setting is ignored.
+
+mode
+	SPI mode to use:
+
+	=====  ================
+	Mode   Meaning
+	=====  ================
+	0      CPOL=0, CPHA=0
+	1      CPOL=0, CPHA=1
+	2      CPOL=1, CPHA=0
+	3      CPOL=1, CPHA=1
+	=====  ================
+
+	Clock phase (CPHA) 0 means that data is transferred (sampled) on the
+	first clock edge; 1 means the second.
+
+	Clock polarity (CPOL) controls the idle state of the clock, 0 for low,
+	1 for high.
+	The active state is the opposite of idle.
+
+	You may find this `SPI documentation`_ useful.
+
+Parameters for other subcommands (described below) are as follows:
+
+addr
+	Memory address to start transfer
+
+offset
+	Flash offset to start transfer
+
+partition
+	If the parameter is not numeric, it is assumed to be a partition
+	description in the format <dev_type><dev_num>,<part_num> which is not
+	covered here. This requires CONFIG_CMD_MTDPARTS.
+
+len
+	Number of bytes to transfer
+
+Read
+~~~~
+
+Use *sf read* to read from SPI flash to memory. The read will fail if an
+attempt is made to read past the end of the flash.
+
+
+Write
+~~~~~
+
+Use *sf write* to write from memory to SPI flash. The SPI flash should be
+erased first, since otherwise the result is undefined.
+
+The write will fail if an attempt is made to read past the end of the flash.
+
+
+Erase
+~~~~~
+
+Use *sf erase* to erase a region of SPI flash. The erase will fail if any part
+of the region to be erased is protected or lies past the end of the flash. It
+may also fail if the start offset or length are not aligned to an erase region
+(e.g. 256 bytes).
+
+
+Update
+~~~~~~
+
+Use *sf update* to automatically erase and update a region of SPI flash from
+memory. This works a sector at a time (typical 4KB or 64KB). For each
+sector it first checks if the sector already has the right data. If so it is
+skipped. If not, the sector is erased and the new data written. Note that if
+the length is not a multiple of the erase size, the space after the data in
+the last sector will be erased. If the offset does not start at the beginning
+of an erase block, the operation will fail.
+
+Speed statistics are shown including the number of bytes that were already
+correct.
+
+
+Protect
+~~~~~~~
+
+SPI-flash chips often have a protection feature where the chip is split up into
+regions which can be locked or unlocked. With *sf protect* it is possible to
+change these settings, if supported by the driver.
+
+lock|unlock
+	Selects whether to lock or unlock the sectors
+
+<sector>
+	Start sector number to lock/unlock. This may be the byte offset or some
+	other value, depending on the chip.
+
+<len>
+	Number of bytes to lock/unlock
+
+
+Test
+~~~~
+
+A convenient and fast *sf test* subcommand provides a way to check that SPI
+flash is working as expected. This works in four stages:
+
+   * erase - erases the entire region
+   * check - checks that the region is erased
+   * write - writes a test pattern to the region, consisting of the U-Boot code
+   * read - reads back the test pattern to check that it was written correctly
+
+Memory is allocated for two buffers, each <len> bytes in size. At typical
+size is 64KB to 1MB. The offset and size must be aligned to an erase boundary.
+
+Note that this test will fail if any part of the SPI flash is write-protected.
+
+
+Examples
+--------
+
+This first example uses sandbox::
+
+   => sf probe
+   SF: Detected m25p16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB
+   => sf read 1000 1100 80000
+   device 0 offset 0x1100, size 0x80000
+   SF: 524288 bytes @ 0x1100 Read: OK
+   => md 1000
+   00001000: edfe0dd0 f33a0000 78000000 84250000    ......:....x..%.
+   00001010: 28000000 11000000 10000000 00000000    ...(............
+   00001020: 6f050000 0c250000 00000000 00000000    ...o..%.........
+   00001030: 00000000 00000000 00000000 00000000    ................
+   00001040: 00000000 00000000 00000000 00000000    ................
+   00001050: 00000000 00000000 00000000 00000000    ................
+   00001060: 00000000 00000000 00000000 00000000    ................
+   00001070: 00000000 00000000 01000000 00000000    ................
+   00001080: 03000000 04000000 00000000 01000000    ................
+   00001090: 03000000 04000000 0f000000 01000000    ................
+   000010a0: 03000000 08000000 1b000000 646e6173    ............sand
+   000010b0: 00786f62 03000000 08000000 21000000    box............!
+   000010c0: 646e6173 00786f62 01000000 61696c61    sandbox.....alia
+   000010d0: 00736573 03000000 07000000 2c000000    ses............,
+   000010e0: 6332692f 00003040 03000000 07000000    /i2c@0..........
+   000010f0: 31000000 6963702f 00003040 03000000    ...1/pci@0......
+   => sf erase 0 80000
+   SF: 524288 bytes @ 0x0 Erased: OK
+   => sf read 1000 1100 80000
+   device 0 offset 0x1100, size 0x80000
+   SF: 524288 bytes @ 0x1100 Read: OK
+   => md 1000
+   00001000: ffffffff ffffffff ffffffff ffffffff    ................
+   00001010: ffffffff ffffffff ffffffff ffffffff    ................
+   00001020: ffffffff ffffffff ffffffff ffffffff    ................
+   00001030: ffffffff ffffffff ffffffff ffffffff    ................
+   00001040: ffffffff ffffffff ffffffff ffffffff    ................
+   00001050: ffffffff ffffffff ffffffff ffffffff    ................
+   00001060: ffffffff ffffffff ffffffff ffffffff    ................
+   00001070: ffffffff ffffffff ffffffff ffffffff    ................
+   00001080: ffffffff ffffffff ffffffff ffffffff    ................
+   00001090: ffffffff ffffffff ffffffff ffffffff    ................
+   000010a0: ffffffff ffffffff ffffffff ffffffff    ................
+   000010b0: ffffffff ffffffff ffffffff ffffffff    ................
+   000010c0: ffffffff ffffffff ffffffff ffffffff    ................
+   000010d0: ffffffff ffffffff ffffffff ffffffff    ................
+   000010e0: ffffffff ffffffff ffffffff ffffffff    ................
+   000010f0: ffffffff ffffffff ffffffff ffffffff    ................
+
+This second example is running on coral, an x86 Chromebook::
+
+   => sf probe
+   SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
+   => sf erase 300000 80000
+   SF: 524288 bytes @ 0x300000 Erased: OK
+   => sf update 1110000 300000 80000
+   device 0 offset 0x300000, size 0x80000
+   524288 bytes written, 0 bytes skipped in 0.457s, speed 1164578 B/s
+
+   # This does nothing as the flash is already updated
+   => sf update 1110000 300000 80000
+   device 0 offset 0x300000, size 0x80000
+   0 bytes written, 524288 bytes skipped in 0.196s, speed 2684354 B/s
+   => sf test 00000 80000   # try a protected region
+   SPI flash test:
+   Erase failed (err = -5)
+   Test failed
+   => sf test 800000 80000
+   SPI flash test:
+   0 erase: 18 ticks, 28444 KiB/s 227.552 Mbps
+   1 check: 192 ticks, 2666 KiB/s 21.328 Mbps
+   2 write: 227 ticks, 2255 KiB/s 18.040 Mbps
+   3 read: 189 ticks, 2708 KiB/s 21.664 Mbps
+   Test passed
+   0 erase: 18 ticks, 28444 KiB/s 227.552 Mbps
+   1 check: 192 ticks, 2666 KiB/s 21.328 Mbps
+   2 write: 227 ticks, 2255 KiB/s 18.040 Mbps
+   3 read: 189 ticks, 2708 KiB/s 21.664 Mbps
+
+
+.. _SPI documentation:
+   https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Jagan Teki]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> This command is fairly complicated so documentation is useful.
> Unfortunately I an not sure how the MTD side of things works and cannot
> find information about that.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> ---

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Heinrich Schuchardt]

On 9/19/21 23:49, Simon Glass wrote:
> This command is fairly complicated so documentation is useful.
> Unfortunately I an not sure how the MTD side of things works and cannot
> find information about that.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>
> Changes in v4:
> - Split out the 'const' change into a separate patch
> - Show the 'sf probe' output in the examples
>
> Changes in v2:
> - Many fixes as suggested by Heinrich
>
>   doc/usage/index.rst |   1 +
>   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 246 insertions(+)
>   create mode 100644 doc/usage/sf.rst
>
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 356f2a56181..9a7b12b7c54 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -43,6 +43,7 @@ Shell commands
>      qfw
>      reset
>      sbi
> +   sf

Please, keep this list in alphabetical order.

>      scp03
>      setexpr
>      size
> diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
> new file mode 100644
> index 00000000000..71bd1be5175
> --- /dev/null
> +++ b/doc/usage/sf.rst
> @@ -0,0 +1,245 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +sf command
> +==========
> +
> +Synopis
> +-------
> +
> +::
> +
> +    sf probe [[[<bus>:]<cs>] [<hz> [<mode>]]]
> +    sf read <addr> <offset>|<partition> <len>
> +    sf write <addr> <offset>|<partition> <len>
> +    sf erase <offset>|<partition> <len>
> +    sf update <addr> <offset>|<partition> <len>
> +    sf protect lock|unlock <sector> <len>
> +    sf test <offset>|<partition> <len>
> +
> +Description
> +-----------
> +
> +The *sf* command is used to access SPI flash, supporting read/write/erase and
> +a few other functions.
> +
> +Probe
> +-----
> +
> +The flash must first be probed with *sf probe* before any of the other
> +subcommands can be used. All of the parameters are optional:
> +
> +bus
> +	SPI bus number containing the SPI-flash chip, e.g. 0. If you don't know
> +	the number, you can use 'dm uclass' to see all the spi devices,
> +	and check the value for 'seq' for each one (here 0 and 2)::

I would have expected the 'spi' command to have an info sub-command like
the other subsystems. But that is missing.

> +
> +	   uclass 89: spi
> +	   0     spi@0 @ 05484960, seq 0
> +	   1     spi@1 @ 05484b40, seq 2
> +
> +cs
> +	SPI chip-select to use for the chip. This is often 0 and can be omitted,
> +	but in some cases multiple slaves are attached to a SPI controller,
> +	selected by a chip-select line for each one.
> +
> +hz
> +	Speed of the SPI bus in hertz. This normally defaults to 100000, i.e.
> +	100KHz, which is very slow. Note that if the device exists in the
> +	device tree, there might be a speed provided there, in which case this
> +	setting is ignored.
> +
> +mode
> +	SPI mode to use:
> +
> +	=====  ================
> +	Mode   Meaning
> +	=====  ================
> +	0      CPOL=0, CPHA=0
> +	1      CPOL=0, CPHA=1
> +	2      CPOL=1, CPHA=0
> +	3      CPOL=1, CPHA=1
> +	=====  ================
> +
> +	Clock phase (CPHA) 0 means that data is transferred (sampled) on the
> +	first clock edge; 1 means the second.
> +
> +	Clock polarity (CPOL) controls the idle state of the clock, 0 for low,
> +	1 for high.
> +	The active state is the opposite of idle.
> +
> +	You may find this `SPI documentation`_ useful.
> +
> +Parameters for other subcommands (described below) are as follows:

I would not expect parameters for other sub-commands in chapter "Probe".

Please put all parameters into a separate section "Parameters". This
makes navigation easier.

> +
> +addr
> +	Memory address to start transfer
> +
> +offset
> +	Flash offset to start transfer
> +
> +partition
> +	If the parameter is not numeric, it is assumed to be a partition
> +	description in the format <dev_type><dev_num>,<part_num> which is not
> +	covered here. This requires CONFIG_CMD_MTDPARTS.
> +
> +len
> +	Number of bytes to transfer
> +
> +Read
> +~~~~
> +
> +Use *sf read* to read from SPI flash to memory. The read will fail if an
> +attempt is made to read past the end of the flash.
> +
> +
> +Write
> +~~~~~
> +
> +Use *sf write* to write from memory to SPI flash. The SPI flash should be
> +erased first, since otherwise the result is undefined.
> +
> +The write will fail if an attempt is made to read past the end of the flash.
> +
> +
> +Erase
> +~~~~~
> +
> +Use *sf erase* to erase a region of SPI flash. The erase will fail if any part
> +of the region to be erased is protected or lies past the end of the flash. It
> +may also fail if the start offset or length are not aligned to an erase region
> +(e.g. 256 bytes).
> +
> +
> +Update
> +~~~~~~
> +
> +Use *sf update* to automatically erase and update a region of SPI flash from
> +memory. This works a sector at a time (typical 4KB or 64KB). For each
> +sector it first checks if the sector already has the right data. If so it is
> +skipped. If not, the sector is erased and the new data written. Note that if
> +the length is not a multiple of the erase size, the space after the data in
> +the last sector will be erased. If the offset does not start at the beginning
> +of an erase block, the operation will fail.
> +
> +Speed statistics are shown including the number of bytes that were already
> +correct.
> +
> +
> +Protect
> +~~~~~~~
> +
> +SPI-flash chips often have a protection feature where the chip is split up into
> +regions which can be locked or unlocked. With *sf protect* it is possible to
> +change these settings, if supported by the driver.
> +
> +lock|unlock
> +	Selects whether to lock or unlock the sectors
> +
> +<sector>
> +	Start sector number to lock/unlock. This may be the byte offset or some
> +	other value, depending on the chip.
> +
> +<len>
> +	Number of bytes to lock/unlock
> +
> +
> +Test
> +~~~~
> +
> +A convenient and fast *sf test* subcommand provides a way to check that SPI
> +flash is working as expected. This works in four stages:
> +
> +   * erase - erases the entire region
> +   * check - checks that the region is erased
> +   * write - writes a test pattern to the region, consisting of the U-Boot code
> +   * read - reads back the test pattern to check that it was written correctly
> +
> +Memory is allocated for two buffers, each <len> bytes in size. At typical
> +size is 64KB to 1MB. The offset and size must be aligned to an erase boundary.
> +
> +Note that this test will fail if any part of the SPI flash is write-protected.
> +
> +
> +Examples
> +--------
> +
> +This first example uses sandbox::

"the sandbox"

> +
> +   => sf probe
> +   SF: Detected m25p16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB
> +   => sf read 1000 1100 80000
> +   device 0 offset 0x1100, size 0x80000
> +   SF: 524288 bytes @ 0x1100 Read: OK
> +   => md 1000
> +   00001000: edfe0dd0 f33a0000 78000000 84250000    ......:....x..%.
> +   00001010: 28000000 11000000 10000000 00000000    ...(............
> +   00001020: 6f050000 0c250000 00000000 00000000    ...o..%.........
> +   00001030: 00000000 00000000 00000000 00000000    ................
> +   00001040: 00000000 00000000 00000000 00000000    ................
> +   00001050: 00000000 00000000 00000000 00000000    ................
> +   00001060: 00000000 00000000 00000000 00000000    ................
> +   00001070: 00000000 00000000 01000000 00000000    ................
> +   00001080: 03000000 04000000 00000000 01000000    ................
> +   00001090: 03000000 04000000 0f000000 01000000    ................
> +   000010a0: 03000000 08000000 1b000000 646e6173    ............sand
> +   000010b0: 00786f62 03000000 08000000 21000000    box............!
> +   000010c0: 646e6173 00786f62 01000000 61696c61    sandbox.....alia
> +   000010d0: 00736573 03000000 07000000 2c000000    ses............,
> +   000010e0: 6332692f 00003040 03000000 07000000    /i2c@0..........
> +   000010f0: 31000000 6963702f 00003040 03000000    ...1/pci@0......
> +   => sf erase 0 80000
> +   SF: 524288 bytes @ 0x0 Erased: OK
> +   => sf read 1000 1100 80000
> +   device 0 offset 0x1100, size 0x80000
> +   SF: 524288 bytes @ 0x1100 Read: OK
> +   => md 1000
> +   00001000: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001010: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001020: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001030: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001040: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001050: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001060: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001070: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001080: ffffffff ffffffff ffffffff ffffffff    ................
> +   00001090: ffffffff ffffffff ffffffff ffffffff    ................
> +   000010a0: ffffffff ffffffff ffffffff ffffffff    ................
> +   000010b0: ffffffff ffffffff ffffffff ffffffff    ................
> +   000010c0: ffffffff ffffffff ffffffff ffffffff    ................
> +   000010d0: ffffffff ffffffff ffffffff ffffffff    ................
> +   000010e0: ffffffff ffffffff ffffffff ffffffff    ................
> +   000010f0: ffffffff ffffffff ffffffff ffffffff    ................
> +
> +This second example is running on coral, an x86 Chromebook::

Proper names should be capitalized: "Google Coral".

> +
> +   => sf probe
> +   SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> +   => sf erase 300000 80000
> +   SF: 524288 bytes @ 0x300000 Erased: OK
> +   => sf update 1110000 300000 80000
> +   device 0 offset 0x300000, size 0x80000
> +   524288 bytes written, 0 bytes skipped in 0.457s, speed 1164578 B/s
> +
> +   # This does nothing as the flash is already updated
> +   => sf update 1110000 300000 80000
> +   device 0 offset 0x300000, size 0x80000
> +   0 bytes written, 524288 bytes skipped in 0.196s, speed 2684354 B/s
> +   => sf test 00000 80000   # try a protected region
> +   SPI flash test:
> +   Erase failed (err = -5)
> +   Test failed
> +   => sf test 800000 80000
> +   SPI flash test:
> +   0 erase: 18 ticks, 28444 KiB/s 227.552 Mbps
> +   1 check: 192 ticks, 2666 KiB/s 21.328 Mbps
> +   2 write: 227 ticks, 2255 KiB/s 18.040 Mbps
> +   3 read: 189 ticks, 2708 KiB/s 21.664 Mbps
> +   Test passed
> +   0 erase: 18 ticks, 28444 KiB/s 227.552 Mbps
> +   1 check: 192 ticks, 2666 KiB/s 21.328 Mbps
> +   2 write: 227 ticks, 2255 KiB/s 18.040 Mbps
> +   3 read: 189 ticks, 2708 KiB/s 21.664 Mbps
> +
> +
> +.. _SPI documentation:
> +   https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>

Please, add a section "Configuration" (cf. loady command). For other
commands we also describe the return value of $? to expect.

Best regards

Heinrich

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Simon Glass]

Hi Heinrich,

On Sat, 13 Nov 2021 at 04:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/19/21 23:49, Simon Glass wrote:
> > This command is fairly complicated so documentation is useful.
> > Unfortunately I an not sure how the MTD side of things works and cannot
> > find information about that.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Acked-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >
> > Changes in v4:
> > - Split out the 'const' change into a separate patch
> > - Show the 'sf probe' output in the examples
> >
> > Changes in v2:
> > - Many fixes as suggested by Heinrich
> >
> >   doc/usage/index.rst |   1 +
> >   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 246 insertions(+)
> >   create mode 100644 doc/usage/sf.rst
> >
> > diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> > index 356f2a56181..9a7b12b7c54 100644
> > --- a/doc/usage/index.rst
> > +++ b/doc/usage/index.rst
> > @@ -43,6 +43,7 @@ Shell commands
> >      qfw
> >      reset
> >      sbi
> > +   sf
>
> Please, keep this list in alphabetical order.
>
> >      scp03
> >      setexpr
> >      size
> > diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
> > new file mode 100644
> > index 00000000000..71bd1be5175
> > --- /dev/null
> > +++ b/doc/usage/sf.rst
> > @@ -0,0 +1,245 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +sf command
> > +==========
> > +
> > +Synopis
> > +-------
> > +
> > +::
> > +
> > +    sf probe [[[<bus>:]<cs>] [<hz> [<mode>]]]
> > +    sf read <addr> <offset>|<partition> <len>
> > +    sf write <addr> <offset>|<partition> <len>
> > +    sf erase <offset>|<partition> <len>
> > +    sf update <addr> <offset>|<partition> <len>
> > +    sf protect lock|unlock <sector> <len>
> > +    sf test <offset>|<partition> <len>
> > +
> > +Description
> > +-----------
> > +
> > +The *sf* command is used to access SPI flash, supporting read/write/erase and
> > +a few other functions.
> > +
> > +Probe
> > +-----
> > +
> > +The flash must first be probed with *sf probe* before any of the other
> > +subcommands can be used. All of the parameters are optional:
> > +
> > +bus
> > +     SPI bus number containing the SPI-flash chip, e.g. 0. If you don't know
> > +     the number, you can use 'dm uclass' to see all the spi devices,
> > +     and check the value for 'seq' for each one (here 0 and 2)::
>
> I would have expected the 'spi' command to have an info sub-command like
> the other subsystems. But that is missing.
>
> > +
> > +        uclass 89: spi
> > +        0     spi@0 @ 05484960, seq 0
> > +        1     spi@1 @ 05484b40, seq 2
> > +
> > +cs
> > +     SPI chip-select to use for the chip. This is often 0 and can be omitted,
> > +     but in some cases multiple slaves are attached to a SPI controller,
> > +     selected by a chip-select line for each one.
> > +
> > +hz
> > +     Speed of the SPI bus in hertz. This normally defaults to 100000, i.e.
> > +     100KHz, which is very slow. Note that if the device exists in the
> > +     device tree, there might be a speed provided there, in which case this
> > +     setting is ignored.
> > +
> > +mode
> > +     SPI mode to use:
> > +
> > +     =====  ================
> > +     Mode   Meaning
> > +     =====  ================
> > +     0      CPOL=0, CPHA=0
> > +     1      CPOL=0, CPHA=1
> > +     2      CPOL=1, CPHA=0
> > +     3      CPOL=1, CPHA=1
> > +     =====  ================
> > +
> > +     Clock phase (CPHA) 0 means that data is transferred (sampled) on the
> > +     first clock edge; 1 means the second.
> > +
> > +     Clock polarity (CPOL) controls the idle state of the clock, 0 for low,
> > +     1 for high.
> > +     The active state is the opposite of idle.
> > +
> > +     You may find this `SPI documentation`_ useful.
> > +
> > +Parameters for other subcommands (described below) are as follows:
>
> I would not expect parameters for other sub-commands in chapter "Probe".
>
> Please put all parameters into a separate section "Parameters". This
> makes navigation easier.

This series was sent back in April and is now at version 5, after
multiple rounds of changes. This version alone sat here for nearly two
months. Who will want to write documentation in U-Boot if this is the
process?

I don't disagree with most of your comments, just the timing, although
the 'spi info' thing is highly debatable, as you cannot memory-map SPI
itself, only SPI flash.

The common.h header removal suffered a similar fate and we have never
resolved that, now 18 months later.

So please, let's get something in and move forward.

Regards,
Simon

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Heinrich Schuchardt]

Am 13. November 2021 15:21:13 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Sat, 13 Nov 2021 at 04:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 9/19/21 23:49, Simon Glass wrote:
>> > This command is fairly complicated so documentation is useful.
>> > Unfortunately I an not sure how the MTD side of things works and cannot
>> > find information about that.
>> >
>> > Signed-off-by: Simon Glass <sjg@chromium.org>
>> >
>> > Acked-by: Pratyush Yadav <p.yadav@ti.com>
>> > ---
>> >
>> > Changes in v4:
>> > - Split out the 'const' change into a separate patch
>> > - Show the 'sf probe' output in the examples
>> >
>> > Changes in v2:
>> > - Many fixes as suggested by Heinrich
>> >
>> >   doc/usage/index.rst |   1 +
>> >   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
>> >   2 files changed, 246 insertions(+)
>> >   create mode 100644 doc/usage/sf.rst
>> >
>> > diff --git a/doc/usage/index.rst b/doc/usage/index.rst
>> > index 356f2a56181..9a7b12b7c54 100644
>> > --- a/doc/usage/index.rst
>> > +++ b/doc/usage/index.rst
>> > @@ -43,6 +43,7 @@ Shell commands
>> >      qfw
>> >      reset
>> >      sbi
>> > +   sf
>>
>> Please, keep this list in alphabetical order.
>>
>> >      scp03
>> >      setexpr
>> >      size
>> > diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
>> > new file mode 100644
>> > index 00000000000..71bd1be5175
>> > --- /dev/null
>> > +++ b/doc/usage/sf.rst
>> > @@ -0,0 +1,245 @@
>> > +.. SPDX-License-Identifier: GPL-2.0+:
>> > +
>> > +sf command
>> > +==========
>> > +
>> > +Synopis
>> > +-------
>> > +
>> > +::
>> > +
>> > +    sf probe [[[<bus>:]<cs>] [<hz> [<mode>]]]
>> > +    sf read <addr> <offset>|<partition> <len>
>> > +    sf write <addr> <offset>|<partition> <len>
>> > +    sf erase <offset>|<partition> <len>
>> > +    sf update <addr> <offset>|<partition> <len>
>> > +    sf protect lock|unlock <sector> <len>
>> > +    sf test <offset>|<partition> <len>
>> > +
>> > +Description
>> > +-----------
>> > +
>> > +The *sf* command is used to access SPI flash, supporting read/write/erase and
>> > +a few other functions.
>> > +
>> > +Probe
>> > +-----
>> > +
>> > +The flash must first be probed with *sf probe* before any of the other
>> > +subcommands can be used. All of the parameters are optional:
>> > +
>> > +bus
>> > +     SPI bus number containing the SPI-flash chip, e.g. 0. If you don't know
>> > +     the number, you can use 'dm uclass' to see all the spi devices,
>> > +     and check the value for 'seq' for each one (here 0 and 2)::
>>
>> I would have expected the 'spi' command to have an info sub-command like
>> the other subsystems. But that is missing.
>>
>> > +
>> > +        uclass 89: spi
>> > +        0     spi@0 @ 05484960, seq 0
>> > +        1     spi@1 @ 05484b40, seq 2
>> > +
>> > +cs
>> > +     SPI chip-select to use for the chip. This is often 0 and can be omitted,
>> > +     but in some cases multiple slaves are attached to a SPI controller,
>> > +     selected by a chip-select line for each one.
>> > +
>> > +hz
>> > +     Speed of the SPI bus in hertz. This normally defaults to 100000, i.e.
>> > +     100KHz, which is very slow. Note that if the device exists in the
>> > +     device tree, there might be a speed provided there, in which case this
>> > +     setting is ignored.
>> > +
>> > +mode
>> > +     SPI mode to use:
>> > +
>> > +     =====  ================
>> > +     Mode   Meaning
>> > +     =====  ================
>> > +     0      CPOL=0, CPHA=0
>> > +     1      CPOL=0, CPHA=1
>> > +     2      CPOL=1, CPHA=0
>> > +     3      CPOL=1, CPHA=1
>> > +     =====  ================
>> > +
>> > +     Clock phase (CPHA) 0 means that data is transferred (sampled) on the
>> > +     first clock edge; 1 means the second.
>> > +
>> > +     Clock polarity (CPOL) controls the idle state of the clock, 0 for low,
>> > +     1 for high.
>> > +     The active state is the opposite of idle.
>> > +
>> > +     You may find this `SPI documentation`_ useful.
>> > +
>> > +Parameters for other subcommands (described below) are as follows:
>>
>> I would not expect parameters for other sub-commands in chapter "Probe".
>>
>> Please put all parameters into a separate section "Parameters". This
>> makes navigation easier.
>
>This series was sent back in April and is now at version 5, after
>multiple rounds of changes. This version alone sat here for nearly two
>months. Who will want to write documentation in U-Boot if this is the
>process?
>
>I don't disagree with most of your comments, just the timing, although
>the 'spi info' thing is highly debatable, as you cannot memory-map SPI
>itself, only SPI flash.
>
>The common.h header removal suffered a similar fate and we have never
>resolved that, now 18 months later.
>
>So please, let's get something in and move forward.

We can still in improve the documentation in a follow up patch.

Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>




>
>Regards,
>Simon

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Simon Glass]

Hi Heinrich,

On Sat, 13 Nov 2021 at 08:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 13. November 2021 15:21:13 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Sat, 13 Nov 2021 at 04:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 9/19/21 23:49, Simon Glass wrote:
> >> > This command is fairly complicated so documentation is useful.
> >> > Unfortunately I an not sure how the MTD side of things works and cannot
> >> > find information about that.
> >> >
> >> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >> >
> >> > Acked-by: Pratyush Yadav <p.yadav@ti.com>
> >> > ---
> >> >
> >> > Changes in v4:
> >> > - Split out the 'const' change into a separate patch
> >> > - Show the 'sf probe' output in the examples
> >> >
> >> > Changes in v2:
> >> > - Many fixes as suggested by Heinrich
> >> >
> >> >   doc/usage/index.rst |   1 +
> >> >   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
> >> >   2 files changed, 246 insertions(+)
> >> >   create mode 100644 doc/usage/sf.rst
> >> >
> >> > diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> >> > index 356f2a56181..9a7b12b7c54 100644
> >> > --- a/doc/usage/index.rst
> >> > +++ b/doc/usage/index.rst
> >> > @@ -43,6 +43,7 @@ Shell commands
> >> >      qfw
> >> >      reset
> >> >      sbi
> >> > +   sf
> >>
> >> Please, keep this list in alphabetical order.
> >>
> >> >      scp03
> >> >      setexpr
> >> >      size
> >> > diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
> >> > new file mode 100644
> >> > index 00000000000..71bd1be5175
> >> > --- /dev/null
> >> > +++ b/doc/usage/sf.rst
> >> > @@ -0,0 +1,245 @@
> >> > +.. SPDX-License-Identifier: GPL-2.0+:
> >> > +
> >> > +sf command
> >> > +==========
> >> > +
> >> > +Synopis
> >> > +-------
> >> > +
> >> > +::
> >> > +
> >> > +    sf probe [[[<bus>:]<cs>] [<hz> [<mode>]]]
> >> > +    sf read <addr> <offset>|<partition> <len>
> >> > +    sf write <addr> <offset>|<partition> <len>
> >> > +    sf erase <offset>|<partition> <len>
> >> > +    sf update <addr> <offset>|<partition> <len>
> >> > +    sf protect lock|unlock <sector> <len>
> >> > +    sf test <offset>|<partition> <len>
> >> > +
> >> > +Description
> >> > +-----------
> >> > +
> >> > +The *sf* command is used to access SPI flash, supporting read/write/erase and
> >> > +a few other functions.
> >> > +
> >> > +Probe
> >> > +-----
> >> > +
> >> > +The flash must first be probed with *sf probe* before any of the other
> >> > +subcommands can be used. All of the parameters are optional:
> >> > +
> >> > +bus
> >> > +     SPI bus number containing the SPI-flash chip, e.g. 0. If you don't know
> >> > +     the number, you can use 'dm uclass' to see all the spi devices,
> >> > +     and check the value for 'seq' for each one (here 0 and 2)::
> >>
> >> I would have expected the 'spi' command to have an info sub-command like
> >> the other subsystems. But that is missing.
> >>
> >> > +
> >> > +        uclass 89: spi
> >> > +        0     spi@0 @ 05484960, seq 0
> >> > +        1     spi@1 @ 05484b40, seq 2
> >> > +
> >> > +cs
> >> > +     SPI chip-select to use for the chip. This is often 0 and can be omitted,
> >> > +     but in some cases multiple slaves are attached to a SPI controller,
> >> > +     selected by a chip-select line for each one.
> >> > +
> >> > +hz
> >> > +     Speed of the SPI bus in hertz. This normally defaults to 100000, i.e.
> >> > +     100KHz, which is very slow. Note that if the device exists in the
> >> > +     device tree, there might be a speed provided there, in which case this
> >> > +     setting is ignored.
> >> > +
> >> > +mode
> >> > +     SPI mode to use:
> >> > +
> >> > +     =====  ================
> >> > +     Mode   Meaning
> >> > +     =====  ================
> >> > +     0      CPOL=0, CPHA=0
> >> > +     1      CPOL=0, CPHA=1
> >> > +     2      CPOL=1, CPHA=0
> >> > +     3      CPOL=1, CPHA=1
> >> > +     =====  ================
> >> > +
> >> > +     Clock phase (CPHA) 0 means that data is transferred (sampled) on the
> >> > +     first clock edge; 1 means the second.
> >> > +
> >> > +     Clock polarity (CPOL) controls the idle state of the clock, 0 for low,
> >> > +     1 for high.
> >> > +     The active state is the opposite of idle.
> >> > +
> >> > +     You may find this `SPI documentation`_ useful.
> >> > +
> >> > +Parameters for other subcommands (described below) are as follows:
> >>
> >> I would not expect parameters for other sub-commands in chapter "Probe".
> >>
> >> Please put all parameters into a separate section "Parameters". This
> >> makes navigation easier.
> >
> >This series was sent back in April and is now at version 5, after
> >multiple rounds of changes. This version alone sat here for nearly two
> >months. Who will want to write documentation in U-Boot if this is the
> >process?
> >
> >I don't disagree with most of your comments, just the timing, although
> >the 'spi info' thing is highly debatable, as you cannot memory-map SPI
> >itself, only SPI flash.
> >
> >The common.h header removal suffered a similar fate and we have never
> >resolved that, now 18 months later.
> >
> >So please, let's get something in and move forward.
>
> We can still in improve the documentation in a follow up patch.
>
> Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Thank you.

Regards,
Simon

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Simon Glass]

Hi Heinrich,

On Sat, 13 Nov 2021 at 08:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 13. November 2021 15:21:13 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Sat, 13 Nov 2021 at 04:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 9/19/21 23:49, Simon Glass wrote:
> >> > This command is fairly complicated so documentation is useful.
> >> > Unfortunately I an not sure how the MTD side of things works and cannot
> >> > find information about that.
> >> >
> >> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >> >
> >> > Acked-by: Pratyush Yadav <p.yadav@ti.com>
> >> > ---
> >> >
> >> > Changes in v4:
> >> > - Split out the 'const' change into a separate patch
> >> > - Show the 'sf probe' output in the examples
> >> >
> >> > Changes in v2:
> >> > - Many fixes as suggested by Heinrich
> >> >
> >> >   doc/usage/index.rst |   1 +
> >> >   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
> >> >   2 files changed, 246 insertions(+)
> >> >   create mode 100644 doc/usage/sf.rst
> >> >
Applied to u-boot-dm, thanks!

Re: [PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Simon Glass]

Hi Heinrich,

On Sat, 13 Nov 2021 at 08:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 13. November 2021 15:21:13 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Sat, 13 Nov 2021 at 04:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 9/19/21 23:49, Simon Glass wrote:
> >> > This command is fairly complicated so documentation is useful.
> >> > Unfortunately I an not sure how the MTD side of things works and cannot
> >> > find information about that.
> >> >
> >> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >> >
> >> > Acked-by: Pratyush Yadav <p.yadav@ti.com>
> >> > ---
> >> >
> >> > Changes in v4:
> >> > - Split out the 'const' change into a separate patch
> >> > - Show the 'sf probe' output in the examples
> >> >
> >> > Changes in v2:
> >> > - Many fixes as suggested by Heinrich
> >> >
> >> >   doc/usage/index.rst |   1 +
> >> >   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
> >> >   2 files changed, 246 insertions(+)
> >> >   create mode 100644 doc/usage/sf.rst
> >> >
Applied to u-boot-dm, thanks!
Applied to u-boot-dm/next, thanks!

[PATCH v4 5/5] sf: Provide a command to access memory-mapped SPI

[Simon Glass]

Add a new 'sf mmap' function to show the address of a SPI offset, if the
hardware supports it. This is useful on x86 systems.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v3)

Changes in v3:
- Add configuration and return value also

 arch/Kconfig     |  2 ++
 cmd/Kconfig      |  8 ++++++
 cmd/sf.c         | 35 +++++++++++++++++++++++++++
 doc/usage/sf.rst | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 108 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index 8f8daadcf92..406e5a6c627 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -136,6 +136,7 @@ config SANDBOX
 	imply CMD_LZMADEC
 	imply CMD_SATA
 	imply CMD_SF
+	imply CMD_SF_MMAP
 	imply CMD_SF_TEST
 	imply CRC32_VERIFY
 	imply FAT_WRITE
@@ -200,6 +201,7 @@ config X86
 	imply CMD_IRQ
 	imply CMD_PCI
 	imply CMD_SF
+	imply CMD_SF_MMAP
 	imply CMD_SF_TEST
 	imply CMD_ZBOOT
 	imply DM_ETH
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 3a857b3f6e2..44485f1588c 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1273,6 +1273,14 @@ config CMD_SF
 	help
 	  SPI Flash support
 
+config CMD_SF_MMAP
+	bool "sf mmap - Access memory-mapped SPI flash"
+	depends on CMD_SF
+	help
+	  On some systems part of the SPI flash is mapped into mmemory. This
+	  command provides a way to map a SPI-flash offset to a memory address,
+	  so that the contents can be browsed using 'md', for example.
+
 config CMD_SF_TEST
 	bool "sf test - Allow testing of SPI flash"
 	depends on CMD_SF
diff --git a/cmd/sf.c b/cmd/sf.c
index 72246d912fe..c78cd7e6342 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -384,6 +384,36 @@ static int do_spi_protect(int argc, char *const argv[])
 	return ret == 0 ? 0 : 1;
 }
 
+static int do_spi_flash_mmap(int argc, char *const argv[])
+{
+	loff_t offset, len, maxsize;
+	uint map_offset, map_size;
+	ulong map_base;
+	int dev = 0;
+	int ret;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	ret = mtd_arg_off_size(argc - 1, &argv[1], &dev, &offset, &len,
+			       &maxsize, MTD_DEV_TYPE_NOR, flash->size);
+	if (ret)
+		return ret;
+
+	ret = dm_spi_get_mmap(flash->dev, &map_base, &map_size, &map_offset);
+	if (ret) {
+		printf("Mapping not available (err=%d)\n", ret);
+		return CMD_RET_FAILURE;
+	}
+	if (offset < 0 || offset + len > map_size) {
+		printf("Offset out of range (map size %x)\n", map_size);
+		return CMD_RET_FAILURE;
+	}
+	printf("%lx\n", map_base + (ulong)offset);
+
+	return 0;
+}
+
 enum {
 	STAGE_ERASE,
 	STAGE_CHECK,
@@ -580,6 +610,8 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
 		ret = do_spi_flash_erase(argc, argv);
 	else if (strcmp(cmd, "protect") == 0)
 		ret = do_spi_protect(argc, argv);
+	else if (IS_ENABLED(CONFIG_CMD_SF_MMAP) && !strcmp(cmd, "mmap"))
+		ret = do_spi_flash_mmap(argc, argv);
 	else if (IS_ENABLED(CONFIG_CMD_SF_TEST) && !strcmp(cmd, "test"))
 		ret = do_spi_flash_test(argc, argv);
 	else
@@ -611,6 +643,9 @@ static const char long_help[] =
 	"					  or to start of mtd `partition'\n"
 	"sf protect lock/unlock sector len	- protect/unprotect 'len' bytes starting\n"
 	"					  at address 'sector'"
+#ifdef CONFIG_CMD_SF_MMAP
+	"\nsf mmap offset len		- get memory address of SPI-flash offset\n"
+#endif
 #ifdef CONFIG_CMD_SF_TEST
 	"\nsf test offset len		- run a very basic destructive test"
 #endif
diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
index 71bd1be5175..93fb8409370 100644
--- a/doc/usage/sf.rst
+++ b/doc/usage/sf.rst
@@ -14,6 +14,7 @@ Synopis
     sf erase <offset>|<partition> <len>
     sf update <addr> <offset>|<partition> <len>
     sf protect lock|unlock <sector> <len>
+    sf mmap <offset>|<partition> <len>
     sf test <offset>|<partition> <len>
 
 Description
@@ -143,6 +144,16 @@ lock|unlock
 	Number of bytes to lock/unlock
 
 
+Memory-mapped flash
+-------------------
+
+On some systems part of the SPI flash is mapped into mmemory. With *sf mmap*
+you can map a SPI-flash offset to a memory address, so that the contents can be
+browsed using 'md', for example.
+
+The command will fail if this is not supported by the hardware or driver.
+
+
 Test
 ~~~~
 
@@ -240,6 +251,58 @@ This second example is running on coral, an x86 Chromebook::
    2 write: 227 ticks, 2255 KiB/s 18.040 Mbps
    3 read: 189 ticks, 2708 KiB/s 21.664 Mbps
 
+   # On coral, SPI flash offset 0 corresponds to address ff081000 in memory
+   => sf mmap 0 1000
+   device 0 offset 0x0, size 0x1000
+   ff081000
+
+   # See below for how this address was obtained
+   => sf mmap e80000 11e18
+   device 0 offset 0xe80000, size 0x11e18
+   fff01000
+   => md fff01000
+   fff01000: b2e8e089 89000030 30b4e8c4 c0310000    ....0......0..1.
+   fff01010: 002c95e8 2ce8e800 feeb0000 dfe8c489    ..,....,........
+   fff01020: f400002c 83f4fdeb d4e80cec 3100001e    ,..............1
+   fff01030: 0cc483c0 f883c3c3 8b0b770a df408504    .........w....@.
+   fff01040: c085fef1 c8b80575 c3fef1e5 53565755    ....u.......UWVS
+   fff01050: 89c38951 2404c7c5 80000002 8924048b    Q......$......$.
+   fff01060: 89a20fdf 89fb89de 75890045 084d8904    ........E..u..M.
+   fff01070: ff0c5589 c5832404 243c8110 80000005    .U...$....<$....
+   fff01080: 43c6da75 3b800030 43037520 d889f8eb    u..C0..; u.C....
+   fff01090: 5f5e5b5a 80e6c35d 535657c3 e7e8c689    Z[^_]....WVS....
+   fff010a0: 89000069 00a164c3 8b000000 408b4c56    i....d......VL.@
+   fff010b0: 0cec837c ddb9ff6a e8fef1e5 00004e01    |...j........N..
+   fff010c0: a1640389 00000000 1c80b60f 66000001    ..d............f
+   fff010d0: b80c4389 00000001 a20fdf89 fb89de89    .C..............
+   fff010e0: 89104389 c4831453 5bc03110 56c35f5e    .C..S....1.[^_.V
+   fff010f0: 14ec8353 ce89d389 0000a164 b60f0000    S.......d.......
+
+
+The offset e80000 was obtained using the following binman command, to find the
+location of U-Boot SPL::
+
+   $ binman ls -i /tmp/b/chromebook_coral/u-boot.rom
+   Name                                   Image-pos  Size      Entry-type        Offset     Uncomp-size
+   ------------------------------------------------------------------------------------------------------
+   main-section                                   0   1000000  section                   0
+     spl                                     e80000     11e18  u-boot-spl         ffe80000
+     u-boot                                  d00000     9106e  u-boot             ffd00000
+     ...
+
 
 .. _SPI documentation:
    https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
+
+
+Configuration
+-------------
+
+The *sf* command is only available if `CONFIG_CMD_SF=y`. Note that it depends on
+`CONFIG_DM_SPI_FLASH`.
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the command succeded and to 1 (false)
+otherwise.
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v4 5/5] sf: Provide a command to access memory-mapped SPI

[Jagan Teki]

On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> Add a new 'sf mmap' function to show the address of a SPI offset, if the
> hardware supports it. This is useful on x86 systems.

I'm not quite sure about growing sf for limited use cases, maybe
support it in existing arguments might be a good option.

Jagan.

Re: [PATCH v4 5/5] sf: Provide a command to access memory-mapped SPI

[Simon Glass]

Hi Jagan,

On Fri, 8 Oct 2021 at 06:47, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Mon, Sep 20, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Add a new 'sf mmap' function to show the address of a SPI offset, if the
> > hardware supports it. This is useful on x86 systems.
>
> I'm not quite sure about growing sf for limited use cases, maybe
> support it in existing arguments might be a good option.

At least on x86 this is a common feature.

What do you suggest for this? Can you give an example of what
arguments might be used?

Regards,
Simon

Re: [PATCH v4 5/5] sf: Provide a command to access memory-mapped SPI

[Heinrich Schuchardt]

On 9/19/21 23:49, Simon Glass wrote:
> Add a new 'sf mmap' function to show the address of a SPI offset, if the

I would expect a 'spi info' command to provide this information.

Best regards

Heinrich

> hardware supports it. This is useful on x86 systems.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Add configuration and return value also
>
>   arch/Kconfig     |  2 ++
>   cmd/Kconfig      |  8 ++++++
>   cmd/sf.c         | 35 +++++++++++++++++++++++++++
>   doc/usage/sf.rst | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 108 insertions(+)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8f8daadcf92..406e5a6c627 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -136,6 +136,7 @@ config SANDBOX
>   	imply CMD_LZMADEC
>   	imply CMD_SATA
>   	imply CMD_SF
> +	imply CMD_SF_MMAP
>   	imply CMD_SF_TEST
>   	imply CRC32_VERIFY
>   	imply FAT_WRITE
> @@ -200,6 +201,7 @@ config X86
>   	imply CMD_IRQ
>   	imply CMD_PCI
>   	imply CMD_SF
> +	imply CMD_SF_MMAP
>   	imply CMD_SF_TEST
>   	imply CMD_ZBOOT
>   	imply DM_ETH
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 3a857b3f6e2..44485f1588c 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1273,6 +1273,14 @@ config CMD_SF
>   	help
>   	  SPI Flash support
>
> +config CMD_SF_MMAP
> +	bool "sf mmap - Access memory-mapped SPI flash"
> +	depends on CMD_SF
> +	help
> +	  On some systems part of the SPI flash is mapped into mmemory. This
> +	  command provides a way to map a SPI-flash offset to a memory address,
> +	  so that the contents can be browsed using 'md', for example.
> +
>   config CMD_SF_TEST
>   	bool "sf test - Allow testing of SPI flash"
>   	depends on CMD_SF
> diff --git a/cmd/sf.c b/cmd/sf.c
> index 72246d912fe..c78cd7e6342 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -384,6 +384,36 @@ static int do_spi_protect(int argc, char *const argv[])
>   	return ret == 0 ? 0 : 1;
>   }
>
> +static int do_spi_flash_mmap(int argc, char *const argv[])
> +{
> +	loff_t offset, len, maxsize;
> +	uint map_offset, map_size;
> +	ulong map_base;
> +	int dev = 0;
> +	int ret;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	ret = mtd_arg_off_size(argc - 1, &argv[1], &dev, &offset, &len,
> +			       &maxsize, MTD_DEV_TYPE_NOR, flash->size);
> +	if (ret)
> +		return ret;
> +
> +	ret = dm_spi_get_mmap(flash->dev, &map_base, &map_size, &map_offset);
> +	if (ret) {
> +		printf("Mapping not available (err=%d)\n", ret);
> +		return CMD_RET_FAILURE;
> +	}
> +	if (offset < 0 || offset + len > map_size) {
> +		printf("Offset out of range (map size %x)\n", map_size);
> +		return CMD_RET_FAILURE;
> +	}
> +	printf("%lx\n", map_base + (ulong)offset);
> +
> +	return 0;
> +}
> +
>   enum {
>   	STAGE_ERASE,
>   	STAGE_CHECK,
> @@ -580,6 +610,8 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
>   		ret = do_spi_flash_erase(argc, argv);
>   	else if (strcmp(cmd, "protect") == 0)
>   		ret = do_spi_protect(argc, argv);
> +	else if (IS_ENABLED(CONFIG_CMD_SF_MMAP) && !strcmp(cmd, "mmap"))
> +		ret = do_spi_flash_mmap(argc, argv);
>   	else if (IS_ENABLED(CONFIG_CMD_SF_TEST) && !strcmp(cmd, "test"))
>   		ret = do_spi_flash_test(argc, argv);
>   	else
> @@ -611,6 +643,9 @@ static const char long_help[] =
>   	"					  or to start of mtd `partition'\n"
>   	"sf protect lock/unlock sector len	- protect/unprotect 'len' bytes starting\n"
>   	"					  at address 'sector'"
> +#ifdef CONFIG_CMD_SF_MMAP
> +	"\nsf mmap offset len		- get memory address of SPI-flash offset\n"
> +#endif
>   #ifdef CONFIG_CMD_SF_TEST
>   	"\nsf test offset len		- run a very basic destructive test"
>   #endif
> diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
> index 71bd1be5175..93fb8409370 100644
> --- a/doc/usage/sf.rst
> +++ b/doc/usage/sf.rst
> @@ -14,6 +14,7 @@ Synopis
>       sf erase <offset>|<partition> <len>
>       sf update <addr> <offset>|<partition> <len>
>       sf protect lock|unlock <sector> <len>
> +    sf mmap <offset>|<partition> <len>
>       sf test <offset>|<partition> <len>
>
>   Description
> @@ -143,6 +144,16 @@ lock|unlock
>   	Number of bytes to lock/unlock
>
>
> +Memory-mapped flash
> +-------------------
> +
> +On some systems part of the SPI flash is mapped into mmemory. With *sf mmap*
> +you can map a SPI-flash offset to a memory address, so that the contents can be
> +browsed using 'md', for example.
> +
> +The command will fail if this is not supported by the hardware or driver.
> +
> +
>   Test
>   ~~~~
>
> @@ -240,6 +251,58 @@ This second example is running on coral, an x86 Chromebook::
>      2 write: 227 ticks, 2255 KiB/s 18.040 Mbps
>      3 read: 189 ticks, 2708 KiB/s 21.664 Mbps
>
> +   # On coral, SPI flash offset 0 corresponds to address ff081000 in memory
> +   => sf mmap 0 1000
> +   device 0 offset 0x0, size 0x1000
> +   ff081000
> +
> +   # See below for how this address was obtained
> +   => sf mmap e80000 11e18
> +   device 0 offset 0xe80000, size 0x11e18
> +   fff01000
> +   => md fff01000
> +   fff01000: b2e8e089 89000030 30b4e8c4 c0310000    ....0......0..1.
> +   fff01010: 002c95e8 2ce8e800 feeb0000 dfe8c489    ..,....,........
> +   fff01020: f400002c 83f4fdeb d4e80cec 3100001e    ,..............1
> +   fff01030: 0cc483c0 f883c3c3 8b0b770a df408504    .........w....@.
> +   fff01040: c085fef1 c8b80575 c3fef1e5 53565755    ....u.......UWVS
> +   fff01050: 89c38951 2404c7c5 80000002 8924048b    Q......$......$.
> +   fff01060: 89a20fdf 89fb89de 75890045 084d8904    ........E..u..M.
> +   fff01070: ff0c5589 c5832404 243c8110 80000005    .U...$....<$....
> +   fff01080: 43c6da75 3b800030 43037520 d889f8eb    u..C0..; u.C....
> +   fff01090: 5f5e5b5a 80e6c35d 535657c3 e7e8c689    Z[^_]....WVS....
> +   fff010a0: 89000069 00a164c3 8b000000 408b4c56    i....d......VL.@
> +   fff010b0: 0cec837c ddb9ff6a e8fef1e5 00004e01    |...j........N..
> +   fff010c0: a1640389 00000000 1c80b60f 66000001    ..d............f
> +   fff010d0: b80c4389 00000001 a20fdf89 fb89de89    .C..............
> +   fff010e0: 89104389 c4831453 5bc03110 56c35f5e    .C..S....1.[^_.V
> +   fff010f0: 14ec8353 ce89d389 0000a164 b60f0000    S.......d.......
> +
> +
> +The offset e80000 was obtained using the following binman command, to find the
> +location of U-Boot SPL::
> +
> +   $ binman ls -i /tmp/b/chromebook_coral/u-boot.rom
> +   Name                                   Image-pos  Size      Entry-type        Offset     Uncomp-size
> +   ------------------------------------------------------------------------------------------------------
> +   main-section                                   0   1000000  section                   0
> +     spl                                     e80000     11e18  u-boot-spl         ffe80000
> +     u-boot                                  d00000     9106e  u-boot             ffd00000
> +     ...
> +
>
>   .. _SPI documentation:
>      https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
> +
> +
> +Configuration
> +-------------
> +
> +The *sf* command is only available if `CONFIG_CMD_SF=y`. Note that it depends on
> +`CONFIG_DM_SPI_FLASH`.
> +
> +Return value
> +------------
> +
> +The return value $? is set to 0 (true) if the command succeded and to 1 (false)
> +otherwise.
>

Re: [PATCH v4 5/5] sf: Provide a command to access memory-mapped SPI

[Simon Glass]

Hi Heinrich,

On Sat, 13 Nov 2021 at 04:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 9/19/21 23:49, Simon Glass wrote:
> > Add a new 'sf mmap' function to show the address of a SPI offset, if the
>
> I would expect a 'spi info' command to provide this information.

Why is that? It depends on the size of actual SPI-flash device. It is
not a property of the SPI bus, so far as I understand it.

Regards,
Simon

Re: [PATCH v4 0/5] sf: Add documentation and an 'sf mmap' command

[Simon Glass]

Hi Jagan,

On Sun, 19 Sept 2021 at 15:49, Simon Glass <sjg@chromium.org> wrote:
>
> This little series adds documentation and a few other tidy-ups to the
> 'sf' command.
>
> It also provides a way to access memory-mapped SPI via the command line.
>
> Changes in v4:
> - Split out the 'const' change into a separate patch
> - Show the 'sf probe' output in the examples
>
> Changes in v3:
> - Add configuration and return value also
>
> Changes in v2:
> - Explain why 'usage' cannot be const
> - Many fixes as suggested by Heinrich
>
> Simon Glass (5):
>   command: Use a constant pointer for the help
>   sf: Use const for the stage name
>   sf: Tidy up code to avoid #ifdef
>   sf: doc: Add documentation for the 'sf' command
>   sf: Provide a command to access memory-mapped SPI
>
>  arch/Kconfig        |   2 +
>  cmd/Kconfig         |   8 ++
>  cmd/sf.c            |  67 +++++++---
>  doc/usage/index.rst |   1 +
>  doc/usage/sf.rst    | 308 ++++++++++++++++++++++++++++++++++++++++++++
>  include/command.h   |   2 +-
>  6 files changed, 369 insertions(+), 19 deletions(-)
>  create mode 100644 doc/usage/sf.rst

Any word on this series please?

Regards,
Simon